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

[Impeller] added a fallback that will make sure the blur fragment shader doesn't overflow #53466

Merged
merged 5 commits into from
Jun 24, 2024
Merged

[Impeller] added a fallback that will make sure the blur fragment shader doesn't overflow #53466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0e1abc9
[Impeller] added a fallback that will make sure the blur fragment sha…
gaaclarke Jun 18, 2024
22107a2
fixed assert
gaaclarke Jun 18, 2024
71aea44
fixes stack overflow
gaaclarke Jun 19, 2024
29b5c0c
cleaned up the overflow fix
gaaclarke Jun 24, 2024
cdf884d
got rid of extra buffer in larger kernel samples
gaaclarke Jun 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions impeller/entity/contents/filters/gaussian_blur_filter_contents.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -174,7 +172,7 @@ fml::StatusOr<RenderTarget> 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();
Expand Down Expand Up @@ -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;

Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

holistic ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both spellings are accepted. After a slight rabbit hole I can report that it isn't some sort of british-ism. "Wholistic" came about about 100 years ago for some goofy reason, probably to sell snake oil.

// 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;
Expand All @@ -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++];
Expand Down
19 changes: 16 additions & 3 deletions impeller/entity/contents/filters/gaussian_blur_filter_contents.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,33 @@

namespace impeller {

// Comes from gaussian.frag.
static constexpr int32_t kGaussianBlurMaxKernelSize = 50;

struct BlurParameters {
Point blur_uv_offset;
Scalar blur_sigma;
int blur_radius;
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.
///
Expand Down
22 changes: 17 additions & 5 deletions impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -482,7 +481,7 @@ TEST(GaussianBlurFilterContentsTest, Coefficients) {
}

TEST(GaussianBlurFilterContentsTest, LerpHackKernelSamplesSimple) {
GaussianBlurPipeline::FragmentShader::KernelSamples kernel_samples = {
KernelSamples kernel_samples = {
.sample_count = 5,
.samples =
{
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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<int32_t>(
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