Make an Impeller render target from a Vulkan backing store by jwinarske · Pull Request #193003 · flutter/flutter

jwinarske · 2026-09-18T15:33:54Z

CreateEmbedderRenderTarget logged Unimplemented for kFlutterBackingStoreTypeVulkan under Impeller and returned nothing, so rasterization failed every frame and the compositor's present callback was never reached. Metal has this. OpenGL has it for the framebuffer case. Vulkan had neither, so an embedder presenting Vulkan backing stores could not run Impeller at all.

Wrap the embedder's image with WrapTextureSourceVK, hand it to SurfaceVK::WrapSwapchainImage with the swapchain transients, and build the render target from the surface — the same construction the root surface uses, so both Vulkan paths resolve identically.

The transients are cached, and that is not an optimization. They are an MSAA colour attachment plus a depth-stencil buffer at frame size, so a set per render target means allocating them per frame. An embedder that asks for a fresh backing store each frame — a scanout ring does — then allocates tens of MB per frame. Measured on V3D 4.2 at 1280x1440: 23.5fps before caching, 58.8 after. The cache belongs to the engine's render-target callback rather than the process, because the textures come from the Impeller context's device and freeing them after that context is gone is a use-after-free — I had it process-wide first and it crashed across engines in one test binary. Keyed by size and format so a multi-view embedder does not rebuild per view.

FlutterVulkanImage.format is read directly rather than through SAFE_ACCESS. The Skia path has always read it directly, so an embedder that leaves struct_size unset works there; gating on it here would fail that embedder only under Impeller.

What the embedder gets back

The image comes back in VK_IMAGE_LAYOUT_GENERAL, where the Skia path leaves COLOR_ATTACHMENT_OPTIMAL — the wrapped image is marked a swapchain image and Impeller picks the final layout from that. embedder.h documents no layout for backing stores, so this is worth deciding rather than leaving to each embedder to discover: happy to document it here, or to transition to a defined layout before handing it back if that is preferred.

Tests

EmbedderTest.CanRenderWithImpellerVulkan: renders through the compositor with --enable-impeller, asserts the layer arrives as a Vulkan backing store, and compares the scene against the existing impeller_test.png golden — Vulkan's output matches the GL one exactly, so no new golden.

Verified on hardware beyond the suite: V3D 4.2 / vc4, 1280x1440@60, Impeller rendering into embedder backing stores and reaching a KMS plane at 59.7fps, against 59.8 for the same app on the root surface.

Fifth of six, from #187525. Contains the third and fourth, which are up separately (#193001, #193002) — only the last commit is new here.

gemini-code-assist

Code Review

This pull request implements the Impeller rendering path for Vulkan backing stores in the embedder API, introducing a reusable TextureWrapperVK and an ImpellerVulkanTransientsCache to cache MSAA and depth-stencil buffers. Feedback on these changes suggests proactively evicting stale Vulkan contexts from the transients cache to prevent memory leaks, and adding a defensive null check for aiks_context to avoid potential crashes.

jwinarske · 2026-09-18T16:43:51Z

Pushed a portability fix: ImpellerVulkanTransientsCache was defined under SHELL_ENABLE_VULKAN && IMPELLER_SUPPORTS_RENDERING but used outside it — the CreateEmbedderRenderTarget signature and the make_shared in InferExternalViewEmbedderFromArgs. Any engine build without Vulkan failed to compile embedder.cc:

embedder.cc:1313:5: error: unknown type name 'ImpellerVulkanTransientsCache'
embedder.cc:1552:5: error: unknown type name 'ImpellerVulkanTransientsCache'
embedder.cc:1744:44: error: use of undeclared identifier 'ImpellerVulkanTransientsCache'

Forward declared for every configuration, defined only where there is a Vulkan Impeller backend, and the shared_ptr left null otherwise — the path that reads it is compiled out in that case anyway.

Checked by compiling embedder.cc with -DSHELL_ENABLE_VULKAN stripped: fails before, clean after. My builds all had Vulkan on, which is why I missed it.

jwinarske · 2026-09-18T20:10:00Z

Force-pushed: restacked. This branch carried #193001 without #193000 under it, so it inherited a file whose .cc used a member the header did not declare — hence failures on every platform.

Landing order is now #192999, #193000, #193001, #193002, then this.

