Re: [igt-dev] [RFC PATCH 5/7] igt/color: Add SW color transform functionality

[Harry Wentland <harry.wentland@amd.com>]

On 2023-09-18 04:02, Pekka Paalanen wrote:
> On Fri, 15 Sep 2023 15:50:52 -0400
> Harry Wentland <harry.wentland@amd.com> wrote:
> 
>> On 2023-09-15 10:52, Pekka Paalanen wrote:
>>> On Fri, 8 Sep 2023 11:03:13 -0400
>>> Harry Wentland <harry.wentland@amd.com> wrote:
>>>   
>>>> In order to test color we want to compare a HW (KMS) transform
>>>> with a SW transform. This introduces color transform for an
>>>> sRGB EOTF but this can be extended to other transforms. Code is
>>>> borrowed from Skia.
>>>>
>>>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>>>> Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
>>>> Cc: Simon Ser <contact@emersion.fr>
>>>> Cc: Harry Wentland <harry.wentland@amd.com>
>>>> Cc: Melissa Wen <mwen@igalia.com>
>>>> Cc: Jonas Ådahl <jadahl@redhat.com>
>>>> Cc: Sebastian Wick <sebastian.wick@redhat.com>
>>>> Cc: Shashank Sharma <shashank.sharma@amd.com>
>>>> Cc: Alexander Goins <agoins@nvidia.com>
>>>> Cc: Joshua Ashton <joshua@froggi.es>
>>>> Cc: Michel Dänzer <mdaenzer@redhat.com>
>>>> Cc: Aleix Pol <aleixpol@kde.org>
>>>> Cc: Xaver Hugl <xaver.hugl@gmail.com>
>>>> Cc: Victoria Brekenfeld <victoria@system76.com>
>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>> Cc: Uma Shankar <uma.shankar@intel.com>
>>>> Cc: Naseer Ahmed <quic_naseer@quicinc.com>
>>>> Cc: Christopher Braga <quic_cbraga@quicinc.com>
>>>> ---
>>>>  lib/igt_color.c | 330 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  lib/igt_color.h | 105 +++++++++++++++
>>>>  lib/igt_fb.c    |   6 +-
>>>>  lib/igt_fb.h    |   2 +
>>>>  lib/meson.build |   1 +
>>>>  5 files changed, 441 insertions(+), 3 deletions(-)
>>>>  create mode 100644 lib/igt_color.c
>>>>  create mode 100644 lib/igt_color.h
> 
> ...
> 
>>>> +/*
>>>> + * A transfer function mapping encoded values to linear values,
>>>> + * represented by this 7-parameter piecewise function:
>>>> + *
>>>> + *   linear = sign(encoded) *  (c*|encoded| + f)       , 0 <= |encoded| < d
>>>> + *          = sign(encoded) * ((a*|encoded| + b)^g + e), d <= |encoded|  
>>>
>>> The code you have does not actually do the extended form (use of sign
>>> and absolute value), but clamps the result to [0.0, 1.0].
>>>   
>>
>> Yes, the intention (and I didn't write this but copied it) is to be able to use
>> this to describe most (all?) standard transfer functions. See
>> https://github.com/google/skia/blob/main/include/core/SkColorSpace.h
> 
> My complaint is about doc and implementation disagreeing.
> 
> xvYCC and integer-encoded scRGB would need an extended range if anyone
> wanted to test those, if they even fit here. I also do not understand
> how it can be possible to express PQ EOTF or HLG OETF with this
> parameterisation. Maybe it's an approximation on a very limited domain?
> 

I have a feeling Skia maps everything to [0.0, 1.0] and agree that this
wouldn't work in the cases you mention (PQ would work with an additional
scaling of the values to 125.0).

What's here seems to work fine for sRGB but I'll have to hook this all up
to amdgpu and test both sRGB and PQ at the very least.

Harry

> 
> Thanks,
> pq
> 
>>>> + *
>>>> + * (A simple gamma transfer function sets g to gamma and a to 1.)
>>>> + */
>>>> +struct igt_color_tf {
>>>> +    float g, a,b,c,d,e,f;
>>>> +};
>>>> +
>>>> +const struct igt_color_tf srgb_tf = {2.4f, (float)(1/1.055), (float)(0.055/1.055), (float)(1/12.92), 0.04045f, 0, 0};
>>>> +
>>>> +/* end of Skia-based code */
>>>> +
>>>> +typedef struct igt_pixel {
>>>> +	float r;
>>>> +	float g;
>>>> +	float b;
>>>> +} igt_pixel_t;  
>>>
>>> Thanks,
>>> pq  
>>
>