From 10c52df4fb1286ab68fda4ec4893b5d2dca8eebe Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Mon, 5 Feb 2024 16:32:44 -0800 Subject: [PATCH] [Impeller] blur: hold on to 1/8 downsample until the kernel overflows (flutter/engine#50363) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit issue: https://github.com/flutter/flutter/issues/142753 After https://github.com/flutter/engine/pull/50262 there were still some sigmas that could show shimmering around the sigma [50, 100] range which had a downsample amount of 1/16. This makes those ranges hang on to 1/8 for as long as possible. I'm unable to see any shimmering with `AiksTest.GaussianBlurAnimatedBackdrop` after this PR. I've also expanded that test so the clip region could be scaled to make sure that there aren't sizes which cause it to reappear. Testing: Expanded on manual testing. Since the error only manifests when evaluating multiple frames of rendering we don't have infrastructure to test that. Here is the graph of `GaussianBlurFilterContents::CalculateScale` after this change: Screenshot 2024-02-05 at 2 10 41 PM ## 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] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat --- .../impeller/aiks/aiks_blur_unittests.cc | 4 +++ .../filters/gaussian_blur_filter_contents.cc | 30 +++++++++++++------ ...gaussian_blur_filter_contents_unittests.cc | 2 ++ 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/engine/src/flutter/impeller/aiks/aiks_blur_unittests.cc b/engine/src/flutter/impeller/aiks/aiks_blur_unittests.cc index e7198aa7a3..7e76260f36 100644 --- a/engine/src/flutter/impeller/aiks/aiks_blur_unittests.cc +++ b/engine/src/flutter/impeller/aiks/aiks_blur_unittests.cc @@ -571,6 +571,10 @@ TEST_P(AiksTest, GaussianBlurAnimatedBackdrop) { Point(1024 / 2 - boston->GetSize().width / 2, (768 / 2 - boston->GetSize().height / 2) + y), {}); + auto [handle_a, handle_b] = IMPELLER_PLAYGROUND_LINE( + Point(100, 100), Point(900, 700), 20, Color::Red(), Color::Red()); + canvas.ClipRect( + Rect::MakeLTRB(handle_a.x, handle_a.y, handle_b.x, handle_b.y)); canvas.ClipRect(Rect::MakeLTRB(100, 100, 900, 700)); canvas.SaveLayer({.blend_mode = BlendMode::kSource}, std::nullopt, ImageFilter::MakeBlur(Sigma(sigma), Sigma(sigma), 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 79d39dde48..36bc0c8606 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 @@ -23,6 +23,9 @@ const int32_t GaussianBlurFilterContents::kBlurFilterRequiredMipCount = 4; namespace { +// 48 comes from kernel.glsl. +const int32_t kMaxKernelSize = 48; + SamplerDescriptor MakeSamplerDescriptor(MinMagFilter filter, SamplerAddressMode address_mode) { SamplerDescriptor sampler_desc; @@ -189,6 +192,10 @@ Rect MakeReferenceUVs(const Rect& reference, const Rect& rect) { rect.GetSize()); return result.Scale(1.0f / Vector2(reference.GetSize())); } + +int ScaleBlurRadius(Scalar radius, Scalar scalar) { + return static_cast(std::round(radius * scalar)); +} } // namespace std::string_view GaussianBlurFilterContents::kNoMipsError = @@ -207,13 +214,21 @@ Scalar GaussianBlurFilterContents::CalculateScale(Scalar sigma) { if (sigma <= 4) { return 1.0; } - Scalar result = 4.0 / sigma; + Scalar raw_result = 4.0 / sigma; // Round to the nearest 1/(2^n) to get the best quality down scaling. - Scalar exponent = round(log2f(result)); + Scalar exponent = round(log2f(raw_result)); // Don't scale down below 1/16th to preserve signal. exponent = std::max(-4.0f, exponent); Scalar rounded = powf(2.0f, exponent); - return rounded; + Scalar result = rounded; + // Only drop below 1/8 if 1/8 would overflow our kernel. + if (rounded < 0.125f) { + Scalar rounded_plus = powf(2.0f, exponent + 1); + Scalar blur_radius = CalculateBlurRadius(sigma); + int kernel_size_plus = (ScaleBlurRadius(blur_radius, rounded_plus) * 2) + 1; + result = kernel_size_plus < kMaxKernelSize ? rounded_plus : rounded; + } + return result; }; std::optional GaussianBlurFilterContents::GetFilterSourceCoverage( @@ -370,8 +385,7 @@ std::optional GaussianBlurFilterContents::RenderFilter( BlurParameters{ .blur_uv_offset = Point(0.0, pass1_pixel_size.y), .blur_sigma = scaled_sigma.y * effective_scalar.y, - .blur_radius = - static_cast(std::round(blur_radius.y * effective_scalar.y)), + .blur_radius = ScaleBlurRadius(blur_radius.y, effective_scalar.y), .step_size = 1, }, /*destination_target=*/std::nullopt, blur_uvs); @@ -392,8 +406,7 @@ std::optional GaussianBlurFilterContents::RenderFilter( BlurParameters{ .blur_uv_offset = Point(pass1_pixel_size.x, 0.0), .blur_sigma = scaled_sigma.x * effective_scalar.x, - .blur_radius = - static_cast(std::round(blur_radius.x * effective_scalar.x)), + .blur_radius = ScaleBlurRadius(blur_radius.x, effective_scalar.x), .step_size = 1, }, pass3_destination, blur_uvs); @@ -457,8 +470,7 @@ KernelPipeline::FragmentShader::KernelSamples GenerateBlurInfo( KernelPipeline::FragmentShader::KernelSamples result; result.sample_count = ((2 * parameters.blur_radius) / parameters.step_size) + 1; - // 48 comes from kernel.glsl. - FML_CHECK(result.sample_count < 48); + FML_CHECK(result.sample_count < kMaxKernelSize); // Chop off the last samples if the radius >= 3 where they account for < 1.56% // of the result. diff --git a/engine/src/flutter/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc b/engine/src/flutter/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc index f75b4f3d31..d84da440a4 100644 --- a/engine/src/flutter/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc +++ b/engine/src/flutter/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc @@ -197,6 +197,8 @@ TEST(GaussianBlurFilterContentsTest, CalculateSigmaValues) { EXPECT_EQ(GaussianBlurFilterContents::CalculateScale(3.0f), 1); EXPECT_EQ(GaussianBlurFilterContents::CalculateScale(4.0f), 1); EXPECT_EQ(GaussianBlurFilterContents::CalculateScale(16.0f), 0.25); + // Hang on to 1/8 as long as possible. + EXPECT_EQ(GaussianBlurFilterContents::CalculateScale(100.0f), 0.125); // Downsample clamped to 1/16th. EXPECT_EQ(GaussianBlurFilterContents::CalculateScale(1024.0f), 0.0625); }