[Impeller] Turned on wide gamut support by default. #39801

gaaclarke · 2023-02-22T21:42:33Z

After having run a memory audit on Flutter Gallery I believe we can turn on wide gamut support by default for impeller. Here are the ramifications of this change:

If an opaque surface is used and no transparent wide gamut images are used, there should be no change in performance or memory usage.
If a transparent surface is used; the surface will take up 2x memory (ex 4MB on iPhone 7) and will have a runtime performance cost.
If wide gamut transparent images are used, they will take up 2x memory. Opaque wide-gamut images will take up the same amount of memory.

Flutter gallery test results

Test 1

Steps:

Tap "Material"
Scroll down and back to "Cards"
Tap "Cards"
Scroll to bottom of "Cards" example

Notice that the card images are wide gamut, AdobeRGB.

Memory increase: 10.401 MB (+8.7%)

Test 2

Steps:

Just launch the gallery

Notice that there shouldn't be any wide gamut images.

Memory Increase: 1.22 MB (+1%, this is probably a fluke of measurement)

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 Hixie said 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.

gaaclarke · 2023-03-13T17:56:17Z

We're getting to the point where we'll get better testing if we just turn this on. Anyone that runs into problems can just use the flag to turn it off.

chinmaygarde

I'd want to hold on this. In the last go/impeller-weekly, this was slated for iOS Preview Exit + 1 release. We just landed the Impeller flag flip test run towards the end of last week and are reasoning about the benchmark updates. I don't want to add another variable to those numbers right now. After the branch cut though, this lgtm.

chinmaygarde · 2023-03-23T20:12:19Z

Closing this and putting it in the icebox so its off the triage queue. But we can re-open once the release train leaves.

gaaclarke · 2023-03-23T20:25:31Z

Can I switch it back to a reopen it and switch it back to a draft so it is out of your review queue? I don't keep track of my closed PRs but drafts I do.

flutter-dashboard · 2023-03-23T20:32:14Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

gaaclarke · 2023-04-17T18:03:13Z

I think this is ready now, no?

chinmaygarde · 2023-04-17T20:45:14Z

I think this is ready now, no?

Yup yup. We discussed this in the last weekly and were waiting for you to be back. Let's land this ASAP. Can you give a heads up in go/flutter-chat when you do so we watch out for memory bench updates?

chinmaygarde · 2023-04-17T20:46:31Z

The ci.yaml stuff was changed recently and needed a rebase. Just did one to kick off the presubs.

…r/engine#39801)

bernaferrari · 2023-04-18T18:44:00Z

I'm kind of lost, but does this mean we should have a Color.p3() soon? Not sure what happens to current hex colors, or if this is "images only".

gaaclarke · 2023-04-18T18:54:44Z

I'm kind of lost, but does this mean we should have a Color.p3() soon? Not sure what happens to current hex colors, or if this is "images only".

There is no plan to make any change to the Color class. This just effects loading wide gamut images, gradients and reading/writing pixels to ui.Image. You can't specify them when selecting the color of a widget, for example. We should make an issue for that if people are interested.

bernaferrari · 2023-04-18T18:55:26Z

Oh, ok. Thanks!

…125057) flutter/engine@72b6862...879308a 2023-04-18 [email protected] [Impeller] Turned on wide gamut support by default. (flutter/engine#39801) 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

This reverts commit 879308a.

Reverts #39801 `customer: chalk` hit some rendering issues that was narrowed down to this: b/278976014

After having run a memory audit on Flutter Gallery I believe we can turn on wide gamut support by default for impeller. Here are the ramifications of this change: 1) If an opaque surface is used and no transparent wide gamut images are used, there should be no change in performance or memory usage. 1) If a transparent surface is used; the surface will take up 2x memory (ex 4MB on iPhone 7) and will have a runtime performance cost. 1) If wide gamut **transparent** images are used, they will take up 2x memory. Opaque wide-gamut images will take up the same amount of memory. ## Flutter gallery test results ### Test 1 Steps: * Tap "Material" * Scroll down and back to "Cards" * Tap "Cards" * Scroll to bottom of "Cards" example Notice that the card images are wide gamut, AdobeRGB. **Memory increase:** 10.401 MB (+8.7%) ### Test 2 Steps: * Just launch the gallery Notice that there shouldn't be any wide gamut images. **Memory Increase:** 1.22 MB (+1%, this is probably a fluke of measurement) ## 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]. - [ ] 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 Hixie said 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 [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

