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. In other words, it tests color model
conversion, not format conversion.
Do you think I need to change something in this test? If so, can you
explain what kind of unit test you are expecting.
Thanks,
Louis Chauvet
> > Beside that modification, did you notice anything else on the series that
> > require more work before adding your Ack-by/Reviewed-by on the other
> > patches?
>
> The rest looked good to me the last time I looked.
>
> Maxime
