Message-ID: <ce5fb86d-f3bc-4196-9cfd-8af41a83beb1@bootlin.com>
Date: Wed, 19 Feb 2025 14:35:14 +0100
From: Louis Chauvet <louis.chauvet@...tlin.com>
To: Maxime Ripard <mripard@...nel.org>,
 Rodrigo Siqueira <rodrigosiqueiramelo@...il.com>,
 Melissa Wen <melissa.srw@...il.com>, Maíra 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>,
 rdunlap@...radead.org, arthurgrillo@...eup.net,
 Jonathan Corbet <corbet@....net>, pekka.paalanen@...oniitty.fi,
 Simona Vetter <simona@...ll.ch>, Simona Vetter <simona.vetter@...ll.ch>,
 dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
 jeremie.dautheribes@...tlin.com, miquel.raynal@...tlin.com,
 thomas.petazzoni@...tlin.com, seanpaul@...gle.com, marcheu@...gle.com,
 nicolejadeyee@...gle.com, linux-doc@...r.kernel.org,
 Pekka Paalanen <pekka.paalanen@...labora.com>
Subject: Re: [PATCH v16 5/7] drm/vkms: Create KUnit tests for YUV conversions



Le 19/02/2025 à 11:15, Maxime Ripard a écrit :
> On Wed, Feb 05, 2025 at 04:32:07PM +0100, Louis Chauvet wrote:
>> On 05/02/25 - 09:55, Maxime Ripard wrote:
>>> 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.
>>
>> I understand your concern, but I believe there might be a slight
>> misunderstanding. The table in question stores reference values for
>> specific color models, not formats. Therefore, it doesn't specify any
>> particular format like XRGB8888 or NV12.
>>
>> To clarify this, I can rename the format_pair struct to value_pair. This
>> should make it clearer that we are dealing with color model values rather
>> than formats.
>>
>> If you want to test a specific format conversion, such as
>> YUV420_to_argbu16, you would need to follow a process like this:
>>
>> 	// Recreate a YUV420 data
>> 	plane_1[0] = test_case.yuv.y
>> 	plane_2[0] = test_case.yuv.u
>> 	plane_2[1] = test_case.yuv.v
>>
>> 	// convertion to test from YUV420 format to argb_u16
>> 	rgb_u16 = convert_YUV420_to_argbu16(plane_1, plane_2)
>>
>> 	// ensure the conversion is valid
>> 	assert_eq(rgb_u16, test_case.rgb)
>>
>> The current test is not performing this kind of format conversion.
>> Instead, it verifies that for given (y, u, v) values, the correct (r, g,
>> b, a) values are obtained.
> 
> You already stated that you check for the A, R, G, and B components. On
> how many bits are the values you are comparing stored? The YUV values
> you are comparing are stored on how many bits for each channel? With
> subsampling?
> 
> If you want to compare values, you need to encode a given color into
> bits, and the way that encoding is done is what the format is about.
> 
> You might not compare the memory layout but each component individually,
> but it's still a format.

Sorry, I think I misunderstood what a format really is. But even with 
this explanation, I don't understand well what you ask me to change. Is 
this better:

The values are computed by converting RGB values, with each component 
stored as u16, to YUV values, with each component stored as u8. The 
conversion is done from RGB full range to YUV BT601 full range using the 
ITU-R BT.601 weights.

TBH, I do not understand what you are asking for exactly. Can you please 
give the sentence you expect directly?

Thanks,
Louis Chauvet

> And then, you have the extra fun on top, like are you comparing
> full-range or limited-range colors?
> 
>> In other words, it tests color model conversion, not format conversion.
> 
> No, you are testing color encoding, format and model conversions, all at
> once.
> 
> Maxime

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists