From 0e1abc9c04c0ab3b602ed6efe5e79d6a30ad917b Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 18 Jun 2024 15:29:12 -0700 Subject: [PATCH 1/5] [Impeller] added a fallback that will make sure the blur fragment shader doesn't overflow --- .../filters/gaussian_blur_filter_contents.cc | 12 ++++++++++++ .../gaussian_blur_filter_contents_unittests.cc | 13 +++++++++++++ 2 files changed, 25 insertions(+) diff --git a/impeller/entity/contents/filters/gaussian_blur_filter_contents.cc b/impeller/entity/contents/filters/gaussian_blur_filter_contents.cc index 035109e51951f..361caa1253492 100644 --- a/impeller/entity/contents/filters/gaussian_blur_filter_contents.cc +++ b/impeller/entity/contents/filters/gaussian_blur_filter_contents.cc @@ -653,6 +653,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 > (2 * (kMaxKernelSize - 1))) { + result.sample_count = 2 * (kMaxKernelSize - 1); + } + Scalar tally = 0.0f; for (int i = 0; i < result.sample_count; ++i) { int x = x_offset + (i * parameters.step_size) - parameters.blur_radius; diff --git a/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc b/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc index b29dd6175eaad..6c2162ac50617 100644 --- a/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc +++ b/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc @@ -614,5 +614,18 @@ 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}; + GaussianBlurPipeline::FragmentShader::KernelSamples kernel_samples = + GenerateBlurInfo(parameters); + EXPECT_EQ(kernel_samples.sample_count, 50); +} + } // namespace testing } // namespace impeller From 22107a245b396e27e7cf1bd4dd0b21a2c5c454cc Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 18 Jun 2024 16:46:20 -0700 Subject: [PATCH 2/5] fixed assert --- .../contents/filters/gaussian_blur_filter_contents_unittests.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc b/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc index 6c2162ac50617..8ab065978ae06 100644 --- a/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc +++ b/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc @@ -624,7 +624,7 @@ TEST(GaussianBlurFilterContentsTest, ChopHugeBlurs) { .step_size = 1}; GaussianBlurPipeline::FragmentShader::KernelSamples kernel_samples = GenerateBlurInfo(parameters); - EXPECT_EQ(kernel_samples.sample_count, 50); + EXPECT_EQ(kernel_samples.sample_count, 98); } } // namespace testing From 71aea44b3081d8585560e939709068021aeefd4c Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 18 Jun 2024 17:28:51 -0700 Subject: [PATCH 3/5] fixes stack overflow --- .../filters/gaussian_blur_filter_contents.cc | 17 ++++++++--------- .../filters/gaussian_blur_filter_contents.h | 16 +++++++++++++--- .../gaussian_blur_filter_contents_unittests.cc | 11 ++++------- 3 files changed, 25 insertions(+), 19 deletions(-) diff --git a/impeller/entity/contents/filters/gaussian_blur_filter_contents.cc b/impeller/entity/contents/filters/gaussian_blur_filter_contents.cc index 361caa1253492..2671f4ee394d3 100644 --- a/impeller/entity/contents/filters/gaussian_blur_filter_contents.cc +++ b/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; +LargeKernelSamples GenerateBlurInfo(BlurParameters parameters) { + LargeKernelSamples result; result.sample_count = ((2 * parameters.blur_radius) / parameters.step_size) + 1; @@ -661,10 +658,12 @@ GaussianBlurPipeline::FragmentShader::KernelSamples GenerateBlurInfo( // 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 > (2 * (kMaxKernelSize - 1))) { - result.sample_count = 2 * (kMaxKernelSize - 1); + if (result.sample_count > (2 * (kGaussianBlurMaxKernelSize - 1))) { + result.sample_count = 2 * (kGaussianBlurMaxKernelSize - 1); } + FML_CHECK(result.sample_count < kGaussianBlurMaxKernelSize * 2); + Scalar tally = 0.0f; for (int i = 0; i < result.sample_count; ++i) { int x = x_offset + (i * parameters.step_size) - parameters.blur_radius; @@ -688,7 +687,7 @@ 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) { + LargeKernelSamples parameters) { GaussianBlurPipeline::FragmentShader::KernelSamples result; result.sample_count = ((parameters.sample_count - 1) / 2) + 1; int32_t middle = result.sample_count / 2; diff --git a/impeller/entity/contents/filters/gaussian_blur_filter_contents.h b/impeller/entity/contents/filters/gaussian_blur_filter_contents.h index 2cee099c43a02..b5c2bdf553045 100644 --- a/impeller/entity/contents/filters/gaussian_blur_filter_contents.h +++ b/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,20 @@ struct BlurParameters { int step_size; }; -GaussianBlurPipeline::FragmentShader::KernelSamples GenerateBlurInfo( - BlurParameters parameters); +/// This can hold 2x the max kernel size since it will get reduced with the +/// lerp hack. +struct LargeKernelSamples { + int sample_count; + GaussianBlurPipeline::FragmentShader::KernelSample + samples[kGaussianBlurMaxKernelSize * 2]; +}; + +LargeKernelSamples 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); + LargeKernelSamples samples); /// Performs a bidirectional Gaussian blur. /// diff --git a/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc b/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc index 8ab065978ae06..fccd3ea11e848 100644 --- a/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc +++ b/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); + LargeKernelSamples 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 = { + LargeKernelSamples 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); + LargeKernelSamples kernel_samples = GenerateBlurInfo(parameters); EXPECT_EQ(kernel_samples.sample_count, 33); GaussianBlurPipeline::FragmentShader::KernelSamples fast_kernel_samples = LerpHackKernelSamples(kernel_samples); @@ -622,8 +620,7 @@ TEST(GaussianBlurFilterContentsTest, ChopHugeBlurs) { .blur_sigma = sigma, .blur_radius = blur_radius, .step_size = 1}; - GaussianBlurPipeline::FragmentShader::KernelSamples kernel_samples = - GenerateBlurInfo(parameters); + LargeKernelSamples kernel_samples = GenerateBlurInfo(parameters); EXPECT_EQ(kernel_samples.sample_count, 98); } From 29b5c0c491a747e8bf9b423f2a6341adba6962ad Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Mon, 24 Jun 2024 10:10:35 -0700 Subject: [PATCH 4/5] cleaned up the overflow fix --- .../filters/gaussian_blur_filter_contents.cc | 15 +++++++-------- .../filters/gaussian_blur_filter_contents.h | 17 ++++++++++------- .../gaussian_blur_filter_contents_unittests.cc | 12 +++++++----- 3 files changed, 24 insertions(+), 20 deletions(-) diff --git a/impeller/entity/contents/filters/gaussian_blur_filter_contents.cc b/impeller/entity/contents/filters/gaussian_blur_filter_contents.cc index 2671f4ee394d3..efc56bc471372 100644 --- a/impeller/entity/contents/filters/gaussian_blur_filter_contents.cc +++ b/impeller/entity/contents/filters/gaussian_blur_filter_contents.cc @@ -172,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 < kGaussianBlurMaxKernelSize); + FML_CHECK(kernel_samples.sample_count <= kGaussianBlurMaxKernelSize); GaussianBlurFragmentShader::BindKernelSamples( pass, host_buffer.EmplaceUniform(kernel_samples)); return pass.Draw().ok(); @@ -637,8 +637,8 @@ Scalar GaussianBlurFilterContents::ScaleSigma(Scalar sigma) { return clamped * scalar; } -LargeKernelSamples GenerateBlurInfo(BlurParameters parameters) { - LargeKernelSamples result; +KernelSamples GenerateBlurInfo(BlurParameters parameters) { + KernelSamples result; result.sample_count = ((2 * parameters.blur_radius) / parameters.step_size) + 1; @@ -658,12 +658,10 @@ LargeKernelSamples GenerateBlurInfo(BlurParameters parameters) { // 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 > (2 * (kGaussianBlurMaxKernelSize - 1))) { - result.sample_count = 2 * (kGaussianBlurMaxKernelSize - 1); + if (result.sample_count > KernelSamples::kMaxKernelSize) { + result.sample_count = KernelSamples::kMaxKernelSize; } - FML_CHECK(result.sample_count < kGaussianBlurMaxKernelSize * 2); - Scalar tally = 0.0f; for (int i = 0; i < result.sample_count; ++i) { int x = x_offset + (i * parameters.step_size) - parameters.blur_radius; @@ -687,11 +685,12 @@ LargeKernelSamples GenerateBlurInfo(BlurParameters parameters) { // This works by shrinking the kernel size by 2 and relying on lerp to read // between the samples. GaussianBlurPipeline::FragmentShader::KernelSamples LerpHackKernelSamples( - LargeKernelSamples 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/impeller/entity/contents/filters/gaussian_blur_filter_contents.h b/impeller/entity/contents/filters/gaussian_blur_filter_contents.h index b5c2bdf553045..8fb8d6db5f5f8 100644 --- a/impeller/entity/contents/filters/gaussian_blur_filter_contents.h +++ b/impeller/entity/contents/filters/gaussian_blur_filter_contents.h @@ -22,20 +22,23 @@ struct BlurParameters { int step_size; }; -/// This can hold 2x the max kernel size since it will get reduced with the -/// lerp hack. -struct LargeKernelSamples { +/// 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) - 1; int sample_count; - GaussianBlurPipeline::FragmentShader::KernelSample - samples[kGaussianBlurMaxKernelSize * 2]; + GaussianBlurPipeline::FragmentShader::KernelSample samples[kMaxKernelSize]; }; -LargeKernelSamples GenerateBlurInfo(BlurParameters parameters); +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( - LargeKernelSamples samples); + KernelSamples samples); /// Performs a bidirectional Gaussian blur. /// diff --git a/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc b/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc index fccd3ea11e848..59618db565d14 100644 --- a/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc +++ b/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc @@ -461,7 +461,7 @@ TEST(GaussianBlurFilterContentsTest, Coefficients) { .blur_sigma = 1, .blur_radius = 5, .step_size = 1}; - LargeKernelSamples samples = GenerateBlurInfo(parameters); + KernelSamples samples = GenerateBlurInfo(parameters); EXPECT_EQ(samples.sample_count, 9); // Coefficients should add up to 1. @@ -481,7 +481,7 @@ TEST(GaussianBlurFilterContentsTest, Coefficients) { } TEST(GaussianBlurFilterContentsTest, LerpHackKernelSamplesSimple) { - LargeKernelSamples kernel_samples = { + KernelSamples kernel_samples = { .sample_count = 5, .samples = { @@ -566,7 +566,7 @@ TEST(GaussianBlurFilterContentsTest, LerpHackKernelSamplesComplex) { .blur_sigma = sigma, .blur_radius = blur_radius, .step_size = 1}; - LargeKernelSamples 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); @@ -620,8 +620,10 @@ TEST(GaussianBlurFilterContentsTest, ChopHugeBlurs) { .blur_sigma = sigma, .blur_radius = blur_radius, .step_size = 1}; - LargeKernelSamples kernel_samples = GenerateBlurInfo(parameters); - EXPECT_EQ(kernel_samples.sample_count, 98); + KernelSamples kernel_samples = GenerateBlurInfo(parameters); + GaussianBlurPipeline::FragmentShader::KernelSamples frag_kernel_samples = + LerpHackKernelSamples(kernel_samples); + EXPECT_TRUE(frag_kernel_samples.sample_count <= kGaussianBlurMaxKernelSize); } } // namespace testing From cdf884da158d76500278ef64b49a66ed6244c5ea Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Mon, 24 Jun 2024 10:16:22 -0700 Subject: [PATCH 5/5] got rid of extra buffer in larger kernel samples --- .../entity/contents/filters/gaussian_blur_filter_contents.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/entity/contents/filters/gaussian_blur_filter_contents.h b/impeller/entity/contents/filters/gaussian_blur_filter_contents.h index 8fb8d6db5f5f8..6c53cfae95a18 100644 --- a/impeller/entity/contents/filters/gaussian_blur_filter_contents.h +++ b/impeller/entity/contents/filters/gaussian_blur_filter_contents.h @@ -28,7 +28,7 @@ struct BlurParameters { /// can hold 2x the max kernel size since it will get reduced with the lerp /// hack. struct KernelSamples { - static constexpr int kMaxKernelSize = (kGaussianBlurMaxKernelSize * 2) - 1; + static constexpr int kMaxKernelSize = kGaussianBlurMaxKernelSize * 2; int sample_count; GaussianBlurPipeline::FragmentShader::KernelSample samples[kMaxKernelSize]; };