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

[Pekka Paalanen]

On Fri, 7 Mar 2025 15:50:41 +0100
Louis Chauvet <louis.chauvet@xxxxxxxxxxx> wrote:

> 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.

Hi Maxime,

Louis' proposed comment is good and accurate. I can elaborate further on
it.

pixel_argb_u16 is an internal structure used only for temporary pixel
storage: the intermediate format. It's aim is to make computations on
pixel values easy: every input format is converted to it before
computations, and after computations it is converted to each output
format. This allows VKMS to implement computations, e.g. a matrix
operation, in simple code for only one cpu-endian "pixel format", the
intermediate format. (drm_fourcc.h has no cpu-endian formats at all,
and that is good.)

That VKMS never stores complete images in the intermediate format. To
strike a balance between temporary memory requirements and
computational overhead, VKMS processes images line-by-line. Only one
(or two) line's worth of pixels is needed to be kept in memory per
source or destination framebuffer at a time.

16-bit precision is required not just because some writeback and
framebuffer formats are 16-bit. We also need extra precision due to the
color value encoding. Transfer functions can convert pixel data between
the optical and electrical domains. Framebuffers usually contain
electrical domain data, because it takes less bits per pixel in order
to achieve a specific level of visual image quality (think of color
gradient banding). However, some computations, like color space
conversion with a matrix, must be done in the optical domain, which
requires more bits per pixel in order to not degrade the image quality.

In the future I would even expect needing 32-bit or even 64-bit per
channel precision in the intermediate format once higher-than-16 bits
per channel framebuffer formats require testing.

YUV can work with 8 bits per pixel for now, because in practice YUV is
always stored in electrical domain due its definition. YUV in optical
domain is simply never used. However, there are framebuffer formats
with more than 8 bits of YUV channels, so this may need extending too.


Thanks,
pq

Attachment: pgp9bKDCPOLNI.pgp
Description: OpenPGP digital signature