Bug 221443 - Generalize color conversion code to reduce number of overloads required
Summary: Generalize color conversion code to reduce number of overloads required
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-04 18:24 PST by Sam Weinig
Modified: 2021-02-05 13:09 PST (History)
14 users (show)

See Also:


Attachments
Patch (62.69 KB, patch)
2021-02-04 19:35 PST, Sam Weinig
darin: review+
Details | Formatted Diff | Diff
Patch (62.93 KB, patch)
2021-02-05 12:26 PST, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2021-02-04 18:24:31 PST
"Generalize color conversion code to reduce number of overloads required"
Comment 1 Sam Weinig 2021-02-04 19:35:40 PST
Created attachment 419355 [details]
Patch
Comment 2 Sam Weinig 2021-02-04 20:40:38 PST
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.
Comment 3 Darin Adler 2021-02-05 11:46:01 PST
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?
Comment 4 Sam Weinig 2021-02-05 12:23:49 PST
(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.
Comment 5 Sam Weinig 2021-02-05 12:26:56 PST
Created attachment 419445 [details]
Patch
Comment 6 EWS 2021-02-05 13:08:41 PST
Committed r272436: <https://trac.webkit.org/changeset/272436>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419445 [details].
Comment 7 Radar WebKit Bug Importer 2021-02-05 13:09:23 PST
<rdar://problem/74037844>