Changes DlColor to support wide gamut colors #54473

gaaclarke · 2024-08-09T18:30:26Z

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.

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.

flutter-dashboard · 2024-08-15T16:11:35Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

flar · 2024-08-19T23:02:56Z

I'm a bit worried that there were inconsequential golden changes. Fingers crossed that this doesn't trip up other tests downstream.

I am too. Was it a case of using a uint32_t color that now maps to something slightly different when going through floats and back to ints? It was an off-by-1 error so that makes sense.

gaaclarke · 2024-08-19T23:03:44Z

I am too. Was it a case of using a uint32_t color that now maps to something slightly different when going through floats and back to ints? It was an off-by-1 error so that makes sense.

Yea, if you check out the changes they look identical, just an off by one from the floats.

jonahwilliams · 2024-08-19T23:25:11Z

Godspeed

jonahwilliams · 2024-08-20T02:14:33Z

reason for revert: golden diffs like https://flutter-engine-gold.skia.org/detail?grouping=name%3Dimpeller_Play_AiksTest_BlendModeSrcAlphaLuminosity_OpenGLES%26source_type%3Dflutter-engine&digest=107ccd2cd1170746b1ffc4d31184e789 look incorrect, potentially an alpha issue

This reverts commit 93e5c81.

Reverts: #54473 Initiated by: jonahwilliams Reason for reverting: golden diffs like https://flutter-engine-gold.skia.org/detail?grouping=name%3Dimpeller_Play_AiksTest_BlendModeSrcAlphaLuminosity_OpenGLES%26source_type%3Dflutter-engine&digest=107ccd2cd1170746b1ffc4d31184e789 look incorrect, potentially an alpha issue Original PR Author: gaaclarke Reviewed By: {flar} This change reverts the following previous change: 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. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…e#54473)

…153739) flutter/engine@5355c0b...86f269b 2024-08-20 [email protected] Roll Skia from e65e02c50d66 to 30a00629e135 (3 revisions) (flutter/engine#54634) 2024-08-20 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Changes DlColor to support wide gamut colors (#54473)" (flutter/engine#54636) 2024-08-19 [email protected] [Impeller] account for negative scale in max basis xy. (flutter/engine#54630) 2024-08-19 [email protected] Changes DlColor to support wide gamut colors (flutter/engine#54473) 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] 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

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

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 1) The default color is now opaque black, not transparent black (not the issue for revert) 1) Updated a test to send in valid numbers when constructing a color and added asserts to avoid those problems in the future. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

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 ## 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

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

[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

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

[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#153739) flutter/engine@5355c0b...86f269b 2024-08-20 [email protected] Roll Skia from e65e02c50d66 to 30a00629e135 (3 revisions) (flutter/engine#54634) 2024-08-20 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Changes DlColor to support wide gamut colors (flutter#54473)" (flutter/engine#54636) 2024-08-19 [email protected] [Impeller] account for negative scale in max basis xy. (flutter/engine#54630) 2024-08-19 [email protected] Changes DlColor to support wide gamut colors (flutter/engine#54473) 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] 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 10, 2024

gaaclarke force-pushed the framework-wide-color-dlcolor branch from 5cb24dc to e8d8de5 Compare August 12, 2024 17:14

gaaclarke mentioned this pull request Aug 12, 2024

Framework wide color #54415

Merged

8 tasks

gaaclarke force-pushed the framework-wide-color-dlcolor branch from e8d8de5 to fdc36bc Compare August 12, 2024 17:58

gaaclarke added 16 commits August 15, 2024 08:38

Adds DlColor wide gamut colors support.

1071a2b

fixed translation issue

624e4c6

fixed memory crashes in the dl tests

190b22a

fixed some test failures

bb6f2cc

updated more tests

6dfc01a

updated more literals

3802bf3

++

07d3a6a

more tests

468bc63

tests

fe103df

++

0c52081

++

fc37d07

++

3ce239c

++

ac388e5

++

edd3dcd

updated equality

cbf1e99

added p3->srgb_xr transform

cf35445

gaaclarke force-pushed the framework-wide-color-dlcolor branch from 85a426f to cf35445 Compare August 15, 2024 15:43

gaaclarke marked this pull request as ready for review August 15, 2024 15:46

gaaclarke marked this pull request as draft August 15, 2024 16:05

gaaclarke added 5 commits August 15, 2024 09:13

license update

6ecf978

fixed unsafe skia conversions

961577c

fixed vertices without color

5cfa175

removed more reinterpret casts

9866b13

fixed conversion loops

4b276e9

gaaclarke marked this pull request as ready for review August 16, 2024 21:41

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

auto-submit bot merged commit 93e5c81 into flutter:main Aug 19, 2024
33 checks passed

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

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

Revert "Changes DlColor to support wide gamut colors (#54473)"

c9e390b

This reverts commit 93e5c81.

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

Reverts "Changes DlColor to support wide gamut colors (#54473)" #54636

Merged

8 tasks

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

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

Roll Flutter Engine from 5355c0b60e6e to 5c17e465f61a (3 revisions) flutter/flutter#153736

Closed

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

93e5c810a Changes DlColor to support wide gamut colors (flutter/engin…

49197a9

…e#54473)

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

Roll Flutter Engine from 5355c0b60e6e to 86f269bd5261 (4 revisions) flutter/flutter#153739

Merged

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

93e5c810a Changes DlColor to support wide gamut colors (flutter/engin…

d9ff6de

…e#54473)

gaaclarke mentioned this pull request Aug 20, 2024

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

Merged

8 tasks

gaaclarke mentioned this pull request Aug 23, 2024

Reland: Framework wide color (#54415) #54737

Merged

8 tasks

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

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

Merged

8 tasks

gaaclarke mentioned this pull request Aug 30, 2024

Reland: Framework wide color (#54415) (#54737) #54905

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes DlColor to support wide gamut colors #54473

Changes DlColor to support wide gamut colors #54473

gaaclarke commented Aug 9, 2024 •

edited

Loading

flutter-dashboard bot commented Aug 15, 2024

flar commented Aug 19, 2024

gaaclarke commented Aug 19, 2024

jonahwilliams commented Aug 19, 2024

jonahwilliams commented Aug 20, 2024

Changes DlColor to support wide gamut colors #54473

Changes DlColor to support wide gamut colors #54473

Conversation

gaaclarke commented Aug 9, 2024 • edited Loading

Pre-launch Checklist

flutter-dashboard bot commented Aug 15, 2024

flar commented Aug 19, 2024

gaaclarke commented Aug 19, 2024

jonahwilliams commented Aug 19, 2024

jonahwilliams commented Aug 20, 2024

gaaclarke commented Aug 9, 2024 •

edited

Loading