[Impeller] Use Vulkan by default #40809

dnfield · 2023-03-30T23:11:24Z

Adds a flag --impeller-force-gl to allow us to force the GLES backend to be used for testing purposes. Wired up in the tool in flutter/flutter#123828.

Attempts to create the Vulkan backend first (unless flag requesting GL is true). If the context can't be created, then creates the GLES backend instead. Updates the logging statements about using impeller to specify whether it's GL or Vulkan.

I've renamed AndroidRenderingApi::kOpenGLES to AndroidRenderingApi::kGPU because it's confusing to have it referred to that way for Vulkan. Also renamed AndroidContextGLImpeller to AndroidContextImpeller - although that class is not really used at this point.

Fixes flutter/flutter#116801

chinmaygarde

Some nits but all minor. LGTM. Thanks!

chinmaygarde · 2023-03-30T23:13:53Z

shell/common/switches.h

@@ -266,6 +266,11 @@ DEF_SWITCH(EnableVulkanValidation,
           "Enable loading Vulkan validation layers. The layers must be "
           "available to the application and loadable. On non-Vulkan backends, "
           "this flag does nothing.")
+DEF_SWITCH(ImpellerUseGL,
+           "impeller-use-gl",


--impeller-force-gl makes it a teeny bit clearer IMO.

chinmaygarde · 2023-03-30T23:14:46Z

shell/common/switches.h

+DEF_SWITCH(ImpellerUseGL,
+           "impeller-use-gl",
+           "Force Impeller to use the GL backend, if available. If Impeller is "
+           "not enabled or it does not support a GL backend for the target "


Nit: commas in help text.

chinmaygarde · 2023-03-30T23:16:07Z

shell/platform/android/android_surface_vulkan_impeller.cc

@@ -56,6 +56,9 @@ AndroidSurfaceVulkanImpeller::AndroidSurfaceVulkanImpeller(
      CreateImpellerContext(proc_table_, workers_, enable_vulkan_validation);
  is_valid_ =
      proc_table_->HasAcquiredMandatoryProcAddresses() && impeller_context_;
+  if (is_valid_) {
+    FML_LOG(ERROR) << "Using the Impeller Vulkan rendering backend.";


Perhaps just an info log. Not really an error that we end up using Vulkan.

This matches the behavior on the GL side, and I assume it was done this way because info logs by default do not show up from flutter run.

chinmaygarde · 2023-03-30T23:16:15Z

shell/platform/android/context/android_context.h

@@ -13,7 +13,7 @@ namespace flutter {

 enum class AndroidRenderingAPI {
  kSoftware,
-  kOpenGLES,
+  kGPU,


chinmaygarde · 2023-03-30T23:17:06Z

shell/platform/android/platform_view_android.cc

+          if (vk_surface->IsValid()) {
+            return vk_surface;
+          }
+        }


An info log here saying we are falling back to GL would be good.

There's a log later saying we're using ht eGL backend in this path.

chinmaygarde · 2023-03-30T23:17:30Z

shell/platform/android/android_surface_gl_impeller.cc

@@ -75,10 +75,9 @@ static std::shared_ptr<impeller::Context> CreateImpellerContext(
    FML_LOG(ERROR) << "Could not add reactor worker.";
    return nullptr;
  }
-  FML_LOG(ERROR) << "Using the Impeller rendering backend.";
+  FML_LOG(ERROR) << "Using the Impeller GL rendering backend.";


I guess the same comment about this being an info log.

(At some point this shoul dbecome an info log but let's handle that in a separate patch)

auto-submit · 2023-03-30T23:46:46Z

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

The status or check suite Linux Host Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

flutter-dashboard · 2023-03-31T02:53:28Z

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 #40809 at sha f34e877

zanderso · 2023-03-31T03:32:52Z

@gaaclarke What's your interpretation of the flagged skia gold diff? Since this PR probably only affected Android/Vulkan, I'm wondering if it means that the test might be flaky?

dnfield · 2023-03-31T05:24:05Z

I asked in the impeller chat, I think thi particular one is flaky. It's related to text rendering and probably has some difference depending on whehter it's running on an Intel or Arm mac.

Sibling of the engine patch flutter/engine#40809 Part of flutter#116801

This reverts commit a6421ed.

This reverts commit a6421ed. Reverting for flutter/flutter#123859

dnfield added 2 commits March 30, 2023 15:41

[Impeller] Use Vulkan by default

0d5ad27

Flag for GL

bab7686

dnfield requested review from chinmaygarde, bdero, zanderso and jonahwilliams March 30, 2023 23:11

flutter-dashboard bot added the platform-android label Mar 30, 2023

chinmaygarde approved these changes Mar 30, 2023

View reviewed changes

nits

71a05a9

dnfield self-assigned this Mar 30, 2023

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 30, 2023

chinmaygarde approved these changes Mar 30, 2023

View reviewed changes

Merge branch 'main' into vulkan

bc4fa1d

chinmaygarde mentioned this pull request Mar 30, 2023

[Impeller] Plumb through the impeller-force-gl flag. flutter/flutter#123828

Merged

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 30, 2023

dnfield added 2 commits March 30, 2023 18:23

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

646bc6c

Missing include from bad merge

f34e877

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 31, 2023

flutter-dashboard bot added the will affect goldens label Mar 31, 2023

auto-submit bot merged commit a6421ed into flutter:main Mar 31, 2023

dnfield deleted the vulkan branch March 31, 2023 05:24

engine-flutter-autoroll mentioned this pull request Mar 31, 2023

Roll Flutter Engine from 013e936fee7b to a6421ed6bbb7 (1 revision) flutter/flutter#123839

Merged

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

a6421ed6b [Impeller] Use Vulkan by default (flutter/engine#40809)

ea408ea

chinmaygarde added a commit to chinmaygarde/flutter that referenced this pull request Mar 31, 2023

[Impeller] Plumb through the impeller-force-gl flag.

d13ff37

Sibling of the engine patch flutter/engine#40809 Part of flutter#116801

zanderso mentioned this pull request Mar 31, 2023

[Impeller] Vulkan with GL fallback PR failing on device lab Moto G4s flutter/flutter#123859

Closed

zanderso added a commit to zanderso/engine that referenced this pull request Mar 31, 2023

Revert "[Impeller] Use Vulkan by default (flutter#40809)"

786939d

This reverts commit a6421ed.

zanderso added a commit to zanderso/engine that referenced this pull request Mar 31, 2023

Revert "[Impeller] Use Vulkan by default (flutter#40809)"

2a0d97f

This reverts commit a6421ed.

zanderso added a commit that referenced this pull request Mar 31, 2023

Revert "[Impeller] Use Vulkan by default (#40809)" (#40820)

0776f38

This reverts commit a6421ed. Reverting for flutter/flutter#123859

engine-flutter-autoroll mentioned this pull request Mar 31, 2023

Roll Flutter Engine from 013e936fee7b to 252f6cfd0335 (8 revisions) flutter/flutter#123874

Merged

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

a6421ed6b [Impeller] Use Vulkan by default (flutter/engine#40809)

0015f0c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Impeller] Use Vulkan by default #40809

[Impeller] Use Vulkan by default #40809

Uh oh!

dnfield commented Mar 30, 2023 •

edited by chinmaygarde

Loading

Uh oh!

chinmaygarde left a comment

Uh oh!

chinmaygarde Mar 30, 2023

Uh oh!

dnfield Mar 30, 2023

Uh oh!

chinmaygarde Mar 30, 2023

Uh oh!

chinmaygarde Mar 30, 2023

Uh oh!

dnfield Mar 30, 2023

Uh oh!

chinmaygarde Mar 30, 2023

Uh oh!

chinmaygarde Mar 30, 2023

Uh oh!

dnfield Mar 30, 2023

Uh oh!

chinmaygarde Mar 30, 2023

Uh oh!

dnfield Mar 30, 2023

Uh oh!

auto-submit bot commented Mar 30, 2023

Uh oh!

flutter-dashboard bot commented Mar 31, 2023

Uh oh!

zanderso commented Mar 31, 2023

Uh oh!

dnfield commented Mar 31, 2023

Uh oh!

Uh oh!

[Impeller] Use Vulkan by default #40809

[Impeller] Use Vulkan by default #40809

Uh oh!

Conversation

dnfield commented Mar 30, 2023 • edited by chinmaygarde Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinmaygarde 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!

auto-submit bot commented Mar 30, 2023

Uh oh!

flutter-dashboard bot commented Mar 31, 2023

Uh oh!

zanderso commented Mar 31, 2023

Uh oh!

dnfield commented Mar 31, 2023

Uh oh!

Uh oh!

dnfield commented Mar 30, 2023 •

edited by chinmaygarde

Loading