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

[Benjamin Gaignard <benjamin.gaignard@collabora.com>]

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!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel