linux-kernel - Re: [PATCH v1 02/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: <20220329083241.ctatuvbbhkvswokl@basti-XPS-13-9310>
Date:   Tue, 29 Mar 2022 10:32:41 +0200
From:   Sebastian Fricke <sebastian.fricke@...labora.com>
To:     Nicolas Dufresne <nicolas.dufresne@...labora.com>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>, kernel@...labora.com,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 02/24] media: doc: Document dual use of H.264
 pic_num/frame_num

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>These two fields needs documentation as they have dual meaning. It is also

s/needs/need/

>confusing since pic_num is a derived value from frame_num, so this should
>help application developpers. If we ever need to make a V2 of this API, I

s/developpers/developers/
(seems to be a common typo among native French speakers ;))

>would suggest to remove pic_num entirely.

I think that suggestion should be placed as a FIXME/TODO comment into the
source code as it is way easier to find for future developers creating a V2.

>
>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..f634f20bcfbe 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 reference, this should match the derived value PicNum
>+	(8-28) and for long term references it should match the derived value

Minor suggestion, for short term you use singular (reference) and for
long term you use plural (references), I would stick to one of both.
(Below you only use plural so maybe stick to plural here as well)

>+	LongTermPicNum (8-29). Note that pic_num is the same as FrameNumWrap
>+	for frame decoding.
>     * - __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
>+	long term references, this should be set to the value of
>+	long_term_frame_idx describes in the dec_ref_pic_marking() syntax.

s/describes/described/

Greetings,
Sebastian

>     * - __u8
>       - ``fields``
>       - Specifies how the DPB entry is referenced. See :ref:`Reference Fields <h264_ref_fields>`
>-- 
>2.34.1
>