Bug 212184 - Extended Color Cleanup: Stop allowing direct access to the underlying SimpleColor (it is almost always incorrect with respect to extended colors)
Summary: Extended Color Cleanup: Stop allowing direct access to the underlying SimpleC...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on: 212231 212247 212265
Blocks:
  Show dependency treegraph
 
Reported: 2020-05-20 20:36 PDT by Sam Weinig
Modified: 2020-05-24 08:54 PDT (History)
34 users (show)

See Also:


Attachments
Patch (37.06 KB, patch)
2020-05-20 20:37 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (41.50 KB, patch)
2020-05-20 21:41 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (42.46 KB, patch)
2020-05-21 04:04 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (42.57 KB, patch)
2020-05-21 11:13 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (43.62 KB, patch)
2020-05-21 11:50 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (43.62 KB, patch)
2020-05-21 13:16 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (43.04 KB, patch)
2020-05-23 11:31 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (42.35 KB, patch)
2020-05-23 12:26 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (43.93 KB, patch)
2020-05-23 13:25 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (48.53 KB, patch)
2020-05-23 13:48 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (47.42 KB, patch)
2020-05-23 13:58 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (51.97 KB, patch)
2020-05-23 17:18 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (51.15 KB, patch)
2020-05-23 17:21 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (54.25 KB, patch)
2020-05-23 20:31 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (53.97 KB, patch)
2020-05-24 08:16 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2020-05-20 20:36:20 PDT
Extended Color Cleanup: Make Color::rgb() private
Comment 1 Sam Weinig 2020-05-20 20:37:34 PDT Comment hidden (obsolete)
Comment 2 Sam Weinig 2020-05-20 21:41:45 PDT Comment hidden (obsolete)
Comment 3 Sam Weinig 2020-05-21 04:04:14 PDT Comment hidden (obsolete)
Comment 4 Sam Weinig 2020-05-21 11:13:39 PDT Comment hidden (obsolete)
Comment 5 Sam Weinig 2020-05-21 11:30:50 PDT
Not ready for real review yet but this change does a few things all in the name of making Color::rgb() private:

- Replaces a weird common idiom I don't quite understand the reasoning of where a Color was created out of another color.rgb():
    OLD: Color(myColor.rgb())
    NEW: myColor
 Best I can tell, this would just drop the Semantic bit and is just broken completely with ExtendedColor

- Replaces colorWithOverrideAlpha(RGBA) with a Color:: colorWithAlphaUsingAlternativeRounding(). This variant works with ExtendedColors and makes it clear we need to reconcile the two variants (just not in this patch, as it will require changing test results)

- Makes premultipliedARGBFromColor/colorFromPremultipliedARGB work strictly with RGBA32s, as they don't make any sense with ExtendedColor as you lose the ColorSpace information. The one remaining bit on Color itself is a new premultipliedARGB() function, which is only used by TextureMapperGL.cpp, and should be replaced with instead premultiplying in FloatComponents instead.

- Move IPC coders from WebKit to Color via the normal template encoded/decoder pattern.

- Other easier removals of rgb() usage where equivalent means existed.


One straggler is a change in RenderThemeIOS::systemColor(), where the code was taking a Color from a global map and creating a new Color as return Color(it->value.rgb(), Color::Semantic), essentially adding the Semantic bit to it. I am not sure exactly why this is needed, but rather than removing it, in the next iteration here, I will probably add a new function in Color which returns the same Color with the Semantic bit set, and leave this mystery for another time. (One guess I have is that that the Colors in this map were coming over IPC from the UI process and were losing their Semantic bit, as that is not included in the IPC currently. If so, the solution would be to encode that bit, and drop this silliness).
Comment 6 Sam Weinig 2020-05-21 11:50:37 PDT Comment hidden (obsolete)
Comment 7 Sam Weinig 2020-05-21 13:16:14 PDT Comment hidden (obsolete)
Comment 8 Sam Weinig 2020-05-21 14:29:05 PDT
Split out a chunk of this into https://bugs.webkit.org/show_bug.cgi?id=212231
Comment 9 Sam Weinig 2020-05-22 10:37:10 PDT Comment hidden (obsolete)
Comment 10 Sam Weinig 2020-05-22 10:37:19 PDT
Split out another chunk in https://bugs.webkit.org/show_bug.cgi?id=212265
Comment 11 Sam Weinig 2020-05-23 11:31:09 PDT Comment hidden (obsolete)
Comment 12 Sam Weinig 2020-05-23 12:26:57 PDT Comment hidden (obsolete)
Comment 13 Sam Weinig 2020-05-23 13:25:59 PDT Comment hidden (obsolete)
Comment 14 Sam Weinig 2020-05-23 13:48:36 PDT Comment hidden (obsolete)
Comment 15 Sam Weinig 2020-05-23 13:58:06 PDT Comment hidden (obsolete)
Comment 16 Sam Weinig 2020-05-23 17:18:35 PDT Comment hidden (obsolete)
Comment 17 Sam Weinig 2020-05-23 17:19:08 PDT
Should be ready for review now.
Comment 18 Sam Weinig 2020-05-23 17:21:19 PDT
Created attachment 400144 [details]
Patch
Comment 19 Sam Weinig 2020-05-23 20:31:06 PDT
Created attachment 400148 [details]
Patch
Comment 20 Dean Jackson 2020-05-24 03:06:03 PDT
Comment on attachment 400148 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1968
> +    auto color = downcast<HTMLInputElement>(*node()).valueAsColor().toSRGBASimpleColorLossy();
> +    r = color.redComponent();
> +    g = color.greenComponent();
> +    b = color.blueComponent();

