From b6e6849f95d36cb7beafd42d08d3699f7f96489f Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Mon, 24 Jun 2024 11:01:00 -0700 Subject: [PATCH] [Impeller] added a fallback that will make sure the blur fragment shader doesn't overflow (flutter/engine#53466) issue: https://github.com/flutter/flutter/issues/150462 This makes that usage never crash. I would like to reevaluate for a better fix later though. Something like: 1) Rework the downsample logic to make sure this gap doesn't exist. 1) Simply increase the kernel size. We already did that before and technically we should be clamped to the 500 sigma so we won't have to do it forever. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style --- .../filters/gaussian_blur_filter_contents.cc | 24 +++++++++++++------ .../filters/gaussian_blur_filter_contents.h | 19 ++++++++++++--- ...gaussian_blur_filter_contents_unittests.cc | 22 +++++++++++++---- 3 files changed, 50 insertions(+), 15 deletions(-) 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 035109e519..efc56bc471 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,8 +23,6 @@ const int32_t GaussianBlurFilterContents::kBlurFilterRequiredMipCount = 4; namespace { -// 48 comes from gaussian.frag. -const int32_t kMaxKernelSize = 50; constexpr Scalar kMaxSigma = 500.0f; SamplerDescriptor MakeSamplerDescriptor(MinMagFilter filter, @@ -174,7 +172,7 @@ fml::StatusOr MakeBlurSubpass( pass, host_buffer.EmplaceUniform(frame_info)); GaussianBlurPipeline::FragmentShader::KernelSamples kernel_samples = LerpHackKernelSamples(GenerateBlurInfo(blur_info)); - FML_CHECK(kernel_samples.sample_count < kMaxKernelSize); + FML_CHECK(kernel_samples.sample_count <= kGaussianBlurMaxKernelSize); GaussianBlurFragmentShader::BindKernelSamples( pass, host_buffer.EmplaceUniform(kernel_samples)); return pass.Draw().ok(); @@ -639,9 +637,8 @@ Scalar GaussianBlurFilterContents::ScaleSigma(Scalar sigma) { return clamped * scalar; } -GaussianBlurPipeline::FragmentShader::KernelSamples GenerateBlurInfo( - BlurParameters parameters) { - GaussianBlurPipeline::FragmentShader::KernelSamples result; +KernelSamples GenerateBlurInfo(BlurParameters parameters) { + KernelSamples result; result.sample_count = ((2 * parameters.blur_radius) / parameters.step_size) + 1; @@ -653,6 +650,18 @@ GaussianBlurPipeline::FragmentShader::KernelSamples GenerateBlurInfo( x_offset = 1; } + // This is a safe-guard to make sure we don't overflow the fragment shader. + // The kernel size is multiplied by 2 since we'll use the lerp hack on the + // result. In practice this isn't throwing away much data since the blur radii + // are around 53 before the down-sampling and max sigma of 500 kick in. + // + // TODO(https://github.com/flutter/flutter/issues/150462): Come up with a more + // wholistic remedy for this. A proper downsample size should not make this + // required. Or we can increase the kernel size. + if (result.sample_count > KernelSamples::kMaxKernelSize) { + result.sample_count = KernelSamples::kMaxKernelSize; + } + Scalar tally = 0.0f; for (int i = 0; i < result.sample_count; ++i) { int x = x_offset + (i * parameters.step_size) - parameters.blur_radius; @@ -676,11 +685,12 @@ GaussianBlurPipeline::FragmentShader::KernelSamples GenerateBlurInfo( // This works by shrinking the kernel size by 2 and relying on lerp to read // between the samples. GaussianBlurPipeline::FragmentShader::KernelSamples LerpHackKernelSamples( - GaussianBlurPipeline::FragmentShader::KernelSamples parameters) { + KernelSamples parameters) { GaussianBlurPipeline::FragmentShader::KernelSamples result; result.sample_count = ((parameters.sample_count - 1) / 2) + 1; int32_t middle = result.sample_count / 2; int32_t j = 0; + FML_DCHECK(result.sample_count <= kGaussianBlurMaxKernelSize); for (int i = 0; i < result.sample_count; i++) { if (i == middle) { result.samples[i] = parameters.samples[j++]; diff --git a/engine/src/flutter/impeller/entity/contents/filters/gaussian_blur_filter_contents.h b/engine/src/flutter/impeller/entity/contents/filters/gaussian_blur_filter_contents.h index 2cee099c43..6c53cfae95 100644 --- a/engine/src/flutter/impeller/entity/contents/filters/gaussian_blur_filter_contents.h +++ b/engine/src/flutter/impeller/entity/contents/filters/gaussian_blur_filter_contents.h @@ -12,6 +12,9 @@ namespace impeller { +// Comes from gaussian.frag. +static constexpr int32_t kGaussianBlurMaxKernelSize = 50; + struct BlurParameters { Point blur_uv_offset; Scalar blur_sigma; @@ -19,13 +22,23 @@ struct BlurParameters { int step_size; }; -GaussianBlurPipeline::FragmentShader::KernelSamples GenerateBlurInfo( - BlurParameters parameters); +/// A larger mirror of GaussianBlurPipeline::FragmentShader::KernelSamples. +/// +/// This is a mirror of GaussianBlurPipeline::FragmentShader::KernelSamples that +/// can hold 2x the max kernel size since it will get reduced with the lerp +/// hack. +struct KernelSamples { + static constexpr int kMaxKernelSize = kGaussianBlurMaxKernelSize * 2; + int sample_count; + GaussianBlurPipeline::FragmentShader::KernelSample samples[kMaxKernelSize]; +}; + +KernelSamples GenerateBlurInfo(BlurParameters parameters); /// This will shrink the size of a kernel by roughly half by sampling between /// samples and relying on linear interpolation between the samples. GaussianBlurPipeline::FragmentShader::KernelSamples LerpHackKernelSamples( - GaussianBlurPipeline::FragmentShader::KernelSamples samples); + KernelSamples samples); /// Performs a bidirectional Gaussian blur. /// 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 b29dd6175e..59618db565 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 @@ -461,8 +461,7 @@ TEST(GaussianBlurFilterContentsTest, Coefficients) { .blur_sigma = 1, .blur_radius = 5, .step_size = 1}; - GaussianBlurPipeline::FragmentShader::KernelSamples samples = - GenerateBlurInfo(parameters); + KernelSamples samples = GenerateBlurInfo(parameters); EXPECT_EQ(samples.sample_count, 9); // Coefficients should add up to 1. @@ -482,7 +481,7 @@ TEST(GaussianBlurFilterContentsTest, Coefficients) { } TEST(GaussianBlurFilterContentsTest, LerpHackKernelSamplesSimple) { - GaussianBlurPipeline::FragmentShader::KernelSamples kernel_samples = { + KernelSamples kernel_samples = { .sample_count = 5, .samples = { @@ -567,8 +566,7 @@ TEST(GaussianBlurFilterContentsTest, LerpHackKernelSamplesComplex) { .blur_sigma = sigma, .blur_radius = blur_radius, .step_size = 1}; - GaussianBlurPipeline::FragmentShader::KernelSamples kernel_samples = - GenerateBlurInfo(parameters); + KernelSamples kernel_samples = GenerateBlurInfo(parameters); EXPECT_EQ(kernel_samples.sample_count, 33); GaussianBlurPipeline::FragmentShader::KernelSamples fast_kernel_samples = LerpHackKernelSamples(kernel_samples); @@ -614,5 +612,19 @@ TEST(GaussianBlurFilterContentsTest, LerpHackKernelSamplesComplex) { EXPECT_NEAR(output, fast_output, 0.1); } +TEST(GaussianBlurFilterContentsTest, ChopHugeBlurs) { + Scalar sigma = 30.5f; + int32_t blur_radius = static_cast( + std::ceil(GaussianBlurFilterContents::CalculateBlurRadius(sigma))); + BlurParameters parameters = {.blur_uv_offset = Point(1, 0), + .blur_sigma = sigma, + .blur_radius = blur_radius, + .step_size = 1}; + KernelSamples kernel_samples = GenerateBlurInfo(parameters); + GaussianBlurPipeline::FragmentShader::KernelSamples frag_kernel_samples = + LerpHackKernelSamples(kernel_samples); + EXPECT_TRUE(frag_kernel_samples.sample_count <= kGaussianBlurMaxKernelSize); +} + } // namespace testing } // namespace impeller