mbox series

[RFC,v2,0/8] Move HEVC stateless controls out of staging

Message ID 20220215110103.241297-1-benjamin.gaignard@collabora.com
Headers show
Series Move HEVC stateless controls out of staging | expand

Message

Benjamin Gaignard Feb. 15, 2022, 11 a.m. UTC
This series aims to make HEVC uapi stable and usable for hardware
decoder. HEVC uapi is used by 2 mainlined drivers (Cedrus and Hantro)
and 2 out of the tree drivers (rkvdec and RPI).

The 3 first patches are from Hans to implement v4l2 dynamic control
feature which is need by patch 7 for V4L2_CID_STATELESS_HEVC_ENTRY_POINT_OFFSET
definition.

Patch 4 move the existing uapi to stable, including definitions renaming 
and CID number change to fit with v4l2 naming.

Patches 5 and 7 add fields needed for rkvdec and RPI decoders.

Patches 6 is cleaning up the uapi of useless field.
Patches 8 change one field description and name to define offset by
bytes rather than by bits

Benjamin

Benjamin Gaignard (5):
  media: uapi: Move HEVC stateless controls out of staging
  media: uapi: Add fields needed for RKVDEC driver
  media: uapi: Remove bit_size field from v4l2_ctrl_hevc_slice_params
  media: uapi: Add V4L2_CID_STATELESS_HEVC_ENTRY_POINT_OFFSET control
  media: uapi: Change data_bit_offset definition

Hans Verkuil (3):
  videodev2.h: add V4L2_CTRL_FLAG_DYNAMIC_ARRAY
  v4l2-ctrls: add support for dynamically allocated arrays.
  vivid: add dynamic array test control

 .../userspace-api/media/drivers/hantro.rst    |   5 -
 .../media/v4l/ext-ctrls-codec.rst             |  58 ++--
 .../media/v4l/vidioc-queryctrl.rst            |   8 +
 .../media/test-drivers/vivid/vivid-ctrls.c    |  15 ++
 drivers/media/v4l2-core/v4l2-ctrls-api.c      | 103 ++++++--
 drivers/media/v4l2-core/v4l2-ctrls-core.c     | 182 ++++++++++---
 drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  32 +--
 drivers/media/v4l2-core/v4l2-ctrls-priv.h     |   3 +-
 drivers/media/v4l2-core/v4l2-ctrls-request.c  |  13 +-
 drivers/staging/media/hantro/hantro_drv.c     |  27 +-
 drivers/staging/media/hantro/hantro_hevc.c    |   8 +-
 drivers/staging/media/sunxi/cedrus/cedrus.c   |  24 +-
 .../staging/media/sunxi/cedrus/cedrus_dec.c   |  10 +-
 .../staging/media/sunxi/cedrus/cedrus_h265.c  |  13 +-
 include/linux/hantro-media.h                  |  17 ++
 include/media/hevc-ctrls.h                    | 250 ------------------
 include/media/v4l2-ctrls.h                    |  48 +++-
 include/uapi/linux/v4l2-controls.h            | 224 ++++++++++++++++
 include/uapi/linux/videodev2.h                |   8 +
 19 files changed, 640 insertions(+), 408 deletions(-)
 create mode 100644 include/linux/hantro-media.h
 delete mode 100644 include/media/hevc-ctrls.h

Comments

John Cox Feb. 16, 2022, 10:38 a.m. UTC | #1
On Tue, 15 Feb 2022 21:27:30 +0100, you wrote:

