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@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 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:
signature.asc
Description: PGP signature
