Enable Skia's Vulkan backend on all non-mobile platforms, and test SwiftShader Vulkan on all GL unittests platforms #29520

bdero · 2021-11-04T07:49:49Z

Enables Skia's Vulkan backend on all platforms (which will be usable through the embedder API).
Gets SwiftShader Vulkan building anywhere the GL embedder unittests can run.
Adds a simple test that initializes Vulkan state with SwiftShader (full test harness integration to come).

Patches that need to land before this is merged:

flutter-dashboard · 2021-11-04T07:49:51Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

chinmaygarde

Mostly LGTM except I don't think setting shell_enable_vulkan to true unconditionally is what we want for iOS and Android.

chinmaygarde · 2021-11-10T23:21:36Z

testing/test_vulkan_context.h

+ public:
+  TestVulkanContext();
+  ~TestVulkanContext();
+  bool is_valid();


Nit: bool IsValid() const;.

The case because its OOL and the const because of const-correctness.

chinmaygarde · 2021-11-10T23:22:12Z

testing/test_vulkan_context.h

+  fml::RefPtr<vulkan::VulkanProcTable> vk_;
+  std::unique_ptr<vulkan::VulkanApplication> application_;
+  std::unique_ptr<vulkan::VulkanDevice> logical_device_;
+};


FML_DISALLOW_COPY_AND_ASSIGN explicitly even though this can't really be copied because of the unique pointers.

chinmaygarde · 2021-11-10T23:22:25Z

testing/test_vulkan_context.h

+  bool is_valid();
+
+ private:
+  bool valid_;


Default to false here in case we add additional ctors and forget to initialize the scalar explicitly in one of them? Your call.

sgtm, done.

chinmaygarde · 2021-11-10T23:27:05Z

vulkan/vulkan_proc_table.cc

 #endif  // VULKAN_LINK_STATICALLY
+  return !!handle_;


lol, I never thought of this to avoid an explicit bool cast without actually using an explicit cast. Love it.

chinmaygarde · 2021-11-10T23:29:08Z

tools/gn

@@ -308,6 +308,12 @@ def to_gn_args(args):
      gn_args['skia_use_metal'] = True
      gn_args['shell_enable_metal'] = True

+    # Enable Vulkan on all platforms.
+    gn_args['skia_use_vulkan'] = True
+    gn_args['shell_enable_vulkan'] = True


I believe this will compile the vulkan shell component even on iOS and Android. The desire is to compile Vulkan on all host unit-test targets right?

My intention was to enable Vulkan support for all libflutter platform distributions including iOS, so that embedder writers have the option to target just Vulkan and use translation ICDs like MoltenVK. Partially because I want it, but also just because we can. :)

With that, you run into the issue of bumping up the binary size on those target. And we are pretty sensitive to that metric.

I'll disable this for Android/iOS for the time being and leave a note since it's easier to flip on than flip off later.

bdero · 2021-11-16T22:31:35Z

@chinmaygarde Could you give this another look when you get a moment? I think this should be good to go.

