Extended Color: Streamline SimpleColor premulitply/unpremultiply code
Created attachment 401404 [details] Patch
Now that the signatures only take SimpleColors, I am not sure if I should just make these members of SimpleColor. For now, this feels a lot more clear.
For simple value types with no hidden state like SimpleColor, I personally want member functions kept to the absolute minimum. Free functions are far more flexible. But also, I wish this was a different programming language, where syntax and encapsulation weren’t so closely tied together.
Created attachment 401461 [details] Patch
Comment on attachment 401461 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401461&action=review > Source/WebCore/ChangeLog:9 > + - Removing overloads that didn't took the seperated components, keeping "didn't took the separated" is not good grammar, and spells "separated" wrong > Source/WebCore/ChangeLog:14 > + - Simplifying the names from makePremultipliedSimpleColor/makeUnpremultipliedSimpleColor > + to premultiplyFlooring/premultiplyCeiling/unpremultiply. The use of "ceil" here as a verb with "ceiling" as its gerund is not awesome. No call for premultiplying with rounding? I’m not sure how I feel about these functions with verb names. I think we would call these "premultiplied" rather than "premultiply", but not sure that fully works with the rest of the naming. > Source/WebCore/ChangeLog:15 > + - Where component order is impotant, use valueAsARGB() explicitly to "impotant" > Source/WebCore/platform/graphics/Color.cpp:269 > + // Since premultiplyCeiling() bails on zero alpha, special-case that. Seems like something we might eventually want to address. > Source/WebCore/platform/graphics/Color.cpp:270 > + auto premultFrom = from.alpha() ? premultiplyCeiling(from.toSRGBASimpleColorLossy()) : Color::transparent; I would have renamed all these "premult" to "premultiplied". > Source/WebCore/platform/graphics/cairo/ImageBufferCairoImageSurfaceBackend.cpp:83 > + auto pixelColor = unpremultiply(SimpleColor { *pixel }); I’m not sure I am on board with the strategy where we use valueAsARGB() to emphasize the meaning of the returned integer, but construct SimpleColor from a 32-bit integer with no disambiguation for the meaning there. Maybe there’s a future step coming to solve that?
(In reply to Darin Adler from comment #5) > Comment on attachment 401461 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=401461&action=review > > > Source/WebCore/ChangeLog:9 > > + - Removing overloads that didn't took the seperated components, keeping > > "didn't took the separated" is not good grammar, and spells "separated" wrong > > > Source/WebCore/ChangeLog:14 > > + - Simplifying the names from makePremultipliedSimpleColor/makeUnpremultipliedSimpleColor > > + to premultiplyFlooring/premultiplyCeiling/unpremultiply. > > The use of "ceil" here as a verb with "ceiling" as its gerund is not awesome. > > No call for premultiplying with rounding? Agreed, though I stole the name from your patch on 180620 :) > > I’m not sure how I feel about these functions with verb names. I think we > would call these "premultiplied" rather than "premultiply", but not sure > that fully works with the rest of the naming. > > > Source/WebCore/ChangeLog:15 > > + - Where component order is impotant, use valueAsARGB() explicitly to > > "impotant" Will fix. > > > Source/WebCore/platform/graphics/Color.cpp:269 > > + // Since premultiplyCeiling() bails on zero alpha, special-case that. > > Seems like something we might eventually want to address. Yeah, since we check for 0 already there, we could totally just return transparent there. Will look into that in a follow up. > > > Source/WebCore/platform/graphics/Color.cpp:270 > > + auto premultFrom = from.alpha() ? premultiplyCeiling(from.toSRGBASimpleColorLossy()) : Color::transparent; > > I would have renamed all these "premult" to "premultiplied". Will fix. > > > Source/WebCore/platform/graphics/cairo/ImageBufferCairoImageSurfaceBackend.cpp:83 > > + auto pixelColor = unpremultiply(SimpleColor { *pixel }); > > I’m not sure I am on board with the strategy where we use valueAsARGB() to > emphasize the meaning of the returned integer, but construct SimpleColor > from a 32-bit integer with no disambiguation for the meaning there. Maybe > there’s a future step coming to solve that? Yeah, my thought was to introduce a makeSimpleColorFromARGB (or some name like that) at some point and remove the constructor. From Simon's comments in other bugs, the fact that we use ARGB as the format is a bit surprising to some, so might as well make it explicit. Thanks for the review.
Committed r262810: <https://trac.webkit.org/changeset/262810>
<rdar://problem/64179100>
Maybe at some point we’ll change our minds and use a different class for the premultiplied data. Presumably you can’t use it in a Color and have it work correctly. It’s another aspect of how to interpret the channel values, analogous to color space. And SimpleColor is 4 bytes that are implicitly in the sRGB space and should probably also be implicitly not premultiplied for the same reason. As opposed to a more general "4 channels with no judgment about how they are used".