Fixes MatrixFilterContents rendering/coverage

[gaaclarke]

fixes: flutter/flutter#147807
relands #43943 (with fixes that hopefully avoid it being reverted again)

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 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.

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

[gaaclarke]

@bdero this is what I was talking about the other day. We should be sharing the math between rendering and coverage.

[bdero]

LGTM! Thanks for looking through this.

[flutter-dashboard]

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 #52880 at sha 4097c92

[gaaclarke]

The PR has some unexpected golden changes I'll have to audit:

example

[flutter-dashboard]

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

Changes reported for pull request #52880 at sha 4f4845b

[flutter-dashboard]

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

Changes reported for pull request #52880 at sha 49b23cb

[gaaclarke]

This passes all tests, but @bdero and I are auditing it to make sure it's correct. So don't land this yet.

[gaaclarke]

I think the check for translation hack works because it is by proxy detecting the Canvas::Save flow as opposed to the ImageFilter flow. In the Canvas::Save path the effect_transform is truncated to its basis before it is passed down (

engine/impeller/entity/entity_pass.cc

Line 590 in 4e33c10

subpass->transform_.Basis(), Entity::RenderingMode::kSubpass);

)

[gaaclarke]

@bdero I made that change we discussed, can you please take a look.

We decided offline that we are clearly seeing 3 different cases that need to be handled at the MatrixImageFilterContents level. So we expanded the RenderMode to reflect those 3 cases. I suspect maybe we are misusing the effect transform where we get it meaning 3 different things in 3 different contexts. Hopefully we can clear that up in the future.

[bdero]

LGTM, thanks!

[jonahwilliams]

This looks like its working incorrectly with some magnifier goldens in the framework:

https://flutter-gold.skia.org/search?crs=github&issue=148593&left_filter=name%3Dwidgets.widgets.magnifier.styled&master=true&patchsets=3&positive=true

Top is the Impeller golden diff and bottom is current Skia (which is unchanged)

[jonahwilliams]

reason for revert: unexpected framework golden change

[gaaclarke]

Here are the matrices for the magnifier.styled test:

• snapshot: translate(-600,-600)
• effect: scale(3,3,1)
• matrix: translate(-150, -150) * scale(2, 2, 1)

It's going through the kBackdropSubpass flow here. Backing out the backdrop flow fixes all tests except Bug147807 (the current state of the code.

All engine and frameworks tests pass with the following patch:

diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc
index ee28a0dd4f..1cc997d5cc 100644
--- a/impeller/entity/entity_pass.cc
+++ b/impeller/entity/entity_pass.cc
@@ -585,9 +586,10 @@ EntityPass::EntityResult EntityPass::GetEntityForElement(
       auto texture = pass_context.GetTexture();
       // Render the backdrop texture before any of the pass elements.
       const auto& proc = subpass->backdrop_filter_proc_;
       subpass_backdrop_filter_contents = proc(
           FilterInput::Make(std::move(texture)), subpass->transform_.Basis(),
-          Entity::RenderingMode::kBackdropSubpass);
+          Entity::RenderingMode::kImageFilterSubpass);

So the concept is right, there is a third state, the EntityPass::GetElementsCoverage flow for backdrop filters isn't passing in the basis of the transform, whereas the EntityPass::GetEntityForElement is. That appears to be the detection we need to differentiate the different cases. I plan on relanding this with the above patch and more apropos names for the different math than BackdropSubpass vs ImageFilterSubpass.

Edit: I was mistaken, that leaves us in the same state as before we landed this (everything works but Bug147807.

[gaaclarke]

As an experiment I tried to remove the Basis() call in EntityPass::GetEntityForElement (here) with Brandon's original patch. It get's the same results as this PR, all the engine tests pass but the framework one fails. It seems like we still need that (barring some sort of deeper refactoring).

[gaaclarke]

This patch seems to make the engine and framework tests happy. I'm not familiar with ContentBoundsPromise but it roughly sounds like it could effect the contents we get from the snapshot and determines how we should be performing the math:

diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc
index 984403629c..13e99b3ced 100644
--- a/impeller/entity/entity_pass.cc
+++ b/impeller/entity/entity_pass.cc
@@ -585,9 +585,17 @@ EntityPass::EntityResult EntityPass::GetEntityForElement(
       auto texture = pass_context.GetTexture();
       // Render the backdrop texture before any of the pass elements.
       const auto& proc = subpass->backdrop_filter_proc_;
       subpass_backdrop_filter_contents = proc(
           FilterInput::Make(std::move(texture)), subpass->transform_.Basis(),
-          Entity::RenderingMode::kImageFilterSubpass);
+          subpass->bounds_promise_ == ContentBoundsPromise::kContainsContents ?
+          Entity::RenderingMode::kImageFilterSubpass :
+          Entity::RenderingMode::kBackdropSubpass);
 
       // If the very first thing we render in this EntityPass is a subpass that
       // happens to have a backdrop filter, than that backdrop filter will end

If this is the correct latch we can rename the render modes accordingly.