[PATCH v12 13/15] drm/vkms: Create KUnit tests for YUV conversions

[Maxime Ripard]

On Fri, Oct 11, 2024 at 04:29:25PM +0200, Louis Chauvet wrote:
> On 11/10/24 - 12:49, Maxime Ripard wrote:
> > On Tue, Oct 08, 2024 at 11:23:22AM GMT, Louis Chauvet wrote:
> > > 
> > > Hi, 
> > > 
> > > > > + * The YUV color representation were acquired via the colour python framework.
> > > > > + * Below are the function calls used for generating each case.
> > > > > + *
> > > > > + * For more information got to the docs:
> > > > > + * https://colour.readthedocs.io/en/master/generated/colour.RGB_to_YCbCr.html
> > > > > + */
> > > > > +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)
> > > > > +	 */
> > > > 
> > > > We should really detail what the intent and expected outcome is supposed
> > > > to be here. Relying on a third-party python library call for
> > > > documentation isn't great.
> > >
> > > This was requested by Pekka in the [v2] of this series.
> > 
> > Ok.
> > 
> > > I can add something like this before each tests, but I think having the 
> > > exact python code used may help people to understand what should be the 
> > > behavior, and refering to the python code to understand the conversion.
> > 
> > Help, sure. Be the *only* documentation, absolutely not.
> > 
> > Let's turn this around. You run kunit, one of these tests fail:
> > 
> >  - It adds cognitive load to try to identify and make sense of an
> >    unknown lib.
> > 
> >  - How can we check that the arguments you provided there are the one
> >    you actually wanted to provide, and you didn't make a typo?
> > 
> > > I can add something like this before each tests to clarify the tested 
> > > case:
> > > 
> > > 	Test cases for conversion between YUV BT601 limited range and 
> > > 	RGB using the ITU-R BT.601 weights.
> > > 
> > > Or maybe just documenting the structure yuv_u8_to_argb_u16_case:
> > > 
> > > 	@encoding: Encoding used to convert RGB to YUV
> > > 	@range: Range used to convert RGB to YUV
> > > 	@n_colors: Count of test colors in this case
> > > 	@format_pair.name: Name used for this color conversion, used to 
> > > 			   clarify the test results
> > > 	@format_pair.rgb: RGB color tested
> > > 	@format_pair.yuv: Same color as @format_pair.rgb, but converted to 
> > > 			  YUV using @encoding and @range.
> > > 
> > > What do you think?
> > 
> > That it's welcome, but it still doesn't allow to figure out what your
> > intent was with this test 2 years from now.
> 
> I don't really understand what you want to add. Can you explain what you 
> expect here? Did you mean you want a description like this above the test 
> function?

I want, for each test case, to have a documentation of what case it's
testing and what the test should expect.

So, for the first one, something like:

/*
 * Test the conversion of full range, BT601-encoded, YUVXXX pixel to
 * ARGBXXXX, for various colors. This has been generated using:
 *
 * colour.RGB_to_YCbCR(...)
 */

And there's other things you need to document. Like, it seems that you
sometimes pass different values for in_legal and out_legal, and that's
not clear to me.

It also that you uses a matrix for NV12 but are converting a different
format. This needs to be documented.

Finally, You should be documented why you are checking that the colors
difference is less than 257, and not exactly equal.

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 273 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20241024/c7f90d4b/attachment.sig>