Reland: Framework wide color (#54415) #54737

gaaclarke · 2024-08-23T16:14:52Z

This PR was reverted because it requires a manual roll into the framework.

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

This does the preliminary work for implementing wide gamut colors in the Flutter framework. Here are the following changes: 1) colors now specify a colorspace with which they are to be interpreted 1) colors now store their components as floats to accommodate bit depths more than 8

The storage of this Color class is weird with float/int storage but that is a temporary solution to support a smooth transition. Here is the plan for landing this:

Land this PR
Wait for it to roll into the Framework
Land Switched CupertinoDynamicColor to implements Color flutter#153938 which will make CupertinoDynamicColor implement Color
Land another engine PR that rips out the int storage: Removes the int storage from Color #54714

Here are follow up PRs:

Changes DlColor to support wide gamut colors #54473 - changes DlColor so the wide gamut colors are rendered
Hooks up framework wide gamut to engine wide gamut #54567 - Hooks up these changes to take advantage of wide DlColor
Adds wide gamut framework test flutter#153319 - the integration test for the framework repo

There are some things that have been left as follow up PRs since they are technically breaking:

The math on lerp hasn't been updated to take advantage of the higher bit depth
operator== hasn't been updated to take advantage of the higher bit depth
hashCode hasn't been updated to take advantage of the higher bit depth
alphaBlend hasn't been updated to take advantage of the higher bit depth
toString hasn't been updated to take advantage of the higher bit depth

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.

jonahwilliams · 2024-08-23T16:17:41Z

I the problem was the customer tests? One of the dependencies in there is https://pub.dev/packages/flutter_color_models which has a bunch of Color implementations

gaaclarke · 2024-08-23T16:18:01Z

I'm going to try to preload a framework patch as best I can from looking at the log before landing this. It is possible it's impossible to get 100% right before landing since some tests may be short circuited.

gaaclarke · 2024-08-23T16:19:44Z

I the problem was the customer tests? One of the dependencies in there is https://pub.dev/packages/flutter_color_models which has a bunch of Color implementations

I haven't dug into it yet, just starting to look now. I'll report back.

gaaclarke · 2024-08-23T16:21:30Z

If the problem is the change in the interface I think we can potentially preload the changes as long as missing @override isn't a CI breaking lint.

gaaclarke · 2024-08-23T16:24:36Z

Okay yea, all the errors seem to be coming from customer testing for that one color package.

gaaclarke · 2024-08-23T16:25:39Z

Everything else looked fine. Here's the failed roll: flutter/flutter#153985

jonahwilliams · 2024-08-23T16:27:53Z

You should be able to add the overrides and // ignore any warnings.

gaaclarke · 2024-08-23T16:31:31Z

You should be able to add the overrides and // ignore any warnings.

@jonahwilliams The customer tests are running against the latest published version? I'll try posting a PR for the dev and see how responsive they are.

jonahwilliams · 2024-08-23T16:33:56Z

Its whatever is in the pubspec. If it comes to it, we can publish a fork and add a dependency override or something like. Please also email the owner of that test as FYI, I think they're generally fairly responsivle

gaaclarke · 2024-08-23T20:40:33Z

I've published a PR for that package: james-alex/color_models#10. I haven't heard back from the package author yet.

gaaclarke · 2024-08-29T17:08:14Z

@jonahwilliams this should be good to land now, the test have been upgraded in flutter/tests#404. I was able to test color_model tests against master and this and they ran correctly locally, so I think we are good. Still no update from the color_models maintainer.

jonahwilliams

LGTM

gaaclarke · 2024-08-29T17:53:44Z

I'll rebase, just to be sure before landing. It's been a week.

issue: flutter/flutter#127855 integration test: flutter#54415 This does the preliminary work for implementing wide gamut colors in the Flutter framework. Here are the following changes: 1) colors now specify a colorspace with which they are to be interpreted 1) colors now store their components as floats to accommodate bit depths more than 8 The storage of this Color class is weird with float/int storage but that is a temporary solution to support a smooth transition. Here is the plan for landing this: 1) Land this PR 1) Wait for it to roll into the Framework 1) Land flutter/flutter#153938 which will make CupertinoDynamicColor implement Color 1) Land another engine PR that rips out the int storage: flutter#54714 Here are follow up PRs: 1) flutter#54473 - changes DlColor so the wide gamut colors are rendered 1) flutter#54567 - Hooks up these changes to take advantage of wide DlColor 1) flutter/flutter#153319 - the integration test for the framework repo There are some things that have been left as follow up PRs since they are technically breaking: 1) The math on `lerp` hasn't been updated to take advantage of the higher bit depth 1) `operator==` hasn't been updated to take advantage of the higher bit depth 1) `hashCode` hasn't been updated to take advantage of the higher bit depth 1) `alphaBlend` hasn't been updated to take advantage of the higher bit depth 1) `toString` hasn't been updated to take advantage of the higher bit depth ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] 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. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

…e#54737)

