[Impeller] simplify invert colors flag by supporting composed color filters. #46391

jonahwilliams · 2023-09-28T23:42:26Z

To avoid the ugly complicated code for conditionally applying both colorfilters, make the dl dispatcher handle merging these filters together in a defined order. The original bug is caused by not checking for invert colors in a CPU fast path.

flutter-dashboard · 2023-09-29T00:14:05Z

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.

Changes reported for pull request #46391 at sha bc1dcab

jonahwilliams · 2023-09-29T00:14:24Z

impeller/display_list/dl_dispatcher.cc

+  if (paint_.color_filter) {
+    auto color_filter = ToColorFilter(filter);
+    paint_.color_filter = std::make_shared<MergedColorFilter>(
+        std::move(paint_.color_filter), color_filter);
+  } else {
+    paint_.color_filter = ToColorFilter(filter);
+  }
 }

 // |flutter::DlOpReceiver|
 void DlDispatcher::setInvertColors(bool invert) {
-  paint_.invert_colors = invert;
+  if (paint_.color_filter) {
+    auto invert_filter = ColorFilter::MakeMatrix(kColorInversion);
+    paint_.color_filter = std::make_shared<MergedColorFilter>(
+        invert_filter, std::move(paint_.color_filter));
+  } else {
+    paint_.color_filter = ColorFilter::MakeMatrix(kColorInversion);
+  }


This sucks but there is no good answer to:

Be resilient to dl dispatch order and

Keep paint as a struct with no private fields.

To solve the composing problem, one possibility would be to add a "GetInverted()" to ColorFilter that returns a new composed filter with the color filter inversion appended.

(This is one of those cases where I would clone things liberally to prevent bugs -- the only reason these things are smart pointers is for dynamic dispatch, but like so many things in Impeller, these should really just be discriminated unions, but we currently have an aversion for those in C++)

Right, this is 2.5 which is "callers need to be aware of both fields" which I guess is just us, but not idea.

jonahwilliams · 2023-09-29T00:20:14Z

Hmm, all of these colors should be the same....

jonahwilliams · 2023-09-29T00:54:28Z

okay, changed the filters around...

flutter-dashboard · 2023-09-29T03:26:48Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #46391 at sha 2ca5a11

dnfield · 2023-09-29T17:07:58Z

impeller/aiks/aiks_unittests.cc

 }

