[Impeller] Fix GLES gaussian implementation. #55329

jonahwilliams · 2024-09-21T16:57:52Z

problems: no uniform structs, no int uniforms, buffer bindings dont work when the struct type doesn't match the uniform name 😓

impeller/entity/shaders/filters/gaussian.frag

impeller/renderer/backend/gles/buffer_bindings_gles.cc

flutter-dashboard · 2024-09-21T20:13:25Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #55329 at sha e1bfaf1

impeller/entity/shaders/filters/gaussian.frag

gaaclarke

Looks good for the most part, nice work eliminating the copy. Here are the important notes:

Keep the kernel size assert (to avoid obscure crashes)
Add named getters for uv_offset and coefficient so that we don't trade off readability for performance.

gaaclarke · 2024-09-23T16:15:36Z

impeller/entity/contents/filters/gaussian_blur_filter_contents.cc

@@ -481,11 +485,9 @@ fml::StatusOr<RenderTarget> MakeBlurSubpass(
                linear_sampler_descriptor));
        GaussianBlurVertexShader::BindFrameInfo(
            pass, host_buffer.EmplaceUniform(frame_info));
-        GaussianBlurPipeline::FragmentShader::KernelSamples kernel_samples =
-            LerpHackKernelSamples(GenerateBlurInfo(blur_info));
-        FML_CHECK(kernel_samples.sample_count <= kGaussianBlurMaxKernelSize);


Please bring back this check. It's important because we'll crash anyways if we violate this.

This is checked in the LerpHackFunction too, I don't think the count can change after that point?

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.

But the kernel samples UBO has a std::array of a fixed size. It cannot be greater than kGaussianBlurMaxKernelSize

I could static assert this though.

Ah yea, the sample_count isn'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.

gaaclarke · 2024-09-23T16:20:18Z

impeller/entity/contents/filters/gaussian_blur_filter_contents.cc

