Bug 213948 – SimpleColor and SRGBA<uint8_t> are essentially the same - let's converge them

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Sam Weinig]

Now that we have SRGBA<uint8_t>, SimpleColor feels a bit redundant. Let's see what it will take to replace SimpleColor with SRGBA<uint8_t>.

[Sam Weinig]

Created attachment 403499 [details]
Part 1

[Sam Weinig]

Comment on attachment 403499 [details]
Part 1

Part 1: Removes red, green, and blue component accessors from SimpleColor, replacing it with easier access to SRGBA<uint8_t> from Color itself. This is done by replacing toSRGBASimpleColorLossy() and toSRGBALossy() on Color, with one templatized toSRGBALossy<>() that can do both.

As a result of this, many call sites that used to deal with SimpleColors now deal with SRGBA<uint8_t>, so utilities like premulitiply/unpremultiply now should be in terms of SRGBA<uint8_t> rather than SimpleColor.

[Sam Weinig]

Created attachment 403500 [details]
Patch

[Sam Weinig]

Created attachment 403501 [details]
Patch

[Sam Weinig]

Created attachment 403502 [details]
Part 1

[Sam Weinig]

Created attachment 403503 [details]
Patch

[Sam Weinig]

Created attachment 403504 [details]
Patch

[Sam Weinig]

Created attachment 403514 [details]
Patch

[Sam Weinig]

Created attachment 403521 [details]
Patch

[Sam Weinig]

Created attachment 403523 [details]
Patch

[Sam Weinig]

Created attachment 403529 [details]
Part 1

[Sam Weinig]

Ok, I think Part 1 is ready for review.

[Darin Adler]

Comment on attachment 403529 [details]
Part 1

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

Love the direction this is going.

> Source/WebCore/ChangeLog:9
> +        Begin converging SimpleColor and SRGBA<uint8_t>, starting with removing usages that
> +        were getting SimpleColors to access or operate on the color's components.

Converging seems great. The strangest thing about SimpleColor is probably how it can seems to prefer to represent all 4 channels in a single integer.

> Source/WebCore/ChangeLog:12
> +        - Replace toSRGBASimpleColorLossy() with toSRGBALossy<uint8_t>
> +        - Replace toSRGBALossy() with toSRGBALossy<float>().

It worries me that the integer vs. floating point type also implies [0-255] vs. [0-1]. Kind of wish there was a "uint8_t to mean [0-1]" type instead of actual uint8_t, but that would also ruin the code I guess. But it would be good to prevent accidental conversion between that and floating point. Also ignores the fact that floating point [0-255] is a thing.

> Source/WebCore/platform/graphics/Color.cpp:152
> +    return { makeSimpleColor(toSRGBALossy<uint8_t>()), Semantic };

This isn’t so pretty. The makeSimpleColor call here seems strange.

> Source/WebCore/platform/graphics/ColorTypes.h:243
> +struct ARGB {

Not sure if this is a specific enough name to imply "stored as a 32-bit integer with 8-bit components". Seems like either 32 or 8 might need to be in the name. SRGBA also defines the color space more precisely by including the letter "S".

[Sam Weinig]

(In reply to Darin Adler from comment #13)
> Comment on attachment 403529 [details]
> Part 1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403529&action=review
> 
> Love the direction this is going.
> 
> > Source/WebCore/ChangeLog:9
> > +        Begin converging SimpleColor and SRGBA<uint8_t>, starting with removing usages that
> > +        were getting SimpleColors to access or operate on the color's components.
> 
> Converging seems great. The strangest thing about SimpleColor is probably
> how it can seems to prefer to represent all 4 channels in a single integer.
> 
> > Source/WebCore/ChangeLog:12
> > +        - Replace toSRGBASimpleColorLossy() with toSRGBALossy<uint8_t>
> > +        - Replace toSRGBALossy() with toSRGBALossy<float>().
> 
> It worries me that the integer vs. floating point type also implies [0-255]
> vs. [0-1]. Kind of wish there was a "uint8_t to mean [0-1]" type instead of
> actual uint8_t, but that would also ruin the code I guess. But it would be
> good to prevent accidental conversion between that and floating point. Also
> ignores the fact that floating point [0-255] is a thing.

When you wrote "uint8_t to mean [0-1]" did you mean "uint8_t to mean [0-255]"? Or maybe I am misunderstanding. Either way, one idea for how to better abstract this was to use something like the c++ std::byte type - https://en.cppreference.com/w/cpp/types/byte (not it specifically, but something akin to it). And require something like std::to_integer to convert it to a usable integer. Going to play around with that more after this round of chances.

> 
> > Source/WebCore/platform/graphics/Color.cpp:152
> > +    return { makeSimpleColor(toSRGBALossy<uint8_t>()), Semantic };
> 
> This isn’t so pretty. The makeSimpleColor call here seems strange.

Yeah, follow up will make this quite a bit nicer by allowing Color to be constructed from typed colors like SRGBA<uint8_t> directly.

> 
> > Source/WebCore/platform/graphics/ColorTypes.h:243
> > +struct ARGB {
> 
> Not sure if this is a specific enough name to imply "stored as a 32-bit
> integer with 8-bit components". Seems like either 32 or 8 might need to be
> in the name. SRGBA also defines the color space more precisely by including
> the letter "S".

Yeah, I am not completely happy with this naming yet. Going to keep iterating on it.

Thanks for the review.

[EWS]

Committed r263941: <https://trac.webkit.org/changeset/263941>

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

[Radar WebKit Bug Importer]

<rdar://problem/65098023>

[Darin Adler]

(In reply to Sam Weinig from comment #14)
> When you wrote "uint8_t to mean [0-1]" did you mean "uint8_t to mean
> [0-255]"?

I meant what you thought; just didn’t say the words right.

A uint8_t that uses integer values in the range 0-255 to represent channel values in the abstract range 0-1.