>Dne torek, 15. februar 2022 ob 17:31:28 CET je John Cox napisal(a):
>> On Tue, 15 Feb 2022 17:11:12 +0100, you wrote:
>> 
>> >Dne torek, 15. februar 2022 ob 17:00:33 CET je John Cox napisal(a):
>> >> On Tue, 15 Feb 2022 10:28:55 -0500, you wrote:
>> >> 
>> >> >Le mardi 15 février 2022 à 14:50 +0000, John Cox a écrit :
>> >> >> On Tue, 15 Feb 2022 15:35:12 +0100, you wrote:
>> >> >> 
>> >> >> > 
>> >> >> > Le 15/02/2022 à 15:17, John Cox a écrit :
>> >> >> > > Hi
>> >> >> > > 
>> >> >> > > > The bit size of the slice could be deduced from the buffer 
>payload
>> >> >> > > > so remove bit_size field to avoid duplicated the information.
>> >> >> > > I think this is a bad idea. In the future we are (I hope) going to 
>> >want
>> >> >> > > to have an array (variable) of slice headers all referring to the 
>> >same
>> >> >> > > bit buffer.  When we do that we will need this field.
>> >> >> > 
>> >> >> > I wonder if that could be considering like another decode mode and 
>so
>> >> >> > use an other control ?
>> >> >> 
>> >> >> I, personally, would be in favour of making the slice header control a
>> >> >> variable array just as it is.  If userland can't cope with multiple
>> >> >> entries then just send them one at a time and the code looks exactly
>> >> >> like it does at the moment and if the driver can't then set max array
>> >> >> entries to 1.
>> >> >> 
>> >> >> Having implemented this in rpi port of ffmpeg and the RPi V4L2 driver I
>> >> >> can say with experience that the code and effort overhead is very low.
>> >> >> 
>> >> >> Either way having a multiple slice header control in the UAPI is
>> >> >> important for efficiency.
>> >> >
>> >> >Just to clarify the idea, we would have a single slice controls, always 
>> >dynamic:
>> >> >
>> >> >1. For sliced based decoder
>> >> >
>> >> >The dynamic array slice control is implemented by the driver and its 
>size 
>> >must
>> >> >be 1.
>> >> 
>> >> Yes
>> >> 
>> >> >2. For frame based decoder that don't care for slices
>> >> >
>> >> >The dynamic array slice controls is not implement. Userland detects that 
>at
>> >> >runtime, similar to the VP9 compressed headers.
>> >> 
>> >> If the driver parses all the slice header then that seems plausible
>> >> 
>> >> >3. For frame based decoders that needs slices (or driver that supports 
>> >offset
>> >> >and can gain performance with such mode)
>> >> >
>> >> >The dynamic array slice controls is implemented, and should contain all 
>the
>> >> >slices found in the OUTPUT buffer.
>> >> >
>> >> >So the reason for this bit_size (not sure why its bits though, perhaps 
>> >someone
>> >> >can educate me ?)
>> >> 
>> >> RPi doesn't need bits and would be happy with bytes however
>> >> slice_segment data isn't byte aligned at the end so its possible that
>> >> there might be decoders out there that want an accurate length for that.
>> >
>> >There are two fields, please don't mix them up:
>> >
>> >__u32	bit_size;
>> >__u32	data_bit_offset; (changed to data_byte_offset in this series)
>> >
>> >data_bit_offset/data_byte_offset is useful, while bit_size is IMO not. If you 
>> >have multiple slices in array, you only need to know start of the slice 
>data 
>> >and that offset is always offset from start of the buffer (absolute, it's not 
>> >relative to previous slice data).
>> 
>> No... or at least I think not. RPi needs the start and end of the
>> slice_segment_data elements of each slice. 
>
>It would be good to know if size needs to be exact or can overshoot, like 
>using end of buffer for that.
>
>Cedrus also wants to know slice data size, but it turns out that bigger than 
>necessary size doesn't pose any problems. If that's not the case, then 
>bit_size needs stay in for sure.

RPi needs the exact size (bytes will do - but I don't see the harm in
specifying it in bits in case some future h/w wants the extra precision
as long as we nail down exactly which bit we mean)

Regards

JC