GPUSurfaceVulkan and GPUSurfaceGLImpeller both take render_to_surface and hand back a no-op SurfaceFrame when it is false. GPUSurfaceVulkanImpeller has no such flag, so with a compositor the engine acquires an embedder image and presents it after every composited frame. The compositor has already presented that frame's layers. The root present puts an unrendered image on top of them. An embedder holding one image sees nothing; one cycling a ring of scanout buffers sees its frame replaced by whatever the ring handed back -- one good frame, then black, at a flip rate that still reads 60fps. Reachable once the embedder can build an Impeller render target from a Vulkan backing store (flutter#187525); the gate is the groundwork for it. Dispose of the thread-local pools before the gate rather than after. It is per-frame upkeep, not part of presenting, and skipping it on the compositor path would leave the raster thread holding its command and descriptor pools for the life of the engine.

GPUSurfaceVulkanImpeller carries a private TextureSourceVK subclass that wraps a VkImage the caller owns and creates a view for it. Nothing about it belongs to the root surface, and the embedder needs the same thing to build a render target from a backing store (flutter#187525). Move it to impeller/renderer/backend/vulkan as WrapTextureSourceVK and call it from the surface. No behavior change; the surface loses 53 lines.

Impeller's capability check reads the extension lists the embedder declares rather than querying the driver: it wants VK_KHR_surface plus one WSI extension on the instance, and VK_KHR_swapchain on the device. The test context enables none of them and reports none, so Impeller refuses the context with "Could not find the surface extension" and no Impeller Vulkan embedder test can run at all. Request the surface extensions the ICD offers, create the logical device with VK_KHR_swapchain, and report both lists through FlutterVulkanRendererConfig. Falls back to a device without the extension when the ICD refuses it, and reports nothing when the surface extensions are missing, so a context that cannot serve Impeller still serves the Skia tests exactly as before. None of this is used to present -- the tests render into images -- but Impeller will not build a context without it. Also set FlutterVulkanImage.struct_size in the backing store producer. The API says it must be sizeof(FlutterVulkanImage); the Skia path never checks, so leaving it zero went unnoticed.

jwinarske · 2026-09-18T20:19:46Z

Second force-push. Two separate things were wrong; the first one is fixed above.

The second: this commit added impeller_flags.antialiased_lines = settings.impeller_antialiased_lines;, which has nothing to do with the render target and which master has since removed from impeller::Flags. CI builds the PR merged into current master, so it broke there while building fine on the base I cut from. Dropped the line, dropped a stray reindent that rode along with it, and rebased the series onto current master.

CreateEmbedderRenderTarget logged "Unimplemented" for kFlutterBackingStoreTypeVulkan under Impeller and returned nothing, so rasterization failed on every frame and the compositor's present callback was never reached. Metal has this. OpenGL has it for the framebuffer case. Vulkan had neither, so an embedder presenting Vulkan backing stores could not run Impeller at all. Wrap the embedder's image with WrapTextureSourceVK, hand it to SurfaceVK::WrapSwapchainImage with the swapchain transients, and build the render target from the surface -- the same construction the root surface uses, so both Vulkan paths resolve identically. The transients are cached, and that is not an optimization. They are an MSAA colour attachment plus a depth-stencil buffer at frame size, so building a set per render target allocates them per frame; an embedder that asks for a fresh backing store each frame then allocates tens of MB per frame. Measured on V3D 4.2 at 1280x1440: 23.5fps before caching, 58.8 after. The cache belongs to the engine's render-target callback rather than the process, because the textures come from the Impeller context's device and freeing them after that context is gone is a use-after-free. Keyed by size and format so a multi-view embedder does not rebuild them per view. Read FlutterVulkanImage.format directly rather than through SAFE_ACCESS. The Skia path has always read it directly, so an embedder that leaves struct_size unset works there; gating on it here would fail that embedder only under Impeller. Verified on hardware: V3D 4.2 / vc4, 1280x1440@60, Impeller rendering into embedder backing stores and reaching a KMS plane at 59.7fps, against 59.8 for the same app on the root surface.

jwinarske · 2026-09-18T20:32:56Z

Third push, and the audit I should have run the first time.

Two more places where this commit reverted upstream rather than adding to it:

the EmbedderSurfaceGLImpeller call site had lost shell.GetShutdownSafeIOTaskRunner(), which master added to that constructor
five }; reindents in Metal and software code, pure formatter churn from a different clang-format version, nothing to do with this change

