Change the DropGuard public API to a callback-based one.

This patch also unifies the behavior of the the `DropGuard`-ish constructs in the Vulkan-based implementation.
gfx-rs · Aug 26, 2024 · 59dcb9f · 59dcb9f
1 parent fac49ee
commit 59dcb9f
Show file tree

Hide file tree

Showing 7 changed files with 49 additions and 19 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -82,6 +82,10 @@ By @wumpf in [#6069](https://github.com/gfx-rs/wgpu/pull/6069), [#6099](https://
 - Reduce the amount of debug and trace logs emitted by wgpu-core and wgpu-hal. By @nical in [#6065](https://github.com/gfx-rs/wgpu/issues/6065)
 - `Rg11b10Float` is renamed to `Rg11b10UFloat`. By @sagudev in [#6108](https://github.com/gfx-rs/wgpu/pull/6108)
 
+#### HAL
+
+- Change the inconsistent `DropGuard` based API on Vulkan and GLES to a consistent, callback-based one. By @jerzywilczek in ...
+
 ### Dependency Updates
 
 #### GLES

diff --git a/wgpu-hal/src/gles/device.rs b/wgpu-hal/src/gles/device.rs
@@ -110,22 +110,22 @@ impl super::Device {
     ///
     /// - `name` must be created respecting `desc`
     /// - `name` must be a texture
-    /// - If `drop_guard` is [`None`], wgpu-hal will take ownership of the texture. If `drop_guard` is
-    ///   [`Some`], the texture must be valid until the drop implementation
+    /// - If `drop_callback` is [`None`], wgpu-hal will take ownership of the texture. If
+    ///   `drop_callback` is [`Some`], the texture must be valid until the drop implementation
     ///   of the drop guard is called.
     #[cfg(any(native, Emscripten))]
     pub unsafe fn texture_from_raw(
         &self,
         name: std::num::NonZeroU32,
         desc: &crate::TextureDescriptor,
-        drop_guard: Option<crate::DropGuard>,
+        drop_callback: Option<crate::DropCallback>,
     ) -> super::Texture {
         super::Texture {
             inner: super::TextureInner::Texture {
                 raw: glow::NativeTexture(name),
                 target: super::Texture::get_info_from_desc(desc),
             },
-            drop_guard,
+            drop_guard: drop_callback.map(|callback| crate::DropGuard { callback }),
             mip_level_count: desc.mip_level_count,
             array_layer_count: desc.array_layer_count(),
             format: desc.format,
@@ -146,13 +146,13 @@ impl super::Device {
         &self,
         name: std::num::NonZeroU32,
         desc: &crate::TextureDescriptor,
-        drop_guard: Option<crate::DropGuard>,
+        drop_callback: Option<crate::DropCallback>,
     ) -> super::Texture {
         super::Texture {
             inner: super::TextureInner::Renderbuffer {
                 raw: glow::NativeRenderbuffer(name),
             },
-            drop_guard,
+            drop_guard: drop_callback.map(|callback| crate::DropGuard { callback }),
             mip_level_count: desc.mip_level_count,
             array_layer_count: desc.array_layer_count(),
             format: desc.format,

diff --git a/wgpu-hal/src/lib.rs b/wgpu-hal/src/lib.rs
@@ -303,8 +303,24 @@ pub type MemoryRange = Range<wgt::BufferAddress>;
 pub type FenceValue = u64;
 pub type AtomicFenceValue = std::sync::atomic::AtomicU64;
 
-/// Drop guard to signal wgpu-hal is no longer using an externally created object.
-pub type DropGuard = Box<dyn std::any::Any + Send + Sync>;
+/// A callback to signal that wgpu is no longer using a resource.
+pub type DropCallback = Box<dyn FnMut() + Send + Sync + 'static>;
+
+pub struct DropGuard {
+    callback: DropCallback,
+}
+
+impl Drop for DropGuard {
+    fn drop(&mut self) {
+        (self.callback)();
+    }
+}
+
+impl std::fmt::Debug for DropGuard {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("DropGuard").finish()
+    }
+}
 
 #[derive(Clone, Debug, PartialEq, Eq, Error)]
 pub enum DeviceError {

diff --git a/wgpu-hal/src/vulkan/adapter.rs b/wgpu-hal/src/vulkan/adapter.rs
@@ -1593,11 +1593,13 @@ impl super::Adapter {
     /// - `raw_device` must be created from this adapter.
     /// - `raw_device` must be created using `family_index`, `enabled_extensions` and `physical_device_features()`
     /// - `enabled_extensions` must be a superset of `required_device_extensions()`.
+    /// - If `drop_callback` is [`None`], wgpu-hal will take ownership of `raw_device`. If
+    ///   `drop_callback` is [`Some`], `raw_device` must be valid until the callback is called.
     #[allow(clippy::too_many_arguments)]
     pub unsafe fn device_from_raw(
         &self,
         raw_device: ash::Device,
-        handle_is_owned: bool,
+        drop_callback: Option<crate::DropCallback>,
         enabled_extensions: &[&'static CStr],
         features: wgt::Features,
         memory_hints: &wgt::MemoryHints,
@@ -1812,12 +1814,14 @@ impl super::Adapter {
             0, 0, 0, 0,
         ];
 
+        let drop_guard = drop_callback.map(|callback| crate::DropGuard { callback });
+
         let shared = Arc::new(super::DeviceShared {
             raw: raw_device,
             family_index,
             queue_index,
             raw_queue,
-            handle_is_owned,
+            drop_guard,
             instance: Arc::clone(&self.instance),
             physical_device: self.raw,
             enabled_extensions: enabled_extensions.into(),
@@ -2005,7 +2009,7 @@ impl crate::Adapter for super::Adapter {
         unsafe {
             self.device_from_raw(
                 raw_device,
-                true,
+                None,
                 &enabled_extensions,
                 features,
                 memory_hints,

diff --git a/wgpu-hal/src/vulkan/device.rs b/wgpu-hal/src/vulkan/device.rs
@@ -290,7 +290,7 @@ impl super::DeviceShared {
         for &raw in self.framebuffers.lock().values() {
             unsafe { self.raw.destroy_framebuffer(raw, None) };
         }
-        if self.handle_is_owned {
+        if self.drop_guard.is_none() {
             unsafe { self.raw.destroy_device(None) };
         }
     }
@@ -648,13 +648,13 @@ impl super::Device {
     /// # Safety
     ///
     /// - `vk_image` must be created respecting `desc`
-    /// - If `drop_guard` is `Some`, the application must manually destroy the image handle. This
-    ///   can be done inside the `Drop` impl of `drop_guard`.
+    /// - If `drop_callback` is [`None`], wgpu-hal will take ownership of `vk_image`. If
+    ///   `drop_callback` is [`Some`], `vk_image` must be valid until the callback is called.
     /// - If the `ImageCreateFlags` does not contain `MUTABLE_FORMAT`, the `view_formats` of `desc` must be empty.
     pub unsafe fn texture_from_raw(
         vk_image: vk::Image,
         desc: &crate::TextureDescriptor,
-        drop_guard: Option<crate::DropGuard>,
+        drop_callback: Option<crate::DropCallback>,
     ) -> super::Texture {
         let mut raw_flags = vk::ImageCreateFlags::empty();
         let mut view_formats = vec![];
@@ -673,6 +673,8 @@ impl super::Device {
             raw_flags |= vk::ImageCreateFlags::MUTABLE_FORMAT;
         }
 
+        let drop_guard = drop_callback.map(|callback| crate::DropGuard { callback });
+
         super::Texture {
             raw: vk_image,
             drop_guard,

diff --git a/wgpu-hal/src/vulkan/instance.rs b/wgpu-hal/src/vulkan/instance.rs
@@ -310,6 +310,8 @@ impl super::Instance {
     /// - `extensions` must be a superset of `desired_extensions()` and must be created from the
     ///   same entry, `instance_api_version`` and flags.
     /// - `android_sdk_version` is ignored and can be `0` for all platforms besides Android
+    /// - If `drop_callback` is [`None`], wgpu-hal will take ownership of `raw_instance`. If
+    ///   `drop_callback` is [`Some`], `raw_instance` must be valid until the callback is called.
     ///
     /// If `debug_utils_user_data` is `Some`, then the validation layer is
     /// available, so create a [`vk::DebugUtilsMessengerEXT`].
@@ -323,7 +325,7 @@ impl super::Instance {
         extensions: Vec<&'static CStr>,
         flags: wgt::InstanceFlags,
         has_nv_optimus: bool,
-        drop_guard: Option<crate::DropGuard>,
+        drop_callback: Option<crate::DropCallback>,
     ) -> Result<Self, crate::InstanceError> {
         log::debug!("Instance version: 0x{:x}", instance_api_version);
 
@@ -364,6 +366,8 @@ impl super::Instance {
                 None
             };
 
+        let drop_guard = drop_callback.map(|callback| crate::DropGuard { callback });
+
         Ok(Self {
             shared: Arc::new(super::InstanceShared {
                 raw: raw_instance,
@@ -555,7 +559,7 @@ impl Drop for super::InstanceShared {
                     .destroy_debug_utils_messenger(du.messenger, None);
                 du
             });
-            if let Some(_drop_guard) = self.drop_guard.take() {
+            if self.drop_guard.is_none() {
                 self.raw.destroy_instance(None);
             }
         }
@@ -829,7 +833,7 @@ impl crate::Instance for super::Instance {
                 extensions,
                 desc.flags,
                 has_nv_optimus,
-                Some(Box::new(())), // `Some` signals that wgpu-hal is in charge of destroying vk_instance
+                None,
             )
         }
     }

diff --git a/wgpu-hal/src/vulkan/mod.rs b/wgpu-hal/src/vulkan/mod.rs
@@ -547,7 +547,7 @@ struct DeviceShared {
     family_index: u32,
     queue_index: u32,
     raw_queue: vk::Queue,
-    handle_is_owned: bool,
+    drop_guard: Option<crate::DropGuard>,
     instance: Arc<InstanceShared>,
     physical_device: vk::PhysicalDevice,
     enabled_extensions: Vec<&'static CStr>,