Re: [PATCH v2 1/4] media: uapi: hevc: Add scaling matrix control

[Hans Verkuil <hverkuil@xs4all.nl>]

On 1/9/20 4:17 PM, Jernej Škrabec wrote:
> Hi!
> 
> Dne sreda, 08. januar 2020 ob 15:43:36 CET je Paul Kocialkowski napisal(a):
>> Hi Mauro,
>>
>> On Wed 08 Jan 20, 15:11, Mauro Carvalho Chehab wrote:
>>> Em Fri, 13 Dec 2019 17:04:25 +0100
>>>
>>> Jernej Skrabec <jernej.skrabec@siol.net> escreveu:
>>>> HEVC has a scaling matrix concept. Add support for it.
>>>>
>>>> +struct v4l2_ctrl_hevc_scaling_matrix {
>>>> +	__u8	scaling_list_4x4[6][16];
>>>> +	__u8	scaling_list_8x8[6][64];
>>>> +	__u8	scaling_list_16x16[6][64];
>>>> +	__u8	scaling_list_32x32[2][64];
>>>> +	__u8	scaling_list_dc_coef_16x16[6];
>>>> +	__u8	scaling_list_dc_coef_32x32[2];
>>>> +};
>>>
>>> I never looked at HEVC spec, but the above seems really weird.
>>>
>>> Please correct me if I am wrong, but each of the above matrixes
>>> is independent, and the driver will use just one of the above on
>>> any specific time (for a given video output node), right?
>>
>> I am not too sure about what the specification really entails, but it is my
>> understanding that HEVC allows simultaneous block sizes between 4x4 and
>> 32x32 to exist within the same coding tree and slice. That suggests that it
>> makes sense to have specific coefficients for each case.
> 
> Specs ITU-T REC. H.265 (06/2019), chapter 7.3.4 shows that multiple different 
> matrices can be present at the same time. If they are not, default values 
> should be used instead. But in general, more than one can be needed at the 
> same time.
> 
> Only real question is if default values should be also provided by userspace 
> or by kernel. Since place has to be reserved for all different scaling lists 
> anyway, we won't save any space by providing default values in kernel. Cedrus 
> VPU has only bit switch for using default values for all matrices at the same 
> time or all custom.
> 
> Note that this control contains slightly processed data. Frame has stored 
> these matrices in form of deltas. But because this is the only driver that use 
> this structure I have no idea what is the most proper form of this data (raw 
> values or deltas). That's why this will stay in staging using private headers 
> until we figure this out.

This definitely needs to be documented! Otherwise this will be forgotten.

Regards,

	Hans

> 
> Best regards,
> Jernej
> 
>>
>> Note that the hardware also has distinct registers for each scaling list.
>>
>> Cheers,
>>
>> Paul
>>
>>> If so, why would userspace be forced to update lots of matrixes, if would
>>> likely use just one at a given time?
>>>
>>> IMO, the proper way would be, instead, to use an uAPI like:
>>>
>>> /*
>>>
>>>  * Actually, as this is uAPI, we will use a fixed size integer type, like
>>>  *  unsigned int
>>>  */
>>>
>>> enum hevc_scaling_matrix_type {
>>>
>>> 	HEVC_SCALING_MATRIX_4x4,
>>> 	HEVC_SCALING_MATRIX_8x8,
>>>
>>> ...
>>>
>>> 	HEVC_SCALING_MATRIX_DC_COEF_32x32,
>>>
>>> };
>>>
>>> struct v4l2_ctrl_hevc_scaling_matrix {
>>>
>>> 	__u32	scaling_type 		/* as defined by enum 
> hevc_scaling_matrix_type */
>>> 	
>>> 	union {
>>> 	
>>> 		__u8	scaling_list_4x4[6][16];
>>> 		__u8	scaling_list_8x8[6][64];
>>> 		__u8	scaling_list_16x16[6][64];
>>> 		__u8	scaling_list_32x32[2][64];
>>> 		__u8	scaling_list_dc_coef_16x16[6];
>>> 		__u8	scaling_list_dc_coef_32x32[2];
>>> 	
>>> 	};
>>>
>>> };
>>>
>>> And let the core use a default for each scaling matrix, if userspace
>>> doesn't set it.
>>>
>>>
>>>
>>> Cheers,
>>> Mauro
> 
> 
> 
>