On Fri, Oct 11, 2024 at 04:29:25PM +0200, Louis Chauvet wrote: > On 11/10/24 - 12:49, Maxime Ripard wrote: > > On Tue, Oct 08, 2024 at 11:23:22AM GMT, Louis Chauvet wrote: > > > > > > Hi, > > > > > > > > + * The YUV color representation were acquired via the colour python framework. > > > > > + * Below are the function calls used for generating each case. > > > > > + * > > > > > + * For more information got to the docs: > > > > > + * https://colour.readthedocs.io/en/master/generated/colour.RGB_to_YCbCr.html > > > > > + */ > > > > > +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) > > > > > + */ > > > > > > > > We should really detail what the intent and expected outcome is supposed > > > > to be here. Relying on a third-party python library call for > > > > documentation isn't great. > > > > > > This was requested by Pekka in the [v2] of this series. > > > > Ok. > > > > > I can add something like this before each tests, but I think having the > > > exact python code used may help people to understand what should be the > > > behavior, and refering to the python code to understand the conversion. > > > > Help, sure. Be the *only* documentation, absolutely not. > > > > Let's turn this around. You run kunit, one of these tests fail: > > > > - It adds cognitive load to try to identify and make sense of an > > unknown lib. > > > > - How can we check that the arguments you provided there are the one > > you actually wanted to provide, and you didn't make a typo? > > > > > I can add something like this before each tests to clarify the tested > > > case: > > > > > > Test cases for conversion between YUV BT601 limited range and > > > RGB using the ITU-R BT.601 weights. > > > > > > Or maybe just documenting the structure yuv_u8_to_argb_u16_case: > > > > > > @encoding: Encoding used to convert RGB to YUV > > > @range: Range used to convert RGB to YUV > > > @n_colors: Count of test colors in this case > > > @format_pair.name: Name used for this color conversion, used to > > > clarify the test results > > > @format_pair.rgb: RGB color tested > > > @format_pair.yuv: Same color as @format_pair.rgb, but converted to > > > YUV using @encoding and @range. > > > > > > What do you think? > > > > That it's welcome, but it still doesn't allow to figure out what your > > intent was with this test 2 years from now. > > I don't really understand what you want to add. Can you explain what you > expect here? Did you mean you want a description like this above the test > function? I want, for each test case, to have a documentation of what case it's testing and what the test should expect. So, for the first one, something like: /* * Test the conversion of full range, BT601-encoded, YUVXXX pixel to * ARGBXXXX, for various colors. This has been generated using: * * colour.RGB_to_YCbCR(...) */ And there's other things you need to document. Like, it seems that you sometimes pass different values for in_legal and out_legal, and that's not clear to me. It also that you uses a matrix for NV12 but are converting a different format. This needs to be documented. Finally, You should be documented why you are checking that the colors difference is less than 257, and not exactly equal. Maxime
Attachment:
signature.asc
Description: PGP signature
