linux-kernel - Re: [PATCH v3 01/24] media: doc: Document dual use of H.264 pic_num/frame

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <59750798-300a-9b90-f257-3a97bcd4bfb5@xs4all.nl>
Date:   Fri, 22 Apr 2022 08:31:50 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Nicolas Dufresne <nicolas.dufresne@...labora.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>
Cc:     kernel@...labora.com, linux-kernel@...r.kernel.org,
        Sebastian Fricke <sebastian.fricke@...labora.com>,
        linux-media@...r.kernel.org
Subject: Re: [PATCH v3 01/24] media: doc: Document dual use of H.264
 pic_num/frame_num

On 05/04/2022 22:44, Nicolas Dufresne wrote:
> These two fields need documentation as they have dual meaning. It is also
> confusing since pic_num is a derived value from frame_num, so this should
> help application developers. If we ever need to make a V2 of this API, I
> would suggest to remove pic_num entirely.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@...labora.com>
> Reviewed-by: Sebastian Fricke <sebastian.fricke@...labora.com>
> ---
>  .../media/v4l/ext-ctrls-codec-stateless.rst            | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
> index 6541e4c32b26..49f89b702068 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
> @@ -649,10 +649,16 @@ Stateless Codec Control ID
>          :c:type:`timeval` in struct :c:type:`v4l2_buffer` to a __u64.
>      * - __u32
>        - ``pic_num``
> -      -
> +      - For short term references, this should match the derived value PicNum

shouldn't 'should' be 'must'? Same elsewhere below.

> +	(8-28) and for long term references it should match the derived value
> +	LongTermPicNum (8-29). Note that pic_num is the same as FrameNumWrap
> +	for frame decoding.

I think this last sentence is a bit confusing. How about:

"When decoding frames (as opposed to fields) pic_num is the same as FrameNumWrap."

>      * - __u16
>        - ``frame_num``
> -      -
> +      - For short term references, this should match the frame_num value from
> +	the slice header syntax (the driver will wrap the value if neeeded). For

neeeded -> needed

> +	long term references, this should be set to the value of
> +	long_term_frame_idx described in the dec_ref_pic_marking() syntax.
>      * - __u8
>        - ``fields``
>        - Specifies how the DPB entry is referenced. See :ref:`Reference Fields <h264_ref_fields>`

Regards,

	Hans