Changes DlColor to support wide gamut colors (#54473) #54648

gaaclarke · 2024-08-20T16:44:22Z

relands #54473
issue: flutter/flutter#127855
integration test: #54415

This is the engine side changes required for wide gamut framework support. It changes the internal representation of DlColor to be floats. It will be married with #54415 when it lands in #54567.

Difference from last attempt

The default color is now opaque black, not transparent black (not the issue for revert)
Updated a test to send in valid numbers when constructing a color and added asserts to avoid those problems in the future.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

issue: flutter/flutter#127855 integration test: flutter#54415 This is the engine side changes required for wide gamut framework support. It changes the internal representation of DlColor to be floats. It will be married with flutter#54415 when it lands in flutter#54567. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

flutter-dashboard · 2024-08-20T18:34:21Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #54648 at sha 65d4573

jonahwilliams · 2024-08-20T18:39:05Z

impeller_Play_AiksTest_BlendModeSrcAlphaColorBurn_Metal still looks very wrong.

flar

Looks good, but I had a question about clamping vs dcheck...

display_list/dl_color.h

display_list/dl_color_unittests.cc

flar · 2024-08-20T18:21:33Z

display_list/dl_color.h

-        color_space_(colorspace) {}
+        color_space_(colorspace) {
+    FML_DCHECK(alpha >= 0.0);
+    FML_DCHECK(alpha <= 1.0);


Should we dcheck or clamp? This is currently, in this PR, helping to catch a problem in a test conversion that we crossed paths with, but that bug-in-a-test would have normally been caught by the test creator. If we dcheck then we might annoy someone who calculated the alpha and ended up with a barely-out-of-range value. They could just have easily have provided 255 for one of the color values and we'd take it as a potential non-normalized Color Space float...?

Rather than dcheck I think we should clamp right? Certain blend modes (plus) can lead to alpha greater than one without a clamp

Rather than dcheck I think we should clamp right? Certain blend modes (plus) can lead to alpha greater than one without a clamp

My concern with that is that it will mask problems like we had where someone is sending in 255 to create a color. The assert forces people creating colors to have a clamp if they are doing arithmetic that will get it out of bounds. I didn't have to change any code as a result of all our tests, so that's a good sign. I think this leaves us in a better position considering we control all the locations where DlColors are created. A public API would be a different story.

You should clamp.

I removed the asserts to duplicate the behavior of impeller::Color. There may be transitory colors that are not valid, clamping or the asserts could mess with that. I don't think we blend with DlColor today, it's more of a transmission format, but I think we plan to use it more broadly in the future.

We will be CPU blending once we merge DlColor and impeller::Color. This will happen sooner than you'd think

gaaclarke · 2024-08-20T19:50:50Z

impeller_Play_AiksTest_BlendModeSrcAlphaColorBurn_Metal still looks very wrong.

@jonahwilliams very wrong? The only thing I'm seeing is the checkerboard looks more that 1/255th brighter. I can dig into those colors to see if this is expected. Different colors will be expected since we were converting floats to uint8 before. We don't want to duplicate that behavior.

before

after

jonahwilliams · 2024-08-20T20:20:47Z

gaaclarke · 2024-08-20T20:20:53Z

Here are the before and after colors for SolidColorContents 0.000630527 0.000630527 0.000630527 0.00392157 is very different from 0.160784 0.160784 0.160784 1. Looks like maybe a number getting normalized twice? That's definitely beyond the range of what we'd expect.

before

[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.74902 0.74902 0.74902 0.74902 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.146867 0.602153 0.146867 0.74902 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0 0 0 0.74902 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.481492 0.624837 0.885798 0.937255 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.110265 0.532949 0.569704 0.937255 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.0735102 0.110265 0.172749 0.937255 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.000630527 0.000630527 0.000630527 0.00392157 
2500
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.0588235 0.0588235 0.0588235 1 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.74902 0.74902 0.74902 0.74902 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.146867 0.602153 0.146867 0.74902 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0 0 0 0.74902 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.481492 0.624837 0.885798 0.937255 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.110265 0.532949 0.569704 0.937255 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.0735102 0.110265 0.172749 0.937255 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.000630527 0.000630527 0.000630527 0.00392157 
2500
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.0588235 0.0588235 0.0588235 1

after

[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.75 0.75 0.75 0.75 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.147059 0.602941 0.147059 0.75 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0 0 0 0.75 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.481618 0.625735 0.884559 0.9375 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.110294 0.53194 0.571029 0.9375 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.0735294 0.109559 0.174265 0.9375 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.160784 0.160784 0.160784 1 
2500
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.0588235 0.0588235 0.0588235 1 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.75 0.75 0.75 0.75 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.147059 0.602941 0.147059 0.75 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0 0 0 0.75 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.481618 0.625735 0.884559 0.9375 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.110294 0.53194 0.571029 0.9375 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.0735294 0.109559 0.174265 0.9375 
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.160784 0.160784 0.160784 1 
2500
[ERROR:flutter/impeller/entity/contents/solid_color_contents.cc(60)] 0.0588235 0.0588235 0.0588235 1

jonahwilliams · 2024-08-20T20:21:05Z

It looks like the grid moved? or changed colors.

gaaclarke · 2024-08-20T20:26:56Z

It looks like the grid moved? or changed colors.

@jonahwilliams oh, yea this was a bug with the old code. This is coming from the same colors defined in

engine/impeller/display_list/aiks_dl_blend_unittests.cc

Line 395 in 3d18f65

paint.setColor(DlColor::RGBA(41 / 255.0, 41 / 255.0, 41 / 255.0, 255));

.

Notice that the old colors would be drawn as ((41/255)/255). Now they are drawn as 41/255. Looking at the test, the intention was actually to draw 41/255.

jonahwilliams · 2024-08-20T20:30:03Z

ack ack!

jonahwilliams

LGTM

flutter-dashboard · 2024-08-20T21:02:18Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #54648 at sha db4f334

flar

LGTM

flutter/engine#54648)

…153804) flutter/engine@3d18f65...5cbf96d 2024-08-20 [email protected] Roll Dart SDK from 04c57423f90d to 49f655b526c7 (2 revisions) (flutter/engine#54660) 2024-08-20 [email protected] Roll Skia from b472cacb48d6 to 51d7e221fef0 (2 revisions) (flutter/engine#54656) 2024-08-20 [email protected] Changes DlColor to support wide gamut colors (#54473) (flutter/engine#54648) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#153804) flutter/engine@3d18f65...5cbf96d 2024-08-20 [email protected] Roll Dart SDK from 04c57423f90d to 49f655b526c7 (2 revisions) (flutter/engine#54660) 2024-08-20 [email protected] Roll Skia from b472cacb48d6 to 51d7e221fef0 (2 revisions) (flutter/engine#54656) 2024-08-20 [email protected] Changes DlColor to support wide gamut colors (flutter#54473) (flutter/engine#54648) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

github-actions bot added the e: impeller label Aug 20, 2024

gaaclarke added 2 commits August 20, 2024 10:18

Made sure dlcolor is opaque black by default

055303c

fixed typo in test and added assert to avoid it

65d4573

gaaclarke marked this pull request as ready for review August 20, 2024 18:09

gaaclarke requested review from flar and jonahwilliams August 20, 2024 18:15

flutter-dashboard bot added the will affect goldens label Aug 20, 2024

flar reviewed Aug 20, 2024

View reviewed changes

asserted colorspace

bbe7dd8

removed asserts

db4f334

gaaclarke requested a review from flar August 20, 2024 20:32

jonahwilliams approved these changes Aug 20, 2024

View reviewed changes

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 20, 2024

flar approved these changes Aug 20, 2024

View reviewed changes

auto-submit bot merged commit d52fe13 into flutter:main Aug 20, 2024
34 checks passed

engine-flutter-autoroll mentioned this pull request Aug 20, 2024

Roll Flutter Engine from 3d18f65c378a to 769db74b702a (2 revisions) flutter/flutter#153802

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 20, 2024

d52fe1314 Changes DlColor to support wide gamut colors (flutter#54473) (

5b6c935

flutter/engine#54648)

engine-flutter-autoroll mentioned this pull request Aug 21, 2024

Roll Flutter Engine from 3d18f65c378a to 5cbf96d0a80f (3 revisions) flutter/flutter#153804

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 21, 2024

d52fe1314 Changes DlColor to support wide gamut colors (flutter#54473) (

a07b630

flutter/engine#54648)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes DlColor to support wide gamut colors (#54473) #54648

Changes DlColor to support wide gamut colors (#54473) #54648

gaaclarke commented Aug 20, 2024 •

edited

Loading

flutter-dashboard bot commented Aug 20, 2024

jonahwilliams commented Aug 20, 2024

flar left a comment

flar Aug 20, 2024

jonahwilliams Aug 20, 2024

gaaclarke Aug 20, 2024

jonahwilliams Aug 20, 2024

gaaclarke Aug 20, 2024

jonahwilliams Aug 20, 2024

gaaclarke commented Aug 20, 2024

jonahwilliams commented Aug 20, 2024

gaaclarke commented Aug 20, 2024

jonahwilliams commented Aug 20, 2024

gaaclarke commented Aug 20, 2024 •

edited

Loading

jonahwilliams commented Aug 20, 2024

jonahwilliams left a comment

flutter-dashboard bot commented Aug 20, 2024

flar left a comment

Changes DlColor to support wide gamut colors (#54473) #54648

Changes DlColor to support wide gamut colors (#54473) #54648

Conversation

gaaclarke commented Aug 20, 2024 • edited Loading

Difference from last attempt

Pre-launch Checklist

flutter-dashboard bot commented Aug 20, 2024

jonahwilliams commented Aug 20, 2024

flar left a comment

Choose a reason for hiding this comment

flar Aug 20, 2024

Choose a reason for hiding this comment

jonahwilliams Aug 20, 2024

Choose a reason for hiding this comment

gaaclarke Aug 20, 2024

Choose a reason for hiding this comment

jonahwilliams Aug 20, 2024

Choose a reason for hiding this comment

gaaclarke Aug 20, 2024

Choose a reason for hiding this comment

jonahwilliams Aug 20, 2024

Choose a reason for hiding this comment

gaaclarke commented Aug 20, 2024

before

after

jonahwilliams commented Aug 20, 2024

gaaclarke commented Aug 20, 2024

before

after

jonahwilliams commented Aug 20, 2024

gaaclarke commented Aug 20, 2024 • edited Loading

jonahwilliams commented Aug 20, 2024

jonahwilliams left a comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Aug 20, 2024

flar left a comment

Choose a reason for hiding this comment

gaaclarke commented Aug 20, 2024 •

edited

Loading

gaaclarke commented Aug 20, 2024 •

edited

Loading