From 11367b79fb0b337772880396bd1a094987ca8a96 Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Wed, 26 Nov 2025 14:54:55 -0800 Subject: [PATCH] Documents and fixes behavior when clipping background filter fragment shader (#178940) fixes https://github.com/flutter/flutter/issues/178798 This passes all the golden tests. It fixes the linked issue by avoiding rasterization which resets the frag coords. I have a feeling there may be some case where this may fail that we haven't considered in a golden test. This at moves us forward, documenting the desired behavior without breaking any golden tests. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [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. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [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 --- .../aiks_dl_runtime_effect_unittests.cc | 115 ++++++++++++++++++ .../filters/gaussian_blur_filter_contents.cc | 3 +- .../filters/runtime_effect_filter_contents.cc | 2 +- .../entity/contents/texture_contents.cc | 8 +- .../entity/contents/texture_contents.h | 4 + engine/src/flutter/impeller/entity/entity.cc | 2 + engine/src/flutter/impeller/fixtures/BUILD.gn | 1 + .../fixtures/runtime_stage_border.frag | 34 ++++++ .../src/flutter/impeller/renderer/snapshot.h | 19 +++ 9 files changed, 185 insertions(+), 3 deletions(-) create mode 100644 engine/src/flutter/impeller/fixtures/runtime_stage_border.frag diff --git a/engine/src/flutter/impeller/display_list/aiks_dl_runtime_effect_unittests.cc b/engine/src/flutter/impeller/display_list/aiks_dl_runtime_effect_unittests.cc index 0f09c9a105c..a96f512185d 100644 --- a/engine/src/flutter/impeller/display_list/aiks_dl_runtime_effect_unittests.cc +++ b/engine/src/flutter/impeller/display_list/aiks_dl_runtime_effect_unittests.cc @@ -335,5 +335,120 @@ TEST_P(AiksTest, ComposeBackdropRuntimeOuterBlurInner) { ASSERT_TRUE(OpenPlaygroundHere(callback)); } +TEST_P(AiksTest, ComposeBackdropRuntimeOuterBlurInnerSmallSigma) { + auto runtime_stages = + OpenAssetAsRuntimeStage("runtime_stage_filter_circle.frag.iplr"); + + std::shared_ptr runtime_stage = + runtime_stages[PlaygroundBackendToRuntimeStageBackend(GetBackend())]; + ASSERT_TRUE(runtime_stage); + ASSERT_TRUE(runtime_stage->IsDirty()); + Scalar sigma = 5.0; + + auto callback = [&]() -> sk_sp { + if (AiksTest::ImGuiBegin("Controls", nullptr, + ImGuiWindowFlags_AlwaysAutoResize)) { + ImGui::SliderFloat("sigma", &sigma, 0, 20); + ImGui::End(); + } + DisplayListBuilder builder; + DlPaint background; + background.setColor(DlColor(1.0, 0.1, 0.1, 0.1, DlColorSpace::kSRGB)); + builder.DrawPaint(background); + + auto blur_filter = + DlImageFilter::MakeBlur(sigma, sigma, DlTileMode::kClamp); + + std::vector> sampler_inputs = { + nullptr, + }; + auto uniform_data = std::make_shared>(); + uniform_data->resize(sizeof(Vector2)); + + auto runtime_filter = DlImageFilter::MakeRuntimeEffect( + DlRuntimeEffectImpeller::Make(runtime_stage), sampler_inputs, + uniform_data); + + auto backdrop_filter = DlImageFilter::MakeCompose(/*outer=*/runtime_filter, + /*inner=*/blur_filter); + + DlPaint paint; + auto image = DlImageImpeller::Make(CreateTextureForFixture("kalimba.jpg")); + builder.DrawImage(image, DlPoint(100.0, 100.0), + DlImageSampling::kNearestNeighbor, &paint); + + DlPaint save_paint; + save_paint.setBlendMode(DlBlendMode::kSrc); + builder.SaveLayer(std::nullopt, &save_paint, backdrop_filter.get()); + builder.Restore(); + + DlPaint green; + green.setColor(DlColor::kGreen()); + builder.DrawLine({100, 100}, {200, 100}, green); + builder.DrawLine({100, 100}, {100, 200}, green); + + return builder.Build(); + }; + + ASSERT_TRUE(OpenPlaygroundHere(callback)); +} + +TEST_P(AiksTest, ClippedBackdropFilterWithShader) { + struct FragUniforms { + Vector2 uSize; + } frag_uniforms = {.uSize = Vector2(400, 400)}; + auto uniform_data = std::make_shared>(); + uniform_data->resize(sizeof(FragUniforms)); + memcpy(uniform_data->data(), &frag_uniforms, sizeof(FragUniforms)); + + auto runtime_stages = + OpenAssetAsRuntimeStage("runtime_stage_border.frag.iplr"); + auto runtime_stage = + runtime_stages[PlaygroundBackendToRuntimeStageBackend(GetBackend())]; + ASSERT_TRUE(runtime_stage); + ASSERT_TRUE(runtime_stage->IsDirty()); + + std::vector> sampler_inputs = { + nullptr, + }; + + auto runtime_filter = DlImageFilter::MakeRuntimeEffect( + DlRuntimeEffectImpeller::Make(runtime_stage), sampler_inputs, + uniform_data); + + DisplayListBuilder builder; + + // Draw a background so the backdrop filter has something to affect + DlPaint background_paint; + background_paint.setColor(DlColor::kWhite()); + builder.DrawPaint(background_paint); + + // Draw some pattern to verify the filter effect + DlPaint pattern_paint; + pattern_paint.setColor(DlColor::kRed()); + builder.DrawRect(DlRect::MakeXYWH(0, 0, 200, 200), pattern_paint); + pattern_paint.setColor(DlColor::kBlue()); + builder.DrawRect(DlRect::MakeXYWH(200, 200, 200, 200), pattern_paint); + + builder.Save(); + + // Replicate the clip rect (inset by 66) + // Assuming a 400x400 screen, inset 66 gives roughly 66, 66, 268, 268 + builder.ClipRect(DlRect::MakeXYWH(66, 66, 268, 268)); + + DlPaint save_paint; + // The Flutter code uses a backdrop filter layer. + // In DisplayList, this corresponds to SaveLayer with a backdrop filter. + builder.SaveLayer(std::nullopt, &save_paint, runtime_filter.get()); + + // The child was empty in the Flutter example, so we don't draw anything + // inside the SaveLayer + + builder.Restore(); // Restore SaveLayer + builder.Restore(); // Restore Save (Clip) + + ASSERT_TRUE(OpenPlaygroundHere(builder.Build())); +} + } // namespace testing } // namespace impeller diff --git a/engine/src/flutter/impeller/entity/contents/filters/gaussian_blur_filter_contents.cc b/engine/src/flutter/impeller/entity/contents/filters/gaussian_blur_filter_contents.cc index 6fa1eb113c6..6e4d18c6e71 100644 --- a/engine/src/flutter/impeller/entity/contents/filters/gaussian_blur_filter_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/filters/gaussian_blur_filter_contents.cc @@ -870,7 +870,8 @@ std::optional GaussianBlurFilterContents::RenderFilter( downsample_pass_args.transform * // Matrix::MakeScale(1 / downsample_pass_args.effective_scalar), .sampler_descriptor = sampler_desc, - .opacity = input_snapshot->opacity}, + .opacity = input_snapshot->opacity, + .needs_rasterization_for_runtime_effects = true}, entity.GetBlendMode()); return ApplyBlurStyle(mask_blur_style_, entity, inputs[0], diff --git a/engine/src/flutter/impeller/entity/contents/filters/runtime_effect_filter_contents.cc b/engine/src/flutter/impeller/entity/contents/filters/runtime_effect_filter_contents.cc index 0ed9236fc7e..41138a46e5c 100644 --- a/engine/src/flutter/impeller/entity/contents/filters/runtime_effect_filter_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/filters/runtime_effect_filter_contents.cc @@ -66,7 +66,7 @@ std::optional RuntimeEffectFilterContents::RenderFilter( // rounding errors and add an offset. Said another way; ideally we would skip // this branch for the unit test `ComposePaintRuntimeOuter`, but do it for // `ComposeBackdropRuntimeOuterBlurInner`. - if (!input_snapshot->transform.IsIdentity()) { + if (input_snapshot->ShouldRasterizeForRuntimeEffects()) { Matrix inverse = input_snapshot->transform.Invert(); Quad quad = inverse.Transform(Quad{ coverage.GetLeftTop(), // diff --git a/engine/src/flutter/impeller/entity/contents/texture_contents.cc b/engine/src/flutter/impeller/entity/contents/texture_contents.cc index b182ebb319c..f6a4e674a05 100644 --- a/engine/src/flutter/impeller/entity/contents/texture_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/texture_contents.cc @@ -87,7 +87,9 @@ std::optional TextureContents::RenderToSnapshot( Matrix::MakeScale(scale), .sampler_descriptor = options.sampler_descriptor.value_or( sampler_descriptor_), - .opacity = opacity}; + .opacity = opacity, + .needs_rasterization_for_runtime_effects = + snapshots_need_rasterization_for_runtime_effects_}; } return Contents::RenderToSnapshot( renderer, entity, @@ -253,4 +255,8 @@ void TextureContents::SetDeferApplyingOpacity(bool defer_applying_opacity) { defer_applying_opacity_ = defer_applying_opacity; } +void TextureContents::SetNeedsRasterizationForRuntimeEffects(bool value) { + snapshots_need_rasterization_for_runtime_effects_ = value; +} + } // namespace impeller diff --git a/engine/src/flutter/impeller/entity/contents/texture_contents.h b/engine/src/flutter/impeller/entity/contents/texture_contents.h index 4f8bd0bd724..8ae8bed0501 100644 --- a/engine/src/flutter/impeller/entity/contents/texture_contents.h +++ b/engine/src/flutter/impeller/entity/contents/texture_contents.h @@ -125,6 +125,9 @@ class TextureContents final : public Contents { /// apply it during rendering. void SetDeferApplyingOpacity(bool defer_applying_opacity); + /// @see Snapshot::needs_rasterization_for_runtime_effects + void SetNeedsRasterizationForRuntimeEffects(bool value); + private: std::string label_; @@ -138,6 +141,7 @@ class TextureContents final : public Contents { Scalar opacity_ = 1.0f; Scalar inherited_opacity_ = 1.0f; bool defer_applying_opacity_ = false; + bool snapshots_need_rasterization_for_runtime_effects_ = false; TextureContents(const TextureContents&) = delete; diff --git a/engine/src/flutter/impeller/entity/entity.cc b/engine/src/flutter/impeller/entity/entity.cc index e3c1f34f5dc..4a63964739e 100644 --- a/engine/src/flutter/impeller/entity/entity.cc +++ b/engine/src/flutter/impeller/entity/entity.cc @@ -23,6 +23,8 @@ Entity Entity::FromSnapshot(const Snapshot& snapshot, BlendMode blend_mode) { contents->SetSamplerDescriptor(snapshot.sampler_descriptor); contents->SetSourceRect(texture_rect); contents->SetOpacity(snapshot.opacity); + contents->SetNeedsRasterizationForRuntimeEffects( + snapshot.needs_rasterization_for_runtime_effects); Entity entity; entity.SetBlendMode(blend_mode); diff --git a/engine/src/flutter/impeller/fixtures/BUILD.gn b/engine/src/flutter/impeller/fixtures/BUILD.gn index 3018c14c49c..85423f6195f 100644 --- a/engine/src/flutter/impeller/fixtures/BUILD.gn +++ b/engine/src/flutter/impeller/fixtures/BUILD.gn @@ -96,6 +96,7 @@ impellerc("runtime_stages") { "gradient.frag", "uniforms_and_sampler_1.frag", "uniforms_and_sampler_2.frag", + "runtime_stage_border.frag", ] sl_file_extension = "iplr" diff --git a/engine/src/flutter/impeller/fixtures/runtime_stage_border.frag b/engine/src/flutter/impeller/fixtures/runtime_stage_border.frag new file mode 100644 index 00000000000..2d2cdc1676a --- /dev/null +++ b/engine/src/flutter/impeller/fixtures/runtime_stage_border.frag @@ -0,0 +1,34 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include + +uniform vec2 uSize; +uniform sampler2D uTexture; + +out vec4 fragColor; + +void main() { + vec2 fragCoord = FlutterFragCoord().xy; + + vec2 screenUV = vec2(fragCoord.x / uSize.x, fragCoord.y / uSize.y); + vec2 correctedScreenUV = screenUV; +#ifdef IMPELLER_TARGET_OPENGLES + correctedScreenUV.y = 1.0 - screenUV.y; +#endif + vec4 texColor = texture(uTexture, correctedScreenUV); + + // Check if we're within 20px of any edge + float borderWidth = 20.0; + bool inBorder = + fragCoord.x < borderWidth || fragCoord.x > (uSize.x - borderWidth) || + fragCoord.y < borderWidth || fragCoord.y > (uSize.y - borderWidth); + + if (inBorder) { + fragColor = vec4(0.5, 0.0, 0.5, 1.0); // Purple border + } else { + fragColor = vec4(screenUV, 0.0, 1.0); + fragColor = mix(fragColor, texColor, .5); + } +} diff --git a/engine/src/flutter/impeller/renderer/snapshot.h b/engine/src/flutter/impeller/renderer/snapshot.h index 41c5ff08bcd..36d3091e3eb 100644 --- a/engine/src/flutter/impeller/renderer/snapshot.h +++ b/engine/src/flutter/impeller/renderer/snapshot.h @@ -34,6 +34,25 @@ struct Snapshot { Scalar opacity = 1.0f; + /// @brief Whether this snapshot needs to be re-rasterized when used as an + /// input to a runtime effect. + /// @details This is required because there is no good heuristic to determine + /// if a `Snapshot` needs to be rerasterized before applying a RuntimeFilter. + /// In particular the GaussianBlurContents will return a Snapshot that + /// includes padding for the blur halo which is not possible for the + /// RuntimeEffectContents to know about. This value will tell + /// RuntimeEffectContents that the Snapshot will have to be rerasterized to + /// capture the padding. + bool needs_rasterization_for_runtime_effects = false; + + /// Any snapshot that is scaled should rerasterize because we should be + /// performing the RuntimeEffect at the resolution of the screen, not the + /// scaled up or scaled down version of the snapshot. + bool ShouldRasterizeForRuntimeEffects() const { + return !transform.IsTranslationOnly() || + needs_rasterization_for_runtime_effects; + } + std::optional GetCoverage() const; /// @brief Get the transform that converts screen space coordinates to the UV