… test SwiftShader Vulkan on all GL unittests platforms (flutter/engine#29520)

knopp · 2021-11-17T09:19:41Z

How is this supposed to work on macos? I get build error when it tries to compile //fluter/vulkan, possibly as part of open gl tests.

bdero · 2021-11-17T09:51:58Z

@knopp The embedder unittests link Swiftshader, which is one of several Vulkan ICDs that support MacOS. Could you open an issue with the error you're getting/repro steps and tag me on it?

knopp · 2021-11-17T10:54:26Z

@bdero, nevermind, I think the problem was missing skia_use_vulkan in args.gn.

Previous commit: flutter@cdbb428 Add test context for loading swiftshader so update Remove unused things Prereqs to GPUSurfaceVulkan rewrite Comment typos

… test SwiftShader Vulkan on all GL unittests platforms (flutter#29520)" Causes the following build breakage (Win32): [19/1564] CXX obj/flutter/vulkan/vulkan.vulkan_window.obj FAILED: obj/flutter/vulkan/vulkan.vulkan_window.obj ninja -t msvc -e environment.x64 -- ../../buildtools/windows-x64/clang/bin/clang-cl.exe /nologo /showIncludes @obj/flutter/vulkan/vulkan.vulkan_window.obj.rsp /c ../../flutter/vulkan/vulkan_window.cc /Foobj/flutter/vulkan/vulkan.vulkan_window.obj /Fdobj/flutter/vulkan/vulkan_cc.pdb ../../flutter/vulkan/vulkan_window.cc(125,24): error: no member named 'MakeVulkan' in 'GrDirectContext' GrDirectContext::MakeVulkan(backend_context, options); ~~~~~~~~~~~~~~~~~^ 1 error generated. This reverts commit bba38c0.

Previous commit: flutter@cdbb428 Add test context for loading swiftshader so update Remove unused things Prereqs to GPUSurfaceVulkan rewrite Comment typos

flutter-dashboard bot added embedder Related to the embedder API platform-android needs tests labels Nov 4, 2021

google-cla bot added the cla: yes label Nov 4, 2021

bdero removed the needs tests label Nov 4, 2021

bdero force-pushed the bdero/minimal-vulkan-test branch 2 times, most recently from 27b0ad1 to ab2206e Compare November 4, 2021 19:54

bdero changed the title ~~Add minimal test to verify Swiftshader Vulkan works~~ WIP: Add minimal test to verify Swiftshader Vulkan works Nov 4, 2021

bdero force-pushed the bdero/minimal-vulkan-test branch 5 times, most recently from da7911f to b19b709 Compare November 8, 2021 20:19

bdero changed the title ~~WIP: Add minimal test to verify Swiftshader Vulkan works~~ WIP: Add minimal Swiftshader Vulkan test and get it building on all platforms where it's possible Nov 8, 2021

This comment has been minimized.

Sign in to view

bdero changed the title ~~WIP: Add minimal Swiftshader Vulkan test and get it building on all platforms where it's possible~~ WIP: Enable Skia's Vulkan backend on all platforms where possible and add minimal Swiftshader Vulkan test Nov 8, 2021

bdero force-pushed the bdero/minimal-vulkan-test branch 4 times, most recently from fc51cc3 to 5a6713e Compare November 9, 2021 08:32

bdero changed the title ~~WIP: Enable Skia's Vulkan backend on all platforms where possible and add minimal Swiftshader Vulkan test~~ WIP: Enable Skia's Vulkan backend on all platform Nov 10, 2021

flutter-dashboard bot added the platform-ios label Nov 10, 2021

bdero mentioned this pull request Nov 10, 2021

Fixes for building SwiftShader Vulkan on Windows flutter/buildroot#524

Merged

bdero changed the title ~~WIP: Enable Skia's Vulkan backend on all platform~~ WIP: Enable Skia's Vulkan backend on all platforms Nov 10, 2021

bdero force-pushed the bdero/minimal-vulkan-test branch from 9498104 to f2789cf Compare November 10, 2021 22:25

bdero changed the title ~~WIP: Enable Skia's Vulkan backend on all platforms~~ Enable Skia's Vulkan backend on all platforms Nov 10, 2021

bdero requested a review from chinmaygarde November 10, 2021 23:15

chinmaygarde reviewed Nov 10, 2021

View reviewed changes

bdero force-pushed the bdero/minimal-vulkan-test branch from 48b7b09 to 1fda20c Compare November 12, 2021 02:08

CaseyHillers changed the base branch from master to main November 15, 2021 18:12

chinmaygarde approved these changes Nov 16, 2021

View reviewed changes

zanderso changed the base branch from master to main November 16, 2021 23:25

bdero merged commit bba38c0 into flutter:main Nov 16, 2021

engine-flutter-autoroll mentioned this pull request Nov 17, 2021

Roll Engine from 2962099077b3 to bba38c0b8312 (11 revisions) flutter/flutter#93759

Closed

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

bba38c0 Enable Skia's Vulkan backend on all non-mobile platforms, and…

0a0d509

… test SwiftShader Vulkan on all GL unittests platforms (flutter/engine#29520)

zanderso mentioned this pull request Nov 17, 2021

Vulkan link error, Fuchsia recipe change. flutter/flutter#93766

Closed

engine-flutter-autoroll mentioned this pull request Nov 17, 2021

Roll Engine from 2962099077b3 to ace1f5f2997b (13 revisions) flutter/flutter#93769

Merged

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

bba38c0 Enable Skia's Vulkan backend on all non-mobile platforms, and…

adeb97b

… test SwiftShader Vulkan on all GL unittests platforms (flutter/engine#29520)

bdero added a commit to bdero/flutter-engine that referenced this pull request Nov 17, 2021

Rebase atop flutter#29520

656a604

Previous commit: flutter@cdbb428 Add test context for loading swiftshader so update Remove unused things Prereqs to GPUSurfaceVulkan rewrite Comment typos

cbracken mentioned this pull request Nov 17, 2021

Revert "Enable Skia's Vulkan backend on all non-mobile platforms, and… #29794

Closed

8 tasks

bdero added a commit to bdero/flutter-engine that referenced this pull request Nov 19, 2021

Rebase atop flutter#29520

a120e51

Previous commit: flutter@cdbb428 Add test context for loading swiftshader so update Remove unused things Prereqs to GPUSurfaceVulkan rewrite Comment typos

bdero added a commit to bdero/flutter-engine that referenced this pull request Dec 2, 2021

Rebase atop flutter#29520

7f74ca0

Previous commit: flutter@cdbb428 Add test context for loading swiftshader so update Remove unused things Prereqs to GPUSurfaceVulkan rewrite Comment typos

bdero added a commit to bdero/flutter-engine that referenced this pull request Dec 2, 2021

Rebase atop flutter#29520

b0e2aa4

Previous commit: flutter@cdbb428 Add test context for loading swiftshader so update Remove unused things Prereqs to GPUSurfaceVulkan rewrite Comment typos

bdero added a commit to bdero/flutter-engine that referenced this pull request Dec 9, 2021

Rebase atop flutter#29520

11e13f3

Previous commit: flutter@cdbb428 Add test context for loading swiftshader so update Remove unused things Prereqs to GPUSurfaceVulkan rewrite Comment typos

bdero added a commit to bdero/flutter-engine that referenced this pull request Dec 13, 2021

Rebase atop flutter#29520

b933684

Previous commit: flutter@cdbb428 Add test context for loading swiftshader so update Remove unused things Prereqs to GPUSurfaceVulkan rewrite Comment typos

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

Rebase atop flutter#29520

80263f1

Previous commit: flutter@cdbb428 Add test context for loading swiftshader so update Remove unused things Prereqs to GPUSurfaceVulkan rewrite Comment typos

bdero added a commit to bdero/flutter-engine that referenced this pull request Dec 15, 2021

Rebase atop flutter#29520

7700137

Previous commit: flutter@cdbb428 Add test context for loading swiftshader so update Remove unused things Prereqs to GPUSurfaceVulkan rewrite Comment typos

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

Rebase atop flutter#29520

f23c83e

Previous commit: flutter@cdbb428 Add test context for loading swiftshader so update Remove unused things Prereqs to GPUSurfaceVulkan rewrite Comment typos

bdero added a commit to bdero/flutter-engine that referenced this pull request Jan 20, 2022

Rebase atop flutter#29520

0f39cbb

Previous commit: flutter@cdbb428 Add test context for loading swiftshader so update Remove unused things Prereqs to GPUSurfaceVulkan rewrite Comment typos

bdero added a commit to bdero/flutter-engine that referenced this pull request Jan 24, 2022

Rebase atop flutter#29520

82b7628

Previous commit: flutter@cdbb428 Add test context for loading swiftshader so update Remove unused things Prereqs to GPUSurfaceVulkan rewrite Comment typos

bdero added a commit to bdero/flutter-engine that referenced this pull request Jan 28, 2022

Rebase atop flutter#29520

e425657

Previous commit: flutter@cdbb428 Add test context for loading swiftshader so update Remove unused things Prereqs to GPUSurfaceVulkan rewrite Comment typos

bdero added a commit to bdero/flutter-engine that referenced this pull request Jan 31, 2022

Rebase atop flutter#29520

66f25d4

Previous commit: flutter@cdbb428 Add test context for loading swiftshader so update Remove unused things Prereqs to GPUSurfaceVulkan rewrite Comment typos

bdero added a commit to bdero/flutter-engine that referenced this pull request Jan 31, 2022

Rebase atop flutter#29520

8a9eed6

Previous commit: flutter@cdbb428 Add test context for loading swiftshader so update Remove unused things Prereqs to GPUSurfaceVulkan rewrite Comment typos

bdero added a commit to bdero/flutter-engine that referenced this pull request Feb 2, 2022

Rebase atop flutter#29520

99a8037

Previous commit: flutter@cdbb428 Add test context for loading swiftshader so update Remove unused things Prereqs to GPUSurfaceVulkan rewrite Comment typos

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Skia's Vulkan backend on all non-mobile platforms, and test SwiftShader Vulkan on all GL unittests platforms #29520

Enable Skia's Vulkan backend on all non-mobile platforms, and test SwiftShader Vulkan on all GL unittests platforms #29520

bdero commented Nov 4, 2021 •

edited

flutter-dashboard bot commented Nov 4, 2021

This comment has been minimized.

chinmaygarde left a comment

chinmaygarde Nov 10, 2021

bdero Nov 12, 2021

chinmaygarde Nov 10, 2021

bdero Nov 12, 2021

chinmaygarde Nov 10, 2021

bdero Nov 12, 2021

chinmaygarde Nov 10, 2021

chinmaygarde Nov 10, 2021

bdero Nov 11, 2021

chinmaygarde Nov 11, 2021

bdero Nov 11, 2021

bdero commented Nov 16, 2021

knopp commented Nov 17, 2021

bdero commented Nov 17, 2021

knopp commented Nov 17, 2021

Enable Skia's Vulkan backend on all non-mobile platforms, and test SwiftShader Vulkan on all GL unittests platforms #29520

Enable Skia's Vulkan backend on all non-mobile platforms, and test SwiftShader Vulkan on all GL unittests platforms #29520

Conversation

bdero commented Nov 4, 2021 • edited

flutter-dashboard bot commented Nov 4, 2021

This comment has been minimized.

chinmaygarde 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

bdero commented Nov 16, 2021

knopp commented Nov 17, 2021

bdero commented Nov 17, 2021

knopp commented Nov 17, 2021

bdero commented Nov 4, 2021 •

edited