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

[Jernej Åkrabec]

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@xxxxxxxx> 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.

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