RESOLVED FIXED 213948
SimpleColor and SRGBA<uint8_t> are essentially the same - let's converge them
https://bugs.webkit.org/show_bug.cgi?id=213948
Summary SimpleColor and SRGBA<uint8_t> are essentially the same - let's converge them
Sam Weinig
Reported 2020-07-03 19:08:09 PDT
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>.
Attachments
Part 1 (60.98 KB, patch)
2020-07-03 19:36 PDT, Sam Weinig
no flags
Patch (61.06 KB, patch)
2020-07-03 19:43 PDT, Sam Weinig
no flags
Patch (69.21 KB, patch)
2020-07-03 19:56 PDT, Sam Weinig
no flags
Part 1 (69.21 KB, patch)
2020-07-03 19:57 PDT, Sam Weinig
no flags
Patch (69.30 KB, patch)
2020-07-03 20:22 PDT, Sam Weinig
no flags
Patch (69.29 KB, patch)
2020-07-03 20:26 PDT, Sam Weinig
no flags
Patch (70.96 KB, patch)
2020-07-04 01:20 PDT, Sam Weinig
no flags
Patch (69.55 KB, patch)
2020-07-04 06:23 PDT, Sam Weinig
no flags
Patch (70.03 KB, patch)
2020-07-04 08:45 PDT, Sam Weinig
no flags
Part 1 (76.19 KB, patch)
2020-07-04 11:10 PDT, Sam Weinig
no flags
Sam Weinig
Comment 1 2020-07-03 19:36:32 PDT Comment hidden (obsolete)
Sam Weinig
Comment 2 2020-07-03 19:41:16 PDT
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
Comment 3 2020-07-03 19:43:50 PDT Comment hidden (obsolete)
Sam Weinig
Comment 4 2020-07-03 19:56:23 PDT Comment hidden (obsolete)
Sam Weinig
Comment 5 2020-07-03 19:57:53 PDT Comment hidden (obsolete)
Sam Weinig
Comment 6 2020-07-03 20:22:41 PDT Comment hidden (obsolete)
Sam Weinig
Comment 7 2020-07-03 20:26:09 PDT Comment hidden (obsolete)
Sam Weinig
Comment 8 2020-07-04 01:20:29 PDT Comment hidden (obsolete)
Sam Weinig
Comment 9 2020-07-04 06:23:43 PDT Comment hidden (obsolete)
Sam Weinig
Comment 10 2020-07-04 08:45:34 PDT Comment hidden (obsolete)
Sam Weinig
Comment 11 2020-07-04 11:10:44 PDT
Sam Weinig
Comment 12 2020-07-04 11:11:13 PDT
Ok, I think Part 1 is ready for review.
Darin Adler
Comment 13 2020-07-04 11:32:12 PDT
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
Comment 14 2020-07-04 12:04:52 PDT
(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
Comment 15 2020-07-04 12:20:54 PDT
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
Comment 16 2020-07-04 12:21:15 PDT
Darin Adler
Comment 17 2020-07-04 19:11:14 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.