Dne četrtek, 25. februar 2021 ob 18:34:48 CET je Ezequiel Garcia napisal(a): > Hey Jernej, > > On Thu, 2021-02-25 at 18:01 +0100, Jernej Škrabec wrote: > > Hi Ezequiel, > > > > Dne četrtek, 25. februar 2021 ob 14:09:52 CET je Ezequiel Garcia napisal(a): > > > 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. > > > > > > > * - __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. > > > > While above two fields may not be used in Hantro, they are for sure useful for > > Cedrus and RPi4. I would like to keep them, otherwise with such approach HEVC > > will stay in staging for a long time. I'm still baffled why scaling matrix > > control was dropped. It would fit well in Cedrus and RPi4 driver and after a > > quick look, it seems that it was used in driver in later patch. > > > > I'd like to make sure each modification we are making to the uAPI > goes in the right direction, that is in the direction of moving > the API out of staging. > > Since reviewing each field is quite hard, and opens some discussions, > I wanted to keep this patchset specific to what's needed for Hantro G2. > > The Scaling matrix control is certainly a good one, as well as the ones > needed for Cedrus and RPi4. However, I feel it's better to discuss > them in their own "uAPI review" series so we can review all the changes > with an API hat. > > This way we decouple the Hantro G2 discussion and work from the API work. > > Also please feel free to submit RFC patches fo Cedrus and RPi4 > (API and driver changes). We can certainly start the discussion around that, > with driver changes in context. I don't know much about RPi4 driver, only few implementation details, so you'll have to ping developer who wrote it. Regarding HEVC on Cedrus - it has one pain point - it needs entry point table which in turn needs support for variable arrays in order to be feasable AFAIK. I don't plan to develop that. Patches for scaling matrix and segment address were sent a bit more than a year ago but were turned down because they change control structures (among other things). Sorry to say, but I work on other things now, so Cedrus will have to wait. Alternatively, someone can take my patches from LibreELEC, update and submit them. They are in use for a long time. Best regards, Jernej > > Hope I'm making sense here :) > > Thanks, > Ezequiel > > > Best regards, > > Jernej > > > > > > > > > * - __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 > > > > > > > > > > > > >