>Best regards,
>Jernej
>
>> If slices are arranged in the
>> buffer with slice_segment_headers attached then I don't see how I get to
>> know that.  Also if the OUTPUT buffer is just a bit of bitstream, which
>> might well be very convienient for some userspace, then it is legitimate
>> to have SEIs between slice headers so you can't even guarantee that your
>> coded slice segments are contiguous.
>> 
>> Regards
>> 
>> JC
>> 
>> >Best regards,
>> >Jernej
>> >
>> >> 
>> >> > Would be to let the driver offset inside the the single
>> >> >OUTPUT/bitstream buffer in case this is not automatically found by the 
>> >driver
>> >> >(or that no start-code is needed). Is that last bit correct ? If so, 
>should 
>> >we
>> >> >change it to an offset rather then a size ? Shall we allow using offesets 
>> >inside
>> >> >larger buffer (e.g. to avoid some memory copies) for the Sliced Base 
>cases ?
>> >> 
>> >> I use (in the current structure) data_bit_offset to find the start of
>> >> each slice's slice_segment_data within the OUTPUT buffer and bit_size to
>> >> find the end.  RPi doesn't / can't parse the slice_header and so wants
>> >> all of that.  Decoders that do parse the header might plausably want
>> >> header offsets too and it would facilitate zero copy of the bit buffer.
>> >> 
>> >>  
>> >> >> Regards
>> >> >> 
>> >> >> John Cox
>> >> >> 
>> >> >> > > > Signed-off-by: Benjamin Gaignard 
><benjamin.gaignard@collabora.com>
>> >> >> > > > ---
>> >> >> > > > .../userspace-api/media/v4l/ext-ctrls-codec.rst       |  3 ---
>> >> >> > > > drivers/staging/media/sunxi/cedrus/cedrus_h265.c      | 11 +++
>> >+-------
>> >> >> > > > include/uapi/linux/v4l2-controls.h                    |  3 +--
>> >> >> > > > 3 files changed, 5 insertions(+), 12 deletions(-)
>> >> >> > > > 
>> >> >> > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-
>> >codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> >> >> > > > index 3296ac3b9fca..c3ae97657fa7 100644
>> >> >> > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> >> >> > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> >> >> > > > @@ -2965,9 +2965,6 @@ enum 
>v4l2_mpeg_video_hevc_size_of_length_field 
>> >-
>> >> >> > > >      :stub-columns: 0
>> >> >> > > >      :widths:       1 1 2
>> >> >> > > > 
>> >> >> > > > -    * - __u32
>> >> >> > > > -      - ``bit_size``
>> >> >> > > > -      - Size (in bits) of the current slice data.
>> >> >> > > >      * - __u32
>> >> >> > > >        - ``data_bit_offset``
>> >> >> > > >        - Offset (in bits) to the video data in the current slice 
>> >data.
>> >> >> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/
>> >drivers/staging/media/sunxi/cedrus/cedrus_h265.c
>> >> >> > > > index 8ab2d9c6f048..db8c7475eeb8 100644
>> >> >> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
>> >> >> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
>> >> >> > > > @@ -312,8 +312,8 @@ static void cedrus_h265_setup(struct 
>cedrus_ctx 
>> >*ctx,
>> >> >> > > > 	const struct v4l2_hevc_pred_weight_table 
>*pred_weight_table;
>> >> >> > > > 	unsigned int width_in_ctb_luma, ctb_size_luma;
>> >> >> > > > 	unsigned int log2_max_luma_coding_block_size;
>> >> >> > > > +	size_t slice_bytes;
>> >> >> > > > 	dma_addr_t src_buf_addr;
>> >> >> > > > -	dma_addr_t src_buf_end_addr;
>> >> >> > > > 	u32 chroma_log2_weight_denom;
>> >> >> > > > 	u32 output_pic_list_index;
>> >> >> > > > 	u32 pic_order_cnt[2];
>> >> >> > > > @@ -370,8 +370,8 @@ static void cedrus_h265_setup(struct 
>cedrus_ctx 
>> >*ctx,
>> >> >> > > > 
>> >> >> > > > 	cedrus_write(dev, VE_DEC_H265_BITS_OFFSET, 0);
>> >> >> > > > 
>> >> >> > > > -	reg = slice_params->bit_size;
>> >> >> > > > -	cedrus_write(dev, VE_DEC_H265_BITS_LEN, reg);
>> >> >> > > > +	slice_bytes = vb2_get_plane_payload(&run->src-
>>vb2_buf, 0);
>> >> >> > > > +	cedrus_write(dev, VE_DEC_H265_BITS_LEN, slice_bytes);
>> >> >> > > I think one of these must be wrong. bit_size is in bits,
>> >> >> > > vb2_get_plane_payload is in bytes?
>> >> >> > 
>> >> >> > You are right it should be vb2_get_plane_payload() * 8 to get the 
>size 
>> >in bits.
>> >> >> > 
>> >> >> > I will change that in v3.
>> >> >> > 
>> >> >> > > 
>> >> >> > > Regards
>> >> >> > > 
>> >> >> > > John Cox
>> >> >> > >   
>> >> >> > > > 	/* Source beginning and end addresses. */
>> >> >> > > > 
>> >> >> > > > @@ -384,10 +384,7 @@ static void cedrus_h265_setup(struct 
>> >cedrus_ctx *ctx,
>> >> >> > > > 
>> >> >> > > > 	cedrus_write(dev, VE_DEC_H265_BITS_ADDR, reg);
>> >> >> > > > 
>> >> >> > > > -	src_buf_end_addr = src_buf_addr +
>> >> >> > > > -			   DIV_ROUND_UP(slice_params-
>>bit_size, 
>> >8);
>> >> >> > > > -
>> >> >> > > > -	reg = 
>VE_DEC_H265_BITS_END_ADDR_BASE(src_buf_end_addr);
>> >> >> > > > +	reg = VE_DEC_H265_BITS_END_ADDR_BASE(src_buf_addr + 
>slice_bytes);
>> >> >> > > > 	cedrus_write(dev, VE_DEC_H265_BITS_END_ADDR, reg);
>> >> >> > > > 
>> >> >> > > > 	/* Coding tree block address */
>> >> >> > > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/
>> >linux/v4l2-controls.h
>> >> >> > > > index b1a3dc05f02f..27f5d272dc43 100644
>> >> >> > > > --- a/include/uapi/linux/v4l2-controls.h
>> >> >> > > > +++ b/include/uapi/linux/v4l2-controls.h
>> >> >> > > > @@ -2457,7 +2457,6 @@ struct v4l2_hevc_pred_weight_table {
>> >> >> > > > #define V4L2_HEVC_SLICE_PARAMS_FLAG_DEPENDENT_SLICE_SEGMENT	
>> >(1ULL << 9)
>> >> >> > > > 
>> >> >> > > > struct v4l2_ctrl_hevc_slice_params {
>> >> >> > > > -	__u32	bit_size;
>> >> >> > > > 	__u32	data_bit_offset;
>> >> >> > > > 
>> >> >> > > > 	/* ISO/IEC 23008-2, ITU-T Rec. H.265: NAL unit header 
>*/
>> >> >> > > > @@ -2484,7 +2483,7 @@ struct v4l2_ctrl_hevc_slice_params {
>> >> >> > > > 	/* ISO/IEC 23008-2, ITU-T Rec. H.265: Picture timing 
>SEI message 
>> >*/
>> >> >> > > > 	__u8	pic_struct;
>> >> >> > > > 
>> >> >> > > > -	__u8	reserved;
>> >> >> > > > +	__u8	reserved[5];
>> >> >> > > > 
>> >> >> > > > 	/* ISO/IEC 23008-2, ITU-T Rec. H.265: General slice 
>segment 
>> >header */
>> >> >> > > > 	__u32	slice_segment_addr;
>> >> 
>> >
>> 
>
Hans Verkuil Feb. 18, 2022, 11:32 a.m. UTC | #2
On 15/02/2022 12:01, Benjamin Gaignard wrote:
> RKVDEC driver requires additional fields to perform HEVC decoding.
> Even if the driver isn't mainlined yet WIP patches could be find here:
> https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Rockchip/patches/linux/default/linux-2000-v4l2-wip-rkvdec-hevc.patch
> 
> This patch only include the change in HEVC uAPI.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
>  .../userspace-api/media/v4l/ext-ctrls-codec.rst  | 16 ++++++++++++++++
>  include/uapi/linux/v4l2-controls.h               |  5 +++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 4f3b3ba8319f..3296ac3b9fca 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -2661,6 +2661,13 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>      :stub-columns: 0
>      :widths:       1 1 2
>  
> +    * - __u8
> +      - ``video_parameter_set_id``
> +      - Identifies the VPS for reference by other syntax elements.
> +    * - __u8
> +      - ``seq_parameter_set_id``
> +      - Provides an identifier for the SPS for reference by other syntax
> +        elements.
>      * - __u16
>        - ``pic_width_in_luma_samples``
>        -
> @@ -2800,6 +2807,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>      :stub-columns: 0
>      :widths:       1 1 2
>  
> +    * - __u8
> +      - ``pic_parameter_set_id``
> +      - Identifies the PPS for reference by other syntax elements.
>      * - __u8
>        - ``num_extra_slice_header_bits``
>        -
> @@ -3026,6 +3036,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>      * - __u8
>        - ``ref_idx_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
>        - The list of L1 reference elements as indices in the DPB.
> +    * - __u16
> +      - ``short_term_ref_pic_set_size``
> +      -
> +    * - __u16
> +      - ``long_term_ref_pic_set_size``
> +      -
>      * - __u8
>        - ``padding``
>        - Applications and drivers must set this to zero.

Just to confirm: these additional fields are all from the H.265 spec, right?
They are not rkvdec specific.

Regards,

	Hans

> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 0e0ec2c61b80..b1a3dc05f02f 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -2341,6 +2341,8 @@ enum v4l2_stateless_hevc_start_code {
>  
>  struct v4l2_ctrl_hevc_sps {
>  	/* ISO/IEC 23008-2, ITU-T Rec. H.265: Sequence parameter set */
> +	__u8	video_parameter_set_id;
> +	__u8	seq_parameter_set_id;
>  	__u16	pic_width_in_luma_samples;
>  	__u16	pic_height_in_luma_samples;
>  	__u8	bit_depth_luma_minus8;
> @@ -2393,6 +2395,7 @@ struct v4l2_ctrl_hevc_sps {
>  
>  struct v4l2_ctrl_hevc_pps {
>  	/* ISO/IEC 23008-2, ITU-T Rec. H.265: Picture parameter set */
> +	__u8	pic_parameter_set_id;
>  	__u8	num_extra_slice_header_bits;
>  	__u8	num_ref_idx_l0_default_active_minus1;
>  	__u8	num_ref_idx_l1_default_active_minus1;
> @@ -2487,6 +2490,8 @@ struct v4l2_ctrl_hevc_slice_params {
>  	__u32	slice_segment_addr;
>  	__u8	ref_idx_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
>  	__u8	ref_idx_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
> +	__u16	short_term_ref_pic_set_size;
> +	__u16	long_term_ref_pic_set_size;
>  
>  	/* ISO/IEC 23008-2, ITU-T Rec. H.265: Weighted prediction parameter */
>  	struct v4l2_hevc_pred_weight_table pred_weight_table;
Benjamin Gaignard Feb. 18, 2022, 12:19 p.m. UTC | #3
Le 18/02/2022 à 12:32, Hans Verkuil a écrit :
> On 15/02/2022 12:01, Benjamin Gaignard wrote:
>> RKVDEC driver requires additional fields to perform HEVC decoding.
>> Even if the driver isn't mainlined yet WIP patches could be find here:
>> https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Rockchip/patches/linux/default/linux-2000-v4l2-wip-rkvdec-hevc.patch
>>
>> This patch only include the change in HEVC uAPI.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>>   .../userspace-api/media/v4l/ext-ctrls-codec.rst  | 16 ++++++++++++++++
>>   include/uapi/linux/v4l2-controls.h               |  5 +++++
>>   2 files changed, 21 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> index 4f3b3ba8319f..3296ac3b9fca 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> @@ -2661,6 +2661,13 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>>       :stub-columns: 0
>>       :widths:       1 1 2
>>   
>> +    * - __u8
>> +      - ``video_parameter_set_id``
>> +      - Identifies the VPS for reference by other syntax elements.
>> +    * - __u8
>> +      - ``seq_parameter_set_id``
>> +      - Provides an identifier for the SPS for reference by other syntax
>> +        elements.
>>       * - __u16
>>         - ``pic_width_in_luma_samples``
>>         -
>> @@ -2800,6 +2807,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>>       :stub-columns: 0
>>       :widths:       1 1 2
>>   
>> +    * - __u8
>> +      - ``pic_parameter_set_id``
>> +      - Identifies the PPS for reference by other syntax elements.
>>       * - __u8
>>         - ``num_extra_slice_header_bits``
>>         -
>> @@ -3026,6 +3036,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>>       * - __u8
>>         - ``ref_idx_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
>>         - The list of L1 reference elements as indices in the DPB.
>> +    * - __u16
>> +      - ``short_term_ref_pic_set_size``
>> +      -
>> +    * - __u16
>> +      - ``long_term_ref_pic_set_size``
>> +      -
>>       * - __u8
>>         - ``padding``
>>         - Applications and drivers must set this to zero.
> Just to confirm: these additional fields are all from the H.265 spec, right?
> They are not rkvdec specific.

They are in H.265 spec section "7.4.3.2.2 Sequence parameter set range extension semantics":
- num_short_term_ref_pic_sets specifies the number of st_ref_pic_set( ) syntax structures included in the SPS. The value
of num_short_term_ref_pic_sets shall be in the range of 0 to 64, inclusive.

- num_long_term_ref_pics_sps specifies the number of candidate long-term reference pictures that are specified in the
SPS. The value of num_long_term_ref_pics_sps shall be in the range of 0 to 32, inclusive.

I mention rkvdec because that it is the only driver to use they (as far I knows)

Regards,
Benjamin

>
> Regards,
>
> 	Hans
>
>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>> index 0e0ec2c61b80..b1a3dc05f02f 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -2341,6 +2341,8 @@ enum v4l2_stateless_hevc_start_code {
>>   
>>   struct v4l2_ctrl_hevc_sps {
>>   	/* ISO/IEC 23008-2, ITU-T Rec. H.265: Sequence parameter set */
>> +	__u8	video_parameter_set_id;
>> +	__u8	seq_parameter_set_id;
>>   	__u16	pic_width_in_luma_samples;
>>   	__u16	pic_height_in_luma_samples;
>>   	__u8	bit_depth_luma_minus8;
>> @@ -2393,6 +2395,7 @@ struct v4l2_ctrl_hevc_sps {
>>   
>>   struct v4l2_ctrl_hevc_pps {
>>   	/* ISO/IEC 23008-2, ITU-T Rec. H.265: Picture parameter set */
>> +	__u8	pic_parameter_set_id;
>>   	__u8	num_extra_slice_header_bits;
>>   	__u8	num_ref_idx_l0_default_active_minus1;
>>   	__u8	num_ref_idx_l1_default_active_minus1;
>> @@ -2487,6 +2490,8 @@ struct v4l2_ctrl_hevc_slice_params {
>>   	__u32	slice_segment_addr;
>>   	__u8	ref_idx_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
>>   	__u8	ref_idx_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
>> +	__u16	short_term_ref_pic_set_size;
>> +	__u16	long_term_ref_pic_set_size;
>>   
>>   	/* ISO/IEC 23008-2, ITU-T Rec. H.265: Weighted prediction parameter */
>>   	struct v4l2_hevc_pred_weight_table pred_weight_table;
Hans Verkuil Feb. 18, 2022, 12:32 p.m. UTC | #4
On 18/02/2022 13:30, Benjamin Gaignard wrote:
> 
> Le 18/02/2022 à 13:22, Hans Verkuil a écrit :
>> On 18/02/2022 13:19, Benjamin Gaignard wrote:
>>> Le 18/02/2022 à 12:32, Hans Verkuil a écrit :
>>>> On 15/02/2022 12:01, Benjamin Gaignard wrote:
>>>>> RKVDEC driver requires additional fields to perform HEVC decoding.
>>>>> Even if the driver isn't mainlined yet WIP patches could be find here:
>>>>> https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Rockchip/patches/linux/default/linux-2000-v4l2-wip-rkvdec-hevc.patch
>>>>>
>>>>> This patch only include the change in HEVC uAPI.
>>>>>
>>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>>>> ---
>>>>>    .../userspace-api/media/v4l/ext-ctrls-codec.rst  | 16 ++++++++++++++++
>>>>>    include/uapi/linux/v4l2-controls.h               |  5 +++++
>>>>>    2 files changed, 21 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>>> index 4f3b3ba8319f..3296ac3b9fca 100644
>>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>>> @@ -2661,6 +2661,13 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>>>>>        :stub-columns: 0
>>>>>        :widths:       1 1 2
>>>>>    +    * - __u8
>>>>> +      - ``video_parameter_set_id``
>>>>> +      - Identifies the VPS for reference by other syntax elements.
>>>>> +    * - __u8
>>>>> +      - ``seq_parameter_set_id``
>>>>> +      - Provides an identifier for the SPS for reference by other syntax
>>>>> +        elements.
>>>>>        * - __u16
>>>>>          - ``pic_width_in_luma_samples``
>>>>>          -
>>>>> @@ -2800,6 +2807,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>>>>>        :stub-columns: 0
>>>>>        :widths:       1 1 2
>>>>>    +    * - __u8
>>>>> +      - ``pic_parameter_set_id``
>>>>> +      - Identifies the PPS for reference by other syntax elements.
>>>>>        * - __u8
>>>>>          - ``num_extra_slice_header_bits``
>>>>>          -
>>>>> @@ -3026,6 +3036,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>>>>>        * - __u8
>>>>>          - ``ref_idx_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
>>>>>          - The list of L1 reference elements as indices in the DPB.
>>>>> +    * - __u16
>>>>> +      - ``short_term_ref_pic_set_size``
>>>>> +      -
>>>>> +    * - __u16
>>>>> +      - ``long_term_ref_pic_set_size``
>>>>> +      -
>>>>>        * - __u8
>>>>>          - ``padding``
>>>>>          - Applications and drivers must set this to zero.
>>>> Just to confirm: these additional fields are all from the H.265 spec, right?
>>>> They are not rkvdec specific.
>>> They are in H.265 spec section "7.4.3.2.2 Sequence parameter set range extension semantics":
>>> - num_short_term_ref_pic_sets specifies the number of st_ref_pic_set( ) syntax structures included in the SPS. The value
>>> of num_short_term_ref_pic_sets shall be in the range of 0 to 64, inclusive.
>>>
>>> - num_long_term_ref_pics_sps specifies the number of candidate long-term reference pictures that are specified in the
>>> SPS. The value of num_long_term_ref_pics_sps shall be in the range of 0 to 32, inclusive.
>> And what about video/seq/pic_parameter_set_id?
> 
> It is the same they come from section "7.4.3.2.1 General sequence parameter set RBSP semantics":
> - sps_video_parameter_set_id specifies the value of the vps_video_parameter_set_id of the active VPS.
> - sps_seq_parameter_set_id provides an identifier for the SPS for reference by other syntax elements.
>   The value of
>  sps_seq_parameter_set_id shall be in the range of 0 to 15, inclusive.

Then I'm satisfied :-)

Thanks!

	Hans

> 
> Regards,
> Benjamin
> 
>>
>> Regards,
>>
>>     Hans
>>
>>> I mention rkvdec because that it is the only driver to use they (as far I knows)
>>>
>>> Regards,
>>> Benjamin
>>>
>>>> Regards,
>>>>
>>>>      Hans
>>>>
>>>>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>>>>> index 0e0ec2c61b80..b1a3dc05f02f 100644
>>>>> --- a/include/uapi/linux/v4l2-controls.h
>>>>> +++ b/include/uapi/linux/v4l2-controls.h
>>>>> @@ -2341,6 +2341,8 @@ enum v4l2_stateless_hevc_start_code {
>>>>>      struct v4l2_ctrl_hevc_sps {
>>>>>        /* ISO/IEC 23008-2, ITU-T Rec. H.265: Sequence parameter set */
>>>>> +    __u8    video_parameter_set_id;
>>>>> +    __u8    seq_parameter_set_id;
>>>>>        __u16    pic_width_in_luma_samples;
>>>>>        __u16    pic_height_in_luma_samples;
>>>>>        __u8    bit_depth_luma_minus8;
>>>>> @@ -2393,6 +2395,7 @@ struct v4l2_ctrl_hevc_sps {
>>>>>      struct v4l2_ctrl_hevc_pps {
>>>>>        /* ISO/IEC 23008-2, ITU-T Rec. H.265: Picture parameter set */
>>>>> +    __u8    pic_parameter_set_id;
>>>>>        __u8    num_extra_slice_header_bits;
>>>>>        __u8    num_ref_idx_l0_default_active_minus1;
>>>>>        __u8    num_ref_idx_l1_default_active_minus1;
>>>>> @@ -2487,6 +2490,8 @@ struct v4l2_ctrl_hevc_slice_params {
>>>>>        __u32    slice_segment_addr;
>>>>>        __u8    ref_idx_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
>>>>>        __u8    ref_idx_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
>>>>> +    __u16    short_term_ref_pic_set_size;
>>>>> +    __u16    long_term_ref_pic_set_size;
>>>>>          /* ISO/IEC 23008-2, ITU-T Rec. H.265: Weighted prediction parameter */
>>>>>        struct v4l2_hevc_pred_weight_table pred_weight_table;