[wgpu-hal] gfx-rs#5956 windows-rs migration followups and cleanups

PR gfx-rs#5956 wasn't fully complete and still had some outstanding minor
issues and cleanups to be done, as well as hidden semantic changes.
This addresses a bunch of them:

- Remove unnecessary `Error` mapping to `String` as `windows-rs`'s
  `Error` has a more complete `Display` representation by itself.
- Remove `into_result()` as every call could have formatted the
  `windows-rs` `Error` in a log call directly.
- Pass `None` instead of a pointer to an empty slice wherever possible
  (waiting for microsoft/win32metadata#1971 to
  trickle down into `windows-rs`).
- Remove `.clone()` on COM objects (temporarily increasing the refcount)
  when it can be avoided by inverting the order of operations on `raw`
  variables.

Author: MarijnS95

CHANGELOG.md

@@ -119,7 +119,7 @@ By @teoxoy [#6134](https://github.com/gfx-rs/wgpu/pull/6134).
 
 #### DX12
 
-- Replace `winapi` code to use the `windows` crate. By @MarijnS95 in [#5956](https://github.com/gfx-rs/wgpu/pull/5956)
+- Replace `winapi` code to use the `windows` crate. By @MarijnS95 in [#5956](https://github.com/gfx-rs/wgpu/pull/5956) and [#6173](https://github.com/gfx-rs/wgpu/pull/6173)
 
 ## 22.0.0 (2024-07-17)
 

wgpu-hal/src/auxil/dxgi/result.rs

@@ -1,56 +1,32 @@
-use std::borrow::Cow;
-
 use windows::Win32::{Foundation, Graphics::Dxgi};
 
 pub(crate) trait HResult<O> {
-    fn into_result(self) -> Result<O, Cow<'static, str>>;
     fn into_device_result(self, description: &str) -> Result<O, crate::DeviceError>;
 }
 impl<T> HResult<T> for windows::core::Result<T> {
-    fn into_result(self) -> Result<T, Cow<'static, str>> {
-        // TODO: use windows-rs built-in error formatting?
-        let description = match self {
-            Ok(t) => return Ok(t),
-            Err(e) if e.code() == Foundation::E_UNEXPECTED => "unexpected",
-            Err(e) if e.code() == Foundation::E_NOTIMPL => "not implemented",
-            Err(e) if e.code() == Foundation::E_OUTOFMEMORY => "out of memory",
-            Err(e) if e.code() == Foundation::E_INVALIDARG => "invalid argument",
-            Err(e) => return Err(Cow::Owned(format!("{e:?}"))),
-        };
-        Err(Cow::Borrowed(description))
-    }
     fn into_device_result(self, description: &str) -> Result<T, crate::DeviceError> {
         #![allow(unreachable_code)]
 
-        let err_code = if let Err(err) = &self {
-            Some(err.code())
-        } else {
-            None
-        };
-        self.into_result().map_err(|err| {
+        self.map_err(|err| {
             log::error!("{} failed: {}", description, err);
 
-            let Some(err_code) = err_code else {
-                unreachable!()
-            };
-
-            match err_code {
+            match err.code() {
                 Foundation::E_OUTOFMEMORY => {
                     #[cfg(feature = "oom_panic")]
                     panic!("{description} failed: Out of memory");
-                    return crate::DeviceError::OutOfMemory;
+                    crate::DeviceError::OutOfMemory
                 }
                 Dxgi::DXGI_ERROR_DEVICE_RESET | Dxgi::DXGI_ERROR_DEVICE_REMOVED => {
                     #[cfg(feature = "device_lost_panic")]
                     panic!("{description} failed: Device lost ({err})");
+                    crate::DeviceError::Lost
                 }
                 _ => {
                     #[cfg(feature = "internal_error_panic")]
                     panic!("{description} failed: {err}");
+                    crate::DeviceError::Unexpected
                 }
             }
-
-            crate::DeviceError::Lost
         })
     }
 }

wgpu-hal/src/dx12/command.rs

@@ -53,7 +53,6 @@ impl crate::BufferTextureCopy {
 
 impl super::Temp {
     fn prepare_marker(&mut self, marker: &str) -> (&[u16], u32) {
-        // TODO: Store in HSTRING
         self.marker.clear();
         self.marker.extend(marker.encode_utf16());
         self.marker.push(0);
@@ -153,7 +152,7 @@ impl super::CommandEncoder {
         self.update_root_elements();
     }
 
-    //Note: we have to call this lazily before draw calls. Otherwise, D3D complains
+    // Note: we have to call this lazily before draw calls. Otherwise, D3D complains
     // about the root parameters being incompatible with root signature.
     fn update_root_elements(&mut self) {
         use super::{BufferViewKind as Bvk, PassKind as Pk};
@@ -265,7 +264,8 @@ impl crate::CommandEncoder for super::CommandEncoder {
     unsafe fn begin_encoding(&mut self, label: crate::Label) -> Result<(), crate::DeviceError> {
         let list = loop {
             if let Some(list) = self.free_lists.pop() {
-                let reset_result = unsafe { list.Reset(&self.allocator, None) }.into_result();
+                // TODO: Is an error expected here and should we print it?
+                let reset_result = unsafe { list.Reset(&self.allocator, None) };
                 if reset_result.is_ok() {
                     break Some(list);
                 }
@@ -314,7 +314,9 @@ impl crate::CommandEncoder for super::CommandEncoder {
         for cmd_buf in command_buffers {
             self.free_lists.push(cmd_buf.raw);
         }
-        let _todo_handle_error = unsafe { self.allocator.Reset() };
+        if let Err(e) = unsafe { self.allocator.Reset() } {
+            log::error!("ID3D12CommandAllocator::Reset() failed with {e}");
+        }
     }
 
     unsafe fn transition_buffers<'a, T>(&mut self, barriers: T)
@@ -724,8 +726,7 @@ impl crate::CommandEncoder for super::CommandEncoder {
                         cat.clear_value.b as f32,
                         cat.clear_value.a as f32,
                     ];
-                    // TODO: Empty slice vs None?
-                    unsafe { list.ClearRenderTargetView(*rtv, &value, Some(&[])) };
+                    unsafe { list.ClearRenderTargetView(*rtv, &value, None) };
                 }
                 if let Some(ref target) = cat.resolve_target {
                     self.pass.resolves.push(super::PassResolve {
@@ -754,12 +755,23 @@ impl crate::CommandEncoder for super::CommandEncoder {
             if let Some(ds_view) = ds_view {
                 if flags != Direct3D12::D3D12_CLEAR_FLAGS::default() {
                     unsafe {
-                        list.ClearDepthStencilView(
-                            ds_view,
+                        // list.ClearDepthStencilView(
+                        //     ds_view,
+                        //     flags,
+                        //     ds.clear_value.0,
+                        //     ds.clear_value.1 as u8,
+                        //     None,
+                        // )
+                        // TODO: Replace with the above in the next breaking windows-rs release,
+                        // when https://github.com/microsoft/win32metadata/pull/1971 is in.
+                        (windows_core::Interface::vtable(list).ClearDepthStencilView)(
+                            windows_core::Interface::as_raw(list),
+                            mem::transmute(ds_view),
                             flags,
                             ds.clear_value.0,
                             ds.clear_value.1 as u8,
-                            &[],
+                            0,
+                            std::ptr::null(),
                         )
                     }
                 }
@@ -796,7 +808,7 @@ impl crate::CommandEncoder for super::CommandEncoder {
                     Type: Direct3D12::D3D12_RESOURCE_BARRIER_TYPE_TRANSITION,
                     Flags: Direct3D12::D3D12_RESOURCE_BARRIER_FLAG_NONE,
                     Anonymous: Direct3D12::D3D12_RESOURCE_BARRIER_0 {
-                        //Note: this assumes `D3D12_RESOURCE_STATE_RENDER_TARGET`.
+                        // Note: this assumes `D3D12_RESOURCE_STATE_RENDER_TARGET`.
                         // If it's not the case, we can include the `TextureUses` in `PassResove`.
                         Transition: mem::ManuallyDrop::new(
                             Direct3D12::D3D12_RESOURCE_TRANSITION_BARRIER {
@@ -813,7 +825,7 @@ impl crate::CommandEncoder for super::CommandEncoder {
                     Type: Direct3D12::D3D12_RESOURCE_BARRIER_TYPE_TRANSITION,
                     Flags: Direct3D12::D3D12_RESOURCE_BARRIER_FLAG_NONE,
                     Anonymous: Direct3D12::D3D12_RESOURCE_BARRIER_0 {
-                        //Note: this assumes `D3D12_RESOURCE_STATE_RENDER_TARGET`.
+                        // Note: this assumes `D3D12_RESOURCE_STATE_RENDER_TARGET`.
                         // If it's not the case, we can include the `TextureUses` in `PassResolve`.
                         Transition: mem::ManuallyDrop::new(
                             Direct3D12::D3D12_RESOURCE_TRANSITION_BARRIER {

wgpu-hal/src/dx12/descriptor.rs

@@ -56,16 +56,18 @@ impl GeneralHeap {
                 .into_device_result("Descriptor heap creation")?
         };
 
+        let start = DualHandle {
+            cpu: unsafe { raw.GetCPUDescriptorHandleForHeapStart() },
+            gpu: unsafe { raw.GetGPUDescriptorHandleForHeapStart() },
+            count: 0,
+        };
+
         Ok(Self {
-            raw: raw.clone(),
+            raw,
             ty,
             handle_size: unsafe { device.GetDescriptorHandleIncrementSize(ty) } as u64,
             total_handles,
-            start: DualHandle {
-                cpu: unsafe { raw.GetCPUDescriptorHandleForHeapStart() },
-                gpu: unsafe { raw.GetGPUDescriptorHandleForHeapStart() },
-                count: 0,
-            },
+            start,
             ranges: Mutex::new(RangeAllocator::new(0..total_handles)),
         })
     }
@@ -268,12 +270,14 @@ impl CpuHeap {
         let raw = unsafe { device.CreateDescriptorHeap::<Direct3D12::ID3D12DescriptorHeap>(&desc) }
             .into_device_result("CPU descriptor heap creation")?;
 
+        let start = unsafe { raw.GetCPUDescriptorHandleForHeapStart() };
+
         Ok(Self {
             inner: Mutex::new(CpuHeapInner {
-                _raw: raw.clone(),
+                _raw: raw,
                 stage: Vec::new(),
             }),
-            start: unsafe { raw.GetCPUDescriptorHandleForHeapStart() },
+            start,
             handle_size,
             total,
         })
@@ -297,7 +301,7 @@ impl fmt::Debug for CpuHeap {
 }
 
 pub(super) unsafe fn upload(
-    device: Direct3D12::ID3D12Device,
+    device: &Direct3D12::ID3D12Device,
     src: &CpuHeapInner,
     dst: &GeneralHeap,
     dummy_copy_counts: &[u32],

wgpu-hal/src/dx12/device.rs

@@ -1295,7 +1295,7 @@ impl crate::Device for super::Device {
             Some(inner) => {
                 let dual = unsafe {
                     descriptor::upload(
-                        self.raw.clone(),
+                        &self.raw,
                         &inner,
                         &self.shared.heap_views,
                         &desc.layout.copy_counts,
@@ -1309,7 +1309,7 @@ impl crate::Device for super::Device {
             Some(inner) => {
                 let dual = unsafe {
                     descriptor::upload(
-                        self.raw.clone(),
+                        &self.raw,
                         &inner,
                         &self.shared.heap_samplers,
                         &desc.layout.copy_counts,

wgpu-hal/src/dx12/instance.rs

@@ -10,10 +10,7 @@ use windows::{
 };
 
 use super::SurfaceTarget;
-use crate::{
-    auxil::{self, dxgi::result::HResult as _},
-    dx12::D3D12Lib,
-};
+use crate::{auxil, dx12::D3D12Lib};
 
 impl Drop for super::Instance {
     fn drop(&mut self) {
@@ -98,8 +95,8 @@ impl crate::Instance for super::Instance {
                 )
             };
 
-            match hr.into_result() {
-                Err(err) => log::warn!("Unable to check for tearing support: {}", err),
+            match hr {
+                Err(err) => log::warn!("Unable to check for tearing support: {err}"),
                 Ok(()) => supports_allow_tearing = true,
             }
         }

wgpu-hal/src/dx12/mod.rs

@@ -49,14 +49,13 @@ use std::{ffi, fmt, mem, num::NonZeroU32, ops::Deref, sync::Arc};
 use arrayvec::ArrayVec;
 use parking_lot::{Mutex, RwLock};
 use windows::{
-    core::{Interface, Param as _},
+    core::{Free, Interface, Param as _},
     Win32::{
         Foundation,
         Graphics::{Direct3D, Direct3D12, DirectComposition, Dxgi},
         System::Threading,
     },
 };
-use windows_core::Free;
 
 use crate::auxil::{
     self,
@@ -1026,8 +1025,8 @@ impl crate::Surface for Surface {
                         flags,
                     )
                 };
-                if let Err(err) = result.into_result() {
-                    log::error!("ResizeBuffers failed: {}", err);
+                if let Err(err) = result {
+                    log::error!("ResizeBuffers failed: {err}");
                     return Err(crate::SurfaceError::Other("window is in use"));
                 }
                 raw
@@ -1092,38 +1091,33 @@ impl crate::Surface for Surface {
 
                 let swap_chain1 = swap_chain1.map_err(|err| {
                     log::error!("SwapChain creation error: {}", err);
-                    crate::SurfaceError::Other("swap chain creation")
+                    crate::SurfaceError::Other("swapchain creation")
                 })?;
 
                 match &self.target {
                     SurfaceTarget::WndHandle(_) | SurfaceTarget::SurfaceHandle(_) => {}
                     SurfaceTarget::Visual(visual) => {
-                        if let Err(err) = unsafe { visual.SetContent(&swap_chain1) }.into_result() {
-                            log::error!("Unable to SetContent: {}", err);
+                        if let Err(err) = unsafe { visual.SetContent(&swap_chain1) } {
+                            log::error!("Unable to SetContent: {err}");
                             return Err(crate::SurfaceError::Other(
                                 "IDCompositionVisual::SetContent",
                             ));
                         }
                     }
                     SurfaceTarget::SwapChainPanel(swap_chain_panel) => {
-                        if let Err(err) =
-                            unsafe { swap_chain_panel.SetSwapChain(&swap_chain1) }.into_result()
-                        {
-                            log::error!("Unable to SetSwapChain: {}", err);
+                        if let Err(err) = unsafe { swap_chain_panel.SetSwapChain(&swap_chain1) } {
+                            log::error!("Unable to SetSwapChain: {err}");
                             return Err(crate::SurfaceError::Other(
                                 "ISwapChainPanelNative::SetSwapChain",
                             ));
                         }
                     }
                 }
 
-                match swap_chain1.cast::<Dxgi::IDXGISwapChain3>() {
-                    Ok(swap_chain3) => swap_chain3,
-                    Err(err) => {
-                        log::error!("Unable to cast swap chain: {}", err);
-                        return Err(crate::SurfaceError::Other("swap chain cast to 3"));
-                    }
-                }
+                swap_chain1.cast::<Dxgi::IDXGISwapChain3>().map_err(|err| {
+                    log::error!("Unable to cast swapchain: {err}");
+                    crate::SurfaceError::Other("swapchain cast to version 3")
+                })?
             }
         };
 

wgpu-hal/src/dx12/shader_compilation.rs

@@ -4,8 +4,6 @@ use std::ptr;
 pub(super) use dxc::{compile_dxc, get_dxc_container, DxcContainer};
 use windows::Win32::Graphics::Direct3D;
 
-use crate::auxil::dxgi::result::HResult;
-
 // This exists so that users who don't want to use dxc can disable the dxc_shader_compiler feature
 // and not have to compile hassle_rs.
 // Currently this will use Dxc if it is chosen as the dx12 compiler at `Instance` creation time, and will
@@ -57,7 +55,7 @@ pub(super) fn compile_fxc(
         )
     };
 
-    match hr.into_result() {
+    match hr {
         Ok(()) => {
             let shader_data = shader_data.unwrap();
             (