Bug 221443 – Generalize color conversion code to reduce number of overloads required

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

"Generalize color conversion code to reduce number of overloads required"

[Sam Weinig]

Created attachment 419355 [details]
Patch

[Sam Weinig]

I went with a struct rather than using function template specialization because in various places I plan to use partial specialization which function template specialization does not support.

If I don't end up actually using partial specialization, I will probably simplify things a bit by replacing:

      template<> struct ColorConversion<Output, Input> {
          Output convert(const Input& color) { ... }
      };

with

      template<> Output convertColor<Output, Input>(const Input&);

directly. But for now, the struct gives me some freedom.

[Darin Adler]

Comment on attachment 419355 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419355&action=review

> Source/WebCore/platform/graphics/ColorConversion.h:342
> +// FIXME: Reduce total number of matrix tranforms by contatentaing the matrices

Typo in "concatenating"

> Source/WebCore/platform/graphics/ColorConversion.h:355
> +// Steps 2, 3, and 4 can be combined into one single matrix if we linearly
> +// concatented the three matrices. To do this, we will have to tag which conversions
> +// are matrix based, expose the matrices, add support for constexpr concatenting
> +// of ColorMatrix and find a way to merge the conversions.

Will we get slightly numerically different results when we concatenate matrices instead of doing multiple transformations? (I love floating point.)

> Source/WebCore/rendering/RenderTheme.cpp:1422
> +    // FIXME: Consider using LCHA<float> rather than HSLA<float> for better perceptual results.

Sure seems like a great idea.

> Source/WebCore/rendering/RenderTheme.cpp:1429
> +    // FIXME: Consider converting back to the initial underlying color type rather than hardcoding SRGBA<float> here.

This one is not quite as obvious a choice. Clearly SRGBA might have insufficient gamut to cover the color. Not perfectly clear what the benefits are of preserving color space further through the pipeline. Better rendering? Better performance? Avoiding unnecessarily limited gamut or loss of precision?

[Sam Weinig]

(In reply to Darin Adler from comment #3)
> Comment on attachment 419355 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419355&action=review
> 
> > Source/WebCore/platform/graphics/ColorConversion.h:342
> > +// FIXME: Reduce total number of matrix tranforms by contatentaing the matrices
> 
> Typo in "concatenating"
> 
> > Source/WebCore/platform/graphics/ColorConversion.h:355
> > +// Steps 2, 3, and 4 can be combined into one single matrix if we linearly
> > +// concatented the three matrices. To do this, we will have to tag which conversions
> > +// are matrix based, expose the matrices, add support for constexpr concatenting
> > +// of ColorMatrix and find a way to merge the conversions.
> 
> Will we get slightly numerically different results when we concatenate
> matrices instead of doing multiple transformations? (I love floating point.)

We likely will, but in practice, our numerical results are not 100% the same as even CoreGraphics at the moment, and we use much higher precision, 32 bit-float vs the specced minimum of 16-bit float, so the actual rendered results are not in practice going to change. That said, this is something we will want to figure out and ensure we have good test coverage for.

> 
> > Source/WebCore/rendering/RenderTheme.cpp:1422
> > +    // FIXME: Consider using LCHA<float> rather than HSLA<float> for better perceptual results.
> 
> Sure seems like a great idea.
> 
> > Source/WebCore/rendering/RenderTheme.cpp:1429
> > +    // FIXME: Consider converting back to the initial underlying color type rather than hardcoding SRGBA<float> here.
> 
> This one is not quite as obvious a choice. Clearly SRGBA might have
> insufficient gamut to cover the color. Not perfectly clear what the benefits
> are of preserving color space further through the pipeline. Better
> rendering? Better performance? Avoiding unnecessarily limited gamut or loss
> of precision?

That's a valid point. I guess what I am really getting at here is that if we switch to LCH for the luminance change, it doesn't really make sense to clamp back down to sRGB. If say, the input was DisplayP3, and we changed the luminance a little, clamping to sRGB seems not ideal. But, it wouldn't have to be the original color space at all, we could just keep it in LCHA.

[Sam Weinig]

Created attachment 419445 [details]
Patch

[EWS]

Committed r272436: <https://trac.webkit.org/changeset/272436>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419445 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74037844>