Messages in this thread | | | Subject | Re: [PATCH v4 8/9] media: hevc: Add scaling matrix control | From | Benjamin Gaignard <> | Date | Thu, 15 Jul 2021 11:43:44 +0200 |
| |
Le 13/07/2021 à 00:21, Ezequiel Garcia a écrit : > Hi Benjamin, > > I believe the scaling matrix uAPI patch(es) and the support > in Hantro G2 could be moved to its own series and maybe > merged sooner than the rest (which may need more discussion). > > Couple remarks below. > > On Fri, 2021-06-25 at 16:11 +0200, Benjamin Gaignard wrote: >> HEVC scaling lists are used for the scaling process for transform >> coefficients. >> V4L2_HEVC_SPS_FLAG_SCALING_LIST_ENABLED has to set when they are >> encoded in the bitstream. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> Note: V4L2_HEVC_SPS_FLAG_SCALING_LIST_ENABLED is not change by this >> patch. There is a thread about the naming/usage of this flag here: >> https://lore.kernel.org/linux-arm-kernel/20210610154442.806107-8-benjamin.gaignard@collabora.com/ >> but that doesn't concern the scaling matrix control by itself. >> > If I am reading the spec correctly, we have fields in > SPS (scaling_list_enabled_flag, scaling_list_data_present_flag) > and PPS (scaling_list_data_present_flag). > > We don't need all that, since all a driver needs to know > is whether a scaling list is to be applied for the current frame. > > Would you mind adding a patch moving the flag to the PPS?
Extract from the spec: - scaling_list_enabled_flag equal 1 specifies that a scaling list is used for scaling process for transform coefficients. - sps_scaling_list_data_present_flag equal to 1 specifies that the scaling_list_data( ) syntax structure is present in the SPS. - pps_scaling_list_data_present_flag equal to 1 specifies that parameters are present in the PPS to modify the scaling lists specified by the active SPS.
For me V4L2_HEVC_SPS_FLAG_SCALING_LIST_ENABLED represent scaling_list_enabled_flag while the other are used to build the scaling lists values. So it is good named from my point of view.
> >> version 2: >> - Fix structure name in ext-ctrls-codec.rst >> >> .../media/v4l/ext-ctrls-codec.rst | 45 +++++++++++++++++++ >> .../media/v4l/vidioc-queryctrl.rst | 6 +++ >> drivers/media/v4l2-core/v4l2-ctrls-core.c | 6 +++ >> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 4 ++ >> include/media/hevc-ctrls.h | 11 +++++ >> 5 files changed, 72 insertions(+) >> >> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >> index dc096a5562cd..3865acb9e0fd 100644 >> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >> @@ -3071,6 +3071,51 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - >> >> \normalsize >> >> +``V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX (struct)`` >> + Specifies the HEVC scaling matrix parameters used for the scaling process >> + for transform coefficients. >> + These matrix and parameters are defined according to :ref:`hevc`. >> + They are described in section 7.4.5 "Scaling list data semantics" of >> + the specification. >> + > This needs some additional documentation about the order of the lists. > See the docs that we've added for the scaling_list_{} fields in > V4L2_CID_STATELESS_H264_SCALING_MATRIX.
I will do dedicated patches for scaling lists feature and add that.
Benjamin
> > Thanks!
| |