Bug 212059

Created attachment 399700 [details] Patch

Here is a first pass at this. Not sure I am happy with the name Color::rgba(). Maybe Color::toRGBAComponents() for symmetry with Color::toSRGBAComponentsLossy()? Or should we just have everyone using Color::toSRGBAComponentsLossy(). Pretty happy with the structured bindings use, but since that is kind of new-ish, would like more feedback on it.

Comment on attachment 399700 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399700&action=review > Source/WebCore/platform/graphics/cairo/GradientCairo.cpp:154 > + if (from.color.isExtended()) > + fromComponents = from.color.asExtended().channels(); > + else > + fromComponents = from.color.rgba(); Should make rgba() just work for extended colors, as long as they are sRGB. This code should really ask about the colorspace, not about the isExtended. Maybe just call toSRGBAComponentsLossy(). > Source/WebCore/platform/graphics/cairo/GradientCairo.cpp:165 > float r = r1 + (r2 - r1) * 0.5f; Maybe we should just put blending into FloatCompontents. > Source/WebCore/platform/graphics/cg/GradientCG.cpp:73 > + auto [r, g, b, a] = stop.color.rgba(); Broken with extended colors (not your fault). Call toSRGBAComponentsLossy()? > Source/WebCore/platform/graphics/filters/FilterOperations.cpp:114 > + FloatComponents components = color.rgba(); Broken with extended colors (not your fault). Call toSRGBAComponentsLossy()? > Source/WebCore/platform/graphics/filters/FilterOperations.cpp:133 > + FloatComponents components = color.rgba(); Ditto. > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:256 > + auto [r, g, b, a] = Color(premultipliedARGBFromColor(color)).rgba(); Would be nicer to do the premultiply on FloatComponents. > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:281 > + auto [r, g, b, a] = color.rgba(); toSRGBAComponentsLossy? > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:421 > + auto [r, g, b, a] = Color(premultipliedARGBFromColor(shadow.color())).rgba(); Would be nicer to do the premultiply on FloatComponents. > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:683 > + auto [r, g, b, a] = Color(premultipliedARGBFromColor(color)).rgba(); Would be nicer to do the premultiply on FloatComponents. > Source/WebCore/rendering/TextPaintStyle.cpp:66 > + return contrastRatio(textColor.rgba(), backgroundColor.rgba()) > 4.5; Broken with extended colors. > Source/WebKit/UIProcess/API/wpe/WebKitColor.cpp:85 > + auto [r, g, b, a] = webCoreColor.rgba().components; Broken with extended colors. > Source/WebKit/WebProcess/WebPage/WebFrame.cpp:663 > + auto [r, g, b, a] = bgColor.rgba(); Broken with extended colors.

Simon, other than the call to Color::rgba() in Color::toSRGBAComponents(), are any of the other calls valid? Should they all be switched to Color::toSRGBAComponents() and Color::rgba() be made private?

