-
Notifications
You must be signed in to change notification settings - Fork 6k
Hooks up framework wide gamut to engine wide gamut #54567
Hooks up framework wide gamut to engine wide gamut #54567
Conversation
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
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
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]. <!-- Links --> [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
b2132f9
to
4956584
Compare
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. |
4956584
to
85418d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I put a breakpoint on this code and that test DrawVerticesWithInvalidIndices never hits this. I think we may have a flakey golden? |
If its just for GLES lets disable it? |
I'm waiting to see if it happens again. Now I keep getting time outs on the mac tests after having rebased =T |
85418d4
to
4956584
Compare
If it happens again I'll disable it. If it doesn't happen again I'll file an issue. It could be 1/1,000,000 in which case we can probably leave it for now. |
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]. <!-- Links --> [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]. <!-- Links --> [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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
lib/ui/painting/paint.cc
Outdated
static_cast<uint8_t>(std::round(blue * 255.f)) << 0; | ||
// TODO(gaaclarke): Pass down color info to DlColor. | ||
return DlColor(encoded_color); | ||
return DlColor(alpha, red, green, blue, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shower thought, don't we need to do any color space conversions to RGBA/extend RGBA here? Or should we do that in dispatch based on the destination surface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last PR added DlColor::withColorSpace
for this conversion. The next PR is an integration test that makes sure everything works. It's been a couple of weeks since I tested the whole flow. It's possible it got lost in all this juggling. Let me double check I didn't lose it, because I'm not seeing a call to DlColor::withColorSpace
anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct, sir, reintroduced. When's that monorepo coming again?
DlColor dl_color(alpha, red, green, blue, | ||
static_cast<DlColorSpace>(colorspace)); | ||
|
||
return dl_color.withColorSpace(DlColorSpace::kExtendedSRGB); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you also need the gradient conversions too, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're jumping ahead, that happens in #54748.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conversion happens in a different place for gradients. It was easier to do it in dart for them. We can reevaluate after we get our integration tests landed. It's ideal to do the conversion in c++.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that you need to move it into the engine eventually so that we can handle android, which may have a displayP3 color space and not an extended srgb space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…154381) flutter/engine@dc7eefb...da87db6 2024-08-30 [email protected] [android] Disable AHB swapchain and import on huawei API 29 devices. (flutter/engine#54879) 2024-08-29 [email protected] Hooks up framework wide gamut to engine wide gamut (flutter/engine#54567) 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
Reason for revert: Breaking internal tests. See b/363125155 |
This reverts commit e3a15e1.
…#54884) Reverts: #54567 Initiated by: chingjun Reason for reverting: Breaking internal tests. See b/363125155 Original PR Author: gaaclarke Reviewed By: {jonahwilliams, chinmaygarde} This change reverts the following previous change: issue: flutter/flutter#127855 integration test: #54415 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
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
issue: flutter/flutter#127855 integration test: flutter#54415 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[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]. <!-- Links --> [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 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
relands #54567 depends on #54905 This was reverted because the PR that it depends on was reverted. There is nothing to be addressed here. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
…lutter#154381) flutter/engine@dc7eefb...da87db6 2024-08-30 [email protected] [android] Disable AHB swapchain and import on huawei API 29 devices. (flutter/engine#54879) 2024-08-29 [email protected] Hooks up framework wide gamut to engine wide gamut (flutter/engine#54567) 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
issue: flutter/flutter#127855
integration test: #54415
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.