[Impeller] Fix issue about saveLayer ignoring opacity of paint with advanced blend mode by ColdPaleLight · Pull Request #41972 · flutter-team-archive/engine

ColdPaleLight · 2023-05-12T08:39:28Z

without patch

with patch

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.

…dvanced blend mode

jonahwilliams · 2023-05-12T15:45:29Z

  ASSERT_TRUE(OpenPlaygroundHere(builder.Build()));
 }

+TEST_P(DisplayListTest, SaveLayerWithOpacityAndAdvancedBlendMode) {


If you make this an Aiks unittest I believe we can get a golden test image out of it so it gets verified on CI.

actually, that would only show an issue if it were an arm64 mac. Still, might be good to do.

If you make this an Aiks unittest I believe we can get a golden test image out of it so it gets verified on CI.

Sounds good, done.

jonahwilliams

LGTM

flutter-dashboard · 2023-05-13T14:56:45Z

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 #41972 at sha 7e4abc5

ColdPaleLight · 2023-05-15T09:32:05Z

@jonahwilliams I moved the test from dl_unittests to aiks_unittests, and verified locally that there are no issues, but the bot added the label "will affect goldens" to this PR, I don't know how to fix it, would you mind teaching me how to do it? Thanks

flutter-dashboard · 2023-05-15T10:02:59Z

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

Changes reported for pull request #41972 at sha 3c61072

jonahwilliams · 2023-05-15T17:30:33Z

Are you able to log into the Skia gold instance and click approve for the new screen shots?

jonahwilliams · 2023-05-15T17:31:20Z

I'll do it for now, If we don't have instructions we should write some up

jonahwilliams · 2023-05-15T17:52:08Z

It looks like this is working correctly on arm macbooks and iOS, but on intel or vulkan/gles where framebuffer blending isn't used the opacity still doesn't look correct:

jonahwilliams · 2023-05-15T18:47:05Z

It looks like blend_filter_contents.cc doesn't have a path to accept the src snapshot opacity.

jonahwilliams · 2023-05-15T18:52:53Z

I'm able to fix the rest of the backends with the following patch:

diff --git a/impeller/entity/contents/filters/blend_filter_contents.cc b/impeller/entity/contents/filters/blend_filter_contents.cc
index 015197dd15..47d0c40911 100644
--- a/impeller/entity/contents/filters/blend_filter_contents.cc
+++ b/impeller/entity/contents/filters/blend_filter_contents.cc
@@ -148,6 +148,7 @@ static std::optional<Entity> AdvancedBlend(
       auto src_sampler = renderer.GetContext()->GetSamplerLibrary()->GetSampler(
           src_sampler_descriptor);
       blend_info.color_factor = 0;
+      blend_info.src_input_alpha = src_snapshot->opacity;
       FS::BindTextureSamplerSrc(cmd, src_snapshot->texture, src_sampler);
       frame_info.src_y_coord_scale = src_snapshot->texture->GetYCoordScale();
     }
diff --git a/impeller/entity/shaders/blending/advanced_blend.glsl b/impeller/entity/shaders/blending/advanced_blend.glsl
index cb8aaa7d1d..d60e9237e1 100644
--- a/impeller/entity/shaders/blending/advanced_blend.glsl
+++ b/impeller/entity/shaders/blending/advanced_blend.glsl
@@ -9,6 +9,7 @@
 
 uniform BlendInfo {
   float16_t dst_input_alpha;
+  float16_t src_input_alpha;
   float16_t color_factor;
   f16vec4 color;  // This color input is expected to be unpremultiplied.
 }
@@ -40,10 +41,11 @@ void main() {
   f16vec4 dst = IPHalfUnpremultiply(dst_sample);
   f16vec4 src = blend_info.color_factor > 0.0hf
                     ? blend_info.color
-                    : IPHalfUnpremultiply(Sample(
-                          texture_sampler_src,  // sampler
-                          v_src_texture_coords  // texture coordinates
-                          ));
+                    : IPHalfUnpremultiply(
+                          Sample(texture_sampler_src,  // sampler
+                                 v_src_texture_coords  // texture coordinates
+                                 ) *
+                          blend_info.src_input_alpha);
 
   f16vec4 blended = f16vec4(Blend(dst.rgb, src.rgb), 1.0hf) * dst.a;

I think this is reasonable? Tagging @dnfield / @bdero to confirm

bdero · 2023-05-15T19:11:59Z

Patch LGTM.

flutter-dashboard · 2023-05-16T03:32:08Z

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

Changes reported for pull request #41972 at sha 27de7cd

ColdPaleLight · 2023-05-16T06:29:03Z

@jonahwilliams Thanks for your patch! I added the patch to this PR. Now still need to approve new screenshots on skia gold, since I don't have access, would you mind approving it?

bdero

I approved the goldens. LGTM, thanks!

…t with advanced blend mode (flutter-team-archive/engine#41972)

flutter-team-archive/engine@c4d4b40...d7a5de6 2023-05-16 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from JCoP2Fekj3MBIqskE... to N4LwCRxg0oIevhQ_O... (flutter-team-archive/engine#42070) 2023-05-16 zhangzhijian.123@bytedance.com [Impeller] Fix issue about saveLayer ignoring opacity of paint with advanced blend mode (flutter-team-archive/engine#41972) 2023-05-16 ychris@google.com [ios_platform_view] Only remove platform views from flutter view in reset. (flutter-team-archive/engine#41709) 2023-05-16 godofredoc@google.com Add drone_dimensions as top level target property. (flutter-team-archive/engine#42064) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from JCoP2Fekj3MB to N4LwCRxg0oIe 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 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

chinmaygarde · 2023-05-17T23:37:55Z

Can we cherry-pick this into stable please? The request template is here.

…th advanced blend mode flutter-team-archive#41972

…ith advanced blend mode #41972 (#42126) cp of #41972

…6919) flutter-team-archive/engine@c4d4b40...d7a5de6 2023-05-16 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from JCoP2Fekj3MBIqskE... to N4LwCRxg0oIevhQ_O... (flutter-team-archive/engine#42070) 2023-05-16 zhangzhijian.123@bytedance.com [Impeller] Fix issue about saveLayer ignoring opacity of paint with advanced blend mode (flutter-team-archive/engine#41972) 2023-05-16 ychris@google.com [ios_platform_view] Only remove platform views from flutter view in reset. (flutter-team-archive/engine#41709) 2023-05-16 godofredoc@google.com Add drone_dimensions as top level target property. (flutter-team-archive/engine#42064) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from JCoP2Fekj3MB to N4LwCRxg0oIe 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 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

[Impeller] Fix issue about saveLayer ignoring opacity of paint with a…

df60aea

…dvanced blend mode

ColdPaleLight changed the title ~~[Impeller] Fix issue about saveLayer ignoring opacity of paint with a…~~ [Impeller] Fix issue about saveLayer ignoring opacity of paint with advanced blend mode May 12, 2023

ColdPaleLight requested a review from jonahwilliams May 12, 2023 08:39

Merge branch 'flutter:main' into blend_with_alpha

e60b775

jonahwilliams reviewed May 12, 2023

View reviewed changes

jonahwilliams approved these changes May 13, 2023

View reviewed changes

Move test form dl_unittests to aiks_unittests

7e4abc5

ColdPaleLight force-pushed the blend_with_alpha branch from d65f8c6 to 7e4abc5 Compare May 13, 2023 14:15

flutter-dashboard Bot added the will affect goldens label May 13, 2023

Merge branch 'flutter:main' into blend_with_alpha

3c61072

ColdPaleLight and others added 3 commits May 16, 2023 09:03

Fix blend filter contents

883167d

Merge branch 'flutter:main' into blend_with_alpha

f4c844b

Update malioc.json

27de7cd

bdero approved these changes May 16, 2023

View reviewed changes

ColdPaleLight self-assigned this May 16, 2023

ColdPaleLight added autosubmit Merge PR when tree becomes green via auto submit App and removed will affect goldens labels May 16, 2023

auto-submit Bot merged commit d06b4ab into flutter-team-archive:main May 16, 2023

engine-flutter-autoroll mentioned this pull request May 16, 2023

Roll Flutter Engine from c4d4b40c6596 to d06b4aba6e04 (3 revisions) flutter/flutter#126917

Closed

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

d06b4ab [Impeller] Fix issue about saveLayer ignoring opacity of pain…

98378c8

…t with advanced blend mode (flutter-team-archive/engine#41972)

engine-flutter-autoroll mentioned this pull request May 16, 2023

Roll Flutter Engine from c4d4b40c6596 to d7a5de672d1e (4 revisions) flutter/flutter#126919

Merged

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

d06b4ab [Impeller] Fix issue about saveLayer ignoring opacity of pain…

b984fb5

…t with advanced blend mode (flutter-team-archive/engine#41972)

ColdPaleLight added a commit to ColdPaleLight/engine that referenced this pull request May 18, 2023

[CP][Impeller] Fix issue about saveLayer ignoring opacity of paint wi…

dd2f164

…th advanced blend mode flutter-team-archive#41972

This was referenced May 18, 2023

[CP] Fix issue about saveLayer ignoring opacity of paint with advanced blend mode flutter/flutter#127100

Closed

[CP] [Impeller] Fix issue about saveLayer ignoring opacity of paint with advanced blend mode #41972 #42126

Merged

CaseyHillers pushed a commit that referenced this pull request May 18, 2023

[CP] [Impeller] Fix issue about saveLayer ignoring opacity of paint w…

de0e7e5

…ith advanced blend mode #41972 (#42126) cp of #41972

Uh oh!

Conversation

ColdPaleLight commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

jonahwilliams May 12, 2023

Choose a reason for hiding this comment

Uh oh!

jonahwilliams May 13, 2023

Choose a reason for hiding this comment

Uh oh!

ColdPaleLight May 13, 2023

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard Bot commented May 13, 2023

Uh oh!

ColdPaleLight commented May 15, 2023

Uh oh!

flutter-dashboard Bot commented May 15, 2023

Uh oh!

jonahwilliams commented May 15, 2023

Uh oh!

jonahwilliams commented May 15, 2023

Uh oh!

jonahwilliams commented May 15, 2023

Uh oh!

jonahwilliams commented May 15, 2023

Uh oh!

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

Uh oh!

Uh oh!

bdero commented May 15, 2023

Uh oh!

flutter-dashboard Bot commented May 16, 2023

Uh oh!

ColdPaleLight commented May 16, 2023

Uh oh!

bdero left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented May 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

ColdPaleLight commented May 12, 2023 •

edited

Loading

jonahwilliams commented May 15, 2023 •

edited

Loading