[impeller] change input order in ColorFilterContents::MakeBlend #39038

jonahwilliams · 2023-01-20T21:02:15Z

Both pipeline and advanced blends internally used the order dst, src, src, however the ColorFilterContents::MakeFilter would jumble the order and the only usage in the engine compensated for this.

(Of course, still need to double check)

jonahwilliams · 2023-01-20T21:02:33Z

impeller/entity/contents/filters/blend_filter_contents.cc

@@ -159,7 +159,7 @@ static std::optional<Snapshot> PipelineBlend(
  using VS = BlendPipeline::VertexShader;
  using FS = BlendPipeline::FragmentShader;

-  auto input_snapshot = inputs[0]->GetSnapshot(renderer, entity);
+  auto dst_snapshot = inputs[0]->GetSnapshot(renderer, entity);


No change here, just renaming a bit for clarity

jonahwilliams · 2023-01-20T21:03:15Z

impeller/entity/contents/filters/color_filter_contents.cc

@@ -37,7 +37,7 @@ std::shared_ptr<ColorFilterContents> ColorFilterContents::MakeBlend(
  std::shared_ptr<BlendFilterContents> new_blend;
  for (auto in_i = inputs.begin() + 1; in_i < inputs.end(); in_i++) {
    new_blend = std::make_shared<BlendFilterContents>();
-    new_blend->SetInputs({*in_i, blend_input});
+    new_blend->SetInputs({blend_input, *in_i});


This was the bug. We start with input 0, and input 1, and but we shifted input 1 and input 0. This was worked around in entity_pass.cc.

(This re-arranging doesn't happen for pipeline blends which exit above. We could also change this code to not do the re-arranging for advanced blends with 2 inputs, but that has consistency implications

jonahwilliams · 2023-01-20T21:20:47Z

impeller/entity/entity_unittests.cc

@@ -856,7 +856,7 @@ TEST_P(EntityTest, Filters) {

    auto blend1 = ColorFilterContents::MakeBlend(
        BlendMode::kScreen,
-        {fi_bridge, FilterInput::Make(blend0), fi_bridge, fi_bridge});
+        {FilterInput::Make(blend0), fi_bridge, fi_bridge, fi_bridge});


This changes the output of this test. I think that is what we want, the way the previous algorithm worked for 2+ inputs was given the following filter inputs:

N0 N1 N2 N3

First N1 becomes dst and N0 becomes src to produce F1
Then N2 becomes dst and F1 becomes src to produce F2
Then N3 becomes dst and F2 becomes src to produce F3

Now this is changed (and this output looks different), first

N0 becomes dst and N1 becomes src to produce F1
F1 becomes dst and N2 becomes src to produce F2
F2 becomes dst and N3 becomes src to produce F3.

I think the latter is more intuitive initially, but the former also sort of makes sense as a kind of polish notation (or reverse, I don't remember which). But of course this ends up with inconsistencies for the most common 2 input case since that works differently if you're just using the pipeline blends.

Maybe we could retain the old behavior if desired with a new constructor, since you can't really express one in terms of the other directly

Since we don't rely on more then 2 inputs anywhere right now (I think the filter graph has bigger dreams, so maybe someday we will), changing the behavior now SGTM. I agree the latter seems more intuitive.

bdero

Assuming AiksTest.ColorWheel (which applies the blend via a savelayer) has identical behavior prior to this fix, LGTM!

bdero · 2023-01-20T22:04:09Z

impeller/entity/entity_unittests.cc

@@ -856,7 +856,7 @@ TEST_P(EntityTest, Filters) {

    auto blend1 = ColorFilterContents::MakeBlend(
        BlendMode::kScreen,
-        {fi_bridge, FilterInput::Make(blend0), fi_bridge, fi_bridge});
+        {FilterInput::Make(blend0), fi_bridge, fi_bridge, fi_bridge});


Since we don't rely on more then 2 inputs anywhere right now (I think the filter graph has bigger dreams, so maybe someday we will), changing the behavior now SGTM. I agree the latter seems more intuitive.

jonahwilliams · 2023-01-20T22:21:02Z

Looks good so far

…Blend (flutter/engine#39038)

…Blend (flutter/engine#39038) (#118913)

…nts::MakeBlend (flutter/engine#39038) (#118913) Commit: 1cdaf9e338412013edafc4b5310100ec6a496ad5

…ts::MakeBlend (flutter/engine#39038) (flutter/flutter#118913)

* ec6ff90 Roll Flutter Engine from ccccee513fb2 to d84b3dc74c9f (2 revisions) (flutter/flutter#118893) * 492d572 Cleanup obsolete --compact-async compiler option (flutter/flutter#118894) * f291eb3 Remove unnecessary null checks in integration_test (flutter/flutter#118861) * ab3c822 Remove unnecessary null checks in dev/devicelab (flutter/flutter#118842) * bf72f5e 58eb1061e Revert "Remove references to Observatory (#38919)" (flutter/engine#39035) (flutter/flutter#118899) * a07e8a6 [reland] Support wireless debugging (flutter/flutter#118895) * 3c769ef Cupertino navbar ellipsis fix (flutter/flutter#118841) * d1be731 3fead63ba Roll Dart SDK from ac4c63168ff2 to 03d35455a8d8 (1 revision) (flutter/engine#39036) (flutter/flutter#118909) * c0ad6ad Marks Mac plugin_test_macos to be unflaky (flutter/flutter#118706) * 8372001 Remove unnecessary null checks in flutter_test (flutter/flutter#118865) * 288a773 Remove unnecessary null checks in flutter_driver (flutter/flutter#118864) * bb73121 Remove unnecessary null checks in flutter/test (flutter/flutter#118905) * 15bc4e4 Marks Mac_android microbenchmarks to be flaky (flutter/flutter#118693) * 1cdaf9e e2c2e5009 [impeller] correct input order in ColorFilterContents::MakeBlend (flutter/engine#39038) (flutter/flutter#118913) * 49e025d Update android defines test to use emulator (flutter/flutter#118808) * bae4c1d Revert "Update android defines test to use emulator (#118808)" (flutter/flutter#118928) * 9837eb6 Remove unnecessary null checks in flutter/rendering (flutter/flutter#118923) * 25843bd Remove macOS impeller benchmarks (flutter/flutter#118917) * 70cecf6 Remove unnecessary null checks in dev/*_tests (flutter/flutter#118844) * c757df3 Remove unnecessary null checks in dev/bots (flutter/flutter#118846) * 5d74b5c Remove unnecessary null checks in flutter/painting (flutter/flutter#118925) * 7272c80 Remove unnecessary null checks in `flutter/{animation,semantics,scheduler}` (flutter/flutter#118922) * 2baea2f 7b68d71b8 Roll Dart SDK from 03d35455a8d8 to 807077cc5d1b (1 revision) (flutter/engine#39042) (flutter/flutter#118933) * 8d60a8c Roll Flutter Engine from 7b68d71b8d03 to 3a444b36657c (3 revisions) (flutter/flutter#118938) * 5ccdb81 bb4e8dfe2 Roll Fuchsia Linux SDK from rPo4_TYHCtkoOfRup... to S6wQW1tLFe-YnReaZ... (flutter/engine#39048) (flutter/flutter#118942) * b1f4070 ef7b1856a Roll Dart SDK from 8c2eb20b5376 to 548678dd684c (1 revision) (flutter/engine#39049) (flutter/flutter#118944)

…ts::MakeBlend (flutter/engine#39038) (flutter/flutter#118913)

* 3c769ef Cupertino navbar ellipsis fix (flutter/flutter#118841) * d1be731 3fead63ba Roll Dart SDK from ac4c63168ff2 to 03d35455a8d8 (1 revision) (flutter/engine#39036) (flutter/flutter#118909) * c0ad6ad Marks Mac plugin_test_macos to be unflaky (flutter/flutter#118706) * 8372001 Remove unnecessary null checks in flutter_test (flutter/flutter#118865) * 288a773 Remove unnecessary null checks in flutter_driver (flutter/flutter#118864) * bb73121 Remove unnecessary null checks in flutter/test (flutter/flutter#118905) * 15bc4e4 Marks Mac_android microbenchmarks to be flaky (flutter/flutter#118693) * 1cdaf9e e2c2e5009 [impeller] correct input order in ColorFilterContents::MakeBlend (flutter/engine#39038) (flutter/flutter#118913) * 49e025d Update android defines test to use emulator (flutter/flutter#118808) * bae4c1d Revert "Update android defines test to use emulator (#118808)" (flutter/flutter#118928) * 9837eb6 Remove unnecessary null checks in flutter/rendering (flutter/flutter#118923) * 25843bd Remove macOS impeller benchmarks (flutter/flutter#118917) * 70cecf6 Remove unnecessary null checks in dev/*_tests (flutter/flutter#118844) * c757df3 Remove unnecessary null checks in dev/bots (flutter/flutter#118846) * 5d74b5c Remove unnecessary null checks in flutter/painting (flutter/flutter#118925) * 7272c80 Remove unnecessary null checks in `flutter/{animation,semantics,scheduler}` (flutter/flutter#118922) * 2baea2f 7b68d71b8 Roll Dart SDK from 03d35455a8d8 to 807077cc5d1b (1 revision) (flutter/engine#39042) (flutter/flutter#118933) * 8d60a8c Roll Flutter Engine from 7b68d71b8d03 to 3a444b36657c (3 revisions) (flutter/flutter#118938) * 5ccdb81 bb4e8dfe2 Roll Fuchsia Linux SDK from rPo4_TYHCtkoOfRup... to S6wQW1tLFe-YnReaZ... (flutter/engine#39048) (flutter/flutter#118942) * b1f4070 ef7b1856a Roll Dart SDK from 8c2eb20b5376 to 548678dd684c (1 revision) (flutter/engine#39049) (flutter/flutter#118944) * 8065887 Add transform flip (flutter/flutter#116705) * 68b6e72 406dce64f Roll Fuchsia Mac SDK from ZTKDeVL1HDAwsZdhl... to l7jVM3Urw73TVWfee... (flutter/engine#39050) (flutter/flutter#118964) * cf628ad aa194347a Roll Fuchsia Linux SDK from S6wQW1tLFe-YnReaZ... to l3c_b-vRr-o6ZFX_M... (flutter/engine#39055) (flutter/flutter#118968) * f33e8d3 2a2dfaafb Roll Fuchsia Mac SDK from l7jVM3Urw73TVWfee... to 5TQ9IL4-Yu3KHCR-H... (flutter/engine#39056) (flutter/flutter#118969)

…tter#39038)

* 3c769ef Cupertino navbar ellipsis fix (flutter/flutter#118841) * d1be731 3fead63ba Roll Dart SDK from ac4c63168ff2 to 03d35455a8d8 (1 revision) (flutter/engine#39036) (flutter/flutter#118909) * c0ad6ad Marks Mac plugin_test_macos to be unflaky (flutter/flutter#118706) * 8372001 Remove unnecessary null checks in flutter_test (flutter/flutter#118865) * 288a773 Remove unnecessary null checks in flutter_driver (flutter/flutter#118864) * bb73121 Remove unnecessary null checks in flutter/test (flutter/flutter#118905) * 15bc4e4 Marks Mac_android microbenchmarks to be flaky (flutter/flutter#118693) * 1cdaf9e e2c2e5009 [impeller] correct input order in ColorFilterContents::MakeBlend (flutter/engine#39038) (flutter/flutter#118913) * 49e025d Update android defines test to use emulator (flutter/flutter#118808) * bae4c1d Revert "Update android defines test to use emulator (#118808)" (flutter/flutter#118928) * 9837eb6 Remove unnecessary null checks in flutter/rendering (flutter/flutter#118923) * 25843bd Remove macOS impeller benchmarks (flutter/flutter#118917) * 70cecf6 Remove unnecessary null checks in dev/*_tests (flutter/flutter#118844) * c757df3 Remove unnecessary null checks in dev/bots (flutter/flutter#118846) * 5d74b5c Remove unnecessary null checks in flutter/painting (flutter/flutter#118925) * 7272c80 Remove unnecessary null checks in `flutter/{animation,semantics,scheduler}` (flutter/flutter#118922) * 2baea2f 7b68d71b8 Roll Dart SDK from 03d35455a8d8 to 807077cc5d1b (1 revision) (flutter/engine#39042) (flutter/flutter#118933) * 8d60a8c Roll Flutter Engine from 7b68d71b8d03 to 3a444b36657c (3 revisions) (flutter/flutter#118938) * 5ccdb81 bb4e8dfe2 Roll Fuchsia Linux SDK from rPo4_TYHCtkoOfRup... to S6wQW1tLFe-YnReaZ... (flutter/engine#39048) (flutter/flutter#118942) * b1f4070 ef7b1856a Roll Dart SDK from 8c2eb20b5376 to 548678dd684c (1 revision) (flutter/engine#39049) (flutter/flutter#118944) * 8065887 Add transform flip (flutter/flutter#116705) * 68b6e72 406dce64f Roll Fuchsia Mac SDK from ZTKDeVL1HDAwsZdhl... to l7jVM3Urw73TVWfee... (flutter/engine#39050) (flutter/flutter#118964) * cf628ad aa194347a Roll Fuchsia Linux SDK from S6wQW1tLFe-YnReaZ... to l3c_b-vRr-o6ZFX_M... (flutter/engine#39055) (flutter/flutter#118968) * f33e8d3 2a2dfaafb Roll Fuchsia Mac SDK from l7jVM3Urw73TVWfee... to 5TQ9IL4-Yu3KHCR-H... (flutter/engine#39056) (flutter/flutter#118969)

* ec6ff90 Roll Flutter Engine from ccccee513fb2 to d84b3dc74c9f (2 revisions) (flutter/flutter#118893) * 492d572 Cleanup obsolete --compact-async compiler option (flutter/flutter#118894) * f291eb3 Remove unnecessary null checks in integration_test (flutter/flutter#118861) * ab3c822 Remove unnecessary null checks in dev/devicelab (flutter/flutter#118842) * bf72f5e 58eb1061e Revert "Remove references to Observatory (#38919)" (flutter/engine#39035) (flutter/flutter#118899) * a07e8a6 [reland] Support wireless debugging (flutter/flutter#118895) * 3c769ef Cupertino navbar ellipsis fix (flutter/flutter#118841) * d1be731 3fead63ba Roll Dart SDK from ac4c63168ff2 to 03d35455a8d8 (1 revision) (flutter/engine#39036) (flutter/flutter#118909) * c0ad6ad Marks Mac plugin_test_macos to be unflaky (flutter/flutter#118706) * 8372001 Remove unnecessary null checks in flutter_test (flutter/flutter#118865) * 288a773 Remove unnecessary null checks in flutter_driver (flutter/flutter#118864) * bb73121 Remove unnecessary null checks in flutter/test (flutter/flutter#118905) * 15bc4e4 Marks Mac_android microbenchmarks to be flaky (flutter/flutter#118693) * 1cdaf9e e2c2e5009 [impeller] correct input order in ColorFilterContents::MakeBlend (flutter/engine#39038) (flutter/flutter#118913) * 49e025d Update android defines test to use emulator (flutter/flutter#118808) * bae4c1d Revert "Update android defines test to use emulator (#118808)" (flutter/flutter#118928) * 9837eb6 Remove unnecessary null checks in flutter/rendering (flutter/flutter#118923) * 25843bd Remove macOS impeller benchmarks (flutter/flutter#118917) * 70cecf6 Remove unnecessary null checks in dev/*_tests (flutter/flutter#118844) * c757df3 Remove unnecessary null checks in dev/bots (flutter/flutter#118846) * 5d74b5c Remove unnecessary null checks in flutter/painting (flutter/flutter#118925) * 7272c80 Remove unnecessary null checks in `flutter/{animation,semantics,scheduler}` (flutter/flutter#118922) * 2baea2f 7b68d71b8 Roll Dart SDK from 03d35455a8d8 to 807077cc5d1b (1 revision) (flutter/engine#39042) (flutter/flutter#118933) * 8d60a8c Roll Flutter Engine from 7b68d71b8d03 to 3a444b36657c (3 revisions) (flutter/flutter#118938) * 5ccdb81 bb4e8dfe2 Roll Fuchsia Linux SDK from rPo4_TYHCtkoOfRup... to S6wQW1tLFe-YnReaZ... (flutter/engine#39048) (flutter/flutter#118942) * b1f4070 ef7b1856a Roll Dart SDK from 8c2eb20b5376 to 548678dd684c (1 revision) (flutter/engine#39049) (flutter/flutter#118944)

[impeller] correct input order in ColorFilterContents::MakeBlend

857f53a

jonahwilliams commented Jan 20, 2023

View reviewed changes

jonahwilliams requested a review from bdero January 20, 2023 21:16

jonahwilliams commented Jan 20, 2023

View reviewed changes

jonahwilliams changed the title ~~[impeller] correct input order in ColorFilterContents::MakeBlend~~ [impeller] change input order in ColorFilterContents::MakeBlend Jan 20, 2023

bdero approved these changes Jan 20, 2023

View reviewed changes

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

auto-submit bot merged commit e2c2e50 into flutter:main Jan 20, 2023

jonahwilliams deleted the fix_blend_constructor_order branch January 20, 2023 22:22

engine-flutter-autoroll mentioned this pull request Jan 20, 2023

Roll Flutter Engine from 3fead63ba100 to e2c2e5009a55 (1 revision) flutter/flutter#118913

Merged

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

e2c2e5009 [impeller] correct input order in ColorFilterContents::Make…

438e794

…Blend (flutter/engine#39038)

auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jan 20, 2023

e2c2e5009 [impeller] correct input order in ColorFilterContents::Make…

1cdaf9e

…Blend (flutter/engine#39038) (#118913)

sourcegraph-bot pushed a commit to sgtest/megarepo that referenced this pull request Jan 20, 2023

flutter: e2c2e5009 [impeller] correct input order in ColorFilterConte…

4162609

…nts::MakeBlend (flutter/engine#39038) (#118913) Commit: 1cdaf9e338412013edafc4b5310100ec6a496ad5

This was referenced Jan 21, 2023

Roll Flutter from 095b1abdad6f to b1f4070d5bd0 (26 revisions) flutter/packages#3078

Merged

Roll Flutter from a07e8a6ac43d to b1f4070d5bd0 (20 revisions) flutter/plugins#7004

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 21, 2023

1cdaf9e e2c2e5009 [impeller] correct input order in ColorFilterConten…

f1a3af9

…ts::MakeBlend (flutter/engine#39038) (flutter/flutter#118913)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 21, 2023

1cdaf9e e2c2e5009 [impeller] correct input order in ColorFilterConten…

1096e83

…ts::MakeBlend (flutter/engine#39038) (flutter/flutter#118913)

engine-flutter-autoroll mentioned this pull request Jan 21, 2023

Roll Flutter from a07e8a6ac43d to b1f4070d5bd0 (20 revisions) flutter/plugins#7005

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 21, 2023

1cdaf9e e2c2e5009 [impeller] correct input order in ColorFilterConten…

0c118cb

…ts::MakeBlend (flutter/engine#39038) (flutter/flutter#118913)

engine-flutter-autoroll mentioned this pull request Jan 22, 2023

Roll Flutter from a07e8a6ac43d to f33e8d3701b5 (24 revisions) flutter/plugins#7006

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 22, 2023

1cdaf9e e2c2e5009 [impeller] correct input order in ColorFilterConten…

c8f3f05

…ts::MakeBlend (flutter/engine#39038) (flutter/flutter#118913)

engine-flutter-autoroll mentioned this pull request Jan 22, 2023

Roll Flutter from a07e8a6ac43d to f33e8d3701b5 (24 revisions) flutter/plugins#7007

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 22, 2023

1cdaf9e e2c2e5009 [impeller] correct input order in ColorFilterConten…

c51dd44

…ts::MakeBlend (flutter/engine#39038) (flutter/flutter#118913)

ricardoamador pushed a commit to ricardoamador/engine that referenced this pull request Jan 25, 2023

[impeller] correct input order in ColorFilterContents::MakeBlend (flu…

173299f

…tter#39038)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[impeller] change input order in ColorFilterContents::MakeBlend #39038

[impeller] change input order in ColorFilterContents::MakeBlend #39038

jonahwilliams commented Jan 20, 2023

jonahwilliams Jan 20, 2023

jonahwilliams Jan 20, 2023

jonahwilliams Jan 20, 2023

jonahwilliams Jan 20, 2023

jonahwilliams Jan 20, 2023

bdero Jan 20, 2023

bdero left a comment

bdero Jan 20, 2023

jonahwilliams commented Jan 20, 2023

[impeller] change input order in ColorFilterContents::MakeBlend #39038

[impeller] change input order in ColorFilterContents::MakeBlend #39038

Conversation

jonahwilliams commented Jan 20, 2023

jonahwilliams Jan 20, 2023

Choose a reason for hiding this comment

jonahwilliams Jan 20, 2023

Choose a reason for hiding this comment

jonahwilliams Jan 20, 2023

Choose a reason for hiding this comment

jonahwilliams Jan 20, 2023

Choose a reason for hiding this comment

jonahwilliams Jan 20, 2023

Choose a reason for hiding this comment

bdero Jan 20, 2023

Choose a reason for hiding this comment

bdero left a comment

Choose a reason for hiding this comment

bdero Jan 20, 2023

Choose a reason for hiding this comment

jonahwilliams commented Jan 20, 2023