Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wide gamut framework gradient test #153976

Merged

Conversation

gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Aug 22, 2024

issue: #127855
depends on flutter/engine#54748 being rolled into the framework

Pre-launch Checklist

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

auto-submit bot pushed a commit to flutter/engine that referenced this pull request Sep 4, 2024
This implements wide gamut color support for gradients in the engine.

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

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
@gaaclarke gaaclarke force-pushed the wide-gamut-framework-gradient-test branch from 38d8fad to 35d63b1 Compare September 5, 2024 00:19
@gaaclarke gaaclarke marked this pull request as ready for review September 5, 2024 00:21
@gaaclarke
Copy link
Member Author

I'm putting this into review, it shouldn't land until flutter/engine#54748 rolls into the framework though.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 5, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 5, 2024
Copy link
Contributor

auto-submit bot commented Sep 5, 2024

auto label is removed for flutter/flutter/153976, due to - The status or check suite Windows tool_integration_tests_2_7 has failed. Please fix the issues identified (or deflake) before re-applying this label.

@gaaclarke gaaclarke force-pushed the wide-gamut-framework-gradient-test branch from 6f64c08 to 9eaf9b5 Compare October 25, 2024 16:14
@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 25, 2024
@auto-submit auto-submit bot merged commit 4f66f13 into flutter:master Oct 25, 2024
148 checks passed
@jonahwilliams
Copy link
Member

reason for revert: failing on postsubmit

@jonahwilliams jonahwilliams added the revert Autorevert PR (with "Reason for revert:" comment) label Oct 25, 2024
@jonahwilliams
Copy link
Member

auto-submit bot pushed a commit that referenced this pull request Oct 25, 2024
@auto-submit auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Oct 25, 2024
auto-submit bot added a commit that referenced this pull request Oct 25, 2024
Reverts: #153976
Initiated by: jonahwilliams
Reason for reverting: failing on postsubmit
Original PR Author: gaaclarke

Reviewed By: {jonahwilliams}

This change reverts the following previous change:
issue: #127855
depends on flutter/engine#54748 being rolled into the framework
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 25, 2024
@gaaclarke
Copy link
Member Author

things got broke in the engine after this PR didn't land because of a flake that blocked the autosubmit =(

@gaaclarke
Copy link
Member Author

I ran these locally against 57c4bd0 and they still didn't pass. Something happened between testing them locally and getting them landed in the separate repo.

@jonahwilliams
Copy link
Member

Lets make sure we don't need to cherry pick anything to get wide gamut working...

@gaaclarke
Copy link
Member Author

Lets make sure we don't need to cherry pick anything to get wide gamut working...

Looking into it. FWIW the standard wide gamut tests are passing, this is just for gradients.

@gaaclarke
Copy link
Member Author

I verified that the colors are wide gamut all the way to sending them to the buffer. On the output side the closest color to p3 red is (0.9965759667968751 -0.0014706249999999477 -0.0014706249999999477). I was wondering if the blending of colors was making looking for p3 red fail by being slightly off but it seems like there may be some clamping done in the fragment shader.

@gaaclarke
Copy link
Member Author

@gaaclarke
Copy link
Member Author

We could fix the dithering math, but dithering actually isn't desirable either when we have wide gamut colors since we have 10bits per color component.

@jonahwilliams
Copy link
Member

Wide gamut doesn't fix gradient banding. But we don't actually need to clamp there, so we can just delete it. For non wide gamut the driver will clamp

@gaaclarke
Copy link
Member Author

Wide gamut doesn't fix gradient banding.

It does in our case, banding with 10bits per color component is beyond the perceptible limit.

But we don't actually need to clamp there, so we can just delete it.

Yea, playing with it.

gaaclarke added a commit to gaaclarke/flutter that referenced this pull request Oct 25, 2024
@gaaclarke
Copy link
Member Author

Removing the dithering works for the linear and the sweep gradient. For the conical and the radial gradient it looks like you aren't guaranteed to get your requested color back so tweaking the threshold for those tests will be necessary.

