On 05/02/25 - 09:55, Maxime Ripard wrote:

On Mon, Jan 27, 2025 at 11:48:23AM +0100, Louis Chauvet wrote:

quoted

On 26/01/25 - 18:06, Maxime Ripard wrote:

quoted

On Tue, Jan 21, 2025 at 11:48:06AM +0100, Louis Chauvet wrote:

quoted

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

quoted

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

quoted

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