[Impeller] Vulkan runtime effects/fragment program API #49543

dnfield · 2024-01-04T22:19:38Z

Fixes flutter/flutter#122823
Fixes flutter/flutter#129659
Fixes flutter/flutter#123741

This patch makes runtime stage/fragment program stuff work on Vulkan for Android.

It will need flutter/flutter#140976 for that to become a reality for flutter_tools users.

Compiling with relaxed Vulkan semantics still has an issue: shaders that use sampler2D with an explicitly set location on the layout will fail to compile with an error documented in flutter/flutter#141219.

I think there might still be some issues with fragment programs on Vulkan, but this should at least be a good starting point and unblocks ink_sparkle.frag usage in the framework.

I've deleted some runtime_stage related code that would never get used - for example, enum related code that indicates we might support a bunch of data types that we do not and probably never will support in this API.

impeller/entity/contents/runtime_effect_contents.cc

impeller/playground/backend/vulkan/playground_impl_vk.cc

jonahwilliams · 2024-01-10T00:01:03Z

impeller/entity/contents/runtime_effect_contents.cc

@@ -79,15 +68,6 @@ static std::shared_ptr<ShaderMetadata> MakeShaderMetadata(
 bool RuntimeEffectContents::Render(const ContentContext& renderer,


A nit on the current design. We should be able to pull together the descriptor set layouts when registering the shader/pipeline somehow. i'm not 100% certain how PipelineDescriptor equality works and I'd be slightly worredi that we would recreate pipelines on each frame.

This isn't blocking of course, but a follow up issue to make this more efficient would be appreciated.

Filed flutter/flutter#141222

jonahwilliams

LGTM with nit

auto-submit · 2024-01-10T00:12:08Z

auto label is removed for flutter/engine/49543, due to - The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

jonahwilliams · 2024-01-10T00:39:08Z

We talked a bit offline and found that the runtime effect wasn't caching the compiled pipeline, which needs to be kept alive for Vulkan. I suggested a RuntimeEffect pipeline cache on the content context that could be used with a cache key composed of descriptor + shader name.

dnfield · 2024-01-10T05:37:59Z

Implemented the cache and updated a test for it in 2b1d8c2

chinmaygarde · 2024-01-10T18:57:11Z

impeller/entity/contents/content_context.h

+
+    struct Hash {
+      std::size_t operator()(const RuntimeEffectPipelineKey& key) const {
+        return fml::HashCombine(key.unique_entrypoint_name,


Perhaps we should just have std::hash specializations for fml::HashCombine and friends can work without having to specify the Hash struct.

Would that be a specialization on impeller::Comparable<T>? Or did you have something else in mind?

chinmaygarde · 2024-01-10T18:58:12Z

impeller/renderer/backend/gles/shader_library_gles.cc

@@ -141,7 +141,7 @@ void ShaderLibraryGLES::UnregisterFunction(std::string name,
  const auto key = ShaderKey{name, stage};

  auto found = functions_.find(key);
-  if (found != functions_.end()) {
+  if (found == functions_.end()) {


Yeah this code was just not tested lol

chinmaygarde · 2024-01-10T19:13:47Z

impeller/runtime_stage/runtime_stage_unittests.cc

 #include "flutter/testing/testing.h"
+#include "gmock/gmock.h"


Did you mean to import this?

Needed for EXPECT_THAT to make it easier to compare vectors.

chinmaygarde · 2024-01-10T19:17:04Z

impeller/runtime_stage/runtime_stage.h

@@ -18,6 +18,8 @@ namespace impeller {

 class RuntimeStage {
 public:
+  static const char* kVulkanUBOName;


Can you add a docstring for this? Something like "The made-up name of the struct used to the contain the non-opaque uniforms specified directly by the user in the custom runtime fragment stage. Vulkan cannot support non-opaque uniforms which is why these must be moved into a UBO with this name."

Or something like that.

chinmaygarde · 2024-01-10T20:16:49Z

impeller/entity/contents/content_context.h

@@ -729,10 +729,42 @@ class ContentContext {
    return render_target_cache_;
  }

+  std::shared_ptr<Pipeline<PipelineDescriptor>> GetCachedRuntimeEffectPipeline(


The immediate thought that came to mind was, "Aren't all runtime stage entrypoints just called 'main'?" I suppose we mangle taking into account the name of the stage. But that still means that different stages with the same name will collide. I am not sure if that issue is purely hypothetical but it will be hard to debug if we run into it.

I suppose we could hash the name and the structure of the inputs before in the compiler and include that in the fbs. Or just generate a UUID.

How the uniqueness of the entrypoint is guaranteed today would be good to add to a docstring.

I'll try to document this. We build up the entrypoint name from the file name in the compiler.

I was going to try to plumb through the asset name for this but realized we basically already have that in the entrypoint name.

A single type-4 UUID would be fine honestly. But I suspect this works fine for now. Filing a followup works.

flutter/flutter#141304

auto-submit · 2024-01-10T20:44:19Z

auto label is removed for flutter/engine/49543, due to - The status or check suite Windows windows_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

The status or check suite Mac mac_clang_tidy has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux linux_android_aot_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux linux_host_desktop_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux linux_arm_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux linux_clang_tidy has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux Framework Smoke Tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2024-01-10T22:14:06Z

auto label is removed for flutter/engine/49543, due to Pull request flutter/engine/49543 is not in a mergeable state.

auto-submit · 2024-01-10T22:49:49Z

auto label is removed for flutter/engine/49543, due to - The status or check suite Linux linux_web_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

flutter-dashboard · 2024-01-11T00:44:34Z

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 #49543 at sha 7f597b1

…tter/engine#49543)

…141337) flutter/engine@42e3e02...ade9f18 2024-01-11 [email protected] [Impeller] Vulkan runtime effects/fragment program API (flutter/engine#49543) 2024-01-11 [email protected] Roll Dart SDK from 9f5a6a2ccace to 3245b92a5930 (1 revision) (flutter/engine#49700) 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 [email protected],[email protected],[email protected] 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

I broke this in #49543 While investigating flutter/flutter#141235 I realized that the specific things that were wrong there were probably artifacts of some intermediate issue in the previous patch - they no longer appear. However, I had broken hot reload. This patch fixes it and updates the `EntityTest.RuntimeEffect` to artificially run the rendering callback a few times simulating a hot reload. The test fails without my changes here.

Vulkan runtime effects started

e5b2071

github-actions bot added the e: impeller label Jan 4, 2024

dnfield mentioned this pull request Jan 4, 2024

[Tool][Impeller] Make impellerc produce Vulkan and GLES shaders for Android. flutter/flutter#140976

Merged

jonahwilliams reviewed Jan 4, 2024

View reviewed changes

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

dnfield added 7 commits January 4, 2024 15:16

Merge remote-tracking branch 'upstream/main' into impellerc

4c765ce

...

7380566

merge

0e48330

It sort of works

eead81f

fix samplers

7a2f981

update test

c48be10

another updated test

a202d34

dnfield marked this pull request as ready for review January 9, 2024 23:41

dnfield requested review from chinmaygarde, bdero and jonahwilliams January 9, 2024 23:41

dnfield changed the title ~~[Impeller] Vulkan runtime effects started~~ [Impeller] Vulkan runtime effects/fragment program API Jan 9, 2024

jonahwilliams reviewed Jan 9, 2024

View reviewed changes

impeller/playground/backend/vulkan/playground_impl_vk.cc Outdated Show resolved Hide resolved

unintended file change

db0ad49

jonahwilliams reviewed Jan 10, 2024

View reviewed changes

jonahwilliams approved these changes Jan 10, 2024

View reviewed changes

dnfield mentioned this pull request Jan 10, 2024

[Impeller] RuntimeEffectContents could calculate the DescriptorSetLayouts earlier and once flutter/flutter#141222

Closed

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 10, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 10, 2024

Cache the pipeline, resolve one more TODO

2b1d8c2

dnfield added 3 commits January 10, 2024 00:02

Skip vulkan test on mac where sources dont match backend type?

1f8b534

GLES issues...

a1bf361

Merge branch 'impellerc' of github.com:dnfield/engine into impellerc

5b86c99

chinmaygarde reviewed Jan 10, 2024

View reviewed changes

dnfield added 4 commits January 10, 2024 12:17

NPE

5851a3e

Fix test failure

6e59efc

docs

622289d

merge

6e46e5f

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 10, 2024

dnfield mentioned this pull request Jan 10, 2024

[Impeller] Generate a UUID for runtime stage shaders to use as a unique key for that shader flutter/flutter#141304

Open

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 10, 2024

merge

9c67107

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 10, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 10, 2024

merge again

7f597b1

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 10, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 10, 2024

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 11, 2024

flutter-dashboard bot added the will affect goldens label Jan 11, 2024

auto-submit bot merged commit ade9f18 into flutter:main Jan 11, 2024
30 checks passed

dnfield deleted the impellerc branch January 11, 2024 02:29

engine-flutter-autoroll mentioned this pull request Jan 11, 2024

Roll Flutter Engine from 42e3e0236112 to ade9f18fa008 (2 revisions) flutter/flutter#141337

Merged

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

ade9f18fa [Impeller] Vulkan runtime effects/fragment program API (flu…

f980b53

…tter/engine#49543)

dnfield mentioned this pull request Jan 12, 2024

[Impeller] Fix hot reload for shaders on Impeller #49739

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] Vulkan runtime effects/fragment program API #49543

[Impeller] Vulkan runtime effects/fragment program API #49543

dnfield commented Jan 4, 2024 •

edited

Loading

jonahwilliams Jan 10, 2024

dnfield Jan 10, 2024

jonahwilliams left a comment

auto-submit bot commented Jan 10, 2024

jonahwilliams commented Jan 10, 2024

dnfield commented Jan 10, 2024

chinmaygarde Jan 10, 2024

dnfield Jan 10, 2024

chinmaygarde Jan 10, 2024

dnfield Jan 10, 2024

chinmaygarde Jan 10, 2024

dnfield Jan 10, 2024

chinmaygarde Jan 10, 2024

chinmaygarde Jan 10, 2024

dnfield Jan 10, 2024

chinmaygarde Jan 10, 2024

chinmaygarde Jan 10, 2024

dnfield Jan 10, 2024

dnfield Jan 10, 2024

chinmaygarde Jan 10, 2024

dnfield Jan 10, 2024

auto-submit bot commented Jan 10, 2024

auto-submit bot commented Jan 10, 2024

auto-submit bot commented Jan 10, 2024

flutter-dashboard bot commented Jan 11, 2024

		@@ -79,15 +68,6 @@ static std::shared_ptr<ShaderMetadata> MakeShaderMetadata(
		bool RuntimeEffectContents::Render(const ContentContext& renderer,

		#include "flutter/testing/testing.h"
		#include "gmock/gmock.h"

[Impeller] Vulkan runtime effects/fragment program API #49543

[Impeller] Vulkan runtime effects/fragment program API #49543

Conversation

dnfield commented Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams left a comment

Choose a reason for hiding this comment

auto-submit bot commented Jan 10, 2024

jonahwilliams commented Jan 10, 2024

dnfield commented Jan 10, 2024

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

auto-submit bot commented Jan 10, 2024

auto-submit bot commented Jan 10, 2024

auto-submit bot commented Jan 10, 2024

flutter-dashboard bot commented Jan 11, 2024

dnfield commented Jan 4, 2024 •

edited

Loading