Message-ID: <6fa7a17f-3932-4b93-a3c7-885619f8ec73@bootlin.com>
Date: Fri, 7 Mar 2025 15:50:41 +0100
From: Louis Chauvet <louis.chauvet@...tlin.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: 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 07/03/2025 à 11:20, Maxime Ripard a écrit :
> On Wed, Feb 19, 2025 at 02:35:14PM +0100, Louis Chauvet wrote:
>>
>>
>> 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.
> 
> Ultimately, a format is how a given "color value" is stored. How many
> bits will you use? If you have an unaligned number of bits, how many
> bits of padding you'll use, where the padding is? If there's multiple
> bytes, what's the endianness?
> 
> The answer to all these questions is "the format", and that's why
> there's so many of them.

Thanks!

>> 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?
> 
> The fourcc[1] code for the input and output format would be nice. And if
> you can't, an ad-hoc definition of the format, answering the questions I
> mentionned earlier (and in the previous mail for YUV).

I don't think any fourcc code will apply in this case, the tests use 
internal VKMS structures pixel_argb_16 and pixel_yuv_u8. How do I 
describe them better? If I add this comment for the structures, is it 
enough?

/**
  * struct pixel_argb_u16 - Internal representation of a pixel color.
  * @r: Red component value, stored in 16 bits, without padding, using
  *     machine endianness
  * @b: [...]
  *
  * The goal of this structure is to keep enough precision to ensure
  * correct composition results in VKMS and simplifying color
  * manipulation by splitting each component into its own field.
  * Caution: the byte ordering of this structure is machine-dependent,
  * you can't cast it directly to AR48 or xR48.
  */
struct pixel_argb_u16 {
	u16 a, r, g, b;
};

(ditto for pixel_yuv_u8)

> I'm really
> surprised about the RGB component values being stored on 16 bits though.
> It's super unusual, to the point where it's almost useless for us to
> test, and we should probably use 8 bits values.

We need to have 16 bits because some of the writeback formats are 16 bits.

Louis Chauvet

> Maxime
> 
> 1: https://elixir.bootlin.com/linux/v6.13.5/source/include/uapi/drm/drm_fourcc.h#L486

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

Powered by blists - more mailing lists