Bug 212396 – Extended Color: Experiment with strongly typed ColorComponents

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

[Sam Weinig]

Extended Color: Experiment with strongly typed FloatComponents

[Sam Weinig]

Created attachment 400296 [details]
Patch

[Sam Weinig]

Posting my proof-of-concept for strongly typed FloatComponents. 

- Separate types for SRGBFloatComponents, DisplayP3FloatComponents, etc. (uses templates so that they all are implemented using template<> FloatComponents).

- ExtendedColor now has a Variant<SRGBFloatComponents, DisplayP3FloatComponents> (didn't add LinearSRGB yet since it's just a proof-of-concept). Currently very explicit about using switchOn() to visit each type even if they are identical to show what it would it would look like with color spaces that were no quite as similar as sRGB and P3 are.

- Conversion from one to the other can now have more simplified names, e.g. toSRGB() since the from is implicit in the type.


Curious to know what others thoughts on this are.

[Darin Adler]

Comment on attachment 400296 [details]
Patch

I think that the typed components should have a member that contains the untyped components; that member can be sort of like a tuple. Some functions can take the typed components. Others can take untyped. This could let us share as much code as we like and write short forwarding functions rather than writing out the logic.

[Darin Adler]

Generally type is a powerful tool for helping us get this right.

[Sam Weinig]

(In reply to Darin Adler from comment #3)
> Comment on attachment 400296 [details]
> Patch
> 
> I think that the typed components should have a member that contains the
> untyped components; that member can be sort of like a tuple. Some functions
> can take the typed components. Others can take untyped. This could let us
> share as much code as we like and write short forwarding functions rather
> than writing out the logic.

They essentially have that already since each of them wraps a std::array<float, 4>. (Note, I think we should probably switch this to a WTF based simd wrapper at somepoint, using simd::float4 on Darwin at least would actually simplify things here and model things better).

[Darin Adler]

Another thought: I suppose the layers of meaning would be:

1) just a bunch of floats
2) one of these is alpha and the rest are components in some color space
3) here's what color space

Not sure if there’s much code that could be shared in (2).

Also is there some kind of thing where the floats are known to be in the range [0,1]? I guess for P3 that’s not how it works?

[Sam Weinig]

Created attachment 401754 [details]
Patch

[Sam Weinig]

Created attachment 401761 [details]
Patch

[Simon Fraser (smfr)]

> Source/WebCore/ChangeLog:10
> +        ColorComponents<float> is east but explicit to make conversions

east

> Source/WebCore/platform/graphics/ColorTypes.h:35
> +struct SRGBAColor {
> +    float red;
> +    float green;
> +    float blue;
> +    float alpha;
> +};

I wonder if this should be more like template<typename T> class ColorComponents { std::array<float, 4> ... } 

enum class sRGBType { };
using sRGBColor = ObjectIdentifier< sRGBType >;

I think I'd like the name to suggest that it's 0-1 float components too. Just SRGBColor doesn't make it sound different enough from Color.

[Darin Adler]

Comment on attachment 401761 [details]
Patch

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

My review comments are more of a "response to the experiment" than a "checking over whether this is OK to land".

> Source/WebCore/ChangeLog:17
> +        us to simplify the naming of functions that convert between color 
> +        types as now only the output type needs to be in the function name.

That’s the part I really like.

> Source/WebCore/platform/graphics/Color.h:136
> +    WEBCORE_EXPORT SRGBAColor toSRGBAColorLossy() const;

I don’t think this function needs the word "Color" in its name.

> Source/WebCore/platform/graphics/ColorTypes.h:35
> +struct SRGBAColor {
> +    float red;
> +    float green;
> +    float blue;
> +    float alpha;
> +};

Seems limiting that we only have this for float, not for 0-255 bytes, and not for double (matches Core Graphics API). On the other hand, I do like that this is just a struct and not a template.

Makes me think about the premultiplication issue. Is this intrinsically not premultiplied, or can it be used for both?

Should the r/g/b component values be typed, or just the struct?

Should we initialize everything to 0 to make it harder to make the mistake of leaving things uninitialized? Are there any range restrictions? What about use of infinity and NaN?

> Source/WebCore/platform/graphics/ColorUtilities.cpp:35
> +static float linearToRGBColorComponent(float c)

Seems like the same kind of issue comes up here. Without type safety, we can accidentally do this twice, or forget to do it.

More food for thought, does this function work on all float values, or must they be in the 0-1 range?

> Source/WebCore/platform/graphics/ColorUtilities.cpp:142
> +    auto linearDisplayP3 = toLinearDisplayP3(displayP3);
> +    auto xyz = toXYZ(linearDisplayP3);
> +    auto linearSRGB = toLinearSRGBA(xyz);
> +    return toSRGBA(linearSRGB);

Better without the local variables, don't you think?

    return toSRGBA(toLinearSRGBA(toXYZ(toLinearDisplayP3(sRGBA))));

