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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241024-illustrious-puma-of-superiority-d24a4d@houat>
Date: Thu, 24 Oct 2024 16:06:03 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Rodrigo Siqueira <rodrigosiqueiramelo@...il.com>, 
	Melissa Wen <melissa.srw@...il.com>, Maaara Canal <mairacanal@...eup.net>, 
	Haneen Mohammed <hamohammed.sa@...il.com>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, 
	Jonathan Corbet <corbet@....net>, Simona Vetter <simona@...ll.ch>, rdunlap@...radead.org, 
	arthurgrillo@...eup.net, pekka.paalanen@...oniitty.fi, 
	Simona Vetter <simona.vetter@...ll.ch>, dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, 
	linux-doc@...r.kernel.org, thomas.petazzoni@...tlin.com, jeremie.dautheribes@...tlin.com, 
	miquel.raynal@...tlin.com, seanpaul@...gle.com, marcheu@...gle.com, 
	nicolejadeyee@...gle.com, Pekka Paalanen <pekka.paalanen@...labora.com>
Subject: Re: [PATCH v12 13/15] drm/vkms: Create KUnit tests for YUV
 conversions

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

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)