[Flutter GPU] Runtime shader import. #48875

bdero · 2023-12-11T05:21:16Z

Add --shader-bundle mode to impellerc that takes a simple JSON spec and produces a single flatbuffer with a pack of named shaders.
Added "single invocation" mode to the impellerc GN templates.
Record vertex attribute reflection information.
Light refactoring of the compiler frontend to make compiler invocations easier to follow.

Example shader bundle spec (json form of the yaml spec as shown in the Flutter GPU doc):

{
    "UnlitFragment": {
        "type": "fragment",
        "file": "shaders/flutter_gpu_unlit.frag"
    },
    "UnlitVertex": {
        "type": "vertex",
        "file": "shaders/flutter_gpu_unlit.vert"
    },
    "TextureFragment": {
        "type": "fragment",
        "file": "shaders/flutter_gpu_texture.frag"
    },
    "TextureVertex": {
        "type": "vertex",
        "file": "shaders/flutter_gpu_texture.vert"
    }
}

Example impellerc invocation:

impellerc \
  --include=~/projects/flutter/engine/src/flutter/impeller/compiler/shader_lib \
  --runtime-stage-metal \
  --sl=assets/TestLibrary.shaderbundle \
  --shader-bundle='{"UnlitFragment": {"type": "fragment", "file": "shaders/flutter_gpu_unlit.frag"}, "UnlitVertex": {"type": "vertex", "file": "shaders/flutter_gpu_unlit.vert"}, "TextureFragment": {"type": "fragment", "file": "shaders/flutter_gpu_texture.frag"}, "TextureVertex": {"type": "vertex", "file": "shaders/flutter_gpu_texture.vert"}}'

Runtime usage:

    /// Add a render pass encoder to the command buffer so that we can start
    /// encoding commands.
    final encoder = commandBuffer.createRenderPass(renderTarget);

    /// Load a shader bundle asset.
    final library =
        gpu.ShaderLibrary.fromAsset('assets/TestLibrary.shaderbundle')!;

    /// Create a RenderPipeline using shaders from the asset.
    final vertex = library['TextureVertex']!;
    final fragment = library['TextureFragment']!;
    final pipeline = gpu.gpuContext.createRenderPipeline(vertex, fragment);

    encoder.bindPipeline(pipeline);

Screen.Recording.2023-12-10.at.5.15.28.AM.mov

bdero · 2023-12-12T07:57:39Z

Alright, I think the testing story is looking pretty good for this. We have:

Tests for all the parser branches/error messages & a CLI switches test.
A test that runs the compiler from json string to flatbuffer and verifies the results.
A shader bundle being built as part of the fixture through the impellerc GN rule.
renderer_dart_unittests deserializes the fixture, and a test is using the shader library to form a pipeline and submit a valid raster command.

jonahwilliams

LGTM, mostly nits

jonahwilliams · 2023-12-12T15:56:53Z

impeller/compiler/reflector.cc

@@ -353,6 +351,35 @@ std::shared_ptr<RuntimeStageData> Reflector::GenerateRuntimeStageData() const {
    uniform_description.array_elements = GetArrayElements(spir_type);
    data->AddUniformDescription(std::move(uniform_description));
  }
+
+  // We only need to worry about reflecting vertex attributes.


don't we already reflect over the vertex attributes somewhere in compiler.cc?

Yeah, for the reflection C header. We pack the same data here.

jonahwilliams · 2023-12-12T15:58:50Z

impeller/compiler/reflector.cc

+      input_description.set = compiler_->get_decoration(
+          input.id, spv::Decoration::DecorationDescriptorSet);


FYI the vulkan backend can't support specifying different descriptor sets right now

Good to know. I just tried to replicate the way we pack the header template for now. My guess is that we can probably get away with removing a couple of these fields everywhere.

jonahwilliams · 2023-12-12T15:59:46Z

impeller/compiler/shader_bundle.cc

+    return nullptr;
+  }
+
+  // TODO(bdero): Vertex attributes.