Not beautiful, but easier to see it’s correct.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:150
> +    auto linearSRGBA = toLinearSRGBA(sRGBA);
> +    auto xyz = toXYZ(linearSRGBA);
> +    auto linearDisplayP3 = toLinearDisplayP3(xyz);
> +    return toDisplayP3(linearDisplayP3);

return toDisplayP3(toLinearDisplayP3(toXYZ(toLinearSRGBA(sRGBA))));

> Source/WebCore/platform/graphics/ColorUtilities.cpp:155
> +    auto [r, g, b, a] = sRGBA;

Annoying that this could scramble values if we wrote the left side wrong. Also annoying that we end up defining the unused alpha. In Swift it would be _ or something.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:165
> +    //      auto linearSRGBAComponents = toLinearSRGBA(sRGBA);
> +    //      return linearSRGBAComponents * linearSRGBToXYZMatrix.row(1);

Code in comment should be a one-liner:

    return toLinearSRGBA(sRGBA) * linearSRGBToXYZMatrix.row(1);

> Source/WebCore/platform/graphics/ColorUtilities.cpp:170
> +float contrastRatio(const SRGBAColor& sRGBA1, const SRGBAColor& sRGBA2)

Having the color space in the variable name seems way less valuable when it’s also in the type! Could just be color1, color2. Same applies many other places.

> Source/WebCore/platform/graphics/ExtendedColor.h:55
> +    SRGBAColor toSRGBAColorLossy() const;

Don’t need the word "Color" in the function name here.

> Source/WebCore/platform/graphics/SimpleColor.h:69
> +    constexpr SRGBAColor asSRGBAColor() const

Don’t need the word "Color" in the function name here.

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:92
>          auto matrix = grayscaleColorMatrix(m_amount);

No need to have this in a local variable. Could just make it part of the one-liner. Same below.

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:163
> +        sRGBAColor.red = 1 - (oneMinusAmount + sRGBAColor.red * (m_amount - oneMinusAmount));

Gotta find a way to apply operations/functions across the three channels other than writing them 3 times.

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:263
> +    hsla.hue = fmod(hsla.hue + 0.5f, 1.0f);

Why are we using the double overload of fmod here? Using std::fmod or fmodf would give us the float version. And I think that’s what we would prefer?

[Sam Weinig]

(In reply to Simon Fraser (smfr) from comment #9)
> > Source/WebCore/platform/graphics/ColorTypes.h:35
> > +struct SRGBAColor {
> > +    float red;
> > +    float green;
> > +    float blue;
> > +    float alpha;
> > +};
> 
> I wonder if this should be more like template<typename T> class
> ColorComponents { std::array<float, 4> ... } 
> 
> enum class sRGBType { };
> using sRGBColor = ObjectIdentifier< sRGBType >;

It could, but you wouldn't get the benefit of having nice names. All the versions I tried ended up being less nice to use for callers. 

> 
> I think I'd like the name to suggest that it's 0-1 float components too.
> Just SRGBColor doesn't make it sound different enough from Color.

Yeah, that's the biggest issue in my mind as well. I am still struggling with what nomenclature to use to suggest float vs. byte. We could go ham and just templatize it:

  SRGBAColor<float>

and then SimpleColor essentially becomes SRGBAColor<uint8_t>.

[Sam Weinig]

Created attachment 401762 [details]
Patch

[Darin Adler]

I can think of a number of separate dimensions that *might* be worth considering:

1) named components vs. numbered (possibly unclear which is which), or std::array with named indices or accessors?

2) aggregates that support operating across multiple elements vs. structs (member function that ties the elements into a tuple?)

3) color space in the name, but also as a value to use for other purposes? using ColorSpace = xxx?

4) premultiplication somehow specified

5) with/without alpha

6) already normalized values (in range, not infinite) vs. unconstrained values

7) 0-1 vs. 0-255 vs. 0-1 concept but valid outside 0-1 as used in HDR

8) float vs. integer vs. double

[Sam Weinig]

Created attachment 401777 [details]
Patch

[Sam Weinig]

I quite like this last version, at least as a first step in the right direction:

- SRGBA<float> replaces ColorComponents<float> in most places.
- color.red replaces color[0]

[Sam Weinig]

Created attachment 401784 [details]
Patch

[Sam Weinig]

Created attachment 401786 [details]
Patch

[Sam Weinig]