flutter-dashboard bot added the platform-ios label Feb 22, 2023

chinmaygarde assigned gaaclarke Feb 28, 2023

chinmaygarde added the e: impeller label Feb 28, 2023

chinmaygarde changed the title ~~[impeller] Turned on wide gamut support by default~~ [Impeller] Turned on wide gamut support by default. Feb 28, 2023

gaaclarke marked this pull request as ready for review March 13, 2023 17:55

gaaclarke requested a review from chinmaygarde March 13, 2023 17:55

chinmaygarde suggested changes Mar 13, 2023

View reviewed changes

chinmaygarde closed this Mar 23, 2023

chinmaygarde reopened this Mar 23, 2023

flutter-dashboard bot added the needs tests label Mar 23, 2023

chinmaygarde marked this pull request as draft March 23, 2023 20:32

gaaclarke marked this pull request as ready for review April 17, 2023 18:02

gaaclarke requested a review from chinmaygarde April 17, 2023 18:02

chinmaygarde approved these changes Apr 17, 2023

View reviewed changes

Turned on wide gamut support by default (for impeller).

b7c6993

chinmaygarde force-pushed the default-wide-gamut branch from c972e8d to b7c6993 Compare April 17, 2023 20:45

gaaclarke merged commit 879308a into flutter:main Apr 18, 2023

engine-flutter-autoroll mentioned this pull request Apr 18, 2023

Roll Flutter Engine from 72b68622fffa to 879308a52228 (1 revision) flutter/flutter#125057

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 18, 2023

879308a52 [Impeller] Turned on wide gamut support by default. (flutte…

1424e1f

…r/engine#39801)

zanderso added a commit that referenced this pull request Apr 21, 2023

Revert "[Impeller] Turned on wide gamut support by default. (#39801)"

703c734

This reverts commit 879308a.

zanderso mentioned this pull request Apr 21, 2023

Revert "[Impeller] Turned on wide gamut support by default." #41382

Merged

auto-submit bot pushed a commit that referenced this pull request Apr 21, 2023

Revert "[Impeller] Turned on wide gamut support by default." (#41382)

a858ced

Reverts #39801 `customer: chalk` hit some rendering issues that was narrowed down to this: b/278976014

gaaclarke mentioned this pull request May 12, 2023

[Impeller] Turned on wide gamut support by default. (#39801) #41965

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Impeller] Turned on wide gamut support by default. #39801

[Impeller] Turned on wide gamut support by default. #39801

Uh oh!

gaaclarke commented Feb 22, 2023

Uh oh!

gaaclarke commented Mar 13, 2023

Uh oh!

chinmaygarde left a comment

Uh oh!

chinmaygarde commented Mar 23, 2023

Uh oh!

gaaclarke commented Mar 23, 2023

Uh oh!

flutter-dashboard bot commented Mar 23, 2023

Uh oh!

gaaclarke commented Apr 17, 2023

Uh oh!

chinmaygarde commented Apr 17, 2023

Uh oh!

chinmaygarde commented Apr 17, 2023

Uh oh!

bernaferrari commented Apr 18, 2023

Uh oh!

gaaclarke commented Apr 18, 2023

Uh oh!

bernaferrari commented Apr 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[Impeller] Turned on wide gamut support by default. #39801

[Impeller] Turned on wide gamut support by default. #39801

Uh oh!

Conversation

gaaclarke commented Feb 22, 2023

Flutter gallery test results

Test 1

Test 2

Pre-launch Checklist

Uh oh!

gaaclarke commented Mar 13, 2023

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Mar 23, 2023

Uh oh!

gaaclarke commented Mar 23, 2023

Uh oh!

flutter-dashboard bot commented Mar 23, 2023

Uh oh!

gaaclarke commented Apr 17, 2023

Uh oh!

chinmaygarde commented Apr 17, 2023

Uh oh!

chinmaygarde commented Apr 17, 2023

Uh oh!

bernaferrari commented Apr 18, 2023

Uh oh!

gaaclarke commented Apr 18, 2023

Uh oh!

bernaferrari commented Apr 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants