Re: [PATCH v4 8/9] media: hevc: Add scaling matrix control

[Benjamin Gaignard]

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@xxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx/
> > 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!