(In reply to Sam Weinig from comment #4) > Simon, other than the call to Color::rgba() in Color::toSRGBAComponents(), > are any of the other calls valid? Should they all be switched to > Color::toSRGBAComponents() and Color::rgba() be made private? I think it's an error to call Color::rgba() without knowing that the color is sRGB. So we could either make Color::rgba() private, or have it return Optional<FloatComponents> if there are clients for which that makes sense. Now that ExtendedColor allows Color to be in any of the supported colorspaces, I think we have to be explicit about colorspace conversions everywhere.

(In reply to Simon Fraser (smfr) from comment #5) > (In reply to Sam Weinig from comment #4) > > Simon, other than the call to Color::rgba() in Color::toSRGBAComponents(), > > are any of the other calls valid? Should they all be switched to > > Color::toSRGBAComponents() and Color::rgba() be made private? > > I think it's an error to call Color::rgba() without knowing that the color > is sRGB. To put it more strongly: Color::rgba() is actively harmful, because it returns garbage for extended colors. We should hide it. For now we can move callers to toSRGBAComponentsLossy(). Callers that want to do the right thing with extended colors can be migrated later.

Created attachment 399752 [details] Patch

Comment on attachment 399752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399752&action=review > Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm:136 > + color = cachedCGColor(SimpleColor { argb }); (Not your fault): It blows my mind that the arguments to SimpleColor are argb. Seems very Cocoa-centric, and easy to get wrong. Maybe SimpleColor needs at more scary name like PlatformPixelValue. > Source/WebCore/platform/graphics/Color.cpp:359 > return Color(0x54, 0x54, 0x54, alpha()); Can we just say 84 rather than 0x54? > Source/WebCore/platform/graphics/Color.cpp:393 > + auto [r, g, b, a] = toSRGBAComponentsLossy(); > + float largestNonAlphaChannel = std::max(r, std::max(g, b)); > + return a > 0.5 && largestNonAlphaChannel < 0.5; This should really compute luminance. Maybe a // FIXME > Source/WebCore/platform/graphics/Color.cpp:544 > + return { ColorSpace::SRGB, FloatComponents { red() / 255.0f, green() / 255.0f, blue() / 255.0f, alpha() / 255.0f } }; I feel like the / 255.0f should be in a helper function somewhere. We've gone back and forth several times on rounding/flooring of color components (e.g. r252598). > Source/WebCore/platform/graphics/cg/ColorCG.cpp:117 > + return CGColorCreate(sRGBColorSpaceRef(), components); We should use the linearRGBColorSpaceRef here (should fix in a separate bug). > Source/WebCore/platform/graphics/cg/ColorCG.cpp:134 > + static CGColorRef whiteCGColor = leakCGColor(color) How does this compile? > Source/WebCore/platform/graphics/cg/GradientCG.cpp:74 > + auto [colorSpace, components] = stop.color.colorSpaceAndComponent(); > + auto [r, g, b, a] = components; This needs a FIXME; we drop colorSpace on the floor. > Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp:202 > + static constexpr SimpleColor spellingPatternColor { makeRGB(255, 0, 0) }; > + static constexpr SimpleColor grammarPatternColor { makeRGB(0, 128, 0) }; Ugh, my brain hurts. RGBA32 makeRGBA(), despite the name, returns ARGB which is the argument to the SimpleColor ctor, so this is OK. I don't know why SimpleColor doesn't take a RGBA32, and the names all need fixing. > Source/WebKit/WebProcess/WebPage/WebFrame.cpp:663 > + auto [r, g, b, a] = bgColor.toSRGBAComponentsLossy(); Sad that I can't have a P3 frame background.

Comment on attachment 399752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399752&action=review >> Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm:136 >> + color = cachedCGColor(SimpleColor { argb }); > > (Not your fault): It blows my mind that the arguments to SimpleColor are argb. Seems very Cocoa-centric, and easy to get wrong. Maybe SimpleColor needs at more scary name like PlatformPixelValue. We should change the arguments, not the class name. The RGBA32 integer type was widespread in KHTML even before it was ported to Mac, so it’s not Cocoa-centric. What arguments would you prefer? An aggregate of four uint8_t in RGBA order? I would love to change this to something that makes sense to you.

(In reply to Darin Adler from comment #9) > Comment on attachment 399752 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=399752&action=review > > >> Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm:136 > >> + color = cachedCGColor(SimpleColor { argb }); > > > > (Not your fault): It blows my mind that the arguments to SimpleColor are argb. Seems very Cocoa-centric, and easy to get wrong. Maybe SimpleColor needs at more scary name like PlatformPixelValue. > > We should change the arguments, not the class name. The RGBA32 integer type > was widespread in KHTML even before it was ported to Mac, so it’s not > Cocoa-centric. > > What arguments would you prefer? An aggregate of four uint8_t in RGBA order? > I would love to change this to something that makes sense to you. Is the long-term plan to replace RGBA32 with SimpleColor? Ideally we'd just remove it; it's too tied to the old notion that color fits into a uint32_t, with no colorspace information. Color could have some kind of adaptor, only usable when constructing a color, that explicitly converts from a ARGB into an sRGB color with those components. Note that ColorComponents, based on std::array<uint8_t, 4>, is basically the same thing with RGBA ordering.

(In reply to Simon Fraser (smfr) from comment #10) > Is the long-term plan to replace RGBA32 with SimpleColor? Yes. > Ideally we'd just remove it; it's too tied to the old notion that color fits > into a uint32_t, with no colorspace information. Color could have some kind > of adaptor, only usable when constructing a color, that explicitly converts > from a ARGB into an sRGB color with those components. I don’t understand what removing it means. I added it as a type safe replacement for the old RGBA32. Which, by the way, is a hilarious name for an integer containing 8-bit color components ordered ARGB. But once it was type safe, I had planned to change it to a better interface. > Note that ColorComponents, based on std::array<uint8_t, 4>, is basically the > same thing with RGBA ordering. Sure, we could find a way to merge ColorComponents with SimpleColor. The idea behind SimpleColor is that it contains 8-bit RGBA components with sRGB color space implied. It should only be used in places where that’s useful. Anywhere we need to handle colors that might not be that simple, we would use Color instead. I borrowed the name SimpleColor from https://html.spec.whatwg.org/#simple-colour where it’s used for RGB colors, without alpha.

Created attachment 399759 [details] Patch

(In reply to Simon Fraser (smfr) from comment #8) > Comment on attachment 399752 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=399752&action=review > > > > Source/WebCore/platform/graphics/Color.cpp:359 > > return Color(0x54, 0x54, 0x54, alpha()); > > Can we just say 84 rather than 0x54? Going to fix this in another change. Really it should be colorWithOverrideAlpha(lightenedBlack, alpha()) but there is no overload that works for this yet. > > > Source/WebCore/platform/graphics/Color.cpp:393 > > + auto [r, g, b, a] = toSRGBAComponentsLossy(); > > + float largestNonAlphaChannel = std::max(r, std::max(g, b)); > > + return a > 0.5 && largestNonAlphaChannel < 0.5; > > This should really compute luminance. Maybe a // FIXME Ok. Will add. > > > Source/WebCore/platform/graphics/Color.cpp:544 > > + return { ColorSpace::SRGB, FloatComponents { red() / 255.0f, green() / 255.0f, blue() / 255.0f, alpha() / 255.0f } }; > > I feel like the / 255.0f should be in a helper function somewhere. We've > gone back and forth several times on rounding/flooring of color components > (e.g. r252598). Have a name in mind? > > > Source/WebCore/platform/graphics/cg/ColorCG.cpp:117 > > + return CGColorCreate(sRGBColorSpaceRef(), components); > > We should use the linearRGBColorSpaceRef here (should fix in a separate bug). Ok. Will leave. > > > Source/WebCore/platform/graphics/cg/ColorCG.cpp:134 > > + static CGColorRef whiteCGColor = leakCGColor(color) > > How does this compile? It didn't. Fixed. > > > Source/WebCore/platform/graphics/cg/GradientCG.cpp:74 > > + auto [colorSpace, components] = stop.color.colorSpaceAndComponent(); > > + auto [r, g, b, a] = components; > > This needs a FIXME; we drop colorSpace on the floor. It's not completely dropping the color space on the floor, but you have to look at the surrounding code. These components will only be used if it's not an ExtendedColor, and therefore we know it's sRGB. The whole function is odd though and wrong. Probably, something needs to confirm that all the colors are in the same color space, or otherwise convert everything to a single colorspace. No sure. Not really clear on how the extendedSRGBColorSpaceRef works. > > Source/WebKit/WebProcess/WebPage/WebFrame.cpp:663 > > + auto [r, g, b, a] = bgColor.toSRGBAComponentsLossy(); > > Sad that I can't have a P3 frame background. Probably time for someone to add WKBundleFrameCopyDocumentBackgroundColor that returns a CGColorRef.

Created attachment 399772 [details] Patch

Comment on attachment 399772 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399772&action=review > Source/WebCore/platform/graphics/Color.cpp:393 > + float largestNonAlphaChannel = std::max(r, std::max(g, b)); I think we could write: std::max({ r, g, b }) > Source/WebCore/platform/graphics/Color.h:97 > +RGBA32 colorWithOverrideAlpha(RGBA32 color, int overrideAlpha); I’d think we’d want this to be uint8_t, not int.

Created attachment 399773 [details] Patch

(In reply to Sam Weinig from comment #16) > Created attachment 399773 [details] > Patch Hm, MSVC is really not happy with those colorSpace switches.

Created attachment 399777 [details] Patch

Created attachment 399796 [details] Patch

Committed r261901: <https://trac.webkit.org/changeset/261901> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399796 [details].

<rdar://problem/63426594>

Comment on attachment 399796 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399796&action=review > Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:75 > + auto [r, g, b, a] stop.color.toSRGBAComponentsLossy(); =

(In reply to Daniel Bates from comment #22) > Comment on attachment 399796 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=399796&action=review > > > Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:75 > > + auto [r, g, b, a] stop.color.toSRGBAComponentsLossy(); > > = erg. Will fix. But, this means we don't have any ews bots building Direct2D support. Do we have any non-EWS build bots building this?