[windows/linux] add ability to enable Impeller on windows and linux desktop. #162684

jonahwilliams · 2025-02-04T18:56:13Z

Currently the --enable-impeller runtime flag works but there is no compile time option for desktop. This adds a flag that can be enabled via modifying the project source.

For Linux:

fl_dart_project_set_enable_impeller(project, true);

For windows:

DartProject project = ...
project.set_enable_impeller(true);

…eller_to_embedder_desktop

…jonahwilliams/flutter into add_enable_impeller_to_embedder_desktop

stuartmorgan · 2025-02-06T21:12:27Z

engine/src/flutter/shell/platform/embedder/embedder.h

+  ///
+  /// When not provided, this value is treated as false. In a future release,
+  /// this setting will become a no-op when the Skia backend is fully removed.
+  bool enable_impeller;


I see that in flutter/engine#42639 (comment) this was rejected; I didn't realize that when I proposed it.

I had also forgotten that command_line_argv is explicitly set up to control things like this in embedder.h, and that for the desktop embedders we explicitly disconnected actual command line arguments from what embedder.h unfortunately calls command line arguments.

Runtime command line arguments passed to a desktop app are actually piped through dart_entrypoint_arg*, which becomes the command line arguments that would be seen by Dart main. What the engine (unfortunately, but we can't change it without breaking the world) calls command_line_arg* is actually "engine flags", not anything related to what any Flutter dev would think of as command line arguments.

So I led you astray. Having paged all of this back in, we shouldn't need to change embedder.h at all, instead we can have the embedders turn whatever higher-level API we add into engine switches, which they are already set up to do. Going a step further, if we don't want to expose future-dead things in the embedding structs, we could instead expose the actual switches system, and have people use that along with the magic string. It looks like Windows already has SetString, so we would just have to expose that in the C++ wrapper. For Linux it looks like we haven't plumber switches up to that level, but we could, and it would avoid the same issue (just one level up) raised in the earlier embedder.h review that we don't want to permanently have explicit APIs for something that will become meaningless.

Stuart's suggestion is excellent, but I'm not sure that avoiding future-dead things in the embedding structs is currently worth the effort for Windows/Linux. I'd personally rather we spend our time on iOS/Android/web improvements. I'd be OK with having a no-op setting on Windows.

That said, +1 for avoiding a future-dead embedder API if possible.

I'm not sure that avoiding future-dead things in the embedding structs is currently worth the effort for Windows/Linux

If we plumb through the ability to set arbitrary engine flags, it won't be future-dead, it'll be available for whatever the next experimental engine features are.

I'm not sure that avoiding future-dead things in the embedding structs is currently worth the effort for Windows/Linux

If we plumb through the ability to set arbitrary engine flags, it won't be future-dead, it'll be available for whatever the next experimental engine features are.

Right. By "future-dead things in the embedding structs", I was referring to set_enable_impeller and get_enable_impeller - I'd be OK adding those future-dead APIs if that's easier for Jonah.

I somehow read "avoiding" as "adding" in the first sentence, which is why my comment doesn't make sense :)

I've updated this PR so that there is no change in embedder API, instead --enable-impeller is passed via embedder switches.

stuartmorgan · 2025-02-06T21:14:43Z

engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm

@@ -601,9 +601,10 @@ - (BOOL)runWithEntrypoint:(NSString*)entrypoint {
  std::vector<std::string> switches = self.switches;

  // Enable Impeller only if specifically asked for from the project or cmdline arguments.
+  bool enable_impeller = false;
  if (_project.enableImpeller ||
      std::find(switches.begin(), switches.end(), "--enable-impeller=true") != switches.end()) {


Out of scope to fix in this specific PR, but this seems like a pretty bad idea to me. Capturing from my comments in the Discord discussion about this PR, I don't think we want to allow end users of Flutter-built software to change the rendering engine. That seems like a decision that should be up to the developer, at the time they build their app, just as it is (IIUC) on Android and iOS.

In fact, we explicitly say not to do this in embedder.h.

I removed this code but even on iOS/Andorid, embedder switches is how we've handled the opt in.

engine/src/flutter/shell/platform/windows/public/flutter_windows.h

engine/src/flutter/shell/platform/linux/public/flutter_linux/fl_dart_project.h

robert-ancell · 2025-02-10T22:10:48Z

The Linux changes look good; no opinion on the correct structure for this feature, I'll leave that to the other reviewers.

…eller_to_embedder_desktop

loic-sharma

Thanks!

stuartmorgan · 2025-03-05T15:45:14Z

engine/src/flutter/shell/platform/windows/client_wrapper/include/flutter/dart_project.h

+  void set_enable_impeller(bool value) { enable_impeller_ = value; }
+
+  // Whether the Impeller rendering engine is enabled.
+  bool get_enable_impeller() const { return enable_impeller_; }


This should just be called enable_impeller; it's more idiomatic for Google C++ style, and it's consistent with the other APIs in this class.

stuartmorgan · 2025-03-05T15:46:09Z

engine/src/flutter/shell/platform/windows/flutter_project_bundle.h

@@ -70,6 +70,9 @@ class FlutterProjectBundle {
    return dart_entrypoint_arguments_;
  }

+  // Whether the Impeller rendering engine is enabled.
+  bool ImpellerEnabled() const { return impeller_enabled_; }


impeller_enabled() since it's a trivial getter.

engine/src/flutter/shell/platform/windows/flutter_project_bundle.h

…jonahwilliams/flutter into add_enable_impeller_to_embedder_desktop

engine/src/flutter/shell/platform/windows/flutter_project_bundle.h

engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc

…eller_to_embedder_desktop

stuartmorgan · 2025-03-12T14:58:02Z

engine/src/flutter/shell/platform/linux/fl_engine.cc

+  const bool has_impeller_flag =
+      std::find_if(
+          switches.begin(), switches.end(), [](std::string_view value) {
+            return value.rfind("--enable-impeller=", 0) != std::string::npos;


Why rfind instead of find for a substring that should be at the start of the string?

stuartmorgan · 2025-03-12T15:00:15Z

engine/src/flutter/shell/platform/windows/client_wrapper/include/flutter/dart_project.h

+  // become a no-op when the Skia backend is fully removed.
+  void set_enable_impeller(bool value) { enable_impeller_ = value; }
+
+  // Whether the Impeller rendering engine is enabled.


I think the flag-override behavior—and, importantly, the fact that this getter will not work correctly when there is a runtime flag—needs to be documented on these methods.

stuartmorgan · 2025-03-12T15:00:41Z

engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc

+  const bool has_impeller_flag =
+      std::find_if(
+          switches.begin(), switches.end(), [](std::string_view value) {
+            return value.rfind("--enable-impeller=", 0) != std::string::npos;


Same question here about rfind.

stuartmorgan · 2025-03-12T15:01:43Z

engine/src/flutter/shell/platform/windows/flutter_project_bundle.h

@@ -70,6 +70,9 @@ class FlutterProjectBundle {
    return dart_entrypoint_arguments_;
  }



Where is the setter?

stuartmorgan · 2025-03-12T15:02:04Z

engine/src/flutter/shell/platform/windows/flutter_project_bundle.h

@@ -70,6 +70,9 @@ class FlutterProjectBundle {
    return dart_entrypoint_arguments_;
  }

+  // Whether the Impeller rendering engine is enabled.


Same comment on this comment; this file should explain the interaction with the runtime flag.

[windows] add ability to enable Impeller on windows desktop.

b36199b

github-actions bot added engine flutter/engine repository. See also e: labels. platform-windows Building on or for Windows specifically a: desktop Running on desktop platform-macos labels Feb 4, 2025

jonahwilliams added 2 commits February 4, 2025 11:28

add 'test'

98da36a

Add API to Linux.

099f7a9

github-actions bot added the platform-linux Building on or for Linux specifically label Feb 4, 2025

jonahwilliams changed the title ~~[windows] add ability to enable Impeller on windows desktop.~~ [windows] add ability to enable Impeller on windows and linux desktop. Feb 4, 2025

jonahwilliams added 8 commits February 4, 2025 12:33

++

851bc81

Update fl_dart_project.h

42eedac

Update fl_dart_project.cc

2e37478

come on now.

d54c1d4

++

ccda6af

make sure flag can be passed all the way down, fix test segfault.

8d02925

Merge branch 'main' of github.com:flutter/flutter into add_enable_imp…

e773a00

…eller_to_embedder_desktop

Merge branch 'add_enable_impeller_to_embedder_desktop' of github.com:…

152ad73

…jonahwilliams/flutter into add_enable_impeller_to_embedder_desktop

jonahwilliams changed the title ~~[windows] add ability to enable Impeller on windows and linux desktop.~~ [windows/linux] add ability to enable Impeller on windows and linux desktop. Feb 5, 2025

jonahwilliams marked this pull request as ready for review February 5, 2025 21:24

jonahwilliams requested review from stuartmorgan, robert-ancell and loic-sharma February 5, 2025 22:30

stuartmorgan requested changes Feb 6, 2025

View reviewed changes

loic-sharma reviewed Feb 10, 2025

View reviewed changes

engine/src/flutter/shell/platform/windows/public/flutter_windows.h Show resolved Hide resolved

robert-ancell reviewed Feb 10, 2025

View reviewed changes

engine/src/flutter/shell/platform/linux/public/flutter_linux/fl_dart_project.h Outdated Show resolved Hide resolved

loic-sharma mentioned this pull request Feb 10, 2025

[windows] Implement merged UI and platform thread #162935

Merged

9 tasks

Merge branch 'main' of github.com:flutter/flutter into add_enable_imp…

9654d97

…eller_to_embedder_desktop

github-actions bot removed the platform-macos label Feb 26, 2025

jonahwilliams marked this pull request as draft February 26, 2025 19:39

jonahwilliams added 8 commits February 26, 2025 11:39

++

966345c

++

4379d7f

Merge branch 'main' of github.com:flutter/flutter into add_enable_imp…

6978387

…eller_to_embedder_desktop

++

99427c9

++

837d3a9

++

8836f9d

add more doc comments.

1657f20

add missing project plumbing.

a12eeba

jonahwilliams marked this pull request as ready for review March 3, 2025 19:29

Merge branch 'master' into add_enable_impeller_to_embedder_desktop

15e0f25

jonahwilliams requested review from robert-ancell, loic-sharma and stuartmorgan March 4, 2025 21:14

loic-sharma approved these changes Mar 4, 2025

View reviewed changes

stuartmorgan requested changes Mar 5, 2025

View reviewed changes

jonahwilliams added 3 commits March 5, 2025 15:43

rename trivial getters.

e148a78

Merge branch 'add_enable_impeller_to_embedder_desktop' of github.com:…

611eb00

…jonahwilliams/flutter into add_enable_impeller_to_embedder_desktop

++

ec787bd

stuartmorgan requested changes Mar 6, 2025

View reviewed changes

engine/src/flutter/shell/platform/windows/flutter_project_bundle.h Show resolved Hide resolved

engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc Show resolved Hide resolved

jonahwilliams added 2 commits March 8, 2025 16:54

Merge branch 'main' of github.com:flutter/flutter into add_enable_imp…

e9a0b87

…eller_to_embedder_desktop

default to command line setting for impeller enabled.

05e2e0d

jonahwilliams requested a review from stuartmorgan March 10, 2025 23:12

stuartmorgan requested changes Mar 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[windows/linux] add ability to enable Impeller on windows and linux desktop. #162684

[windows/linux] add ability to enable Impeller on windows and linux desktop. #162684

jonahwilliams commented Feb 4, 2025 •

edited

Loading

stuartmorgan Feb 6, 2025

loic-sharma Feb 10, 2025 •

edited

Loading

stuartmorgan Feb 10, 2025

loic-sharma Feb 10, 2025 •

edited

Loading

stuartmorgan Feb 10, 2025

jonahwilliams Mar 3, 2025

stuartmorgan Feb 6, 2025

stuartmorgan Feb 6, 2025

jonahwilliams Mar 3, 2025

robert-ancell commented Feb 10, 2025

loic-sharma left a comment

stuartmorgan Mar 5, 2025

stuartmorgan Mar 5, 2025

stuartmorgan Mar 12, 2025

stuartmorgan Mar 12, 2025

stuartmorgan Mar 12, 2025

stuartmorgan Mar 12, 2025

stuartmorgan Mar 12, 2025

		@@ -70,6 +70,9 @@ class FlutterProjectBundle {
		return dart_entrypoint_arguments_;
		}

[windows/linux] add ability to enable Impeller on windows and linux desktop. #162684

Are you sure you want to change the base?

[windows/linux] add ability to enable Impeller on windows and linux desktop. #162684

Conversation

jonahwilliams commented Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

loic-sharma Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loic-sharma Feb 10, 2025 • edited Loading

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

robert-ancell commented Feb 10, 2025

loic-sharma 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

jonahwilliams commented Feb 4, 2025 •

edited

Loading

loic-sharma Feb 10, 2025 •

edited

Loading

loic-sharma Feb 10, 2025 •

edited

Loading