file an issue for this?

Oops, this is a leftover comment. We've already got the attributes packed! :)

jonahwilliams · 2023-12-12T16:01:51Z

impeller/compiler/types.cc

+  for (auto it = name.begin(); it != name.end(); it++) {
+    *it = std::tolower(static_cast<unsigned char>(*it));
+  }


If you need to lowercase the string I think there is a utility for doing that explicitly instead of mutating the function arg

I didn't manage to find one, but I did notice another place where I did this same thing in the compiler like a year ago. So I went ahead and added one to use in both places.

jonahwilliams · 2023-12-12T16:02:27Z

impeller/compiler/types.cc

+  if (source_language == "glsl") {
+    return SourceLanguage::kGLSL;
+  } else if (source_language == "hlsl") {
+    return SourceLanguage::kHLSL;
+  }
+  return SourceLanguage::kUnknown;


Suggested change

if (source_language == "glsl") {

return SourceLanguage::kGLSL;

} else if (source_language == "hlsl") {

return SourceLanguage::kHLSL;

}

return SourceLanguage::kUnknown;

if (source_language == "glsl") {

return SourceLanguage::kGLSL;

}

if (source_language == "hlsl") {

return SourceLanguage::kHLSL;

}

return SourceLanguage::kUnknown;

jonahwilliams · 2023-12-12T16:04:01Z

impeller/compiler/types.h

+  std::string entry_point;
+};
+
+using ShaderBundleConfig = std::map<std::string, ShaderConfig>;


Not that it is super important but this should probably be an unordered_map unless you need the ordering properties.

Nah order isn't important for this, done.

jonahwilliams · 2023-12-12T16:05:07Z

lib/gpu/lib/src/shader_library.dart

+  // Hold a Dart-side reference to shaders in the library as they're wrapped for
+  // the first time. This prevents the wrapper from getting prematurely
+  // destroyed.
+  Map<String, Shader> shaders_ = {};


Suggested change

Map<String, Shader> shaders_ = {};

final Map<String, Shader> shaders_ = {};

jonahwilliams · 2023-12-12T16:06:12Z

lib/gpu/lib/src/shader_library.dart

+      if (result != null) {
+        shaders_[shaderName] = result;


this seems like it should be an error condition?

maybe something like this:

Shader? result = shaders_.putIfAbsent(shaderName, () { var result = _getShader(shaderName, Shader._()); if (result == null) { throw Exception('..'); } }

The intention is to return null if the user tries to look up a shader that doesn't exist in the bundle.

If the shader bundle can't change over time I would let the map contain null entries then:

Map<String, Shader?> shaders_ = {};

Then this function becomes:

return shaders_.putIfAbsent(shaderName, () => _getShader(shaderName, Shader._()));

Ah good point. Done.

zanderso · 2023-12-12T16:27:22Z

impeller/fixtures/BUILD.gn

@@ -76,6 +76,10 @@ test_fixtures("file_fixtures") {
    "blue_noise.png",
    "boston.jpg",
    "embarcadero.jpg",
+    "flutter_gpu_unlit.frag",


nit: alphabetize

Done. Someday I'll learn my abcs :^)

zanderso · 2023-12-12T16:33:13Z

impeller/tools/impeller.gni

-    compiled_action_foreach(target_name) {
-      forward_variables_from(invoker, "*")
-      tool = "//flutter/impeller/compiler:impellerc"
+  if (invoker.single_invocation) {


Please document the template parameter in the comment on the template.

zanderso · 2023-12-12T16:34:36Z

impeller/tools/impeller.gni

             ])

  # Optional: invoker.iplr Causes --sl output to be in iplr format.
-  # Optional: invoker.iplr_bundle specifies a Flutter GPU shader bundle configuration.
+  # Optional: invoker.shader_bundle specifies a Flutter GPU shader bundle configuration.


These comments about the parameters are in a bit of weird spot. They should probably be moved up above line 278, and all the accepted parameters should be documented. Like if this template accepts single_invocation, then it should be documented in the docstring for this template.

Alrighty, I moved these up to the top of the template and tried to document the missing ones accurately.

zanderso · 2023-12-13T16:06:38Z

impeller/compiler/BUILD.gn

@@ -68,6 +70,7 @@ impeller_component("compiler_lib") {
    "//flutter/fml",

    # All third_party deps must be included by the global license script.
+    "//flutter/impeller/shader_bundle:shader_bundle_flatbuffers",


The comment on the line above seems to be referring to the third_party deps on the line below. Maybe this line should instead be grouped right below //flutter/fml instead of below this comment?

zanderso · 2023-12-13T16:14:14Z

impeller/compiler/switches.cc

  stream << "[optional] --source-language=glsl|hlsl (default: glsl)"
         << std::endl;
  stream << "[optional] --entry-point=<entry_point_name> (default: main; "
            "ignored for glsl)"
         << std::endl;
  stream << "[optional] --iplr (causes --sl file to be emitted in iplr format)"
         << std::endl;
-  stream << "[optional] --iplr-bundle=<bundle_spec> (causes --sl file to be "
-            "emitted in the iplr bundle format)"
+  stream << "[optional] --shader-bundle=<bundle_spec> (causes --sl file to be "


Any thoughts about reading the json spec in from a file instead of the flag itself being json-valued? I wonder which will be easier for tools that wrap/call impellerc.

My hunch is that storing it in a file before invoking is more of a pain given the ephemeral nature. Although it would be slightly more convenient for the many of manual invocations I've been doing over the past couple of days. : )

…140130) flutter/engine@9f7004e...923f9e2 2023-12-14 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Windows] Move to `FlutterCompositor` for rendering" (flutter/engine#49015) 2023-12-14 magder@google.com Add xcprivacy privacy manifest to iOS framework (flutter/engine#48951) 2023-12-14 30870216+gaaclarke@users.noreply.github.com [Impeller] Made the new blur support 1D blurs (flutter/engine#49001) 2023-12-14 skia-flutter-autoroll@skia.org Roll Skia from 69c02c9d56b2 to 188515347032 (1 revision) (flutter/engine#49005) 2023-12-14 bdero@google.com [Impeller] Add golden for clipped+transformed blur. (flutter/engine#48886) 2023-12-14 bdero@google.com [Flutter GPU] Runtime shader import. (flutter/engine#48875) 2023-12-13 737941+loic-sharma@users.noreply.github.com [Windows] Move to `FlutterCompositor` for rendering (flutter/engine#48849) 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 jsimmons@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

bdero · 2024-01-03T01:04:46Z

This is part of the Flutter GPU MVP umbrella bug: flutter/flutter#130921

bdero self-assigned this Dec 11, 2023

github-actions bot added the e: impeller label Dec 11, 2023

bdero force-pushed the bdero/dart-gpu-compiler branch from a91b5da to 1e2910d Compare December 11, 2023 06:11

bdero added 20 commits December 11, 2023 15:19

Compiler

2e2cd70

Naming

ea36c73

[Flutter GPU] Add shader bundle format.

e3f8cbf

Split compiler frontend code for readability

f8e4703

Move bundle parser into its own file

91cfa25

Bundle parsing routine

cf61483

Compile the shaders

5243eff

Write flatbuffer

7f45044

Runtime crash fix

35614d4

impellerc GN rules

34e063a

Reflect stage data

fea9317

Runtime shader lib import

d639c2a

Fix entrypoint naming mismatch

fe51086

Implement asset loading

b475d35

Asset dependency

0f12626

Hold on to dart-side refs for shader libraries

1da673e

Remove excess runtime classes that didn't end up getting used

98f3cf5

Licenses

ce7ee8d

Start of tests

e57b927

Error message tests

c9176dc

bdero force-pushed the bdero/dart-gpu-compiler branch from 3b88d00 to c9176dc Compare December 11, 2023 23:19

bdero added 3 commits December 11, 2023 18:56

Remainder of parser tests

cd5ba50

Full bundle->flatbuffer test

ce28f95

Licenses

2146d31

bdero requested review from chinmaygarde, zanderso and jonahwilliams December 12, 2023 07:45

bdero marked this pull request as ready for review December 12, 2023 07:50

jonahwilliams approved these changes Dec 12, 2023

View reviewed changes

zanderso reviewed Dec 12, 2023

View reviewed changes

bdero added 3 commits December 12, 2023 15:14

Jonah review

9e3fccc

Zach review

1919922

Jonah review

d7ef71d

zanderso reviewed Dec 13, 2023

View reviewed changes

zach review

be1078c

bdero added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 14, 2023

auto-submit bot merged commit a141712 into flutter:main Dec 14, 2023
28 checks passed

engine-flutter-autoroll mentioned this pull request Dec 14, 2023

Roll Flutter Engine from 45b95f264d63 to 8bb254ecfd45 (3 revisions) flutter/flutter#140116

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 14, 2023

a14171213 [Flutter GPU] Runtime shader import. (flutter/engine#48875)

562d0b4

engine-flutter-autoroll mentioned this pull request Dec 14, 2023

Roll Flutter Engine from 9f7004e3e30e to cf7c26091403 (6 revisions) flutter/flutter#140124

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 14, 2023

a14171213 [Flutter GPU] Runtime shader import. (flutter/engine#48875)

82c6d6b

engine-flutter-autoroll mentioned this pull request Dec 14, 2023

Roll Flutter Engine from 9f7004e3e30e to 923f9e29d4b5 (7 revisions) flutter/flutter#140130

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 14, 2023

a14171213 [Flutter GPU] Runtime shader import. (flutter/engine#48875)

88b5df2

bdero mentioned this pull request Jan 3, 2024

[Impeller] Flutter GPU: Add compilation of shader bundles. flutter/flutter#130938

Closed

bleroux mentioned this pull request Jan 11, 2024

"Could not open file for bundled shader "UnlitVertex"" building inside src/flutter flutter/flutter#140969

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Flutter GPU] Runtime shader import. #48875

[Flutter GPU] Runtime shader import. #48875

bdero commented Dec 11, 2023 •

edited

Loading

bdero commented Dec 12, 2023

jonahwilliams left a comment

jonahwilliams Dec 12, 2023

bdero Dec 12, 2023

jonahwilliams Dec 12, 2023

bdero Dec 12, 2023

jonahwilliams Dec 12, 2023

bdero Dec 12, 2023

jonahwilliams Dec 12, 2023

bdero Dec 12, 2023

jonahwilliams Dec 12, 2023

bdero Dec 12, 2023

jonahwilliams Dec 12, 2023

bdero Dec 12, 2023

jonahwilliams Dec 12, 2023

bdero Dec 12, 2023

jonahwilliams Dec 12, 2023

jonahwilliams Dec 12, 2023

bdero Dec 12, 2023

jonahwilliams Dec 13, 2023

bdero Dec 13, 2023

zanderso Dec 12, 2023

bdero Dec 13, 2023

zanderso Dec 12, 2023

bdero Dec 13, 2023

zanderso Dec 12, 2023

bdero Dec 13, 2023

zanderso Dec 13, 2023

bdero Dec 13, 2023

zanderso Dec 13, 2023

bdero Dec 13, 2023

bdero commented Jan 3, 2024

		input_description.set = compiler_->get_decoration(
		input.id, spv::Decoration::DecorationDescriptorSet);

	Map<String, Shader> shaders_ = {};
	final Map<String, Shader> shaders_ = {};

[Flutter GPU] Runtime shader import. #48875

[Flutter GPU] Runtime shader import. #48875

Conversation

bdero commented Dec 11, 2023 • edited Loading

bdero commented Dec 12, 2023

jonahwilliams left a 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

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

bdero commented Jan 3, 2024

bdero commented Dec 11, 2023 •

edited

Loading