Re: [PATCH 5/8] media: uapi: HEVC: Add v4l2_ctrl_hevc_ext_sps_rps control

[Nicolas Dufresne <nicolas@ndufresne.ca>]

[-- Attachment #1: Type: text/plain, Size: 5949 bytes --]

Hi,

Le lundi 23 juin 2025 à 12:07 -0400, Detlev Casanova a écrit :
> Some hardware (e.g.: Rockchip's rk3588 hevc decoder) need the parsed
> long and short term reference information for HEVC decoding.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  .../media/v4l/ext-ctrls-codec-stateless.rst   | 73 +++++++++++++++++++
>  .../media/v4l/vidioc-queryctrl.rst            |  6 ++
>  2 files changed, 79 insertions(+)
> 
> 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 0da635691fdcb..745f38a8fe69c 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
> @@ -2958,6 +2958,79 @@ This structure contains all loop filter related parameters. See sections
>        - 0x00000004
>        -
>  
> +.. c:type:: v4l2_ctrl_hevc_ext_sps_rps
> +
> +.. cssclass:: longtable
> +
> +.. flat-table:: struct v4l2_ctrl_hevc_ext_sps_rps
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       1 1 2


Something, somewhere should say this is an array.

> +
> +    * - __u16
> +      - ``lt_ref_pic_poc_lsb_sps``
> +      - Long term reference picture order count as described in section 7.4.3.2.1
> +        "General sequence parameter set RBSP semantics" of the specification.

Each LONG term reference have one of these.

> +    * - __u8
> +      - ``delta_idx_minus1``
> +      - Specifies the delta compare to the index. See details in section 7.4.8 "Short-term
> +        reference picture set semantics" of the specification.


And each SHORT term have one of these. How can both be int he same control ? For
me this could make a bit more sense with two structures:

	v4l2_ctrl_hevc_ext_sps_st_rps[]
	v4l2_ctrl_hevc_ext_sps_lt_rps[]

Did I miss something ?

> +    * - __u8
> +      - ``delta_rps_sign``
> +      - Sign of the delta as specified in section 7.4.8 "Short-term reference picture set
> +        semantics" of the specification.
> +    * - __u16
> +      - ``abs_delta_rps_minus1``
> +      - Absolute delta RPS as specified in section 7.4.8 "Short-term reference picture set
> +        semantics" of the specification.
> +    * - __u8
> +      - ``num_negative_pics``
> +      - Number of short-term RPS entries that have picture order count values less than the
> +        picture order count value of the current picture.
> +    * - __u8
> +      - ``num_positive_pics``
> +      - Number of short-term RPS entries that have picture order count values greater than the
> +        picture order count value of the current picture.
> +    * - __u8
> +      - ``used_by_curr_pic_s0[16]``
> +      - Specifies if short-term RPS i (that has a negative POC) is used by the current picture.

Use bit field for that, a __u16 works, and one per ST.

> 
> +    * - __u8
> +      - ``used_by_curr_pic_s1[16]``
> +      - Specifies if short-term RPS i (that has a positive POC) is used by the current picture.
> 

Same.

> +    * - __s32
> +      - ``delta_poc_s0[16]``
> +      - Specifies the negative picture order count delta for the i-th entry in the short-term RPS.
> +        See details in section 7.4.8 "Short-term reference picture set semantics" of the
> +        specification.

Keep it delta_poc_s0_minus1 like int he spec, and then you can go back so u16.

> +    * - __s32
> +      - ``delta_poc_s1[16]``
> +      - Specifies the positive picture order count delta for the i-th entry in the short-term RPS.
> +        See details in section 7.4.8 "Short-term reference picture set semantics" of the
> +        specification.

Same.


regards,
Nicolas

> +    * - __u8
> +      - ``flags``
> +      - See :ref:`Extended RPS Flags <hevc_ext_sps_rps_flags>`
> +
> +.. _hevc_ext_sps_rps_flags:
> +
> +``Extended SPS RPS Flags``
> +
> +.. cssclass:: longtable
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       1 1 2
> +
> +    * - ``V4L2_HEVC_EXT_SPS_RPS_FLAG_USED_LT``
> +      - 0x00000001
> +      - Specifies if the long-term reference picture is used 7.4.3.2.1 "General sequence parameter
> +        set RBSP semantics" of the specification.
> +    * - ``V4L2_HEVC_EXT_SPS_RPS_FLAG_INTER_REF_PIC_SET_PRED``
> +      - 0x00000002
> +      - Specifies if the short-term RPS is predicted from another short term RPS. See details in
> +        section 7.4.8 "Short-term reference picture set semantics" of the specification.
> +
>  .. _v4l2-codec-stateless-av1:
>  
>  ``V4L2_CID_STATELESS_AV1_SEQUENCE (struct)``
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> index 3549417c7febb..dc7caf4bf6208 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> @@ -523,6 +523,12 @@ See also the examples in :ref:`control`.
>        - n/a
>        - A struct :c:type:`v4l2_ctrl_hevc_decode_params`, containing HEVC
>  	decoding parameters for stateless video decoders.
> +    * - ``V4L2_CTRL_TYPE_HEVC_EXT_SPS_RPS``
> +      - n/a
> +      - n/a
> +      - n/a
> +      - A struct :c:type:`v4l2_ctrl_hevc_ext_sps_rps`, containing HEVC
> +	extended RPS for stateless video decoders.
>      * - ``V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR``
>        - n/a
>        - n/a

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]