-TEST_P(AiksTest, CanRenderInvertedImage) {
+constexpr ColorMatrix kColorInversion = {.array = {


This is now defined in at least three places - it'd be nice if it could live in some common header somewhere.

We could pull this logic all the way up too painting.dart but then we wont be able to test it with impeller yet. For the purposes of this test though it doesn't really matter what the filter is, just that it composes.

dnfield

LGTM with nit

bdero · 2023-09-29T17:56:55Z

impeller/display_list/dl_dispatcher.cc

+  if (paint_.color_filter) {
+    auto color_filter = ToColorFilter(filter);
+    paint_.color_filter =
+        ColorFilter::MakeComposed(paint_.color_filter, color_filter);


Does the paint state get cleared out between every draw call? I think this solution implicitly relies on setColorFilter and setInvertColors always being called exactly once per dispatch.

I hate this

… color filters. (#46391)" This reverts commit 7326ccb.

…d color filters." (#46414) Reverts #46391 Initiated by: jonahwilliams This change reverts the following previous change: Original Description: Fixes flutter/flutter#135699 To avoid the ugly complicated code for conditionally applying both colorfilters, make the dl dispatcher handle merging these filters together in a defined order. The original bug is caused by not checking for invert colors in a CPU fast path.

…ed color filters. (flutter/engine#46391)

…135765) flutter/engine@d5fcd78...48973d7 2023-09-29 [email protected] Manual roll Dart SDK from e042e83c85a5 to 955a3a964621 (2 revisions) (flutter/engine#46412) 2023-09-29 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] simplify invert colors flag by supporting composed color filters." (flutter/engine#46414) 2023-09-29 [email protected] [Impeller] simplify invert colors flag by supporting composed color filters. (flutter/engine#46391) 2023-09-29 [email protected] Manual roll Dart SDK from 077d768accfa to e042e83c85a5 (3 revisions) (flutter/engine#46409) 2023-09-29 [email protected] Roll Skia from 9f8242377455 to 2d4045f55fd5 (3 revisions) (flutter/engine#46408) 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

…lutter#135765) flutter/engine@d5fcd78...48973d7 2023-09-29 [email protected] Manual roll Dart SDK from e042e83c85a5 to 955a3a964621 (2 revisions) (flutter/engine#46412) 2023-09-29 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] simplify invert colors flag by supporting composed color filters." (flutter/engine#46414) 2023-09-29 [email protected] [Impeller] simplify invert colors flag by supporting composed color filters. (flutter/engine#46391) 2023-09-29 [email protected] Manual roll Dart SDK from 077d768accfa to e042e83c85a5 (3 revisions) (flutter/engine#46409) 2023-09-29 [email protected] Roll Skia from 9f8242377455 to 2d4045f55fd5 (3 revisions) (flutter/engine#46408) 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

…ilters. (#46391) Fixes flutter/flutter#135699 To avoid the ugly complicated code for conditionally applying both colorfilters, make the dl dispatcher handle merging these filters together in a defined order. The original bug is caused by not checking for invert colors in a CPU fast path.

…d color filters." (#46414) Reverts #46391 Initiated by: jonahwilliams This change reverts the following previous change: Original Description: Fixes flutter/flutter#135699 To avoid the ugly complicated code for conditionally applying both colorfilters, make the dl dispatcher handle merging these filters together in a defined order. The original bug is caused by not checking for invert colors in a CPU fast path.

jonahwilliams added 2 commits September 28, 2023 16:40

[Impeller] Simplify invert colors.

ca24ee6

++

bc1dcab

github-actions bot added the e: impeller label Sep 28, 2023

jonahwilliams marked this pull request as ready for review September 29, 2023 00:13

flutter-dashboard bot added the will affect goldens label Sep 29, 2023

jonahwilliams commented Sep 29, 2023

View reviewed changes

jonahwilliams requested a review from dnfield September 29, 2023 00:14

rename to composed

baa08b7

jonahwilliams added 2 commits September 28, 2023 17:38

++

ccf0ec5

++

4c15cf3

jonahwilliams added 4 commits September 28, 2023 17:56

++

1b33b1f

Update dl_dispatcher.cc

dad6ac8

Update dl_dispatcher.cc

5ff8158

Update dl_dispatcher.cc

2ca5a11

dnfield reviewed Sep 29, 2023

View reviewed changes

dnfield approved these changes Sep 29, 2023

View reviewed changes

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 29, 2023

auto-submit bot merged commit 7326ccb into flutter:main Sep 29, 2023

bdero reviewed Sep 29, 2023

View reviewed changes

jonahwilliams added the revert Label used to revert changes in a closed and merged pull request. label Sep 29, 2023

auto-submit bot mentioned this pull request Sep 29, 2023

Reverts "[Impeller] simplify invert colors flag by supporting composed color filters." #46414

Merged

auto-submit bot pushed a commit that referenced this pull request Sep 29, 2023

Revert "[Impeller] simplify invert colors flag by supporting composed…

48a26a0

… color filters. (#46391)" This reverts commit 7326ccb.

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Sep 29, 2023

engine-flutter-autoroll mentioned this pull request Sep 29, 2023

Roll Flutter Engine from d5fcd78a879b to 48973d7f9beb (5 revisions) flutter/flutter#135765

Merged

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

7326ccb71 [Impeller] simplify invert colors flag by supporting compos…

ab549e6

…ed color filters. (flutter/engine#46391)

[Impeller] simplify invert colors flag by supporting composed color filters. #46391

[Impeller] simplify invert colors flag by supporting composed color filters. #46391

Uh oh!

Conversation

jonahwilliams commented Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Sep 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Sep 29, 2023

Uh oh!

jonahwilliams commented Sep 29, 2023

Uh oh!

flutter-dashboard bot commented Sep 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

bdero Sep 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonahwilliams commented Sep 28, 2023 •

edited

Loading

bdero Sep 29, 2023 •

edited

Loading