(In reply to Sam Weinig from comment #17)
> Created attachment 401786 [details]
> Patch

I think this is ready for review now.

[Darin Adler]

Comment on attachment 401786 [details]
Patch

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

> Source/WebCore/platform/graphics/Color.cpp:245
> +    return { ColorSpace::SRGB, asColorComponents(asSimple().asSRGBA<float>()) };

my head is spinning about to vs. as

> Source/WebCore/platform/graphics/ColorTypes.h:33
> +template<typename T>
> +struct SRGBA {

I always prefer these on the same line, but I guess I haven’t convinced you yet.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:156
> +    // NOTE: This is the equivilent of `toXYZ(toLinearSRGBA(color)).y`

spelling error I noticed in the existing comment here: "equivilent". Also the use of backquote here ... ?

> Source/WebCore/platform/graphics/ColorUtilities.h:28
> +#include "ColorTypes.h"

Seems like this dependency could be satisfied with a forward declaration?

> Source/WebCore/platform/graphics/ExtendedColor.h:30
> +#include "ColorTypes.h"

Seems like this dependency could be satisfied with a forward declaration?

> Source/WebCore/platform/graphics/SimpleColor.h:76
> -    constexpr ColorComponents<float> asSRGBFloatComponents() const
> +    template<typename T>
> +    constexpr SRGBA<T> asSRGBA() const
>      {
> -        return { convertToComponentFloat(redComponent()), convertToComponentFloat(greenComponent()), convertToComponentFloat(blueComponent()),  convertToComponentFloat(alphaComponent()) };
> +        if constexpr (std::is_floating_point_v<T>)
> +            return { convertToComponentFloat(redComponent()), convertToComponentFloat(greenComponent()), convertToComponentFloat(blueComponent()),  convertToComponentFloat(alphaComponent()) };
> +        else
> +            return { redComponent(), greenComponent(), blueComponent(), alphaComponent() };
>      }

Consider moving these longer function bodies out of the class definition?

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:160
> +            return 1.0f - (oneMinusAmount + component * (m_amount - oneMinusAmount));

I find this math truly mysterious, but at least it’s not changing.

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:167
> +            return std::clamp<float>(intercept + m_amount * component, 0.0f, 1.0f);

Surprised that <float> is needed here.

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:173
> +            return std::max<float>(m_amount * component, 0.0f);

Surprised that <float> is needed here.

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:225
> +bool InvertLightnessFilterOperation::transformColor(SRGBA<float>& color) const

So sad that these use bool plus out argument. Obviously we want these to be Optional instead some day.

[Sam Weinig]

(In reply to Darin Adler from comment #19)
> Comment on attachment 401786 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401786&action=review
> 
> > Source/WebCore/platform/graphics/Color.cpp:245
> > +    return { ColorSpace::SRGB, asColorComponents(asSimple().asSRGBA<float>()) };
> 
> my head is spinning about to vs. as

Yeah, I was thinking that might be too easy to mix up. The intent was to* were  conversions that included some potentially non-invertible math and as* was really just a glorified cast. I am thinking in a follow up of replacing the to* functions with more explicit convertTo* functions to make it clearer.

> 
> > Source/WebCore/platform/graphics/ColorTypes.h:33
> > +template<typename T>
> > +struct SRGBA {
> 
> I always prefer these on the same line, but I guess I haven’t convinced you
> yet.

Heh. My brain does seem to have this preference, but it is such a weak preference I don't think I would remember it if you asked me which one I actually prefer. I think it comes down to not knowing what to do for weird cases like a templated mem

> 
> > Source/WebCore/platform/graphics/ColorUtilities.cpp:156
> > +    // NOTE: This is the equivilent of `toXYZ(toLinearSRGBA(color)).y`
> 
> spelling error I noticed in the existing comment here: "equivilent". Also
> the use of backquote here ... ?

Fixed.

> 
> > Source/WebCore/platform/graphics/ColorUtilities.h:28
> > +#include "ColorTypes.h"
> 
> Seems like this dependency could be satisfied with a forward declaration?

Fixed.

> 
> > Source/WebCore/platform/graphics/ExtendedColor.h:30
> > +#include "ColorTypes.h"
> 
> Seems like this dependency could be satisfied with a forward declaration?

Fixed.

> 
> > Source/WebCore/platform/graphics/SimpleColor.h:76
> > -    constexpr ColorComponents<float> asSRGBFloatComponents() const
> > +    template<typename T>
> > +    constexpr SRGBA<T> asSRGBA() const
> >      {
> > -        return { convertToComponentFloat(redComponent()), convertToComponentFloat(greenComponent()), convertToComponentFloat(blueComponent()),  convertToComponentFloat(alphaComponent()) };
> > +        if constexpr (std::is_floating_point_v<T>)
> > +            return { convertToComponentFloat(redComponent()), convertToComponentFloat(greenComponent()), convertToComponentFloat(blueComponent()),  convertToComponentFloat(alphaComponent()) };
> > +        else
> > +            return { redComponent(), greenComponent(), blueComponent(), alphaComponent() };
> >      }
> 
> Consider moving these longer function bodies out of the class definition?

Fixed.

> 
> > Source/WebCore/platform/graphics/filters/FilterOperation.cpp:167
> > +            return std::clamp<float>(intercept + m_amount * component, 0.0f, 1.0f);
> 
> Surprised that <float> is needed here.

I believe it's due to m_amount being a double.

> 
> > Source/WebCore/platform/graphics/filters/FilterOperation.cpp:173
> > +            return std::max<float>(m_amount * component, 0.0f);
> 
> Surprised that <float> is needed here.

Same. 



Thanks so much for the review.

[Sam Weinig]

Created attachment 401837 [details]
Patch

[EWS]

Committed r263000: <https://trac.webkit.org/changeset/263000>

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

[Radar WebKit Bug Importer]

<rdar://problem/64329874>