Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vulkan: create texture from d3d11 shared handle #6161

Open
wants to merge 13 commits into
base: trunk
Choose a base branch
from

Conversation

xiaopengli89
Copy link
Contributor

Description
VK_KHR_external_memory_win32 extension enables application to import Vulkan memory objects from Windows handles.

Testing

  1. Create D3D11 texture with D3D11_RESOURCE_MISC_SHARED_NTHANDLE and D3D11_RESOURCE_MISC_SHARED_KEYEDMUTEX flags
  2. Create shared handle with IDXGIResource1::CreateSharedHandle
  3. Create vulkan texture with texture_from_d3d11_shared_handle

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@xiaopengli89 xiaopengli89 requested a review from a team as a code owner August 26, 2024 07:39
@xiaopengli89 xiaopengli89 force-pushed the vulkan-texture-from-d3d11-shared-handle branch 3 times, most recently from 989df37 to c9f009e Compare August 27, 2024 10:04
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Some comments

wgpu-hal/src/vulkan/device.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/device.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/device.rs Show resolved Hide resolved
wgpu-hal/src/vulkan/device.rs Outdated Show resolved Hide resolved
@xiaopengli89 xiaopengli89 force-pushed the vulkan-texture-from-d3d11-shared-handle branch from 2ce979b to e2edab2 Compare August 28, 2024 07:09
@xiaopengli89 xiaopengli89 force-pushed the vulkan-texture-from-d3d11-shared-handle branch 5 times, most recently from 1563ef2 to 4ebcdb3 Compare August 30, 2024 12:02
@xiaopengli89 xiaopengli89 changed the title Vulkan: create texture from d3d11 shared handle WIP: Vulkan: create texture from d3d11 shared handle Sep 3, 2024
@xiaopengli89 xiaopengli89 changed the title WIP: Vulkan: create texture from d3d11 shared handle Vulkan: create texture from d3d11 shared handle Sep 3, 2024
@xiaopengli89 xiaopengli89 force-pushed the vulkan-texture-from-d3d11-shared-handle branch 3 times, most recently from d221534 to 6e1d7c6 Compare September 6, 2024 07:49
@xiaopengli89 xiaopengli89 force-pushed the vulkan-texture-from-d3d11-shared-handle branch 5 times, most recently from a0a89e4 to b55c0ba Compare September 13, 2024 03:00
@xiaopengli89
Copy link
Contributor Author

@cwfitzgerald Does this PR need any other improvements? This feature is already working properly in our product.

@xiaopengli89 xiaopengli89 force-pushed the vulkan-texture-from-d3d11-shared-handle branch from b55c0ba to 56a88b6 Compare October 17, 2024 12:07
@ErichDonGubler
Copy link
Member

@cwfitzgerald ☝🏻

@Tsuguri
Copy link
Contributor

Tsuguri commented Oct 18, 2024

Hello. I've been working on sharing textures with wgpu using both Dx12 and Vulkan but without doing any changes to wgpu itself. My memory allocation code ended up looking very similar with important difference: vk::ExternalMemoryHandleTypeFlags::D3D12_RESOURCE instead of vk::ExternalMemoryHandleTypeFlags::D3D11_TEXTURE. Other types could potentially be also usable for storing textures. If possible I would encourage to add it as a parameter next to d3d11_shared_handle.

In fact this new method is called texture_from_d3d11_shared_handle which makes my comment a little misplaced. Still, adding this parameter would make this code more usable.

Change to adapter to add external_memory_win32 capabilities is very nice - otherwise one has to extract raw adapter from wgpu, copy code from here, add required enabled_extensions to the list and then inject it back with adapter .create_device_from_hal which is a pain.

Other considerations: If I'm not wrong, NT handle holds the resource alive and it won't be cleaned up unless you call CloseHandle. I may be wrong here but I think Vulkan will NOT close the handle itself so user should do that manually. In my specific situation (sending textures between processes) I'm managing handles outside of wgpu - I remove injected shared textures from wgpu and it gets sent to different process. I think that this ownership situation should be reflected in documentation of this method (but please don't quote me on that, I'm not 100% sure how exactly this mechanism works after reading documentation and my limited tests).

@AdrianEddy
Copy link
Contributor

I too have the interop code here if that's helpful to anyone. One thing I had to do is copy the texture format (DXGI_FORMAT) conversion functions, it would be nice if they were exposed from wgpu

@cwfitzgerald
Copy link
Member

Sorry my time basically disappeared suddenly - looking now.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with nits

wgpu-hal/src/vulkan/device.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/device.rs Outdated Show resolved Hide resolved
@xiaopengli89 xiaopengli89 force-pushed the vulkan-texture-from-d3d11-shared-handle branch from 56a88b6 to b8c93f5 Compare October 21, 2024 03:21
@xiaopengli89
Copy link
Contributor Author

@Tsuguri VK_EXTERNAL_MEMORY_HANDLE_TYPE_D3D11_TEXTURE_BIT owns a reference to the memory used by the Direct3D resource, after importing the handle can be released safely.

@xiaopengli89 xiaopengli89 force-pushed the vulkan-texture-from-d3d11-shared-handle branch from 0078fbf to 9fb8b4f Compare October 21, 2024 07:03
@xiaopengli89
Copy link
Contributor Author

@MarijnS95 Any other suggestions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants