This repository was archived by the owner on Feb 25, 2025. It is now read-only.
[Impeller] Fix GLES gaussian implementation. #55329
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,11 @@ struct BlurParameters { | |
| int step_size; | ||
| }; | ||
|
|
||
| struct KernelSample { | ||
| Vector2 uv_offset; | ||
| float coefficient; | ||
| }; | ||
|
|
||
| /// A larger mirror of GaussianBlurPipeline::FragmentShader::KernelSamples. | ||
| /// | ||
| /// This is a mirror of GaussianBlurPipeline::FragmentShader::KernelSamples that | ||
|
|
@@ -30,14 +35,14 @@ struct BlurParameters { | |
| struct KernelSamples { | ||
| static constexpr int kMaxKernelSize = kGaussianBlurMaxKernelSize * 2; | ||
| int sample_count; | ||
| 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::BlurInfo LerpHackKernelSamples( | ||
| KernelSamples samples); | ||
|
|
||
| /// Performs a bidirectional Gaussian blur. | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -508,27 +508,24 @@ TEST(GaussianBlurFilterContentsTest, LerpHackKernelSamplesSimple) { | |||||
| }, | ||||||
| }; | ||||||
|
|
||||||
| GaussianBlurPipeline::FragmentShader::BlurInfo blur_info = | ||||||
| LerpHackKernelSamples(kernel_samples); | ||||||
| EXPECT_EQ(blur_info.sample_count, 3); | ||||||
|
|
||||||
| KernelSample* samples = kernel_samples.samples; | ||||||
|
|
||||||
| ////////////////////////////////////////////////////////////////////////////// | ||||||
| // Check output kernel. | ||||||
|
|
||||||
| EXPECT_FLOAT_EQ(blur_info.sample_data[0].x, -1.3333333); | ||||||
| EXPECT_FLOAT_EQ(blur_info.sample_data[0].y, 0); | ||||||
| EXPECT_FLOAT_EQ(blur_info.sample_data[0].z, 0.3); | ||||||
|
Member
There was a problem hiding this comment.
Suggested change
I think we would be better off not losing the names of values. It's very confusing to have to remember that Same thing for |
||||||
| EXPECT_FLOAT_EQ(blur_info.sample_data[1].x, 0); | ||||||
| EXPECT_FLOAT_EQ(blur_info.sample_data[1].y, 0); | ||||||
| EXPECT_FLOAT_EQ(blur_info.sample_data[1].z, 0.4); | ||||||
| EXPECT_FLOAT_EQ(blur_info.sample_data[2].x, 1.3333333); | ||||||
| EXPECT_FLOAT_EQ(blur_info.sample_data[2].y, 0); | ||||||
| EXPECT_FLOAT_EQ(blur_info.sample_data[2].z, 0.3); | ||||||
|
|
||||||
| ////////////////////////////////////////////////////////////////////////////// | ||||||
| // Check output of fast kernel versus original kernel. | ||||||
|
|
@@ -549,11 +546,11 @@ TEST(GaussianBlurFilterContentsTest, LerpHackKernelSamplesSimple) { | |||||
| } | ||||||
| }; | ||||||
| Scalar fast_output = | ||||||
| /*1st*/ lerp(blur_info.sample_data[0].xy(), data[0], data[1]) * | ||||||
| blur_info.sample_data[0].z + | ||||||
| /*2nd*/ data[2] * blur_info.sample_data[1].z + | ||||||
| /*3rd*/ lerp(blur_info.sample_data[2].xy(), data[3], data[4]) * | ||||||
| blur_info.sample_data[2].z; | ||||||
|
|
||||||
| EXPECT_NEAR(original_output, fast_output, 0.01); | ||||||
| } | ||||||
|
|
@@ -568,7 +565,7 @@ TEST(GaussianBlurFilterContentsTest, LerpHackKernelSamplesComplex) { | |||||
| .step_size = 1}; | ||||||
| KernelSamples kernel_samples = GenerateBlurInfo(parameters); | ||||||
| EXPECT_EQ(kernel_samples.sample_count, 33); | ||||||
| GaussianBlurPipeline::FragmentShader::BlurInfo fast_kernel_samples = | ||||||
| LerpHackKernelSamples(kernel_samples); | ||||||
| EXPECT_EQ(fast_kernel_samples.sample_count, 17); | ||||||
| float data[33]; | ||||||
|
|
@@ -604,9 +601,9 @@ TEST(GaussianBlurFilterContentsTest, LerpHackKernelSamplesComplex) { | |||||
| } | ||||||
|
|
||||||
| Scalar fast_output = 0.0; | ||||||
| for (int i = 0; i < fast_kernel_samples.sample_count; i++) { | ||||||
| fast_output += fast_kernel_samples.sample_data[i].z * | ||||||
| sampler(fast_kernel_samples.sample_data[i].xy()); | ||||||
| } | ||||||
|
|
||||||
| EXPECT_NEAR(output, fast_output, 0.1); | ||||||
|
|
@@ -621,7 +618,7 @@ TEST(GaussianBlurFilterContentsTest, ChopHugeBlurs) { | |||||
| .blur_radius = blur_radius, | ||||||
| .step_size = 1}; | ||||||
| KernelSamples kernel_samples = GenerateBlurInfo(parameters); | ||||||
| GaussianBlurPipeline::FragmentShader::BlurInfo frag_kernel_samples = | ||||||
| LerpHackKernelSamples(kernel_samples); | ||||||
| EXPECT_TRUE(frag_kernel_samples.sample_count <= kGaussianBlurMaxKernelSize); | ||||||
| } | ||||||
|
|
||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,14 +11,11 @@ uniform f16sampler2D texture_sampler; | |
|
|
||
| layout(constant_id = 0) const float supports_decal = 1.0; | ||
|
|
||
|
gaaclarke marked this conversation as resolved.
|
||
| uniform BlurInfo { | ||
|
gaaclarke marked this conversation as resolved.
Outdated
|
||
| float sample_count; | ||
|
|
||
| // X, Y are uv offset and Z is Coefficient. W is padding. | ||
| vec4 sample_data[50]; | ||
|
gaaclarke marked this conversation as resolved.
|
||
| } | ||
| blur_info; | ||
|
|
||
|
|
@@ -36,11 +33,11 @@ out f16vec4 frag_color; | |
| void main() { | ||
| f16vec4 total_color = f16vec4(0.0hf); | ||
|
|
||
| for (int i = 0; i < int(blur_info.sample_count); i++) { | ||
|
gaaclarke marked this conversation as resolved.
Outdated
|
||
| float16_t coefficient = float16_t(blur_info.sample_data[i].z); | ||
| total_color += | ||
| coefficient * | ||
| Sample(texture_sampler, v_texture_coords + blur_info.sample_data[i].xy); | ||
| } | ||
|
|
||
| frag_color = total_color; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -310,6 +310,8 @@ struct Vector4 { | |
| return *this + (v - *this) * t; | ||
| } | ||
|
|
||
| constexpr Vector2 xy() const { return Vector2(x, y); } | ||
|
|
||
| std::string ToString() const; | ||
| }; | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -279,20 +279,19 @@ bool BufferBindingsGLES::BindUniformBuffer(const ProcTableGLES& gl, | |
| auto* buffer_data = | ||
| reinterpret_cast<const GLfloat*>(buffer_ptr + member.offset); | ||
|
|
||
| // When binding uniform arrays, the elements must be contiguous. Copy | ||
| // the uniforms to a temp buffer to eliminate any padding needed by the | ||
| // other backends if the array elements have padding. | ||
| if (element_count > 1 && element_stride != member.size) { | ||
|
gaaclarke marked this conversation as resolved.
|
||
| array_element_buffer_.resize(member.size * element_count); | ||
| for (size_t element_i = 0; element_i < element_count; element_i++) { | ||
| std::memcpy(array_element_buffer_.data() + element_i * member.size, | ||
| reinterpret_cast<const char*>(buffer_data) + | ||
| element_i * element_stride, | ||
| member.size); | ||
| } | ||
| buffer_data = | ||
| reinterpret_cast<const GLfloat*>(array_element_buffer_.data()); | ||
| } | ||
|
|
||
| switch (member.type) { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,6 +57,7 @@ class BufferBindingsGLES { | |
| std::vector<VertexAttribPointer> vertex_attrib_arrays_; | ||
|
|
||
| std::unordered_map<std::string, GLint> uniform_locations_; | ||
| std::vector<uint8_t> array_element_buffer_; | ||
|
Member
There was a problem hiding this comment. Why are we adding this as an instance variable? I'd rather keep it localized to where it is used to avoid stale data bugs.
Contributor
Author
There was a problem hiding this comment. I was trying to avoid creating the empty vector, since I can't safely conditionally initialize it. But that is probably not a worthwhile change |
||
|
|
||
| using BindingMap = std::unordered_map<std::string, std::vector<GLint>>; | ||
| BindingMap binding_map_ = {}; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2581,9 +2581,9 @@ | |
| "arith_cvt" | ||
| ], | ||
| "shortest_path_cycles": [ | ||
| 0.09375, | ||
| 0.0, | ||
| 0.09375, | ||
| 0.0, | ||
| 0.0, | ||
| 0.0, | ||
|
|
@@ -2593,11 +2593,11 @@ | |
| "load_store" | ||
| ], | ||
| "total_cycles": [ | ||
| 0.265625, | ||
| 0.09375, | ||
| 0.265625, | ||
| 0.0, | ||
| 1.0, | ||
| 0.25, | ||
| 0.25 | ||
| ] | ||
|
|
@@ -2641,10 +2641,11 @@ | |
| 0.0 | ||
| ], | ||
| "total_bound_pipelines": [ | ||
| "arithmetic", | ||
| "load_store" | ||
| ], | ||
| "total_cycles": [ | ||
| 2.0, | ||
| 2.0, | ||
| 1.0 | ||
| ] | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please bring back this check. It's important because we'll crash anyways if we violate this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is checked in the LerpHackFunction too, I don't think the count can change after that point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay yea, in that case I'd rather keep this one here since it is the final point before we'd actually get the error. We can remove the lerp hack one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the kernel samples UBO has a std::array of a fixed size. It cannot be greater than kGaussianBlurMaxKernelSize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could static assert this though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yea, the
sample_countisn't even consequential here because emplace isn't looking at it. Lets keep the runtime DCHECK in the lerp hack and a static assert sounds good to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done