@jonahwilliams
Copy link
Member

I think if you want the test to be stable you need to either use an epsilon or make it a single color gradient

gaaclarke added a commit to flutter/engine that referenced this pull request Oct 25, 2024
tests: flutter/flutter#157643
fixes revert from flutter/flutter#153976

The pixel format will clamp if it needs to anyway so this isn't
necessary.

## 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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 26, 2024
gaaclarke added a commit that referenced this pull request Oct 26, 2024
This was reverted because it failed to run. Colors were getting clamped
in the dithering fragment shader.

One change was made when relanding, i increased the epsilon for the
radial and conical gradients. They don't appear to give back the exact
color you asked for.

Do not land until flutter/engine#56140 is rolled
into the framework.

## 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], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [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/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 26, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Oct 26, 2024
Roll Flutter from 4faa4a415ec9 to 5a11904383d1 (67 revisions)

flutter/flutter@4faa4a4...5a11904

2024-10-26 [email protected] Relands "Wide gamut framework gradient test (#153976)" (flutter/flutter#157643)
2024-10-26 [email protected] Roll Flutter Engine from 7c5c5fe5c84d to c9b8ac96f6ce (3 revisions) (flutter/flutter#157662)
2024-10-26 [email protected] Add test for `navigator_state.restorable_push_and_remove_until.0.dart` (flutter/flutter#157595)
2024-10-26 [email protected] Tighten up `throwToolExit`, explain when to use it. (flutter/flutter#157561)
2024-10-26 [email protected] Remove extraneous `throw`. (flutter/flutter#157658)
2024-10-26 [email protected] Add tests for `navigator.restorable_push.0.dart` (flutter/flutter#157492)
2024-10-25 [email protected] Roll Flutter Engine from 43e4d9a30666 to 7c5c5fe5c84d (1 revision) (flutter/flutter#157644)
2024-10-25 [email protected] Roll Flutter Engine from 5061358e255f to 43e4d9a30666 (1 revision) (flutter/flutter#157637)
2024-10-25 [email protected] Roll Flutter Engine from eb867e055790 to 5061358e255f (2 revisions) (flutter/flutter#157623)
2024-10-25 [email protected] Create flutter specific leak troubleshooting guidance. (flutter/flutter#157396)
2024-10-25 [email protected] Update CupertinoNavigationBar to support large layout (flutter/flutter#157133)
2024-10-25 [email protected] Roll Flutter Engine from 38e9be1f74fa to eb867e055790 (3 revisions) (flutter/flutter#157613)
2024-10-25 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Wide gamut framework gradient test (#153976)" (flutter/flutter#157615)
2024-10-25 [email protected] Wide gamut framework gradient test (flutter/flutter#153976)
2024-10-25 [email protected] Roll Flutter Engine from b413d9996c86 to 38e9be1f74fa (2 revisions) (flutter/flutter#157604)
2024-10-25 [email protected] Roll Packages from a556f0f to e0c4f55 (2 revisions) (flutter/flutter#157605)
2024-10-25 [email protected] Support backdrop key in flutter framework. (flutter/flutter#157278)
2024-10-25 [email protected] Add 3.24.4 changelog to master (flutter/flutter#157600)
2024-10-25 [email protected] Update flutter.groovy to catch unknown task exception when finding api task (flutter/flutter#157282)
2024-10-25 [email protected] Roll Flutter Engine from c4b0184c8783 to b413d9996c86 (1 revision) (flutter/flutter#157580)
2024-10-25 [email protected] Roll Flutter Engine from b1c2ba8c4d52 to c4b0184c8783 (1 revision) (flutter/flutter#157578)
2024-10-25 [email protected] Add test for `build_owner.0.dart` (flutter/flutter#157499)
2024-10-25 [email protected] Add tests  for `focusable_action_detector.0.dart` (flutter/flutter#157575)
2024-10-25 [email protected] Add test for `navigator.restorable_push_and_remove_until.0.dart` (flutter/flutter#157487)
2024-10-25 [email protected] Roll Flutter Engine from 29440ed1e225 to b1c2ba8c4d52 (1 revision) (flutter/flutter#157572)
2024-10-25 [email protected] Roll Flutter Engine from 88716d804aef to 29440ed1e225 (1 revision) (flutter/flutter#157569)
2024-10-25 [email protected] Roll Flutter Engine from b8b28c80a737 to 88716d804aef (2 revisions) (flutter/flutter#157567)
2024-10-24 [email protected] Roll Flutter Engine from 48ff670d256b to b8b28c80a737 (2 revisions) (flutter/flutter#157564)
2024-10-24 [email protected] Use discenrable characters (replace `' ð��� ð��� '` in error logs) (flutter/flutter#157548)
2024-10-24 [email protected] Remove unused `PubDependenciesProjectValidator`. (flutter/flutter#157516)
2024-10-24 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Upgrade tests to AGP 8.7/Gradle 8.10.2/Kotlin 1.8.10 (#157032)" (flutter/flutter#157559)
2024-10-24 [email protected] Mark mac impeller as bringup. (flutter/flutter#157551)
2024-10-24 [email protected] Deprecate `ThemeData.dialogBackgroundColor` in favor of `DialogThemeData.backgroundColor` (flutter/flutter#155072)
2024-10-24 [email protected] Upgrade tests to AGP 8.7/Gradle 8.10.2/Kotlin 1.8.10 (flutter/flutter#157032)
2024-10-24 [email protected] Roll Flutter Engine from 246344f26edc to 48ff670d256b (2 revisions) (flutter/flutter#157544)
2024-10-24 [email protected] Allow opting out of `.flutter-plugins`, opt-out in `refreshPluginsList`. (flutter/flutter#157527)
2024-10-24 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reverts "Added a warning if `flutter.groovy` uses a `.flutter-plugins` file. (#157388)" (#157541)" (flutter/flutter#157549)
2024-10-24 [email protected] Changes the offset computation to first item for RenderSliverMainAxisGroup (flutter/flutter#154688)
2024-10-24 [email protected] Roll Packages from 5e03bb1 to a556f0f (7 revisions) (flutter/flutter#157539)
2024-10-24 [email protected] Add partial test for flutter build ios-framework on non-module (flutter/flutter#157482)
2024-10-24 [email protected] Add example to SafeArea docs (flutter/flutter#157228)
2024-10-24 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Added a warning if `flutter.groovy` uses a `.flutter-plugins` file. (#157388)" (flutter/flutter#157541)
2024-10-24 [email protected] Added a warning if `flutter.groovy` uses a `.flutter-plugins` file. (flutter/flutter#157388)
2024-10-24 [email protected] Roll Flutter Engine from be56084344d1 to 246344f26edc (2 revisions) (flutter/flutter#157504)
2024-10-24 [email protected] Add ability to disable CupertinoSegmentedControl (flutter/flutter#152813)
2024-10-24 [email protected] Update `Tab.height` parameter doc for tab height lower than default (flutter/flutter#157443)
...
gaaclarke added a commit to gaaclarke/engine that referenced this pull request Oct 28, 2024
tests: flutter/flutter#157643
fixes revert from flutter/flutter#153976

The pixel format will clamp if it needs to anyway so this isn't
necessary.

## 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
auto-submit bot pushed a commit to flutter/engine that referenced this pull request Nov 5, 2024
tests: flutter/flutter#157643
fixes revert from flutter/flutter#153976

The pixel format will clamp if it needs to anyway so this isn't necessary.

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/
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

*Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.*

*List which issues are fixed by this PR. You must list at least one issue.*

*If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
nick9822 pushed a commit to nick9822/flutter that referenced this pull request Dec 18, 2024
tests: flutter#157643
fixes revert from flutter#153976

The pixel format will clamp if it needs to anyway so this isn't
necessary.

## 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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants