aaron review.

Author: jonahwilliams

impeller/entity/contents/filters/gaussian_blur_filter_contents.cc

@@ -12,6 +12,7 @@
 #include "impeller/entity/texture_downsample.frag.h"
 #include "impeller/entity/texture_fill.frag.h"
 #include "impeller/entity/texture_fill.vert.h"
+#include "impeller/geometry/color.h"
 #include "impeller/renderer/render_pass.h"
 #include "impeller/renderer/vertex_buffer_builder.h"
 
@@ -485,7 +486,7 @@ fml::StatusOr<RenderTarget> MakeBlurSubpass(
                 linear_sampler_descriptor));
         GaussianBlurVertexShader::BindFrameInfo(
             pass, host_buffer.EmplaceUniform(frame_info));
-        GaussianBlurFragmentShader::BindBlurInfo(
+        GaussianBlurFragmentShader::BindKernelSamples(
             pass, host_buffer.EmplaceUniform(
                       LerpHackKernelSamples(GenerateBlurInfo(blur_info))));
         return pass.Draw().ok();
@@ -919,13 +920,15 @@ KernelSamples GenerateBlurInfo(BlurParameters parameters) {
 
 // This works by shrinking the kernel size by 2 and relying on lerp to read
 // between the samples.
-GaussianBlurPipeline::FragmentShader::BlurInfo LerpHackKernelSamples(
+GaussianBlurPipeline::FragmentShader::KernelSamples LerpHackKernelSamples(
     KernelSamples parameters) {
-  GaussianBlurPipeline::FragmentShader::BlurInfo result = {};
+  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);
+  static_assert(sizeof(result.sample_data) ==
+                sizeof(std::array<Vector4, kGaussianBlurMaxKernelSize>));
 
   for (int i = 0; i < result.sample_count; i++) {
     if (i == middle) {
@@ -939,9 +942,9 @@ GaussianBlurPipeline::FragmentShader::BlurInfo LerpHackKernelSamples(
 
       result.sample_data[i].z = left.coefficient + right.coefficient;
 
-      auto uv = (left.uv_offset * left.coefficient +
-                 right.uv_offset * right.coefficient) /
-                (left.coefficient + right.coefficient);
+      Point uv = (left.uv_offset * left.coefficient +
+                  right.uv_offset * right.coefficient) /
+                 (left.coefficient + right.coefficient);
       result.sample_data[i].x = uv.x;
       result.sample_data[i].y = uv.y;
       j += 2;

impeller/entity/contents/filters/gaussian_blur_filter_contents.h

@@ -9,12 +9,16 @@
 #include "impeller/entity/contents/content_context.h"
 #include "impeller/entity/contents/filters/filter_contents.h"
 #include "impeller/entity/geometry/geometry.h"
+#include "impeller/geometry/color.h"
 
 namespace impeller {
 
 // Comes from gaussian.frag.
 static constexpr int32_t kGaussianBlurMaxKernelSize = 50;
 
+static_assert(sizeof(GaussianBlurPipeline::FragmentShader::KernelSamples) ==
+              sizeof(Vector4) * kGaussianBlurMaxKernelSize + sizeof(Vector4));
+
 struct BlurParameters {
   Point blur_uv_offset;
   Scalar blur_sigma;
@@ -42,7 +46,7 @@ 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(
+GaussianBlurPipeline::FragmentShader::KernelSamples LerpHackKernelSamples(
     KernelSamples samples);
 
 /// Performs a bidirectional Gaussian blur.

impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc

@@ -9,6 +9,7 @@
 #include "impeller/entity/contents/filters/gaussian_blur_filter_contents.h"
 #include "impeller/entity/contents/texture_contents.h"
 #include "impeller/entity/entity_playground.h"
+#include "impeller/geometry/color.h"
 #include "impeller/geometry/geometry_asserts.h"
 #include "impeller/renderer/testing/mocks.h"
 
@@ -51,6 +52,14 @@ fml::StatusOr<float> LowerBoundNewtonianMethod(
   return x;
 }
 
+Scalar GetCoefficient(const Vector4& vec) {
+  return vec.z;
+}
+
+Vector2 GetUVOffset(const Vector4& vec) {
+  return vec.xy();
+}
+
 fml::StatusOr<Scalar> CalculateSigmaForBlurRadius(
     Scalar radius,
     const Matrix& effect_transform) {
@@ -508,7 +517,7 @@ TEST(GaussianBlurFilterContentsTest, LerpHackKernelSamplesSimple) {
           },
   };
 
-  GaussianBlurPipeline::FragmentShader::BlurInfo blur_info =
+  GaussianBlurPipeline::FragmentShader::KernelSamples blur_info =
       LerpHackKernelSamples(kernel_samples);
   EXPECT_EQ(blur_info.sample_count, 3);
 
@@ -517,15 +526,15 @@ TEST(GaussianBlurFilterContentsTest, LerpHackKernelSamplesSimple) {
   //////////////////////////////////////////////////////////////////////////////
   // 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);
-  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);
+  EXPECT_POINT_NEAR(GetUVOffset(blur_info.sample_data[0]),
+                    Point(-1.3333333, 0));
+  EXPECT_FLOAT_EQ(GetCoefficient(blur_info.sample_data[0]), 0.3);
+
+  EXPECT_POINT_NEAR(GetUVOffset(blur_info.sample_data[1]), Point(0, 0));
+  EXPECT_FLOAT_EQ(GetCoefficient(blur_info.sample_data[1]), 0.4);
+
+  EXPECT_POINT_NEAR(GetUVOffset(blur_info.sample_data[2]), Point(1.333333, 0));
+  EXPECT_FLOAT_EQ(GetCoefficient(blur_info.sample_data[2]), 0.3);
 
   //////////////////////////////////////////////////////////////////////////////
   // Check output of fast kernel versus original kernel.
@@ -546,11 +555,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;
+      /*1st*/ lerp(GetUVOffset(blur_info.sample_data[0]), data[0], data[1]) *
+          GetCoefficient(blur_info.sample_data[0]) +
+      /*2nd*/ data[2] * GetCoefficient(blur_info.sample_data[1]) +
+      /*3rd*/ lerp(GetUVOffset(blur_info.sample_data[2]), data[3], data[4]) *
+          GetCoefficient(blur_info.sample_data[2]);
 
   EXPECT_NEAR(original_output, fast_output, 0.01);
 }
@@ -565,7 +574,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 =
+  GaussianBlurPipeline::FragmentShader::KernelSamples fast_kernel_samples =
       LerpHackKernelSamples(kernel_samples);
   EXPECT_EQ(fast_kernel_samples.sample_count, 17);
   float data[33];
@@ -602,8 +611,8 @@ 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());
+    fast_output += GetCoefficient(fast_kernel_samples.sample_data[i]) *
+                   sampler(GetUVOffset(fast_kernel_samples.sample_data[i]));
   }
 
   EXPECT_NEAR(output, fast_output, 0.1);
@@ -618,7 +627,7 @@ TEST(GaussianBlurFilterContentsTest, ChopHugeBlurs) {
                                .blur_radius = blur_radius,
                                .step_size = 1};
   KernelSamples kernel_samples = GenerateBlurInfo(parameters);
-  GaussianBlurPipeline::FragmentShader::BlurInfo frag_kernel_samples =
+  GaussianBlurPipeline::FragmentShader::KernelSamples frag_kernel_samples =
       LerpHackKernelSamples(kernel_samples);
   EXPECT_TRUE(frag_kernel_samples.sample_count <= kGaussianBlurMaxKernelSize);
 }

impeller/entity/shaders/filters/gaussian.frag

@@ -11,13 +11,13 @@ uniform f16sampler2D texture_sampler;
 
 layout(constant_id = 0) const float supports_decal = 1.0;
 
-uniform BlurInfo {
+uniform KernelSamples {
   float sample_count;
 
   // X, Y are uv offset and Z is Coefficient. W is padding.
   vec4 sample_data[50];
 }
-blur_info;
+kernel_samples;
 
 f16vec4 Sample(f16sampler2D tex, vec2 coords) {
   if (supports_decal == 1.0) {
@@ -33,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++) {
-    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);
+  for (int i = 0; i < int(kernel_samples.sample_count); i++) {
+    float16_t coefficient = float16_t(kernel_samples.sample_data[i].z);
+    total_color += coefficient *
+                   Sample(texture_sampler,
+                          v_texture_coords + kernel_samples.sample_data[i].xy);
   }
 
   frag_color = total_color;

impeller/renderer/backend/gles/buffer_bindings_gles.cc

@@ -282,6 +282,7 @@ bool BufferBindingsGLES::BindUniformBuffer(const ProcTableGLES& gl,
     // 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.
+    std::vector<uint8_t> array_element_buffer_;
     if (element_count > 1 && element_stride != member.size) {
       array_element_buffer_.resize(member.size * element_count);
       for (size_t element_i = 0; element_i < element_count; element_i++) {

impeller/renderer/backend/gles/buffer_bindings_gles.h

@@ -57,7 +57,6 @@ class BufferBindingsGLES {
   std::vector<VertexAttribPointer> vertex_attrib_arrays_;
 
   std::unordered_map<std::string, GLint> uniform_locations_;
-  std::vector<uint8_t> array_element_buffer_;
 
   using BindingMap = std::unordered_map<std::string, std::vector<GLint>>;
   BindingMap binding_map_ = {};