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

Pushing BackdropFilter Mutator #34355

Merged
merged 53 commits into from
Aug 10, 2022

Conversation

JTKryptic
Copy link
Contributor

@JTKryptic JTKryptic commented Jun 28, 2022

Creates the container functions for the BackdropFilter mutation that will push the mutator to the mutator stack of each visited PlatformView. Continues from #34408 (Create blanket backdrop filter mutator) and to be combined with #34596 (PlatformView Blur for Backdrop Filter).

Related Issues: #43902

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.

Sorry, something went wrong.

@flutter-dashboard flutter-dashboard bot added embedder Related to the embedder API platform-ios labels Jun 28, 2022
@chinmaygarde
Copy link
Member

@cyanglaz Looks like this is good for another review? @JTKryptic Can you address the presubmit failures please?

@cyanglaz
Copy link
Contributor

@chinmaygarde
@JTKryptic Is going to explore some other approach and I'm going to mark this PR as draft for now.

@cyanglaz cyanglaz marked this pull request as draft June 30, 2022 22:53
@flutter-dashboard
Copy link

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

@cyanglaz cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 10, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 10, 2022

  • This commit is not mergeable and has conflicts. Please rebase your PR and fix all the conflicts.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 10, 2022
@jmagman
Copy link
Member

jmagman commented Aug 10, 2022

This isn't compiling:

../../flutter/shell/common/shell_test_external_view_embedder.h:72:13: error: functions that differ only in their return type cannot be overloaded
  SkCanvas* CompositeEmbeddedView(int view_id) override;
  ~~~~~~~~~ ^
../../flutter/shell/common/shell_test_external_view_embedder.h:69:24: note: previous declaration is here
  EmbedderPaintContext CompositeEmbeddedView(int view_id) override;
  ~~~~~~~~~~~~~~~~~~~~ ^
1 error generated.

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.

Still LGTM after resolving conflicts.

@cyanglaz cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 10, 2022
@auto-submit auto-submit bot merged commit 3ec9f21 into flutter:main Aug 10, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 10, 2022
emilyabest pushed a commit to emilyabest/engine that referenced this pull request Aug 12, 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The filter is being pushed but never cleared. The platform view gets an ever-increasing number of filters until app is unresponsive.

Copy link
Contributor

@cyanglaz cyanglaz Aug 18, 2022

Choose a reason for hiding this comment

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

Can you point me to where the filter isn't cleared? IIRC at each frame, the preroll context is re-created, meaning the mutator stack is re-created, so the filters in the stacks are cleared.

I agree with the other comment, the visitedPlatformView list is not cleared.

Copy link
Contributor

Choose a reason for hiding this comment

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

I ran with a print statement of the number of mutators in vector_ in MutatorsStack::PushBackdropFilter, it was continuously increasing

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yeah, that's because the visitedPlatformView is increasing and multiple filters are pushed into the same platform view. This is the symptom of not clearing visitedPlatformView list, #34596 will fix it.

@@ -29,6 +29,7 @@ void PlatformViewLayer::Preroll(PrerollContext* context,
context->display_list_enabled);
context->view_embedder->PrerollCompositeEmbeddedView(view_id_,
std::move(params));
context->view_embedder->PushVisitedPlatformView(view_id_);
Copy link
Contributor

Choose a reason for hiding this comment

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

Platform view is pushed but nothing ever clears that list, so it will have the same issue as the filters, it slows down rendering until app is unresponsive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we are fixing it here: #34596

@cyanglaz
Copy link
Contributor

Created an issue to track the platform view list not cleared bug: flutter/flutter#109783

@zanderso
Copy link
Member

@cyanglaz In light of the P2 here flutter/flutter#109831, this should be reverted, and relanded only with the fix.

zanderso added a commit to zanderso/engine that referenced this pull request Aug 19, 2022
zanderso added a commit that referenced this pull request Aug 19, 2022
cyanglaz pushed a commit to emilyabest/engine that referenced this pull request Aug 22, 2022
CaseyHillers pushed a commit to CaseyHillers/engine that referenced this pull request Aug 24, 2022
CaseyHillers pushed a commit that referenced this pull request Aug 24, 2022
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 embedder Related to the embedder API platform-ios
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants