[v3,01/24] media: doc: Document dual use of H.264 pic_num/frame_num
Commit Message
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@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>
---
.../media/v4l/ext-ctrls-codec-stateless.rst | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
Comments
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@collabora.com>
> Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.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
@@ -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
+ (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.
* - __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 described in the dec_ref_pic_marking() syntax.
* - __u8
- ``fields``
- Specifies how the DPB entry is referenced. See :ref:`Reference Fields <h264_ref_fields>`