Skip to content
This repository was archived by the owner on Feb 12, 2025. It is now read-only.

Commit 2badafd

Browse files
authored
[Impeller] added a fallback that will make sure the blur fragment shader doesn't overflow (flutter#53466)
issue: flutter/flutter#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
1 parent 226e02c commit 2badafd

File tree

3 files changed

+50
-15
lines changed

3 files changed

+50
-15
lines changed

impeller/entity/contents/filters/gaussian_blur_filter_contents.cc

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,6 @@ const int32_t GaussianBlurFilterContents::kBlurFilterRequiredMipCount = 4;
2323

2424
namespace {
2525

26-
// 48 comes from gaussian.frag.
27-
const int32_t kMaxKernelSize = 50;
2826
constexpr Scalar kMaxSigma = 500.0f;
2927

3028
SamplerDescriptor MakeSamplerDescriptor(MinMagFilter filter,
@@ -174,7 +172,7 @@ fml::StatusOr<RenderTarget> MakeBlurSubpass(
174172
pass, host_buffer.EmplaceUniform(frame_info));
175173
GaussianBlurPipeline::FragmentShader::KernelSamples kernel_samples =
176174
LerpHackKernelSamples(GenerateBlurInfo(blur_info));
177-
FML_CHECK(kernel_samples.sample_count < kMaxKernelSize);
175+
FML_CHECK(kernel_samples.sample_count <= kGaussianBlurMaxKernelSize);
178176
GaussianBlurFragmentShader::BindKernelSamples(
179177
pass, host_buffer.EmplaceUniform(kernel_samples));
180178
return pass.Draw().ok();
@@ -639,9 +637,8 @@ Scalar GaussianBlurFilterContents::ScaleSigma(Scalar sigma) {
639637
return clamped * scalar;
640638
}
641639

642-
GaussianBlurPipeline::FragmentShader::KernelSamples GenerateBlurInfo(
643-
BlurParameters parameters) {
644-
GaussianBlurPipeline::FragmentShader::KernelSamples result;
640+
KernelSamples GenerateBlurInfo(BlurParameters parameters) {
641+
KernelSamples result;
645642
result.sample_count =
646643
((2 * parameters.blur_radius) / parameters.step_size) + 1;
647644

@@ -653,6 +650,18 @@ GaussianBlurPipeline::FragmentShader::KernelSamples GenerateBlurInfo(
653650
x_offset = 1;
654651
}
655652

653+
// This is a safe-guard to make sure we don't overflow the fragment shader.
654+
// The kernel size is multiplied by 2 since we'll use the lerp hack on the
655+
// result. In practice this isn't throwing away much data since the blur radii
656+
// are around 53 before the down-sampling and max sigma of 500 kick in.
657+
//
658+
// TODO(https://github.com/flutter/flutter/issues/150462): Come up with a more
659+
// wholistic remedy for this. A proper downsample size should not make this
660+
// required. Or we can increase the kernel size.
661+
if (result.sample_count > KernelSamples::kMaxKernelSize) {
662+
result.sample_count = KernelSamples::kMaxKernelSize;
663+
}
664+
656665
Scalar tally = 0.0f;
657666
for (int i = 0; i < result.sample_count; ++i) {
658667
int x = x_offset + (i * parameters.step_size) - parameters.blur_radius;
@@ -676,11 +685,12 @@ GaussianBlurPipeline::FragmentShader::KernelSamples GenerateBlurInfo(
676685
// This works by shrinking the kernel size by 2 and relying on lerp to read
677686
// between the samples.
678687
GaussianBlurPipeline::FragmentShader::KernelSamples LerpHackKernelSamples(
679-
GaussianBlurPipeline::FragmentShader::KernelSamples parameters) {
688+
KernelSamples parameters) {
680689
GaussianBlurPipeline::FragmentShader::KernelSamples result;
681690
result.sample_count = ((parameters.sample_count - 1) / 2) + 1;
682691
int32_t middle = result.sample_count / 2;
683692
int32_t j = 0;
693+
FML_DCHECK(result.sample_count <= kGaussianBlurMaxKernelSize);
684694
for (int i = 0; i < result.sample_count; i++) {
685695
if (i == middle) {
686696
result.samples[i] = parameters.samples[j++];

impeller/entity/contents/filters/gaussian_blur_filter_contents.h

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,20 +12,33 @@
1212

1313
namespace impeller {
1414

15+
// Comes from gaussian.frag.
16+
static constexpr int32_t kGaussianBlurMaxKernelSize = 50;
17+
1518
struct BlurParameters {
1619
Point blur_uv_offset;
1720
Scalar blur_sigma;
1821
int blur_radius;
1922
int step_size;
2023
};
2124

22-
GaussianBlurPipeline::FragmentShader::KernelSamples GenerateBlurInfo(
23-
BlurParameters parameters);
25+
/// A larger mirror of GaussianBlurPipeline::FragmentShader::KernelSamples.
26+
///
27+
/// This is a mirror of GaussianBlurPipeline::FragmentShader::KernelSamples that
28+
/// can hold 2x the max kernel size since it will get reduced with the lerp
29+
/// hack.
30+
struct KernelSamples {
31+
static constexpr int kMaxKernelSize = kGaussianBlurMaxKernelSize * 2;
32+
int sample_count;
33+
GaussianBlurPipeline::FragmentShader::KernelSample samples[kMaxKernelSize];
34+
};
35+
36+
KernelSamples GenerateBlurInfo(BlurParameters parameters);
2437

2538
/// This will shrink the size of a kernel by roughly half by sampling between
2639
/// samples and relying on linear interpolation between the samples.
2740
GaussianBlurPipeline::FragmentShader::KernelSamples LerpHackKernelSamples(
28-
GaussianBlurPipeline::FragmentShader::KernelSamples samples);
41+
KernelSamples samples);
2942

3043
/// Performs a bidirectional Gaussian blur.
3144
///

impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -461,8 +461,7 @@ TEST(GaussianBlurFilterContentsTest, Coefficients) {
461461
.blur_sigma = 1,
462462
.blur_radius = 5,
463463
.step_size = 1};
464-
GaussianBlurPipeline::FragmentShader::KernelSamples samples =
465-
GenerateBlurInfo(parameters);
464+
KernelSamples samples = GenerateBlurInfo(parameters);
466465
EXPECT_EQ(samples.sample_count, 9);
467466

468467
// Coefficients should add up to 1.
@@ -482,7 +481,7 @@ TEST(GaussianBlurFilterContentsTest, Coefficients) {
482481
}
483482

484483
TEST(GaussianBlurFilterContentsTest, LerpHackKernelSamplesSimple) {
485-
GaussianBlurPipeline::FragmentShader::KernelSamples kernel_samples = {
484+
KernelSamples kernel_samples = {
486485
.sample_count = 5,
487486
.samples =
488487
{
@@ -567,8 +566,7 @@ TEST(GaussianBlurFilterContentsTest, LerpHackKernelSamplesComplex) {
567566
.blur_sigma = sigma,
568567
.blur_radius = blur_radius,
569568
.step_size = 1};
570-
GaussianBlurPipeline::FragmentShader::KernelSamples kernel_samples =
571-
GenerateBlurInfo(parameters);
569+
KernelSamples kernel_samples = GenerateBlurInfo(parameters);
572570
EXPECT_EQ(kernel_samples.sample_count, 33);
573571
GaussianBlurPipeline::FragmentShader::KernelSamples fast_kernel_samples =
574572
LerpHackKernelSamples(kernel_samples);
@@ -614,5 +612,19 @@ TEST(GaussianBlurFilterContentsTest, LerpHackKernelSamplesComplex) {
614612
EXPECT_NEAR(output, fast_output, 0.1);
615613
}
616614

615+
TEST(GaussianBlurFilterContentsTest, ChopHugeBlurs) {
616+
Scalar sigma = 30.5f;
617+
int32_t blur_radius = static_cast<int32_t>(
618+
std::ceil(GaussianBlurFilterContents::CalculateBlurRadius(sigma)));
619+
BlurParameters parameters = {.blur_uv_offset = Point(1, 0),
620+
.blur_sigma = sigma,
621+
.blur_radius = blur_radius,
622+
.step_size = 1};
623+
KernelSamples kernel_samples = GenerateBlurInfo(parameters);
624+
GaussianBlurPipeline::FragmentShader::KernelSamples frag_kernel_samples =
625+
LerpHackKernelSamples(kernel_samples);
626+
EXPECT_TRUE(frag_kernel_samples.sample_count <= kGaussianBlurMaxKernelSize);
627+
}
628+
617629
} // namespace testing
618630
} // namespace impeller

0 commit comments

Comments
 (0)