[Impeller] use SSBOs for gradients where supported (metal/vulkan) #37654

jonahwilliams · 2022-11-16T00:30:04Z

Use SSBO to pass the gradient color buffer to the shader, which dramatically improves performance for small gradients. When creating this color buffer, reusing the existing color buffer if the stops are even.

Fixes flutter/flutter#115208 (for iOS).
Fixes flutter/flutter#115523

On Android, any usage of arrays in the fragment shaders leads to a link error with no other messages. Assuming that we're using the gles fragment shaders, I'm not really sure what the issue is exactly - but I assume this has something to do with targeting gles 2.0.

impeller/entity/contents/content_context.cc

jonahwilliams · 2022-11-17T02:08:39Z

I have mixed feelings about how I've currently handled the lack of support for this on Android, but also zoom zoom go fast.

jonahwilliams · 2022-11-17T02:48:34Z

Converting to draft, we should use SSBOs for this

jonahwilliams · 2022-11-17T18:41:43Z

impeller/entity/BUILD.gn

@@ -66,6 +66,20 @@ impeller_shaders("entity_shaders") {
  ]
 }

+impeller_shaders("modern_entity_shaders") {


This almost works, except that I fail to load the fixed_fill shaders at runtime. I suspect something somewhere is hard coded but I haven't found it. If I place these shaders in the entity shaders set (and force the gles_language version to 460 everywhere) it works as expected

jonahwilliams · 2022-11-17T18:42:06Z

impeller/entity/contents/content_context.cc

@@ -150,13 +150,26 @@ ContentContext::ContentContext(std::shared_ptr<Context> context)
  if (!context_ || !context_->IsValid()) {
    return;
  }
+#ifdef FML_OS_ANDROID


TBD: actual feature selection when we have vulkan support

Consider having a define for IMPELLER_SUPPORT_SSBO that's just IMPELLER_ENABLE_METAL || IMPELLER_ENABLE_VULKAN. Or just use the more verbose one here. You'll need some runtime detection, so an update to the Context object to have some method or struct or something that says whether SSBOs are allowed.

Oh I see some of the runtime stuff is started.

We need to leave this as runtime support as far as I can tell, once Vulkan is on by default. Avoids literring the code base with ifdefs too

Can we have this on on impeller::Context instead? You can ask the context for its feature set. That way, only ES will return the legacy stuff.

chinmaygarde · 2022-11-17T18:58:35Z

impeller/entity/contents/backend_features.h

+/// @brief Lowest common denominator feature sets.
+constexpr BackendFeatures kLegacyBackendFeatures = {.ssbo_support = false};
+
+}  // namespace impeller


Newline at EOF.

chinmaygarde · 2022-11-17T18:59:43Z

impeller/entity/contents/backend_features.h