Can you destructure this? If you had an "int a" that was discarded?

[r, g, b, a] = downcast<HTMLInputElement>(*node()).valueAsColor().toSRGBASimpleColorLossy();

> Source/WebCore/platform/graphics/Color.cpp:182
>      // FIXME: This should probably return a floating point number, but many of the call
>      // sites have picked comparison values based on feel. We'd need to break out
>      // our logarithm tables to change them :)

Have you looked at the places that use this method? I wonder if the comment is still true.

> Source/WebCore/platform/graphics/Color.cpp:433
> +    int r = (selfR * selfA * (255 - sourceA) + 255 * sourceA * sourceR) / d;
> +    int g = (selfG * selfA * (255 - sourceA) + 255 * sourceA * sourceG) / d;
> +    int b = (selfB * selfA * (255 - sourceA) + 255 * sourceA * sourceB) / d;

You use 0xff below, which I like.

> Source/WebCore/platform/graphics/Color.cpp:497
> +    uint8_t newAlpha = alpha * 255;

You use 0xff below, which I like.

Also, is there a reason you used uint8_t? Could we do it everywhere?

> Source/WebCore/platform/graphics/Color.cpp:509
> +        return Color { extendedColor.red(), extendedColor.green(), extendedColor.blue(), alpha, extendedColor.colorSpace() };

I wonder if we should have a constructor that is colorReplacingAlpha(const Color& color, float alpha), or something like that (since we have invertedColorWithAlpha)

> Source/WebCore/platform/graphics/Color.cpp:512
> +    // FIXME: This is where this function differs from colorWithAlphaUsing

Nit: missing period.

> Source/WebCore/platform/graphics/Color.cpp:526
> +    auto [r, g, b, existingAlpha] = rgb();

It's a bit confusing to me that a function called "rgb()" would return 4 components.

> Source/WebCore/platform/graphics/Color.h:72
> +    constexpr float alphaComponentAsFloat() const { return static_cast<float>(alphaComponent()) / 0xFF; }

Nit: Lowercase 0xff to be consistent.

> Source/WebCore/platform/graphics/Color.h:81
> +    constexpr SimpleColor colorWithAlpha(uint8_t alpha) const { return { (m_value & 0x00FFFFFF) | alpha << 24 }; }

Nit: Lowercase fs

> Source/WebCore/platform/graphics/ExtendedColor.cpp:66
> +Ref<ExtendedColor> ExtendedColor::invertedColorWithAlpha(float alpha) const

I wonder if we'll ever need to get color-nerdy and actually work out what we mean by "inverted". Do we mean brightness? Won't inversion depend on the color space?

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:399
> +    auto simpleColor = value.toSRGBASimpleColorLossy();