+
+      result.sample_data[i].z = left.coefficient + right.coefficient;
+
+      auto uv = (left.uv_offset * left.coefficient +


Suggested change

auto uv = (left.uv_offset * left.coefficient +

Point uv = (left.uv_offset * left.coefficient +

gaaclarke · 2024-09-23T16:28:44Z

impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc

-  EXPECT_FLOAT_EQ(fast_samples[2].coefficient, 0.3);
+  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);


Suggested change

EXPECT_FLOAT_EQ(blur_info.sample_data[0].z, 0.3);

EXPECT_FLOAT_EQ(GetCoefficient(blur_info.sample_data[0]), 0.3);

I think we would be better off not losing the names of values. It's very confusing to have to remember that .z means "get the coefficient". Can we codify that into some tiny functions?

Same thing for uv_offset.

impeller/entity/shaders/filters/gaussian.frag

gaaclarke · 2024-09-23T16:44:15Z

impeller/renderer/backend/gles/buffer_bindings_gles.h

@@ -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_;


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.

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

gaaclarke

lgtm, thanks

flutter-dashboard · 2024-09-23T19:01:25Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #55329 at sha dd01c63

…e#55329)

…155583) flutter/engine@95c5a09...8a5af19 2024-09-23 skia-flutter-autoroll@skia.org Roll Skia from 7174ab7a984d to 7b0669f89aef (1 revision) (flutter/engine#55371) 2024-09-23 jonahwilliams@google.com [Impeller] Fix GLES gaussian implementation. (flutter/engine#55329) 2024-09-23 30870216+gaaclarke@users.noreply.github.com Made YAML version of vscode workspace to avoid redundancy (flutter/engine#55322) 2024-09-23 skia-flutter-autoroll@skia.org Roll Skia from acb93a9f241f to 7174ab7a984d (6 revisions) (flutter/engine#55369) 2024-09-23 jonahwilliams@google.com [Impeller] add triangle fan support and remove drawVertices copying. (flutter/engine#55236) 2024-09-23 jason-simmons@users.noreply.github.com Copy Dart SDK vm_shared sources into the sky_engine package (flutter/engine#55158) 2024-09-23 jonahwilliams@google.com [Impeller] use BufferSubData to update gles device buffer. (flutter/engine#55330) 2024-09-23 jonahwilliams@google.com [Impeller] remove usage of VBB when allocating vertices of a fixed size. (flutter/engine#55235) 2024-09-23 6844906+zijiehe-google-com@users.noreply.github.com [fuchsia] Update fuchsia instruction in Compiling-the-engine.md (flutter/engine#55365) 2024-09-23 jonahwilliams@google.com [iOS] sprinkle some null checks on BringLayersIntoView. (flutter/engine#55334) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC chinmaygarde@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#155583) flutter/engine@95c5a09...8a5af19 2024-09-23 skia-flutter-autoroll@skia.org Roll Skia from 7174ab7a984d to 7b0669f89aef (1 revision) (flutter/engine#55371) 2024-09-23 jonahwilliams@google.com [Impeller] Fix GLES gaussian implementation. (flutter/engine#55329) 2024-09-23 30870216+gaaclarke@users.noreply.github.com Made YAML version of vscode workspace to avoid redundancy (flutter/engine#55322) 2024-09-23 skia-flutter-autoroll@skia.org Roll Skia from acb93a9f241f to 7174ab7a984d (6 revisions) (flutter/engine#55369) 2024-09-23 jonahwilliams@google.com [Impeller] add triangle fan support and remove drawVertices copying. (flutter/engine#55236) 2024-09-23 jason-simmons@users.noreply.github.com Copy Dart SDK vm_shared sources into the sky_engine package (flutter/engine#55158) 2024-09-23 jonahwilliams@google.com [Impeller] use BufferSubData to update gles device buffer. (flutter/engine#55330) 2024-09-23 jonahwilliams@google.com [Impeller] remove usage of VBB when allocating vertices of a fixed size. (flutter/engine#55235) 2024-09-23 6844906+zijiehe-google-com@users.noreply.github.com [fuchsia] Update fuchsia instruction in Compiling-the-engine.md (flutter/engine#55365) 2024-09-23 jonahwilliams@google.com [iOS] sprinkle some null checks on BringLayersIntoView. (flutter/engine#55334) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC chinmaygarde@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

[Impeller] Fix GLES gaussian implementation.

2f90472

github-actions bot added the e: impeller label Sep 21, 2024

++

fee9a39

jonahwilliams marked this pull request as ready for review September 21, 2024 18:02

fix padding.

ced0ea0

jonahwilliams commented Sep 21, 2024

View reviewed changes

impeller/entity/shaders/filters/gaussian.frag Show resolved Hide resolved

jonahwilliams commented Sep 21, 2024

View reviewed changes

impeller/renderer/backend/gles/buffer_bindings_gles.cc Show resolved Hide resolved

malioc.

e1bfaf1

flutter-dashboard bot added the will affect goldens label Sep 21, 2024

jonahwilliams commented Sep 23, 2024

View reviewed changes

impeller/entity/shaders/filters/gaussian.frag Show resolved Hide resolved

jonahwilliams requested a review from gaaclarke September 23, 2024 15:53

gaaclarke suggested changes Sep 23, 2024

View reviewed changes

aaron review.

dd01c63

gaaclarke approved these changes Sep 23, 2024

View reviewed changes

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 23, 2024

auto-submit bot merged commit 22d3abd into flutter:main Sep 23, 2024
30 checks passed

engine-flutter-autoroll mentioned this pull request Sep 24, 2024

Roll Flutter Engine from 95c5a0940ad9 to 8a5af19a43f3 (10 revisions) flutter/flutter#155583

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 24, 2024

22d3abdd5 [Impeller] Fix GLES gaussian implementation. (flutter/engin…

4b2103f

…e#55329)


		result.sample_data[i].z = left.coefficient + right.coefficient;

		auto uv = (left.uv_offset * left.coefficient +

	auto uv = (left.uv_offset * left.coefficient +
	Point uv = (left.uv_offset * left.coefficient +

	EXPECT_FLOAT_EQ(blur_info.sample_data[0].z, 0.3);
	EXPECT_FLOAT_EQ(GetCoefficient(blur_info.sample_data[0]), 0.3);

[Impeller] Fix GLES gaussian implementation. #55329

[Impeller] Fix GLES gaussian implementation. #55329

Uh oh!

Conversation

jonahwilliams commented Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flutter-dashboard bot commented Sep 21, 2024

Uh oh!

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Sep 23, 2024

Uh oh!

Uh oh!

Uh oh!

jonahwilliams commented Sep 21, 2024 •

edited

Loading