Summary: | SimpleColor and SRGBA<uint8_t> are essentially the same - let's converge them | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||||||||||||||||
Component: | Platform | Assignee: | Sam Weinig <sam> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | aboxhall, andersca, apinheiro, cfleizach, changseok, cmarcelo, darin, dino, dmazzoni, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, jcraig, jdiggs, kondapallykalyan, luiz, macpherson, menard, mifenton, noam, pdr, sabouhallawa, samuel_white, schenney, sergio, simon.fraser, webkit-bug-importer | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Attachments: |
|
Description
Sam Weinig
2020-07-03 19:08:09 PDT
Created attachment 403499 [details]
Part 1
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.
Created attachment 403500 [details]
Patch
Created attachment 403501 [details]
Patch
Created attachment 403502 [details]
Part 1
Created attachment 403503 [details]
Patch
Created attachment 403504 [details]
Patch
Created attachment 403514 [details]
Patch
Created attachment 403521 [details]
Patch
Created attachment 403523 [details]
Patch
Created attachment 403529 [details]
Part 1
Ok, I think Part 1 is ready for review. 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". (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. Committed r263941: <https://trac.webkit.org/changeset/263941> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403529 [details]. (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. |