Another case where the destructuring would be nice. Since you're the template wizard, is there a way to destructure into 3 components, ignoring the fourth?
Comment 21 Sam Weinig 2020-05-24 08:15:19 PDT
(In reply to Dean Jackson from comment #20)
> Comment on attachment 400148 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400148&action=review
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1968
> > +    auto color = downcast<HTMLInputElement>(*node()).valueAsColor().toSRGBASimpleColorLossy();
> > +    r = color.redComponent();
> > +    g = color.greenComponent();
> > +    b = color.blueComponent();
> 
> Can you destructure this? If you had an "int a" that was discarded?
> 
> [r, g, b, a] =
> downcast<HTMLInputElement>(*node()).valueAsColor().toSRGBASimpleColorLossy();

There is no explicit syntax in c++ yet for discarding a value (it's being discussed a lot in the standards body) but you can just not use one of the variables. In this case, I did,'t use destructuring since the out parameters the function has made it awkward.

> 
> > Source/WebCore/platform/graphics/Color.cpp:182
> >      // FIXME: This should probably return a floating point number, but many of the call
> >      // sites have picked comparison values based on feel. We'd need to break out
> >      // our logarithm tables to change them :)
> 
> Have you looked at the places that use this method? I wonder if the comment
> is still true.

I have not. Worth a followup.

> 
> > Source/WebCore/platform/graphics/Color.cpp:433
> > +    int r = (selfR * selfA * (255 - sourceA) + 255 * sourceA * sourceR) / d;
> > +    int g = (selfG * selfA * (255 - sourceA) + 255 * sourceA * sourceG) / d;
> > +    int b = (selfB * selfA * (255 - sourceA) + 255 * sourceA * sourceB) / d;
> 
> You use 0xff below, which I like.

Changed. Though I used 0xFF since that is more prevalent in the file (also changed the one place I used 0xff).

> 
> > Source/WebCore/platform/graphics/Color.cpp:497
> > +    uint8_t newAlpha = alpha * 255;
> 
> You use 0xff below, which I like.

Changed.

> 
> Also, is there a reason you used uint8_t? Could we do it everywhere?

Yeah, we should use it more consistently.

> 
> > Source/WebCore/platform/graphics/Color.cpp:509
> > +        return Color { extendedColor.red(), extendedColor.green(), extendedColor.blue(), alpha, extendedColor.colorSpace() };
> 
> I wonder if we should have a constructor that is colorReplacingAlpha(const
> Color& color, float alpha), or something like that (since we have
> invertedColorWithAlpha)

Will be doing more of this in a follow up.

> 
> > Source/WebCore/platform/graphics/Color.cpp:512
> > +    // FIXME: This is where this function differs from colorWithAlphaUsing
> 
> Nit: missing period.

Fixed.

> 
> > Source/WebCore/platform/graphics/Color.cpp:526
> > +    auto [r, g, b, existingAlpha] = rgb();
> 
> It's a bit confusing to me that a function called "rgb()" would return 4
> components.

Yeah. This function will get a name change soon.

> 
> > Source/WebCore/platform/graphics/Color.h:72
> > +    constexpr float alphaComponentAsFloat() const { return static_cast<float>(alphaComponent()) / 0xFF; }
> 
> Nit: Lowercase 0xff to be consistent.

Kept it 0xFF as that is more prevalent in this file.

> 
> > Source/WebCore/platform/graphics/Color.h:81
> > +    constexpr SimpleColor colorWithAlpha(uint8_t alpha) const { return { (m_value & 0x00FFFFFF) | alpha << 24 }; }
> 
> Nit: Lowercase fs

Kept it 0xFF as that is more prevalent in this file.

> 
> > Source/WebCore/platform/graphics/ExtendedColor.cpp:66
> > +Ref<ExtendedColor> ExtendedColor::invertedColorWithAlpha(float alpha) const
> 
> I wonder if we'll ever need to get color-nerdy and actually work out what we
> mean by "inverted". Do we mean brightness? Won't inversion depend on the
> color space?

Probably :(. There are a bunch of weird things we do around selection including this insert and the blendWithWhite() function that probably need to be reconsidered.

> 
> > Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:399
> > +    auto simpleColor = value.toSRGBASimpleColorLossy();
> 
> Another case where the destructuring would be nice. Since you're the
> template wizard, is there a way to destructure into 3 components, ignoring
> the fourth?

Fixed.
Comment 22 Sam Weinig 2020-05-24 08:16:48 PDT
Created attachment 400161 [details]
Patch
Comment 23 EWS 2020-05-24 08:53:24 PDT
Committed r262108: <https://trac.webkit.org/changeset/262108>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400161 [details].
Comment 24 Radar WebKit Bug Importer 2020-05-24 08:54:14 PDT
<rdar://problem/63579502>