…154364) flutter/engine@6d4cb16...3075094 2024-08-29 [email protected] Roll Skia from 10e44e318a72 to 1d16eab57a8e (3 revisions) (flutter/engine#54874) 2024-08-29 [email protected] Reland: Framework wide color (#54415) (flutter/engine#54737) 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

chingjun · 2024-08-30T04:28:01Z

Reason for revert: Breaking internal tests. See b/363125155

This reverts commit 62154d3.

Reverts: #54737 Initiated by: chingjun Reason for reverting: Breaking internal tests. See b/363125155 Original PR Author: gaaclarke Reviewed By: {matanlurey, jonahwilliams} This change reverts the following previous change: [This PR](#54415) was reverted because it requires a manual roll into the framework. issue: flutter/flutter#127855 integration test: #54415 This does the preliminary work for implementing wide gamut colors in the Flutter framework. Here are the following changes: 1) colors now specify a colorspace with which they are to be interpreted 1) colors now store their components as floats to accommodate bit depths more than 8 The storage of this Color class is weird with float/int storage but that is a temporary solution to support a smooth transition. Here is the plan for landing this: 1) Land this PR 1) Wait for it to roll into the Framework 1) Land flutter/flutter#153938 which will make CupertinoDynamicColor implement Color 1) Land another engine PR that rips out the int storage: #54714 Here are follow up PRs: 1) #54473 - changes DlColor so the wide gamut colors are rendered 1) #54567 - Hooks up these changes to take advantage of wide DlColor 1) flutter/flutter#153319 - the integration test for the framework repo There are some things that have been left as follow up PRs since they are technically breaking: 1) The math on `lerp` hasn't been updated to take advantage of the higher bit depth 1) `operator==` hasn't been updated to take advantage of the higher bit depth 1) `hashCode` hasn't been updated to take advantage of the higher bit depth 1) `alphaBlend` hasn't been updated to take advantage of the higher bit depth 1) `toString` hasn't been updated to take advantage of the higher bit depth

jonahwilliams · 2024-08-30T04:56:30Z

lib/ui/lerp.dart

@@ -26,7 +26,7 @@ double? lerpDouble(num? a, num? b, double t) {
 ///
 /// Same as [lerpDouble] but specialized for non-null `double` type.
 double _lerpDouble(double a, double b, double t) {
-  return a * (1.0 - t) + b * t;
+  return a + (b - a) * t;


I think this is what may be causing the breakages. Throw inf in here for a or b and some lerp cases now produce nans:

[This PR](flutter#54415) was reverted because it requires a manual roll into the framework. issue: flutter/flutter#127855 integration test: flutter#54415 This does the preliminary work for implementing wide gamut colors in the Flutter framework. Here are the following changes: 1) colors now specify a colorspace with which they are to be interpreted 1) colors now store their components as floats to accommodate bit depths more than 8 The storage of this Color class is weird with float/int storage but that is a temporary solution to support a smooth transition. Here is the plan for landing this: 1) Land this PR 1) Wait for it to roll into the Framework 1) Land flutter/flutter#153938 which will make CupertinoDynamicColor implement Color 1) Land another engine PR that rips out the int storage: flutter#54714 Here are follow up PRs: 1) flutter#54473 - changes DlColor so the wide gamut colors are rendered 1) flutter#54567 - Hooks up these changes to take advantage of wide DlColor 1) flutter/flutter#153319 - the integration test for the framework repo There are some things that have been left as follow up PRs since they are technically breaking: 1) The math on `lerp` hasn't been updated to take advantage of the higher bit depth 1) `operator==` hasn't been updated to take advantage of the higher bit depth 1) `hashCode` hasn't been updated to take advantage of the higher bit depth 1) `alphaBlend` hasn't been updated to take advantage of the higher bit depth 1) `toString` hasn't been updated to take advantage of the higher bit depth

[This PR](#54415) was reverted because it required customer testing updates. issue: flutter/flutter#127855 integration test: #54415 This does the preliminary work for implementing wide gamut colors in the Flutter framework. Here are the following changes: 1) colors now specify a colorspace with which they are to be interpreted 1) colors now store their components as floats to accommodate bit depths more than 8 The storage of this Color class is weird with float/int storage but that is a temporary solution to support a smooth transition. Here is the plan for landing this: 1) Land this PR 1) Wait for it to roll into the Framework 1) Land flutter/flutter#153938 which will make CupertinoDynamicColor implement Color 1) Land another engine PR that rips out the int storage: #54714 Here are follow up PRs: 1) #54473 - changes DlColor so the wide gamut colors are rendered 1) #54567 - Hooks up these changes to take advantage of wide DlColor 1) flutter/flutter#153319 - the integration test for the framework repo There are some things that have been left as follow up PRs since they are technically breaking: 1) The math on `lerp` hasn't been updated to take advantage of the higher bit depth 1) `operator==` hasn't been updated to take advantage of the higher bit depth 1) `hashCode` hasn't been updated to take advantage of the higher bit depth 1) `alphaBlend` hasn't been updated to take advantage of the higher bit depth 1) `toString` hasn't been updated to take advantage of the higher bit depth ## Reland 2 notes This was reverted because it changes the math on `_lerpDouble`. While those changes were mathematcially equivalent, they had different behaviors when working with non-numbers which created unexpected changes. The change has been reverted and a test added. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] 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. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

…lutter#154364) flutter/engine@6d4cb16...3075094 2024-08-29 [email protected] Roll Skia from 10e44e318a72 to 1d16eab57a8e (3 revisions) (flutter/engine#54874) 2024-08-29 [email protected] Reland: Framework wide color (flutter#54415) (flutter/engine#54737) 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

## Description of what this PR is changing or adding, and why: Adding breaking change notice for wide gamut colors in the framework. ## Issues fixed by this PR (if any): flutter/flutter#127855 ## PRs or commits this PR depends on (if any): flutter/engine#54737 ## Presubmit checklist - [x] This PR is marked as draft with an explanation if not meant to land until a future stable release. - [x] This PR doesn’t contain automatically generated corrections (Grammarly or similar). - [x] This PR follows the [Google Developer Documentation Style Guidelines](https://developers.google.com/style) — for example, it doesn’t use _i.e._ or _e.g._, and it avoids _I_ and _we_ (first person). - [x] This PR uses [semantic line breaks](https://github.com/dart-lang/site-shared/blob/main/doc/writing-for-dart-and-flutter-websites.md#semantic-line-breaks) of 80 characters or fewer. --------- Co-authored-by: Shams Zakhour (ignore Sfshaza) <[email protected]> Co-authored-by: Loïc Sharma <[email protected]>

github-actions bot added the platform-web Code specifically for the web engine label Aug 23, 2024

gaaclarke requested review from jonahwilliams, matanlurey and flar August 23, 2024 16:16

gaaclarke mentioned this pull request Aug 23, 2024

color_models doesn't accommodate wide gamut adjustments to Flutter's Color james-alex/color_models#9

Open

gaaclarke mentioned this pull request Aug 26, 2024

Adds wide gamut framework breakage notice. flutter/website#11082

Merged

4 tasks

jonahwilliams approved these changes Aug 29, 2024

View reviewed changes

gaaclarke force-pushed the framework-wide-color-reland branch from a0db762 to 41b6c8d Compare August 29, 2024 17:53

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

matanlurey approved these changes Aug 29, 2024

View reviewed changes

auto-submit bot merged commit 62154d3 into flutter:main Aug 29, 2024
34 checks passed

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

Roll Flutter Engine from 6d4cb16395ca to 30750940b60a (2 revisions) flutter/flutter#154364

Merged

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

62154d33a Reland: Framework wide color (flutter#54415) (flutter/engin…

b670fb8

…e#54737)

gaaclarke mentioned this pull request Aug 29, 2024

Switched CupertinoDynamicColor to implements Color flutter/flutter#153938

Merged

9 tasks

chingjun added the revert Label used to revert changes in a closed and merged pull request. label Aug 30, 2024

auto-submit bot pushed a commit that referenced this pull request Aug 30, 2024

Revert "Reland: Framework wide color (#54415) (#54737)"

91abba5

This reverts commit 62154d3.

auto-submit bot mentioned this pull request Aug 30, 2024

Reverts "Reland: Framework wide color (#54415) (#54737)" #54885

Merged

8 tasks

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Aug 30, 2024

jonahwilliams reviewed Aug 30, 2024

View reviewed changes

Piinks mentioned this pull request Dec 19, 2024

Color deprecations should have a dart fix flutter/flutter#160617

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reland: Framework wide color (#54415) #54737

Reland: Framework wide color (#54415) #54737

gaaclarke commented Aug 23, 2024 •

edited

Loading

jonahwilliams commented Aug 23, 2024

gaaclarke commented Aug 23, 2024

gaaclarke commented Aug 23, 2024

gaaclarke commented Aug 23, 2024

gaaclarke commented Aug 23, 2024

gaaclarke commented Aug 23, 2024

jonahwilliams commented Aug 23, 2024

gaaclarke commented Aug 23, 2024

jonahwilliams commented Aug 23, 2024

gaaclarke commented Aug 23, 2024

gaaclarke commented Aug 29, 2024

jonahwilliams left a comment

gaaclarke commented Aug 29, 2024

chingjun commented Aug 30, 2024

jonahwilliams Aug 30, 2024

Reland: Framework wide color (#54415) #54737

Reland: Framework wide color (#54415) #54737

Conversation

gaaclarke commented Aug 23, 2024 • edited Loading

Pre-launch Checklist

jonahwilliams commented Aug 23, 2024

gaaclarke commented Aug 23, 2024

gaaclarke commented Aug 23, 2024

gaaclarke commented Aug 23, 2024

gaaclarke commented Aug 23, 2024

gaaclarke commented Aug 23, 2024

jonahwilliams commented Aug 23, 2024

gaaclarke commented Aug 23, 2024

jonahwilliams commented Aug 23, 2024

gaaclarke commented Aug 23, 2024

gaaclarke commented Aug 29, 2024

jonahwilliams left a comment

Choose a reason for hiding this comment

gaaclarke commented Aug 29, 2024

chingjun commented Aug 30, 2024

jonahwilliams Aug 30, 2024

Choose a reason for hiding this comment

gaaclarke commented Aug 23, 2024 •

edited

Loading