Repository navigation
[Impeller] Reland clipped BackdropFilter square corners fix - #195716
Conversation
c71e245 to
5b009b4
Compare
This comment was marked as resolved.
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.
There was a problem hiding this comment.
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.
|
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 Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
gaaclarke
left a comment
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
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


Fixes #162172
Fixes #190602
Fixes #192992
Fixes #195423
Revised reland of the square corners fix in #191096.
Previously,
EntityPassClipStack::RecordRestoreunconditionally 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 acrossBackdropFilterflips (see my comment on #190602).EntityPassClipStacknow tracksclip_heightonReplayResultsoRecordRestoreonly pops entries aboverestore_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
CanvasandClipContentschanges back tomaster. In #191096, I gatedshould_use_onscreenonis_onscreen_inCanvas::SaveLayerto make unit testing easier, which routedDisplayListToTexturesnapshots throughCanvas::BlitToOnscreenwith an uncleared 4x MSAA depth attachment and caused theaction_sheet_test.dartalpha flake (see my comment on #195423). Reverting theCanvaschanges fixes that flake and removes the clip-depth clamping that @flar flagged in review.Instead of modifying
Canvasfor testability, I replaced theCreateTestCanvastests from Commit 1 withDisplayListToTexturepixel-readback tests incanvas_unittests.cc, along with unit tests inclip_stack_unittests.ccandaiks_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
///).