Both came from carrying a patch cut against an older tree. I have now gone through every line this commit removes from master; what is left is the Unimplemented path being replaced and three signatures gaining the transients-cache argument, which is the change itself.

…-target

jwinarske · 2026-10-08T13:58:59Z

@andywolff a signature question before this goes further.

WrapTextureSourceVK takes const std::shared_ptr<Context>& and does ContextVK::Cast(*context).GetDevice() (texture_wrapper_vk.cc:70). Impeller has no RTTI, so the helper cannot check it. Sound today: the one caller is on GPUSurfaceVulkanImpeller's delegate_ != nullptr branch, which holds a ContextVK.

This PR adds the second caller, and it is the one that may not: embedder.cc passes aiks_context->GetContext(), a plain shared_ptr<Context>. Nothing says it is not a SurfaceContextVK.

Three ways:

const std::shared_ptr<ContextVK>& — precondition becomes a compile error. Callers holding a Context gain a static_pointer_cast, so it relocates the unchecked cast rather than removing it.
const ContextVK& — caller writes ContextVK::Cast(*ctx), which is the existing idiom in gpu_surface_vulkan_impeller.cc. The helper takes no ownership and only needs GetDevice().
Leave it and document the precondition.

I lean 2. Which do you want?

andywolff

I appreciate the benchmarking and questions you've raised. I have requested a few changes. Please let me know if any of my comments are unclear

andywolff · 2026-10-07T20:23:32Z

 #include "flutter/fml/synchronization/count_down_latch.h"
 #include "flutter/shell/platform/embedder/tests/embedder_config_builder.h"
 #include "flutter/shell/platform/embedder/tests/embedder_test.h"
+#include "flutter/fml/synchronization/waitable_event.h"


nit: please sort includes

andywolff · 2026-10-07T20:25:16Z

+/// reaches the compositor as a Vulkan backing store, which is exactly what did
+/// not happen before.
+///
+/// Deliberately not a golden-image comparison. The failure being guarded


"Deliberately not a golden-image comparison" but the last line of this test calls ImageMatchesFixture. Please reconcile

andywolff · 2026-10-09T21:40:51Z

+
+  return std::make_unique<flutter::EmbedderRenderTargetImpeller>(
+      backing_store, aiks_context, std::move(render_target), on_release,
+      fml::closure());


If an embedder provides destruction_callback on FlutterVulkanBackingStore, please wrap and execute it like Skia does.

Also, please add a unit test in embedder_vk_unittests.cc asserting that destruction_callback is called. Skia is currently missing an explicit assertion for this on Vulkan as well, but you can see how OpenGL tests verify it in embedder_gl_unittests.cc.

andywolff · 2026-10-09T21:44:02Z

+  ASSERT_EQ(FlutterEngineSendWindowMetricsEvent(engine.get(), &event),
+            kSuccess);
+
+  // Before the Impeller path existed this never fired: with no render target


nit: "Before the Impeller path existed this never fired" is historical context which belongs on a PR description or comment, not in the code. Please rewrite it to focus on the present code

andywolff · 2026-10-09T21:47:38Z

+    return nullptr;
+  }
+
+  // Read directly, as the Skia path does. `FlutterVulkanImage.struct_size` has


nit: if Impeller eventually fully replaces skia, this wording will become obsolete. Please reword with a focus on the permanent stability contract. Maybe:

// Read directly without struct_size validation to maintain backwards // compatibility with embedders which leave FlutterVulkanImage unpopulated.

andywolff · 2026-10-12T19:36:39Z

