Repository navigation
Move the Vulkan wrapped-texture helper out of the GPU surface - #193001
Conversation
|
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. |
![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 refactors the Vulkan backend by extracting the WrappedTextureSourceVK class into a reusable utility (texture_wrapper_vk.h/cc) and updating GPUSurfaceVulkanImpeller to use it. The review feedback suggests parameterizing the IsSwapchainImage property instead of hardcoding it to true, which will allow the utility to support non-swapchain embedder-provided backing stores.
github-actions
Bot
added
engine
|
Test exemption: this is a move. Same code, same behavior, no new API — the class goes from a private one in gpu_surface_vulkan_impeller.cc to impeller/renderer/backend/vulkan, and the surface calls it instead of defining it. Existing coverage follows it: gpu_surface_unittests exercises the surface that calls it, including RecreatesTransientsWhenFrameSizeChanges, which goes through the wrapped image. #193003 is where it gets a second caller and new tests. |
7eb88b4 to
b349835
Compare
|
Force-pushed: restacked onto #193000. This never built standalone. The .cc here uses Land #193000 first. |
b349835 to
9984cd6
Compare
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_context_ is a shared_ptr<impeller::Context> handed in by the caller, and which concrete type it holds depends on the construction path: the delegate_ == nullptr branch casts it to SurfaceContextVK, the backing-store branch to ContextVK. DisposeThreadLocalCachedResources runs before that branch, on both paths, so ContextVK::Cast there is an unchecked downcast whenever the context is a SurfaceContextVK, which derives from Context rather than ContextVK. It then locks desc_pool_mutex_ at the wrong offset, pthread_mutex_lock returns EINVAL, and the process aborts. DisposeThreadLocalCachedResources, CreateCommandBuffer and GetCommandQueue are all virtual on Context, and SurfaceContextVK overrides or delegates them, so call through the base and drop both casts.
3445f43 to
a9d4970
Compare
|
@andywolff rebased onto master now that #193000 has landed, which dismissed your approval — sorry for the re-review. Same change you approved, with one difference worth pointing at. #193000 was squash-merged, so its commit is no longer an ancestor here; I cherry-picked this branch's own two commits onto master and dropped it along with the two master merges. The cast fix therefore carries only its submit-callback half now, because the Net is 4 files, +124/-56: the helper move plus that one hunk. The remaining casts in |
andywolff
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
2 participants
GPUSurfaceVulkanImpellercarries a privateTextureSourceVKsubclass that wraps aVkImagethe 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 (#187525).Move it to
impeller/renderer/backend/vulkanasWrapTextureSourceVK. No behavior change; the surface loses 53 lines.Tests
No new ones — this is a move.
gpu_surface_unittestscovers the surface that now calls it, including the transients-recreation test that exercises the wrapped image path.Third of six, from #187525. The fifth needs this one. Conflicts textually with the second (same file), so whichever lands second wants a rebase.