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
…#13220) Manual roll Flutter from d03768eef055 to 4249ccbc18a3 (158 revisions) Manual roll requested by [email protected] flutter/flutter@d03768e...4249ccb 2026-10-12 [email protected] [AGP 9.1.0 Migration #8] Unify add-to-app module wiring on the variant API (flutter/flutter#193717) 2026-10-12 [email protected] [master] Update Flutter DEPS to Dart 5933cb486b4aef2f8cf6b9f89a5651c0d5b0955e (flutter/flutter#193982) 2026-10-12 [email protected] [Android] Fix use-after-free of SurfaceControl.Transaction in HCPP (flutter/flutter#192969) 2026-10-12 [email protected] [windows] Add OpenGL compositor tests against ANGLE at D3D11 feature level 10_0 (flutter/flutter#194114) 2026-10-12 [email protected] Fix HDR image packing for ios (flutter/flutter#192526) 2026-10-12 [email protected] Bump customer tests to pick up material_ui and cupertino_ui (flutter/flutter#194236) 2026-10-12 [email protected] Remove no-shuffle from text_field_test.dart (flutter/flutter#193550) 2026-10-12 [email protected] [web] Clip semantics DOM text labels to stop stealing taps (flutter/flutter#194011) 2026-10-12 [email protected] Roll Skia from 9b6ceba4728d to 17f6986c226c (6 revisions) (flutter/flutter#194232) 2026-10-12 [email protected] [web] Skip non-focusable containers in Focusable.focusAsRouteDefault (flutter/flutter#194138) 2026-10-12 [email protected] [web_paragraph] Honor kTextHeightNone in TextStyle and ParagraphStyle (flutter/flutter#193733) 2026-10-12 [email protected] Roll Skia from c88b49d1d6c7 to 9b6ceba4728d (4 revisions) (flutter/flutter#194226) 2026-10-12 [email protected] Migrate remaining test to macOS 26 (flutter/flutter#193825) 2026-10-12 [email protected] Roll Dart SDK from 047ca322aa9c to 221363557163 (1 revision) (flutter/flutter#194219) 2026-10-12 [email protected] [flutter_tools] Include dev_dependencies hooks for release integration tests (flutter/flutter#193436) 2026-10-12 [email protected] Roll Skia from d8afad1b1829 to c88b49d1d6c7 (1 revision) (flutter/flutter#194198) 2026-10-12 [email protected] Roll Skia from b23ddbff1265 to d8afad1b1829 (4 revisions) (flutter/flutter#194195) 2026-10-11 [email protected] Roll Skia from a168863e44ec to b23ddbff1265 (1 revision) (flutter/flutter#194185) 2026-10-11 [email protected] Roll Skia from 0c1d93abfa04 to a168863e44ec (1 revision) (flutter/flutter#194181) 2026-10-11 [email protected] iOS: Re-enable rendering on willEnterForeground (flutter/flutter#191350) 2026-10-11 [email protected] Roll Skia from 93c9d3ffda25 to 0c1d93abfa04 (1 revision) (flutter/flutter#194179) 2026-10-11 [email protected] Remove more instances of deprecated `withOpacity` (flutter/flutter#193559) 2026-10-11 [email protected] Fix another `parameter_assignments` warning (flutter/flutter#193669) 2026-10-11 [email protected] Roll Skia from 3136abef8f58 to 93c9d3ffda25 (1 revision) (flutter/flutter#194169) 2026-10-11 [email protected] [Impeller] Guard the sampler library caches with a mutex (flutter/flutter#193738) 2026-10-11 [email protected] Roll Dart SDK from e9c89888bcc1 to 047ca322aa9c (1 revision) (flutter/flutter#194157) 2026-10-10 [email protected] Roll Dart SDK from 787baa8aae78 to e9c89888bcc1 (2 revisions) (flutter/flutter#194141) 2026-10-10 [email protected] Fix OverlayPortal layout in shrink-wrapped overlays (flutter/flutter#193079) 2026-10-10 [email protected] Roll Skia from 353076e27b1c to 3136abef8f58 (1 revision) (flutter/flutter#194130) 2026-10-10 [email protected] Roll Dart SDK from 88a07589b1ff to 787baa8aae78 (1 revision) (flutter/flutter#194124) 2026-10-10 [email protected] Route windowing view lookups through WidgetsBinding (flutter/flutter#192666) 2026-10-10 [email protected] Roll Dart SDK from 2a06cab34715 to 88a07589b1ff (1 revision) (flutter/flutter#194117) 2026-10-10 [email protected] Roll Skia from f9043b7aa798 to 353076e27b1c (2 revisions) (flutter/flutter#194115) 2026-10-10 [email protected] Refactored PipelineLibraryGLES (flutter/flutter#193985) 2026-10-09 [email protected] add shorthands for scroll physics subclasses in framework (flutter/flutter#193082) 2026-10-09 [email protected] Roll Dart SDK from ba889e3cee8a to 2a06cab34715 (1 revision) (flutter/flutter#194105) 2026-10-09 [email protected] Roll Skia from 5ea03d8a53a6 to f9043b7aa798 (2 revisions) (flutter/flutter#194099) 2026-10-09 [email protected] Add docs for using bb and led with LUCI (flutter/flutter#193859) 2026-10-09 [email protected] Roll Skia from e329bb617820 to 5ea03d8a53a6 (5 revisions) (flutter/flutter#194087) 2026-10-09 [email protected] Roll Packages from 54706bd to d8d9e97 (5 revisions) (flutter/flutter#194080) 2026-10-09 [email protected] Move the Vulkan wrapped-texture helper out of the GPU surface (flutter/flutter#193001) 2026-10-09 [email protected] Roll Skia from 3e54bcf4c963 to e329bb617820 (1 revision) (flutter/flutter#194078) 2026-10-09 [email protected] Roll Dart SDK from 23f0308f9af8 to ba889e3cee8a (2 revisions) (flutter/flutter#194077) 2026-10-09 [email protected] Bump dartdoc to 9.0.11 (flutter/flutter#194076) ...
…r#193001) `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`. No behavior change; the surface loses 53 lines. ## Tests No new ones — this is a move. `gpu_surface_unittests` covers the surface that now calls it, including the transients-recreation test that exercises the wrapped image path. Third of six, from flutter#187525. The fifth needs this one. Conflicts textually with the second (same file), so whichever lands second wants a rebase.
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.