Le jeudi 25 février 2021 à 10:09 -0300, Ezequiel Garcia a écrit : > Hi Benjamin, > > Thanks for the good work. > > On Mon, 2021-02-22 at 13:23 +0100, Benjamin Gaignard wrote: > > The H.265 ITU specification (section 7.4) define the general > > slice segment header semantics. > > Modified/added fields are: > > - video_parameter_set_id: (7.4.3.1) identifies the VPS for > > reference by other syntax elements. > > - seq_parameter_set_id: (7.4.3.2.1) specifies the value of > > the vps_video_parameter_set_id of the active VPS. > > - chroma_format_idc: (7.4.3.2.1) specifies the chroma sampling > > relative to the luma sampling > > - pic_parameter_set_id: (7.4.3.3.1) identifies the PPS for > > reference by other syntax elements > > - num_ref_idx_l0_default_active_minus1: (7.4.3.3.1) specifies > > the inferred value of num_ref_idx_l0_active_minus1 > > - num_ref_idx_l1_default_active_minus1: (7.4.3.3.1) specifies > > the inferred value of num_ref_idx_l1_active_minus1 > > - slice_segment_addr: (7.4.7.1) specifies the address of > > the first coding tree block in the slice segment > > - num_entry_point_offsets: (7.4.7.1) specifies the number of > > entry_point_offset_minus1[ i ] syntax elements in the slice header > > > > Add HEVC decode params contains the information used in section > > "8.3 Slice decoding process" of the specification to let the hardware > > perform decoding of a slices. > > > > Adapt Cedrus driver according to these changes. > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxxxxx> > > --- > > version 3: > > - Add documentation about the new structuers and fields. > > > > version 2: > > - remove all change related to scaling > > - squash commits to a coherent split > > - be more verbose about the added fields > > > > .../media/v4l/ext-ctrls-codec.rst | 126 +++++++++++++++--- > > .../media/v4l/vidioc-queryctrl.rst | 6 + > > drivers/media/v4l2-core/v4l2-ctrls.c | 26 +++- > > drivers/staging/media/sunxi/cedrus/cedrus.c | 6 + > > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 + > > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 + > > .../staging/media/sunxi/cedrus/cedrus_h265.c | 6 +- > > include/media/hevc-ctrls.h | 45 +++++-- > > 8 files changed, 186 insertions(+), 32 deletions(-) > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > index 00944e97d638..5e6d77e858c0 100644 > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > @@ -3109,6 +3109,15 @@ 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̀`` > > + - Specifies the value of the vps_video_parameter_set_id of the active > > VPS > > + * - __u8 > > + - ``chroma_format_idc`` > > + - Specifies the chroma sampling relative to the luma sampling > > None of these fields seem needed for the Hantro G2 driver, > so I suggest you drop them for now. You should be using chroma_format_idc in your validation. I think Hantro G2 does not do YUV 4:4:4, which is what chroma_format_idc tells the driver. Our implementation might also be missing 4:0:0 (black and white). As this value will aftect the buffer size, please keep them, and ideally validate them. Note that it feels rather problematic / unsafe situation as we endup having to trust userspace, hopefully the HW validates it's buffer size ? (do we tell the HW the buffer sizes ?) > > > * - __u16 > > - ``pic_width_in_luma_samples`` > > - > > @@ -3172,6 +3181,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - > > * - __u8 > > - ``chroma_format_idc`` > > - > > + * - __u8 > > + - ``num_slices`` > > + > > Not used, but also doesn't seem part of the SPS syntax. If we have to > pass the number of slices, we'll need another mechanism. > > > - > > * - __u64 > > - ``flags`` > > - See :ref:`Sequence Parameter Set Flags <hevc_sps_flags>` > > @@ -3231,9 +3243,18 @@ 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 > > Not used. > > > * - __u8 > > - ``num_extra_slice_header_bits`` > > - > > + * - __u8 > > + - ``num_ref_idx_l0_default_active_minus1`` > > + - Specifies the inferred value of num_ref_idx_l0_active_minus1 > > + * - __u8 > > + - ``num_ref_idx_l1_default_active_minus1`` > > + - Specifies the inferred value of num_ref_idx_l1_active_minus1 > > * - __s8 > > - ``init_qp_minus26`` > > - > > @@ -3342,6 +3363,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - > > * - ``V4L2_HEVC_PPS_FLAG_SLICE_SEGMENT_HEADER_EXTENSION_PRESENT`` > > - 0x00040000 > > - > > + * - ``V4L2_HEVC_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT`` > > + - 0x00080000 > > + - > > + * - ``V4L2_HEVC_PPS_FLAG_UNIFORM_SPACING`` > > + - 0x00100000 > > + - > > > > I suggest to do all the PPS control changes in a separate patch, > feels easier to review and cleaner as you can explain the > changes with more detail in the commit description. > > Looking at the PPS syntax for tiles, I'm wondering if these > deserve their own control, which would be used if tiles are enabled, > i.e. V4L2_HEVC_PPS_FLAG_TILES_ENABLED is set. > > __u8 > num_tile_columns_minus1; > __u8 > num_tile_rows_minus1; > __u8 > column_width_minus1[20]; > __u8 row_height_minus1[22]; > > Not something we necessarily have to tackle now. > > > ``V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS (struct)`` > > Specifies various slice-specific parameters, especially from the NAL > > unit > > @@ -3366,6 +3393,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - > > * - __u32 > > - ``data_bit_offset`` > > - Offset (in bits) to the video data in the current slice data. > > + * - __u32 > > + - ``slice_segment_addr`` > > + - Specifies the address of the first coding tree block in the slice > > segment > > Not used. > > > + * - __u32 > > + - ``num_entry_point_offsets`` > > + - Specifies the number of entry_point_offset_minus1[ i ] syntax > > elements in the slice header > > Not used. > > > * - __u8 > > - ``nal_unit_type`` > > - > > @@ -3422,28 +3455,20 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - > > * - __u8 > > - ``pic_struct`` > > - > > - * - __u8 > > - - ``num_active_dpb_entries`` > > - - The number of entries in ``dpb``. > > Need to explain in the commit description why this field is moved. > > > * - __u8 > > - ``ref_idx_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]`` > > - The list of L0 reference elements as indices in the DPB. > > * - __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`` > > + > > Not used. > > > - > > + * - __u16 > > + - ``long_term_ref_pic_set_size`` > > + - > > Not used. > > > * - __u8 > > - - ``num_rps_poc_st_curr_before`` > > - - The number of reference pictures in the short-term set that come > > before > > - the current frame. > > If this matches NumPocStCurrBefore from section 8.3.2 "Decoding process for > reference picture set" > then I would document that. And perhaps rename it to num_poc_st_curr_before. > > > - * - __u8 > > - - ``num_rps_poc_st_curr_after`` > > - - The number of reference pictures in the short-term set that come > > after > > - the current frame. > > Ditto. > > > - * - __u8 > > - - ``num_rps_poc_lt_curr`` > > - - The number of reference pictures in the long-term set. > > Ditto. > > Also, I'd like the changes that move fields from > V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS > to the new V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS control, to be in their > patch. > > That will allow us to put in the commit description a proper > explanation of why are fields being moved. Nothing fancy, simply > explaining that these variables come from section 8.3.2 > "Decoding process for reference picture set", which describes > a process invoked once per picture, so they are not per-slice. > > > - * - __u8 > > - - ``padding[7]`` > > + - ``padding`` > > - Applications and drivers must set this to zero. > > * - struct :c:type:`v4l2_hevc_dpb_entry` > > - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]`` > > @@ -3646,3 +3671,74 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - > > so this has to come from client. > > This is applicable to H264 and valid Range is from 0 to 63. > > Source Rec. ITU-T H.264 (06/2019); G.7.4.1.1, G.8.8.1. > > + > > +``V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS (struct)`` > > + Specifies various decode parameters, especially the references picture > > order > > + count (POC) for all the lists (short, long, before, current, after) and > > the > > + number of entries for each of them. > > + These parameters are defined according to :ref:`hevc`. > > + They are described in section 8.3 "Slice decoding process" of the > > + specification. > > + > > +.. c:type:: v4l2_ctrl_hevc_decode_params > > + > > +.. cssclass:: longtable > > + > > +.. flat-table:: struct v4l2_ctrl_hevc_decode_params > > + :header-rows: 0 > > + :stub-columns: 0 > > + :widths: 1 1 2 > > + > > + * - __s32 > > + - ``pic_order_cnt_val`` > > + - > > Can be documented as: > > """ > PicOrderCntVal as described in section 8.3.1 "Decoding process > for picture order count" of the specification. > """ > > Note that snake case is used to match the kernel style, > but other than that we try to keep the HEVC spec variable > names. > > > + * - __u8 > > + - ``num_active_dpb_entries`` > > + - The number of entries in ``dpb``. > > + * - struct :c:type:`v4l2_hevc_dpb_entry` > > + - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]`` > > + - The decoded picture buffer, for meta-data about reference frames. > > The DPB is here, but it seems it's also in the slice control? > > > + * - __u8 > > + - ``num_rps_poc_st_curr_before`` > > + - The number of reference pictures in the short-term set that come > > before > > + the current frame. > > + * - __u8 > > + - ``num_rps_poc_st_curr_after`` > > + - The number of reference pictures in the short-term set that come > > after > > + the current frame. > > + * - __u8 > > + - ``num_rps_poc_lt_curr`` > > + - The number of reference pictures in the long-term set. > > + * - __u8 > > + - ``rps_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]`` > > + - > > + * - __u8 > > + - ``rps_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]`` > > + - > > + * - __u8 > > + - ``rps_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]`` > > + - > > Could you document these as well? > > Thanks a lot, > Ezequiel > >
