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

[Impeller] Add CPU implementations for all color filters #42692

Merged
merged 1 commit into from
Jun 9, 2023

Conversation

bdero
Copy link
Member

@bdero bdero commented Jun 9, 2023

Part of flutter/flutter#127232.

Adds a CPU implementation for ColorMatrix and the color space conversions. Also changes the blend signature for consistency.

These will be necessary to make the mask blur fast path continue working in the presence of color filters. In general, we can use these to eliminate a needlessly expensive image-based color filter for the non-image color sources.

@bdero bdero self-assigned this Jun 9, 2023
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

@@ -1931,238 +1931,6 @@ TEST_P(EntityTest, SrgbToLinearFilter) {
ASSERT_TRUE(OpenPlaygroundHere(callback));
}

TEST_P(EntityTest, TTTBlendColor) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's replacing these tests?

See also flutter/flutter#128606. It's possible some of these tests are encoding bad behavior, but it seems veyr likely to me that if they are not encoding bad behavior they're missing cases where there's bad behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I see below the new tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I just moved this existing test into GeometryTest.ColorBlend. And yes, some of them are wrong -- we used to use these for absorbing the DrawPaints, which introduced bugs. Some blends are also missing from the test at the moment.

Copy link
Member Author

@bdero bdero Jun 9, 2023

Choose a reason for hiding this comment

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

The test also has too many cases and needs to be rewritten in general, or at least be annotated to state why each cases is useful and not redundant.

@bdero bdero merged commit 35e14c5 into flutter:main Jun 9, 2023
30 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 9, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jun 9, 2023
…128612)

flutter/engine@071e1fb...488876e

2023-06-09 skia-flutter-autoroll@skia.org Roll ANGLE from a185cb8c8924 to 3e4f4caebcb0 (2 revisions) (flutter/engine#42701)
2023-06-09 bdero@google.com [Impeller] Add CPU implementations for all color filters (flutter/engine#42692)
2023-06-09 jonahwilliams@google.com [Impeller] add explicit VMA flush to device memory writes. (flutter/engine#42685)
2023-06-09 30870216+gaaclarke@users.noreply.github.com [Impeller] Makes validation layers flag work for android (flutter/engine#42625)
2023-06-09 jmccandless@google.com Platform channel for predictive back (flutter/engine#39208)

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 jacksongardner@google.com,rmistry@google.com,zra@google.com 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Archived in project
3 participants