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
auto-submit 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 1 commit
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
Prev Previous commit
Next Next commit
fixes stack overflow
  • Loading branch information
@gaaclarke
gaaclarke committed Jun 19, 2024
commit 71aea44b3081d8585560e939709068021aeefd4c
17 changes: 8 additions & 9 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;
LargeKernelSamples GenerateBlurInfo(BlurParameters parameters) {
LargeKernelSamples result;
result.sample_count =
((2 * parameters.blur_radius) / parameters.step_size) + 1;

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

@jonahwilliams jonahwilliams Jun 24, 2024

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

@gaaclarke gaaclarke Jun 24, 2024

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 > (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;
Expand All @@ -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;
Expand Down
16 changes: 13 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,30 @@

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);
/// 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.
///
Expand Down
11 changes: 4 additions & 7 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);
LargeKernelSamples 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 = {
LargeKernelSamples 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);
LargeKernelSamples kernel_samples = GenerateBlurInfo(parameters);
EXPECT_EQ(kernel_samples.sample_count, 33);
GaussianBlurPipeline::FragmentShader::KernelSamples fast_kernel_samples =
LerpHackKernelSamples(kernel_samples);
Expand Down Expand Up @@ -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);
}

Expand Down