[Impeller] Fix VkImageView leak in WrappedTextureSourceVK#181966
[Impeller] Fix VkImageView leak in WrappedTextureSourceVK#181966gaaclarke merged 1 commit intoflutter:masterfrom
Conversation
WrappedTextureSourceVK creates a VkImageView every frame via createImageView() but never destroys it. The handle is stored as a raw non-owning vk::ImageView and the destructor is empty, leaking one VkImageView per frame. Switch to vk::UniqueImageView for RAII ownership so the image view is destroyed when the WrappedTextureSourceVK is released. The resource lifetime is correct because Impeller's TrackedObjectsVK holds a shared_ptr to the TextureSourceVK until the GPU fence signals completion. This only affects the embedder delegate code path. Android passes nullptr as the delegate and uses AcquireNextSurface() instead, so this change has no effect on Android.
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. 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. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a VkImageView resource leak in WrappedTextureSourceVK by adopting a RAII approach. The raw vk::ImageView handle has been replaced with vk::UniqueImageView to ensure the view is automatically destroyed when no longer needed. The related code has been updated to use createImageViewUnique for creating the handle and std::move to correctly transfer ownership. This change effectively prevents a memory leak in the Vulkan backend's embedder delegate path and improves the overall robustness of resource management.
|
@jason-simmons this seems reasonable to me, can you do a pass on it please? |
jason-simmons
left a comment
jason-simmons
left a comment
There was a problem hiding this comment.
Thank you for finding and fixing this!
Ran the https://github.com/flutter/flutter/tree/master/engine/src/flutter/examples/vulkan_glfw sample app with Vulkan validation layers enabled.
Confirmed that the object tracking layer reports leaks of VkImageView objects on the current head. With this PR, the validation layers do not show any errors.
|
@RickyvdBerg can you sign the CLA ( https://cla.developers.google.com/clas ) so we can accept the PR please? |
|
No problem, happy to help! I believe I did sign it, well I signed it with the wrong email initially but then I added a second one which is the same as this github account. |
Should be done now! |
…1966) ## Description Fixes flutter#181967 `WrappedTextureSourceVK` in the embedder delegate path creates a `VkImageView` every frame via `createImageView()` but never destroys it. The handle is stored as a raw non-owning `vk::ImageView` and the destructor is empty, leaking one `VkImageView` per frame (~60/sec on a typical app). This switches to `vk::UniqueImageView` for RAII ownership so the image view is destroyed when the `WrappedTextureSourceVK` is released. ## Why this is safe The resource lifetime is correct because Impeller's `TrackedObjectsVK` holds a `shared_ptr` to the `TextureSourceVK` until the GPU fence signals completion: 1. `RenderPassVK` calls `Track(attachment.texture)` on all render target textures 2. `CommandBufferVK::Track()` stores `shared_ptr<TextureSourceVK>` in `TrackedObjectsVK` 3. `TrackedObjectsVK` is cleared only in the fence completion callback, after the GPU finishes The `VkImageView` cannot be destroyed while the GPU is still using it. ## Scope This only affects the **embedder delegate code path** (when `delegate_ != nullptr` in `GPUSurfaceVulkanImpeller::AcquireFrame`). Android passes `nullptr` as the delegate and uses `AcquireNextSurface()` instead, so this change has **no effect on Android, iOS, or standard swapchain rendering**. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide]. - [x] I listed at least one issue that this PR fixes in the description above. - [ ] I added a test for the change if applicable. *(See note below)* **Note on testing:** This is a resource leak fix in a Vulkan code path that requires a running Vulkan device and embedder delegate. I was unable to find existing test infrastructure for the embedder delegate path in `GPUSurfaceVulkanImpeller`. Happy to add a test if reviewers can point me to the right test harness. Found while working on Impeller Vulkan support for Linux/Wayland with a custom embedder. [Contributor Guide]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md [Flutter Style Guide]: https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md
3 participants
Description
Fixes #181967
WrappedTextureSourceVKin the embedder delegate path creates aVkImageViewevery frame viacreateImageView()but never destroys it. The handle is stored as a raw non-owningvk::ImageViewand the destructor is empty, leaking oneVkImageViewper frame (~60/sec on a typical app).This switches to
vk::UniqueImageViewfor RAII ownership so the image view is destroyed when theWrappedTextureSourceVKis released.Why this is safe
The resource lifetime is correct because Impeller's
TrackedObjectsVKholds ashared_ptrto theTextureSourceVKuntil the GPU fence signals completion:RenderPassVKcallsTrack(attachment.texture)on all render target texturesCommandBufferVK::Track()storesshared_ptr<TextureSourceVK>inTrackedObjectsVKTrackedObjectsVKis cleared only in the fence completion callback, after the GPU finishesThe
VkImageViewcannot be destroyed while the GPU is still using it.Scope
This only affects the embedder delegate code path (when
delegate_ != nullptrinGPUSurfaceVulkanImpeller::AcquireFrame). Android passesnullptras the delegate and usesAcquireNextSurface()instead, so this change has no effect on Android, iOS, or standard swapchain rendering.Pre-launch Checklist
Note on testing: This is a resource leak fix in a Vulkan code path that requires a running Vulkan device and embedder delegate. I was unable to find existing test infrastructure for the embedder delegate path in
GPUSurfaceVulkanImpeller. Happy to add a test if reviewers can point me to the right test harness.Found while working on Impeller Vulkan support for Linux/Wayland with a custom embedder.