Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

PlatformView Blur for Backdrop Filter #34596

Merged
merged 55 commits into from
Sep 6, 2022
Merged

Conversation

emilyabest
Copy link
Contributor

@emilyabest emilyabest commented Jul 11, 2022

Description:
This PR applies a Gaussian Blur filter to PlatformViews. Currently, the blur is applied when transform mutations are called, however this PR will be combined with PR #34355 (Create BackdropFilter Mutator) to blur PlatformViews for the backdrop filter mutator.

Fixed issues:

  • PlatformView was not programmed to blur

Fixes: flutter/flutter#109783

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.

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

@cyanglaz
Copy link
Contributor

Also need to rebase or merge upstream/main and resolve conflicts.

@cyanglaz
Copy link
Contributor

The new tests seem to be failing: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8808283911804283841/+/u/test:_Host_Tests_for_ios_debug_sim__2_/stdout?format=raw

@cyanglaz cyanglaz self-assigned this Jul 18, 2022
@chinmaygarde
Copy link
Member

@cyanglaz Looks like all the comments have been addressed. But @emilyabest, the test failures do look related.

@emilyabest
Copy link
Contributor Author

@cyanglaz Looks like all the comments have been addressed. But @emilyabest, the test failures do look related.

@chinmaygarde We realized the implementation is missing functionality for multiple backdrop filters and removing backdrop filters. I'm testing that code now and will look into the failing tests after pushing it to the PR. Thanks for looking over the comments!

@cyanglaz
Copy link
Contributor

The scenario test is failing due to blur is implemented in this PR. However, the scenario app uses a none-zero origin for the PlatformView, making the blur not working probably.
We need to fix the step 1 in flutter/flutter#109700 before landing this PR.

@cyanglaz
Copy link
Contributor

Need to merge #34355 before landing this.

@chinmaygarde
Copy link
Member

@cyanglaz Trying to understand the status here. Per your last comment, this patch depends on #34355 which landed but had to be reverted due to flutter/flutter#109831 and now the perf fixes are going to be incorporated here. Is that right?

@cyanglaz
Copy link
Contributor

cyanglaz commented Aug 25, 2022

@cyanglaz Trying to understand the status here. Per your last comment, this patch depends on #34355 which landed but had to be reverted due to flutter/flutter#109831 and now the perf fixes are going to be incorporated here. Is that right?

Yes, #34355 is now included in this PR with the fix. This PR is now waiting for #35501 to land so the scenario app's backdrop filter blur screenshot can be updated more appropriately in this PR.

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

This PR now contains re-landing of #34355 with the fix that the PlatformViews are cleared at the start of each frame + tests.

cc @zanderso Do you mind give it a review?

@LongCatIsLooong I did some change since last LGTM:

  1. Add the re-land of Pushing BackdropFilter Mutator #34355.
  2. Add the code that clears the visited_platform_views_ list at the start of a frame + test.
  3. Updated the screenshot of the scenario app for blurring PlatformView.

}

- (void)dealloc {
[self.blurEffectView release];
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not reference self in its dealloc state, so

[_blurEffectView release];
_blurEffectView = nil;

@@ -777,6 +805,7 @@ - (BOOL)flt_hasFirstResponderInViewHierarchySubtree {
void FlutterPlatformViewsController::ResetFrameState() {
slices_.clear();
composition_order_.clear();
visited_platform_views_.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the fix for flutter/flutter#109783

@@ -296,6 +1096,86 @@ - (void)testCompositePlatformView {
XCTAssertTrue(CGRectEqualToRect(platformViewRectInFlutterView, CGRectMake(100, 100, 300, 300)));
}

- (void)testBackdropFilterCorrectlyPushedAndReset {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the test for This if the fix for flutter/flutter#109783

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong left a comment

Choose a reason for hiding this comment

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

The objective C part LGTM

Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

I am not super familiar with this code, but I do see where the list of platform views is being cleared, so rslgtm.

@cyanglaz
Copy link
Contributor

cyanglaz commented Sep 1, 2022

Wait until flutter/flutter#110688 is landed before landing this.

@cyanglaz cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 6, 2022
@auto-submit auto-submit bot merged commit 81f0924 into flutter:main Sep 6, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 6, 2022
@@ -43,6 +43,9 @@ void BackdropFilterLayer::Preroll(PrerollContext* context,
const SkMatrix& matrix) {
Layer::AutoPrerollSaveLayerState save =
Layer::AutoPrerollSaveLayerState::Create(context, true, bool(filter_));
if (context->view_embedder != nullptr) {
context->view_embedder->PushFilterToVisitedPlatformViews(filter_);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this push the outstanding clips as well? The filter will only be applied to the region inside those clips. More specifically, the output of the filter on the PlatformView will be clipped by the clips.

Copy link
Contributor

@cyanglaz cyanglaz Sep 8, 2022

Choose a reason for hiding this comment

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

Yes, it should when we figure out how to filter a specific region inside the PlatformView.
We should have a follow up PR to support filter only a part of the PlatformView (at least a rect), which will fix flutter/flutter#50183
And the clip path should be added to the method in that PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-ios
Projects
None yet
Development

Successfully merging this pull request may close these issues.

visited platform views list not cleared
6 participants