+
+/// @brief A struct for describing available backend features for runtime
+/// selection.
+struct BackendFeatures {


Oh, I was thinking of this being an enum with the modern stuff implying SSBO support. But this is way better.

chinmaygarde · 2022-11-17T19:00:57Z

impeller/entity/contents/content_context.cc

@@ -150,13 +150,26 @@ ContentContext::ContentContext(std::shared_ptr<Context> context)
  if (!context_ || !context_->IsValid()) {
    return;
  }
+#ifdef FML_OS_ANDROID


Can we have this on on impeller::Context instead? You can ask the context for its feature set. That way, only ES will return the legacy stuff.

chinmaygarde · 2022-11-17T19:02:24Z

ci/licenses_golden/licenses_flutter

@@ -1285,6 +1286,7 @@ FILE: ../../../flutter/impeller/entity/shaders/glyph_atlas_sdf.frag
 FILE: ../../../flutter/impeller/entity/shaders/glyph_atlas_sdf.vert
 FILE: ../../../flutter/impeller/entity/shaders/gradient_fill.vert
 FILE: ../../../flutter/impeller/entity/shaders/linear_gradient_fill.frag
+FILE: ../../../flutter/impeller/entity/shaders/linear_gradient_fixed_fill.frag


lets call this ssbo_fill perhaps?

chinmaygarde · 2022-11-17T19:03:13Z

impeller/compiler/compiler.cc

@@ -45,10 +45,14 @@ static CompilerBackend CreateGLSLCompiler(const spirv_cross::ParsedIR& ir,
  sl_options.force_zero_initialized_variables = true;
  sl_options.vertex.fixup_clipspace = true;
  if (source_options.target_platform == TargetPlatform::kOpenGLES) {
-    sl_options.version = 100;
+    sl_options.version = source_options.gles_language_version > 0


I wonder if we should do source language selection based on #version directives now instead of applying command line options. Omitting them was fine when we only had a single version.

I think that sounds reasonable, but I'm not quite sure how to make make shaderc and spirvcross work together on this. I'll file a bug

flutter/flutter#115587

chinmaygarde · 2022-11-17T19:03:59Z

impeller/geometry/geometry_unittests.h

@@ -141,3 +158,4 @@ inline ::testing::AssertionResult ColorBufferNear(
 #define ASSERT_SIZE_NEAR(a, b) ASSERT_PRED2(&::SizeNear, a, b)
 #define ASSERT_ARRAY_4_NEAR(a, b) ASSERT_PRED2(&::Array4Near, a, b)
 #define ASSERT_COLOR_BUFFER_NEAR(a, b) ASSERT_PRED2(&::ColorBufferNear, a, b)
+#define ASSERT_COLORS_NEAR(a, b) ASSERT_PRED2(&::ColorsNear, a, b)


Newline at EOF.

chinmaygarde · 2022-11-17T19:04:35Z

For clarity, where we support SSBOs, there is no case where we need to use the texture upload thingy right?

jonahwilliams · 2022-11-17T19:10:29Z

For clarity, where we support SSBOs, there is no case where we need to use the texture upload thingy right?

Not that I can tell.

chinmaygarde · 2022-11-17T19:13:31Z

impeller/geometry/gradient.cc

+  for (size_t i = 1; i < stops.size(); i++) {
+    auto value = stops[i] - stops[i - 1];
+    // Smaller than kEhCloseEnough
+    if (value < 0.0001) {


Curious, why not just use the constant instead of hardcoding the value?

context was sort of #36218

TLDR, if you have really close stops you need a smaller number

in the future, we'll need to stop computing the gradients on the CPU for certain stop values

chinmaygarde · 2022-11-17T19:16:28Z

Using purely the SSBO approach on Metal and following up with the jumbo textures thing on legacy backends sounds good to me.

I reviewed the rest of the patch and it looks good to me barring the comments (mostly nits) already added. lgtm after it moves out of draft status.

jonahwilliams · 2022-11-17T19:51:37Z

Needs more tests still, and need to figure out why I can't find the shaders at runtime

jonahwilliams · 2022-11-17T20:43:06Z

Fun times:

gen/flutter/impeller/entity/radial_gradient_ssbo_fill.frag.cc(22,1): error: static assertion failed due to requirement 'sizeof(impeller::RadialGradientSsboFillFragmentShader::ColorData<0>) == 0'
static_assert(sizeof(Shader::ColorData<0>) == 0);
^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
gen/flutter/impeller/entity/radial_gradient_ssbo_fill.frag.cc(22,44): note: expression evaluates to '4 == 0'
static_assert(sizeof(Shader::ColorData<0>) == 0);

jonahwilliams · 2022-11-17T21:07:54Z

impeller/compiler/code_gen_template.h

@@ -202,7 +202,6 @@ using Shader = {{camel_case(shader_name)}}{{camel_case(shader_stage)}}Shader;
 // Sanity checks for {{def.name}}
 {% if last(def.members).array_elements == 0 %}
 static_assert(std::is_standard_layout_v<Shader::{{def.name}}<0>>);
-static_assert(sizeof(Shader::{{def.name}}<0>) == {{def.byte_length}});


This assert appears to be invalid on windows hosts, which claim this always has a sizeof 4.

Hmm, I suppose having platform specific differences here is possible. Was the shader name in the generated shaders correct though?

jonahwilliams · 2022-11-18T17:55:54Z

This is ready 😄

chinmaygarde · 2022-11-18T18:50:19Z

impeller/compiler/code_gen_template.h

@@ -202,7 +202,6 @@ using Shader = {{camel_case(shader_name)}}{{camel_case(shader_stage)}}Shader;
 // Sanity checks for {{def.name}}
 {% if last(def.members).array_elements == 0 %}
 static_assert(std::is_standard_layout_v<Shader::{{def.name}}<0>>);
-static_assert(sizeof(Shader::{{def.name}}<0>) == {{def.byte_length}});


Hmm, I suppose having platform specific differences here is possible. Was the shader name in the generated shaders correct though?

jonahwilliams · 2022-11-18T18:53:46Z

Yes all of the generated code worked correctly. Though TBH I didn't find a way to use the struct generated for the SSBO, since it is dynamically sized - so I ended up creating a BufferView myself. I think that is OK though

…ulkan) (flutter/engine#37654)

…115672) * a76ec9158 Roll Fuchsia Linux SDK from lnmSnyJi-2H07tBnV... to WdtwlLEce90PjFJ9z... (flutter/engine#37747) * 44e2f5854 [Impeller] Change texture upload pipeline in Vulkan (flutter/engine#37623) * 6a3ad3c14 Roll Skia from 345bceacd298 to 5270b1d26b5f (4 revisions) (flutter/engine#37748) * 01271891c Do not abort if a MultiFrameCodec is unable to allocate a bitmap buffer (flutter/engine#37534) * 54232a4c3 Roll Skia from 5270b1d26b5f to cf967e6b1c00 (5 revisions) (flutter/engine#37751) * 30aa3cc38 [fuchsia][a11y] Set explicit hit regions in flatland embedder (flutter/engine#37338) * da07c33d2 Make NotifyIdle reject close and past deadlines. (flutter/engine#37737) * e3844cc1e Add third_party/dart/third_party/binaryen/src as a dependency (flutter/engine#37749) * aeb2cd95b [Impeller] use SSBOs for gradients where supported (metal/vulkan) (flutter/engine#37654) * e8aa1c192 Roll Skia from cf967e6b1c00 to f1f59de17204 (2 revisions) (flutter/engine#37756) * 4311774fb [Impeller] register modern shaders on Vulkan too (flutter/engine#37757) * 8e4a718d0 Made FlutterTextField that outlive FlutterTextPlatformNode not crash (flutter/engine#37735) * 446a09dfc [macOS] Use consistent filenames for tests (flutter/engine#37755) * 7a390f97c Roll Skia from f1f59de17204 to 12f01bc5b57e (1 revision) (flutter/engine#37760)

…lutter#115672) * a76ec9158 Roll Fuchsia Linux SDK from lnmSnyJi-2H07tBnV... to WdtwlLEce90PjFJ9z... (flutter/engine#37747) * 44e2f5854 [Impeller] Change texture upload pipeline in Vulkan (flutter/engine#37623) * 6a3ad3c14 Roll Skia from 345bceacd298 to 5270b1d26b5f (4 revisions) (flutter/engine#37748) * 01271891c Do not abort if a MultiFrameCodec is unable to allocate a bitmap buffer (flutter/engine#37534) * 54232a4c3 Roll Skia from 5270b1d26b5f to cf967e6b1c00 (5 revisions) (flutter/engine#37751) * 30aa3cc38 [fuchsia][a11y] Set explicit hit regions in flatland embedder (flutter/engine#37338) * da07c33d2 Make NotifyIdle reject close and past deadlines. (flutter/engine#37737) * e3844cc1e Add third_party/dart/third_party/binaryen/src as a dependency (flutter/engine#37749) * aeb2cd95b [Impeller] use SSBOs for gradients where supported (metal/vulkan) (flutter/engine#37654) * e8aa1c192 Roll Skia from cf967e6b1c00 to f1f59de17204 (2 revisions) (flutter/engine#37756) * 4311774fb [Impeller] register modern shaders on Vulkan too (flutter/engine#37757) * 8e4a718d0 Made FlutterTextField that outlive FlutterTextPlatformNode not crash (flutter/engine#37735) * 446a09dfc [macOS] Use consistent filenames for tests (flutter/engine#37755) * 7a390f97c Roll Skia from f1f59de17204 to 12f01bc5b57e (1 revision) (flutter/engine#37760)

jonahwilliams added 2 commits November 15, 2022 16:28

[Impeller] use uniform array for more efficient small gradients

ac5289a

++

a993117

jonahwilliams force-pushed the ssbo_gradient branch from a7921a2 to a993117 Compare November 16, 2022 00:30

jonahwilliams added 2 commits November 16, 2022 17:31

Make this work on Android

1e0b407

++

6464e63

jonahwilliams changed the title ~~[Impeller] used fixed size array for small gradients~~ [Impeller] used fixed size array for small gradients on iOS Nov 17, 2022

jonahwilliams commented Nov 17, 2022

View reviewed changes

impeller/entity/contents/content_context.cc Outdated Show resolved Hide resolved

jonahwilliams added 2 commits November 16, 2022 17:45

++

c6764e1

++

ad2913d

jonahwilliams marked this pull request as ready for review November 17, 2022 02:08

jonahwilliams requested review from bdero and dnfield November 17, 2022 02:08

jonahwilliams marked this pull request as draft November 17, 2022 02:48

jonahwilliams changed the title ~~[Impeller] used fixed size array for small gradients on iOS~~ [Impeller] used SSBOs gradients where supported Nov 17, 2022

jonahwilliams changed the title ~~[Impeller] used SSBOs gradients where supported~~ [Impeller] use SSBOs gradients where supported Nov 17, 2022

jonahwilliams added 2 commits November 17, 2022 10:39

compiler support and basic feature detection

6205d78

++

0693dbd

jonahwilliams commented Nov 17, 2022

View reviewed changes

chinmaygarde reviewed Nov 17, 2022

View reviewed changes

chinmaygarde mentioned this pull request Nov 17, 2022

[Impeller] Allow coalescing small texture uploads just like buffers. flutter/flutter#115583

Closed

++

da02aea

chinmaygarde reviewed Nov 17, 2022

View reviewed changes

rename to ssbo_fill, move to context

377b3e5

++

8428aea

jonahwilliams mentioned this pull request Nov 17, 2022

[Impeller] Support using the language version specified in the source shader as the target version. flutter/flutter#115587

Closed

jonahwilliams added 4 commits November 17, 2022 12:08

++

b843436

++

652553e

++

21cc74a

tests

283957a

jonahwilliams added 2 commits November 17, 2022 13:06

++

642a3e9

++

ace492f

jonahwilliams marked this pull request as ready for review November 17, 2022 21:07

jonahwilliams requested a review from chinmaygarde November 17, 2022 21:07

jonahwilliams commented Nov 17, 2022

View reviewed changes

jonahwilliams changed the title ~~[Impeller] use SSBOs gradients where supported~~ [Impeller] use SSBOs for gradients where supported (metal/vulkan) Nov 17, 2022

chinmaygarde approved these changes Nov 18, 2022

View reviewed changes

chinmaygarde assigned jonahwilliams Nov 18, 2022

chinmaygarde added the e: impeller label Nov 18, 2022

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 18, 2022

auto-submit bot merged commit aeb2cd9 into flutter:main Nov 18, 2022

jonahwilliams deleted the ssbo_gradient branch November 18, 2022 19:02

engine-flutter-autoroll mentioned this pull request Nov 18, 2022

Roll Flutter Engine from b32c3a307bb5 to 7a390f97c798 (14 revisions) flutter/flutter#115672

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 18, 2022

aeb2cd95b [Impeller] use SSBOs for gradients where supported (metal/v…

155e70b

…ulkan) (flutter/engine#37654)

[Impeller] use SSBOs for gradients where supported (metal/vulkan) #37654

[Impeller] use SSBOs for gradients where supported (metal/vulkan) #37654

Conversation

jonahwilliams commented Nov 16, 2022 • edited

jonahwilliams commented Nov 17, 2022

jonahwilliams commented Nov 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaygarde commented Nov 17, 2022

jonahwilliams commented Nov 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaygarde commented Nov 17, 2022

jonahwilliams commented Nov 17, 2022

jonahwilliams commented Nov 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Nov 18, 2022

Choose a reason for hiding this comment

jonahwilliams commented Nov 18, 2022

jonahwilliams commented Nov 16, 2022 •

edited