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

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 9391 bytes --]

Hi,

On Mon, Mar 10, 2025 at 11:12:59AM +0200, Pekka Paalanen wrote:
> On Fri, 7 Mar 2025 15:50:41 +0100
> Louis Chauvet <louis.chauvet@bootlin.com> 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 for your explanations, and yes Louis, I think it's in a much
better shape with your suggestion.

We'd still some additional info like whether you're testing limited vs
full range, but it's most likely going to be on a per-test basis.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]