+/// Owned by the engine's render-target callback so that it is destroyed with
+/// the engine, while its Impeller context -- which owns the device the textures
+/// were allocated from -- is still alive.
+class ImpellerVulkanTransientsCache {


embedder.cc is the wrong architectural home for this cache. Let's keep the embedder shell responsibility focused on API translation.

I understand the rationale in the comments: you see a huge performance benefit from caching these transients, and you're following the precedent set by GPUSurfaceVulkanImpeller. But I have a few problems with this.

GPUSurfaceVulkanImpeller's cache is a bad precedent to follow. Directionally, I prefer we work to consolidate texture cache management to fewer places rather than introduce another one. We also need caches to be clearable in response to low memory warnings, which I don't see a clean way to do at this layer. Finally, tying GPU resources to the engine callback closure is brittle. Impeller already provides robust primitives to manage GPU resource lifetimes and drop allocations safely after context destruction using std::weak_ptr<DeviceHolderVK> (see CommandPoolVK, PipelineLibraryVK, etc.). For now, please remove the transients caching from this PR. I filed #194239, please add a TODO in the code pointing to that issue. Then we can align on a caching approach separately from this PR.

andywolff · 2026-10-12T19:37:42Z

+        aiks_context->GetContext(), desc, /*enable_msaa=*/true);
+  }
+
+  auto surface = impeller::SurfaceVK::WrapSwapchainImage(


Regarding your question about whether to document VK_IMAGE_LAYOUT_GENERAL or transition to a defined layout before presentation:

Let's document VK_IMAGE_LAYOUT_GENERAL clearly in embedder.h for now.

I understand that if an embedder using skia's backingstore implementation switched to impeller with Vulkan, it would break because skia used VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL and because of the WrapSwapchainImage shortcut you're using, impeller will use VK_IMAGE_LAYOUT_GENERAL. That's unfortunate, but I don't expect it to pose an imminent obstacle to anyone. Please document it clearly in embedder.h and in the PR description. I filed #194238 to follow up, please add a TODO in the code near the WrapSwapchainImage call. In that issue, I recommend we add a presentation_layout field on FlutterVulkanBackingStore and extract a helper method for creating a render target which we can use instead of WrapSwapchainImage with a no-op present call.

andywolff · 2026-10-12T19:38:54Z

+///             Returns nullptr if the view could not be created.
+///
+/// @see        `WrapTextureMTL`, the Metal equivalent.
+std::shared_ptr<TextureSourceVK> WrapTextureSourceVK(


Regarding the options you listed in your comment, I agree Option 2 is the best one.

WrapTextureSourceVK takes no ownership and only needs to observe the device during the call, so borrowing via const ContextVK& is standard C++ and makes the concrete backend requirement explicit at compile time rather than hiding an unchecked reinterpret_cast inside the helper.

flutter-dashboard Bot added the CICD Run CI/CD label Sep 18, 2026

This was referenced Sep 18, 2026

Run the shared Vulkan embedder tests under Impeller as well as Skia #193004

Open

[Embedder API] Support Making a Render Target from Vulkan backing store #187525

Open

gemini-code-assist Bot reviewed Sep 18, 2026

View reviewed changes

Comment thread engine/src/flutter/shell/platform/embedder/embedder.cc

Comment thread engine/src/flutter/shell/platform/embedder/embedder.cc

github-actions Bot added engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Sep 18, 2026

jwinarske force-pushed the pr5/embedder-render-target branch from 0a34801 to 6140cec Compare September 18, 2026 16:17

jwinarske mentioned this pull request Sep 18, 2026

Move the Vulkan wrapped-texture helper out of the GPU surface #193001

Merged

jwinarske force-pushed the pr5/embedder-render-target branch 2 times, most recently from 388165b to 0458960 Compare September 18, 2026 20:09

jwinarske added 3 commits September 18, 2026 13:18

jwinarske force-pushed the pr5/embedder-render-target branch from 0458960 to e63f04c Compare September 18, 2026 20:19

jwinarske force-pushed the pr5/embedder-render-target branch from e63f04c to 43b001d Compare September 18, 2026 20:32

gaaclarke requested a review from andywolff September 21, 2026 18:12

Merge remote-tracking branch 'origin/master' into pr5/embedder-render…

ee60a9a

…-target

This was referenced Oct 12, 2026

[Impeller] Support configurable Vulkan image layout handoff for embedder backing stores #194238

Open

[Impeller] Unified Vulkan transient attachment caching in Impeller core #194239

Open

andywolff requested changes Oct 12, 2026

View reviewed changes

Conversation

jwinarske commented Sep 18, 2026

What the embedder gets back

Tests

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

jwinarske commented Sep 18, 2026

Uh oh!

jwinarske commented Sep 18, 2026

Uh oh!

jwinarske commented Sep 18, 2026

Uh oh!

jwinarske commented Sep 18, 2026

Uh oh!

jwinarske commented Oct 8, 2026

Uh oh!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants