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