Repository navigation
Conversation
Selects the Vulkan backend via engine switches, presents through a KHR swapchain into a synchronized wl_subsurface (Wayland) or the toplevel window (X11), and integrates with the FlViewRenderer architecture via a new FlViewRendererVulkan. Removes the public rendering-backend getter.
github-actions
Bot
added
engine
|
I built this branch to test frame pacing on top of it. Five findings on NVIDIA + KDE Wayland, all in the Vulkan path only (the OpenGL path is unaffected), plus where the pacing ended up. Environment: RTX 4070 SUPER (proprietary driver), KDE Plasma 6.7.4 Wayland, GTK 3.24.52, branch at d45d937, 1. SIGSEGV on realizeEvery launch, before any frame:
// Flutter engine will need to make the context current from raster thread
- // during initialization.
- fl_opengl_manager_clear_current(fl_engine_get_opengl_manager(self->engine));
+ // during initialization. The Vulkan renderer has no FlOpenGLManager - see
+ // where it is created in fl_engine.cc.
+ if (fl_engine_get_renderer_type(self->engine) != kVulkan) {
+ fl_opengl_manager_clear_current(fl_engine_get_opengl_manager(self->engine));
+ }With this the backend starts and reports 2. The window never appearsAfter the crash fix the process runs and Impeller renders, but nothing is ever shown. Verified by screenshot with only this instance running: no window on screen at all, and The dependency is circular:
3. The parent surface commits at 48HzTo get past #2 I added a temporary Two things worth separating there. The good half: The other half: This is a direct counter-measurement to the PR description, which says the synchronized-subsurface tick driver was confirmed to keep the parent committing every frame, verified on WSLg/llvmpipe. On a 144Hz display with a real GPU it commits 48 times a second - below the 60 the OpenGL path manages.
4.
|
|
@ales-drnz Sorry for the late reply. This is great work, thank you for testing on real hardware. My verification was WSLg/llvmpipe only and that clearly hid a lot. 1 and 2 mean the backend did not even start on a real desktop, better to know that now than after review. Please send 1, 2, 4 and 5 as a PR against On the pacing piece I agree with your conclusion, lets keep that separate until the subsurface direction from #49757 is clear. Your 48Hz measurement contradicts my PR description, I will correct that part. And the frame-callback waiter results look very promising, when the team direction is clear that deserves to be its own PR from you. |
The runner shows the window on the "first-frame" signal, which the Vulkan renderer only emitted from its draw handler - and the draw handler cannot run before the window is shown, so the window never appeared. Announce it from FlutterEngineSetNextFrameCallback instead: it fires on the raster thread once the first frame is actually in the swapchain, regardless of window visibility. Routed to the view through a new FlRenderable.notify_first_frame so backends that present outside the GTK draw cycle can signal it. Tested on an unrealized view, matching how the runner receives it.
wl_subsurface.set_position is double-buffered state of the parent surface - "the scheduled coordinates will take effect whenever the state of the parent surface is applied" - so committing the child was a no-op for it. The position only applied when the tick-callback redraw happened to commit the parent.
Mode change only; the engine's pre-push formatting hook trips on it.
|
Taking this out of draft. @ales-drnz was kind enough to test it on NVIDIA + KDE Plasma Wayland, and it turned out the backend did not start there at all: a null dereference on realize, and a first frame that was announced from the GTK draw handler, which cannot run before the runner shows the window. His four fixes are in the PR as his own commits, and the full There are two decisions I would rather not guess at, so if a maintainer could weigh in I would appreciate it:
Neither of them blocks review of what is in the PR now. Happy to take this in whatever direction suits the team. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds Vulkan rendering support with Impeller on Linux, introducing FlVulkanManager and FlViewRendererVulkan to manage Vulkan resources and handle frame presentation. Feedback on the implementation identifies critical issues in FlVulkanManager, including undefined behavior from unconstructed C++ members in the GObject initialization, compilation failures from calling non-existent Wayland destructors and non-standard template destructor syntax, and a potential memory retention issue in embedder.cc where capturing a weak pointer instead of a strong pointer is recommended to avoid delaying the destruction of SurfaceContextVK.
| if (self->wl_subcompositor_iface != nullptr) { | ||
| wl_subcompositor_destroy(self->wl_subcompositor_iface); | ||
| self->wl_subcompositor_iface = nullptr; | ||
| } | ||
| if (self->wl_compositor_iface != nullptr) { | ||
| wl_compositor_destroy(self->wl_compositor_iface); | ||
| self->wl_compositor_iface = nullptr; | ||
| } |
There was a problem hiding this comment.
The Wayland interfaces wl_compositor and wl_subcompositor do not have destructor requests defined in the Wayland XML protocol, meaning wl_compositor_destroy and wl_subcompositor_destroy do not exist and calling them will cause compilation errors. Use wl_proxy_destroy to properly release these client-side proxies.
if (self->wl_subcompositor_iface != nullptr) {
wl_proxy_destroy(reinterpret_cast<struct wl_proxy*>(self->wl_subcompositor_iface));
self->wl_subcompositor_iface = nullptr;
}
if (self->wl_compositor_iface != nullptr) {
wl_proxy_destroy(reinterpret_cast<struct wl_proxy*>(self->wl_compositor_iface));
self->wl_compositor_iface = nullptr;
}There was a problem hiding this comment.
I do not think this one is right, and the reasoning is inverted: the absence of a destructor request is exactly why wl_compositor_destroy exists in this form.
wayland-scanner emits a <interface>_destroy inline for every interface that has no destructor request, and that inline is a plain wl_proxy_destroy wrapper (scanner.c#L1227-L1237):
if (!has_destroy && strcmp(interface->name, "wl_display") != 0) {
printf("/** @ingroup iface_%s */\n", interface->name);
printf("static inline void\n"
"%s_destroy(struct %s *%s)\n"
"{\n"
"\twl_proxy_destroy("
"(struct wl_proxy *) %s);\n"
"}\n\n",
interface->name, interface->name, interface->name,
interface->name);
}So in the sysroot this builds against, build/linux/debian_bullseye_amd64-sysroot/usr/include/wayland-client-protocol.h (generated by wayland-scanner 1.18.0), lines 1198-1202 are:
/** @ingroup iface_wl_compositor */
static inline void
wl_compositor_destroy(struct wl_compositor *wl_compositor)
{
wl_proxy_destroy((struct wl_proxy *) wl_compositor);
}with wl_subcompositor_destroy at line 5546 in the same shape. That is the identical wl_proxy_destroy call the suggestion proposes, so the current code already does exactly that, and it compiles and links here. Leaving it as is.
…ze capture Review feedback on the Vulkan GTK backend: - FlVulkanManager holds a RefPtr and several vectors, but GObject hands out zeroed raw memory and never runs their constructors. They are constructed in place in _init now, pairing with the explicit destruction that was already in _finalize. Both lists use the decltype form so they stay symmetric and do not depend on the injected class name. - The embedder surface-size updater captured the SurfaceContextVK shared_ptr strongly. The updater is owned by the engine and outlives the surface, so that kept the Vulkan context and its resources alive until engine teardown. It captures a weak_ptr and locks it now.
| return; | ||
| } | ||
|
|
||
| FlVulkanManager* vulkan_manager = fl_vulkan_manager_new(window); |
There was a problem hiding this comment.
This could be g_autoptr(FlVulkanManager) to avoid the g_object_unref later.
|
Thanks for working on this. I've done an initial review, first thoughts:
|
|
@robert-ancell Thanks as well. Currently I don't have that much time, but I'll probably get it done by the weekend or something (same counts for my other pending PRs). What would be a good plan on splitting this into smaller PRs? I was thinking of one feature having one branch, even if that feature is a bit larger. |
|
@robert-ancell thanks for the review. @sero583, before you spend the weekend on the split I wanted to share what I found, because the first point goes deeper than where the manager is created. This PR gives the engine a single So I rebuilt it on the compositor and on
While testing I also found a synchronization bug in Impeller's Vulkan render passes that the validation layers report on every frame, which I'll send separately. On NVIDIA with KWin at 144 Hz the Vulkan path keeps up with OpenGL on master and the validation layers stay clean, including while resizing and with two windows. I haven't been able to try Mesa. Sero, the device selection, the features Impeller expects and the Dozen handling all come from your work, and you're a co-author of the renderer commit. The three commits are on https://github.com/ales-drnz/flutter/tree/linux-vulkan-subsurface, on top of current master, so you can fetch them into this PR or split them however you like. Your branch is based on July's master, before |
Really anything that can land in separate chunks. For example, if there are any assumptions in the current code that will not work with Vulkan then we can land fixes for those. You could start a minimal Vulkan manager that doesn't do any rendering now, and then build on that. This PR shows where we're going, and we can build towards that. This PR can then merge/rebase and will shrink over time. |
3 participants
Adds an opt-in Vulkan rendering path to the Linux GTK embedder: Impeller renders with its existing Vulkan backend through a KHR swapchain, presented into a synchronized
wl_subsurfaceon Wayland or the toplevel window on X11. This is the Linux half of the desktop Impeller Vulkan effort (#181711); the Windows half is presented through DirectComposition in a separate PR because knopp's review of #183382 asked Windows off the KHR-swapchain path, while KHR presentation remains the right fit on Linux where the Mesa/driver WSI is the native presentation mechanism.Activation uses the engine switch mechanism, matching the Windows backend and in line with the review guidance to gate the backend behind flags rather than public API:
--enable-impellertogether with--impeller-backend=vulkan. Without the switches, or when no capable Vulkan device exists, nothing changes: OpenGL remains the default and the fallback, and the existing software renderer stays opt-in through theFLUTTER_LINUX_RENDERERenvironment variable. When Vulkan is active, EGL is not initialized at all.How it works
FlVulkanManagerowns theVkInstance,VkDevice,VkQueue, and the presentation surface. On Wayland it creates a childwl_surface, makes it a subsurface of the toplevel positioned at the content area (below the header bar), sets an empty input region so events pass through to GTK, and marks it opaque. On X11 it presents to the toplevel window directly.SurfaceContextVK); the engine renders into the acquired swapchain images and presents through the Vulkan queue.FlEngineselects the renderer from the engine switches and skips EGL / OpenGL manager creation when Vulkan is chosen, so a Vulkan-only session does not load libepoxy.FlViewRendererVulkanis a newFlViewRenderersubclass alongsideFlViewRendererOpenGLandFlViewRendererSoftware.FlViewinstantiates it for the Vulkan renderer type. Unlike the OpenGL/software renderers it does not composite layers (Impeller owns presentation through the swapchain); it creates theFlVulkanManageron realize, keeps the subsurface positioned, and drives parent-surface commits (see below).Synchronized subsurface (Wayland)
The Vulkan subsurface runs in synchronized mode (
wl_subsurface_set_sync) so its buffer is applied atomically with the parent surface's commit. This keeps the Vulkan content and the window geometry in lockstep during resizes, avoiding the tearing a desynchronized subsurface produces when the parent resizes before the child presents its new-size buffer. Because a synchronized subsurface only becomes visible when the parent commits, and GTK does not redraw the toplevel on its own while only the Vulkan content changes,FlViewRendererVulkankeeps a frame-clock tick callback registered; it queues a draw each frame, and the renderer's draw handler paints the background (committing the parent), which flushes the subsurface's latest frame. The tick callback is removed on dispose.Embedder plumbing
The Vulkan renderer config gains an optional
surfacefield (FlutterVulkanSurfaceHandle). When the embedder supplies a surface, Impeller drives the swapchain and theget_next_image/present_imagecallbacks are optional; when it does not, the existing embedder-managed image callbacks are used unchanged. The field isstruct_sizecompatible, so existing embedders are unaffected. The legacyvulkan/support library gains the Xlib and Wayland surface procs and the Linux native surface wrapper.Startup fixes on real hardware
@ales-drnz built this branch on NVIDIA + KDE Plasma Wayland and found the backend did not start there at all. His fixes are in this PR as his own commits:
realize_cbcalledfl_opengl_manager_clear_currentunconditionally, but there is noFlOpenGLManagerwhen the renderer is Vulkan, so every launch hit a SIGSEGV before the first frame.FlutterEngineSetNextFrameCallbackthrough a new optionalFlRenderable.notify_first_frame, which fires once the frame is in the swapchain regardless of window visibility.wl_subsurface.set_positionis double-buffered state of the parent surface, so the position is applied by committing the parent instead of the child. That also removes a commit issued on the child from the platform thread while the WSI commits the same surface from the raster thread.vulkan_native_surface_linux.{cc,h}.Not in this PR
FlRendererType,fl_engine_get_rendering_backend); those are removed per the review guidance, and selection is internal via switches only.wp_viewport, which means vendoring a Wayland protocol into the engine, and there is no third-party location for those files today (build_overrides/wayland.gnisetswayland_gn_dir = ""). Deferred until it is clear where they belong;wl_surface_set_buffer_scaleis not an alternative here, because the swapchain starts at a fallback size rather than at the widget allocation times the scale.Testing
New unit tests in
flutter_linux_unittests(hardware-dependent cases skip cleanly without Vulkan):FlVulkanManagerTest(6): availability, create-or-null without a window, shutdown/wait-idle, deferred-deletion null safety, memory properties, surface release.VulkanNativeSurfaceLinuxTest(7): X11 and Wayland surface validity and extension-name checks, zero/negative dimension rejection.FlEngineTest.RendererTypeVulkanplus the existing software/OpenGL/default/unknown selection tests: the Vulkan backend is selected only through the switches and otherwise falls back to OpenGL.RendererTypeVulkanEnvVarFallsBackToOpenGLcovers the migration path:FLUTTER_LINUX_RENDERER=vulkanis not a selector, warns with a pointer to the switches, and falls back to OpenGL.FlViewTest.NotifyFirstFrameEmitsFirstFrameSignal: the first-frame signal reaches the runner throughFlRenderablewithout the widget being realized, and is not re-emitted for later frames.The startup fixes were found and verified on NVIDIA + KDE Plasma Wayland; the full
flutter_linux_unittestssuite passes with them.Manual verification on AMD Radeon RX 6750 XT: the backend was exercised on a real Wayland compositor (WSLg, which exposes
VK_KHR_wayland_surfaceover both the D3D12 translation layer and llvmpipe). The switch selection picks Vulkan, the KHR swapchain initializes on the subsurface, and the app renders continuously. The synchronized-subsurface tick driver kept the parent committing every frame on this WSLg setup, but that does not hold on real desktop hardware: on NVIDIA + KDE at 144Hz the parent was measured at 48 commits/s (#189584 (comment)), the GDK frame clock does not run at display rate there. Frame pacing needs a proper vsync source, that is a follow up (see the comment thread).Part of #181711. This PR has no build-time dependency on the Impeller hardening PR (#189580) and can be reviewed in parallel; landing the hardening PR first is recommended because its KHR swapchain robustness fixes (fence re-signal on failed acquire, zero-extent minimized-window guard) directly improve the runtime behavior of the swapchain path this PR enables. Design doc: https://flutter.dev/go/impeller-backend-desktop
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.