Re: [PATCH v16 5/7] drm/vkms: Create KUnit tests for YUV conversions

[Maxime Ripard]

On Mon, Jan 27, 2025 at 11:48:23AM +0100, Louis Chauvet wrote:
> On 26/01/25 - 18:06, Maxime Ripard wrote:
> > On Tue, Jan 21, 2025 at 11:48:06AM +0100, Louis Chauvet wrote:
> > > +static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
> > > +	/*
> > > +	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
> > > +	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
> > > +	 *                     in_bits = 16,
> > > +	 *                     in_legal = False,
> > > +	 *                     in_int = True,
> > > +	 *                     out_bits = 8,
> > > +	 *                     out_legal = False,
> > > +	 *                     out_int = True)
> > > +	 *
> > > +	 * Test cases for conversion between YUV BT601 full range and RGB
> > > +	 * using the ITU-R BT.601 weights.
> > > +	 */
> > 
> > What are the input and output formats?
> > 
> > Ditto for all the other tests.
> 
> There is no really "input" and "output" format, they are reference values 
> for conversion, you should be able to use it in both direction. They are 
> generated by RGB_to_YCbCr (RGB input, YUV output) just because it was 
> easier to create the colors from RGB values.

RGB and YUV aren't formats, they are color models. XRGB8888 is a format.
NV12 is a format.

> If you think we should specify what is was used as input and output to 
> generate those values, I can modify the comment to:
> 
> 	Tests cases for color conversion generated by converting RGB 
> 	values to YUV BT601 full range using the ITU-R BT.601 weights.

My point is that those comments should provide a way to reimplement the
test from scratch, and compare to the actual implementation. It's useful
when you have a test failure and start to wonder if the implementation
or the test is at fault.

By saying only RGB and YUV, you can't possibly do that.

> Beside that modification, did you notice anything else on the series that 
> require more work before adding your Ack-by/Reviewed-by on the other 
> patches?

The rest looked good to me the last time I looked.

Maxime

Attachment: signature.asc
Description: PGP signature