Skip to content

[Impeller] Reland clipped BackdropFilter square corners fix - #195716

Merged
auto-submit[bot] merged 3 commits into
flutter:masterfrom
andywolff:sqfix
Nov 10, 2026
Merged

auto-submit[bot] merged 3 commits into
flutter:masterfrom
andywolff:sqfix

Conversation

@andywolff

@andywolff andywolff commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #162172
Fixes #190602
Fixes #192992
Fixes #195423

Revised reland of the square corners fix in #191096.

Previously, EntityPassClipStack::RecordRestore unconditionally popped a single clip replay entry even when the restored scope had skipped a redundant clip or recorded multiple clips, which caused parent rounded clips to get dropped across BackdropFilter flips (see my comment on #190602). EntityPassClipStack now tracks clip_height on ReplayResult so RecordRestore only pops entries above restore_height. This is the only production code change needed to fix the issue.

I structured this PR as two commits to make the diff against the original landing easy to see. Commit 1 is #191096 unmodified, and Commit 2 reverts all of the Canvas and ClipContents changes back to master. In #191096, I gated should_use_onscreen on is_onscreen_ in Canvas::SaveLayer to make unit testing easier, which routed DisplayListToTexture snapshots through Canvas::BlitToOnscreen with an uncleared 4x MSAA depth attachment and caused the action_sheet_test.dart alpha flake (see my comment on #195423). Reverting the Canvas changes fixes that flake and removes the clip-depth clamping that @flar flagged in review.

Instead of modifying Canvas for testability, I replaced the CreateTestCanvas tests from Commit 1 with DisplayListToTexture pixel-readback tests in canvas_unittests.cc, along with unit tests in clip_stack_unittests.cc and aiks_dl_clip_unittests.cc. I rewrote those tests significantly so they are smaller for the same coverage, and added a test which covers the problem which resulted in the alpha flake behavior.

Pre-launch Checklist

@github-actions github-actions Bot added engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Oct 30, 2026
@andywolff andywolff added the CICD Run CI/CD label Nov 2, 2026
@andywolff
andywolff force-pushed the sqfix branch 9 times, most recently from c71e245 to 5b009b4 Compare November 5, 2026 19:56
@flutter-dashboard

This comment was marked as resolved.

Fixes flutter#190602, see my
[comment](flutter#190602 (comment))
on that issue for details about the problems I identified.
Also fixes flutter#162172

When a route contained rounded clips (such as `CupertinoSheetRoute`),
intermediate child ink features, and a backdrop filter (such as a
blurred app bar), the clip stack replay logic prematurely popped parent
clips and exceeded the depth buffer budget across render pass splits,
causing subsequent header draws to render with 90° square corners. This
PR addresses both mechanisms:

First, `EntityPassClipStack::RecordRestore` was popping replay entities
unconditionally whenever any clip was restored. In a sheet containing
ink decorations (like list item ripple effects), child clip restores at
deeper heights would prematurely discard the root parent clip. We now
track `clip_height` in `ReplayResult` so that restorations only pop
entities when `clip_height > restore_height`, preserving parent clips
across sibling restores. When replaying those clips in
`ClipContents::Render`, we also supply the entity's model matrix to
properly recompute the MVP matrix against the new pass's orthographic
projection instead of reusing the stale matrix from the prior pass.

Second, the pass depth budget was exhausting across backdrop splits.
`Canvas::kMaxDepth` (`1 << 24`) was mismatched with
`Entity::kDepthEpsilon` (`1.0 / 262144.0`, or `2^-18`), allowing integer
depths to exceed the 18-bit precision of the depth buffer. Aligning
`Canvas::kMaxDepth` to `262,143` and capping depth allocations in
`Save`/`SaveLayer` to `min(current_depth_ + total_content_depth,
parent_clip_depth)` keeps clip replay and subsequent draw operations
within valid precision bounds. In `Canvas::Restore`, we also prevent
unconditional depth jumps to `kMaxDepth` for unclipped saves, preserving
the allocated budget for subsequent sibling draws.

Reproducing this interaction in automated tests required testing the
onscreen pass-splitting branch (`IsOnscreen()`), which normal offscreen
unit tests bypass. We added a `ScopedOnscreenOverride` test hook in
`Canvas` and constructed a multi-layer display list recreating the route
clip, child ink items, blurred backdrop bar, and trailing app bar:

* `clip_stack_unittests.cc`: Added
`EntityPassClipStackTest.RestoringChildClipPreservesParentReplayEntities`
to isolate parent clip retention across sibling restores.
* `canvas_unittests.cc`: Added
`AiksTest.ClipDepthMaintainedAcrossBackdropFilterAndLayers` to verify
depth invariants and precision bounds (`<= kMaxDepth`), and
`AiksTest.ParentClipDepthBudgetPreservedAfterBackdropLayerRestore` to
verify parent depth budget restoration.
* `dl_golden_unittests.cc` (`impeller_golden_tests`): Added
`DlGoldenTest.CanRenderClippedBackdropFilterWithSuperellipse` for
pixel-accurate golden image verification in CI against Skia Gold.

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [AI contribution guidelines] and understand my
responsibilities, or I am not using AI tools.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[AI contribution guidelines]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
- Revert Canvas and ClipContents changes from the original PR back to master,
  restoring direct root-target flips in DisplayListToTexture snapshots and
  strict clip-depth FML_DCHECKs without std::min clamping.
- Replace is_onscreen-gated CreateTestCanvas tests with direct
  DisplayListToTexture GPU pixel-readback tests in canvas_unittests.cc and
  clip-stack replay tests in clip_stack_unittests.cc.
@andywolff
andywolff marked this pull request as ready for review November 5, 2026 22:10
@andywolff
andywolff requested review from flar and gaaclarke November 5, 2026 22:10

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates EntityPassClipStack to track the nesting depth (clip_height) of recorded clip entities, ensuring that RecordRestore correctly pops all clip entities from restored child scopes while preserving active ancestor clips. Corresponding unit tests have been added to verify this behavior. The feedback suggests adding defensive null checks in the test helper ReadbackTextureBytes to prevent potential null pointer dereferences.

@andywolff

Copy link
Copy Markdown
Contributor Author

Regarding CanRenderClippedBackdropFilterWithSuperellipse, here's what the new result image looks like:

image

And with my fix reverted, it shows the square corners:
image

@flutter-dashboard

Copy link
Copy Markdown

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.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #195716 at sha c897a86

@flutter-dashboard flutter-dashboard Bot added the will affect goldens Changes to golden files label Nov 5, 2026

@gaaclarke gaaclarke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code looks good to me. My only question is about inspecting a bitmap in a test versus just doing a golden test. Also, is that canvas test testing something different than the golden test? Is it redundant?

// entry.
if (!subpass_state.rendered_clip_entities.empty()) {
// `rendered_clip_entities` is a sparse subset of `clip_coverage`:
// containing axis-aligned intersect rects push coverage without pushing a

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

containing axis-aligned intersect rects push coverage without pushing a replay entity

This is hard to parse =T I think there is a word missing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hmmm yeah I see what you mean, "containing" is an adjective here which reads like a verb. I tried rewording. I think it's better but still not great, wdyt?


// Exercises the root-pass FlipBackdrop path (should_use_onscreen == true on
// framebuffer-fetch backends).
TEST_P(AiksTest, ClippedBackdropFilterInRootPassPreservesRoundedCorners) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of doing a ReadbackTextureBytes, should we instead make this a golden test? The failure experience is much more pleasant with a golden test. Plus it gets rid of all that extra mechanism.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm yeah. I can pare this down.

I wrote these with the belief that unit tests with targeted expectations can be easier to debug than goldens sometimes. Here, though, confirming whether a corner is square or rounded at the Canvas level requires all the texture-readback machinery and specific pixel coordinate checks, which feels like too much overhead. I removed them. The golden test and the EntityPassClipStack unit test are enough to cover the square corners fix.

However, those don't cover the alpha flake that caused the reverts. Reproducing that alpha flake requires cludging in stale depth values, which isn't straightforward to set up in a golden test. But I believe the alpha flake actually represents a separate latent bug in BlitToOnscreen (it leaves the depth attachment uninitialized on the non-Metal render pass fallback). I'm going to file a follow-up issue for BlitToOnscreen and add a test for the alpha issue when fixing that.

@andywolff
andywolff requested a review from gaaclarke November 5, 2026 23:48

@gaaclarke gaaclarke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm!

@flutter-dashboard

Copy link
Copy Markdown

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

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #195716 at sha 0993835

@flar flar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@andywolff andywolff added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 9, 2026
@auto-submit
auto-submit Bot added this pull request to the merge queue Nov 9, 2026
Merged via the queue into flutter:master with commit e1fee50 Nov 10, 2026
22 checks passed
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 10, 2026
auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Nov 10, 2026
flutter/flutter@d463821...fffd8c2

2026-11-10 [email protected] Roll Fuchsia Test Scripts from 8QcsP_SmbsT09VgUs... to PXc5D7S_q6sIYpfFJ... (flutter/flutter#196273)
2026-11-10 [email protected] [web] Accept the capped letter spacing line shift in kitchensink (flutter/flutter#196148)
2026-11-10 [email protected] Fix Vulkan MSAA diagonal seam in FramebufferBlend (flutter/flutter#196197)
2026-11-10 [email protected] Read back images on the raster thread on OpenGLES (flutter/flutter#195927)
2026-11-10 [email protected] [web] Unify ui.Paint across CanvasKit and Skwasm (flutter/flutter#196071)
2026-11-10 [email protected] Roll Fuchsia Test Scripts from p3N3B-Kchh1bthB6n... to 8QcsP_SmbsT09VgUs... (flutter/flutter#196249)
2026-11-09 [email protected] [Impeller] Reland clipped BackdropFilter square corners fix (flutter/flutter#195716)
2026-11-09 [email protected] [web] Recreate canvas and context on CkOffscreenSurface resize to fix WebGL clipping (flutter/flutter#192769)
2026-11-09 [email protected] Roll Dart SDK from ee0f66fa0ad2 to a2baae273bb2 (1 revision) (flutter/flutter#196245)
2026-11-09 [email protected] Add Mac web_tool_tests_1_2 and _2_2 in bringup (flutter/flutter#196241)
2026-11-09 [email protected] Roll pub packages (flutter/flutter#196246)
2026-11-09 [email protected] Increase number of 'Windows tool_integration_tests' subshards (flutter/flutter#196243)
2026-11-09 [email protected] Roll Fuchsia Test Scripts from Jo-ZgQSNDuPsjlZ4s... to p3N3B-Kchh1bthB6n... (flutter/flutter#196238)
2026-11-09 [email protected] Skip UIScene pre-migration test cases on Xcode 27 (flutter/flutter#194755)
2026-11-09 [email protected] [flutter_tools] Share Analytics.isBot and make isRunningOnBot sync (flutter/flutter#194744)
2026-11-09 [email protected] [flutter_tools] Retry chromedriver startup when its port is taken (flutter/flutter#196150)
2026-11-09 [email protected] Remove unused vulkan_provider.cc (flutter/flutter#195513)
2026-11-09 [email protected] Roll Dart SDK from 3b174b7217fe to ee0f66fa0ad2 (13 revisions) (flutter/flutter#196217)
2026-11-09 [email protected] Goma is dead, long live RBE! (Take 2) (flutter/flutter#196212)
2026-11-09 [email protected] Split DevTools customer tests into its own customer_testing_devtools shard (flutter/flutter#195871)
2026-11-09 [email protected] Split mac_host_{debug,profile,release}_arm64_tests into one step per type (flutter/flutter#196031)

If this roll has caused a breakage, revert this CL and set the roller
to dry run mode using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CICD Run CI/CD e: impeller Impeller rendering backend issues and features requests engine flutter/engine related. See also e: labels. will affect goldens Changes to golden files

Projects

None yet

3 participants