Expose the full Result type to the rust map_async callback (#2939)

gfx-rs · Oct 13, 2022 · d1ff383 · d1ff383
1 parent 2158841
commit d1ff383
Show file tree

Hide file tree

Showing 7 changed files with 56 additions and 56 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -308,6 +308,7 @@ Added items to the public API
 - Validate that map_async's range is not negative by @nical in [#2938](https://github.com/gfx-rs/wgpu/pull/2938)
 - Fix calculation/validation of layer/mip ranges in create_texture_view by @nical in [#2955](https://github.com/gfx-rs/wgpu/pull/2955)
 - Validate the sample count and mip level in `copy_texture_to_buffer` by @nical in [#2958](https://github.com/gfx-rs/wgpu/pull/2958)
+- Expose the cause of the error in the `map_async` callback in [#2939](https://github.com/gfx-rs/wgpu/pull/2939)
 
 #### DX12
 

diff --git a/deno_webgpu/src/buffer.rs b/deno_webgpu/src/buffer.rs
@@ -13,6 +13,7 @@ use std::cell::RefCell;
 use std::convert::TryFrom;
 use std::rc::Rc;
 use std::time::Duration;
+use wgpu_core::resource::BufferAccessResult;
 
 use super::error::DomExceptionOperationError;
 use super::error::WebGpuResult;
@@ -70,7 +71,7 @@ pub async fn op_webgpu_buffer_get_map_async(
     offset: u64,
     size: u64,
 ) -> Result<WebGpuResult, AnyError> {
-    let (sender, receiver) = oneshot::channel::<Result<(), AnyError>>();
+    let (sender, receiver) = oneshot::channel::<BufferAccessResult>();
 
     let device;
     {
@@ -84,12 +85,7 @@ pub async fn op_webgpu_buffer_get_map_async(
         device = device_resource.0;
 
         let callback = Box::new(move |status| {
-            sender
-                .send(match status {
-                    wgpu_core::resource::BufferMapAsyncStatus::Success => Ok(()),
-                    _ => unreachable!(), // TODO
-                })
-                .unwrap();
+            sender.send(status).unwrap();
         });
 
         // TODO(lucacasonato): error handling

diff --git a/player/tests/test.rs b/player/tests/test.rs
@@ -55,10 +55,9 @@ struct Test<'a> {
     actions: Vec<wgc::device::trace::Action<'a>>,
 }
 
-fn map_callback(status: wgc::resource::BufferMapAsyncStatus) {
-    match status {
-        wgc::resource::BufferMapAsyncStatus::Success => (),
-        _ => panic!("Unable to map"),
+fn map_callback(status: Result<(), wgc::resource::BufferAccessError>) {
+    if let Err(e) = status {
+        panic!("Buffer map error: {}", e);
     }
 }
 

diff --git a/wgpu-core/src/device/life.rs b/wgpu-core/src/device/life.rs
@@ -887,11 +887,11 @@ impl<A: HalApi> LifetimeTracker<A> {
                                 range: mapping.range.start..mapping.range.start + size,
                                 host,
                             };
-                            resource::BufferMapAsyncStatus::Success
+                            Ok(())
                         }
                         Err(e) => {
                             log::error!("Mapping failed {:?}", e);
-                            resource::BufferMapAsyncStatus::Error
+                            Err(e)
                         }
                     }
                 } else {
@@ -900,7 +900,7 @@ impl<A: HalApi> LifetimeTracker<A> {
                         range: mapping.range,
                         host: mapping.op.host,
                     };
-                    resource::BufferMapAsyncStatus::Success
+                    Ok(())
                 };
                 pending_callbacks.push((mapping.op, status));
             }

diff --git a/wgpu-core/src/device/mod.rs b/wgpu-core/src/device/mod.rs
@@ -9,8 +9,8 @@ use crate::{
     },
     instance::{self, Adapter, Surface},
     pipeline, present,
-    resource::{self, BufferMapState},
-    resource::{BufferAccessError, BufferMapAsyncStatus, BufferMapOperation},
+    resource::{self, BufferAccessResult, BufferMapState},
+    resource::{BufferAccessError, BufferMapOperation},
     track::{BindGroupStates, TextureSelector, Tracker},
     validation::{self, check_buffer_usage, check_texture_usage},
     FastHashMap, Label, LabelHelpers as _, LifeGuard, MultiRefCount, RefCount, Stored,
@@ -130,7 +130,7 @@ impl RenderPassContext {
     }
 }
 
-pub type BufferMapPendingClosure = (BufferMapOperation, BufferMapAsyncStatus);
+pub type BufferMapPendingClosure = (BufferMapOperation, BufferAccessResult);
 
 #[derive(Default)]
 pub struct UserClosures {
@@ -3498,7 +3498,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
         buffer_id: id::BufferId,
         offset: BufferAddress,
         data: &[u8],
-    ) -> Result<(), BufferAccessError> {
+    ) -> BufferAccessResult {
         profiling::scope!("Device::set_buffer_sub_data");
 
         let hub = A::hub(self);
@@ -3555,7 +3555,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
         buffer_id: id::BufferId,
         offset: BufferAddress,
         data: &mut [u8],
-    ) -> Result<(), BufferAccessError> {
+    ) -> BufferAccessResult {
         profiling::scope!("Device::get_buffer_sub_data");
 
         let hub = A::hub(self);
@@ -5499,33 +5499,12 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
         buffer_id: id::BufferId,
         range: Range<BufferAddress>,
         op: BufferMapOperation,
-    ) -> Result<(), BufferAccessError> {
+    ) -> BufferAccessResult {
         // User callbacks must not be called while holding buffer_map_async_inner's locks, so we
         // defer the error callback if it needs to be called immediately (typically when running
         // into errors).
         if let Err((op, err)) = self.buffer_map_async_inner::<A>(buffer_id, range, op) {
-            let status = match &err {
-                &BufferAccessError::Device(_) => BufferMapAsyncStatus::ContextLost,
-                &BufferAccessError::Invalid | &BufferAccessError::Destroyed => {
-                    BufferMapAsyncStatus::Invalid
-                }
-                &BufferAccessError::AlreadyMapped => BufferMapAsyncStatus::AlreadyMapped,
-                &BufferAccessError::MapAlreadyPending => BufferMapAsyncStatus::MapAlreadyPending,
-                &BufferAccessError::MissingBufferUsage(_) => {
-                    BufferMapAsyncStatus::InvalidUsageFlags
-                }
-                &BufferAccessError::UnalignedRange
-                | &BufferAccessError::UnalignedRangeSize { .. }
-                | &BufferAccessError::UnalignedOffset { .. } => {
-                    BufferMapAsyncStatus::InvalidAlignment
-                }
-                &BufferAccessError::OutOfBoundsUnderrun { .. }
-                | &BufferAccessError::OutOfBoundsOverrun { .. }
-                | &BufferAccessError::NegativeRange { .. } => BufferMapAsyncStatus::InvalidRange,
-                _ => BufferMapAsyncStatus::Error,
-            };
-
-            op.callback.call(status);
+            op.callback.call(Err(err.clone()));
 
             return Err(err);
         }
@@ -5761,7 +5740,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
                 return Err(BufferAccessError::NotMapped);
             }
             resource::BufferMapState::Waiting(pending) => {
-                return Ok(Some((pending.op, resource::BufferMapAsyncStatus::Aborted)));
+                return Ok(Some((pending.op, Err(BufferAccessError::MapAborted))));
             }
             resource::BufferMapState::Active { ptr, range, host } => {
                 if host == HostMap::Write {
@@ -5792,10 +5771,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
         Ok(None)
     }
 
-    pub fn buffer_unmap<A: HalApi>(
-        &self,
-        buffer_id: id::BufferId,
-    ) -> Result<(), BufferAccessError> {
+    pub fn buffer_unmap<A: HalApi>(&self, buffer_id: id::BufferId) -> BufferAccessResult {
         profiling::scope!("unmap", "Buffer");
 
         let closure;

diff --git a/wgpu-core/src/resource.rs b/wgpu-core/src/resource.rs
@@ -15,7 +15,7 @@ use std::{borrow::Borrow, num::NonZeroU8, ops::Range, ptr::NonNull};
 
 /// The status code provided to the buffer mapping callback.
 ///
-/// This is very similar to `Result<(), BufferAccessError>`, except that this is FFI-friendly.
+/// This is very similar to `BufferAccessResult`, except that this is FFI-friendly.
 #[repr(C)]
 #[derive(Debug)]
 pub enum BufferMapAsyncStatus {
@@ -84,15 +84,15 @@ pub struct BufferMapCallback {
 
 enum BufferMapCallbackInner {
     Rust {
-        callback: Box<dyn FnOnce(BufferMapAsyncStatus) + Send + 'static>,
+        callback: Box<dyn FnOnce(BufferAccessResult) + Send + 'static>,
     },
     C {
         inner: BufferMapCallbackC,
     },
 }
 
 impl BufferMapCallback {
-    pub fn from_rust(callback: Box<dyn FnOnce(BufferMapAsyncStatus) + Send + 'static>) -> Self {
+    pub fn from_rust(callback: Box<dyn FnOnce(BufferAccessResult) + Send + 'static>) -> Self {
         Self {
             inner: Some(BufferMapCallbackInner::Rust { callback }),
         }
@@ -111,13 +111,39 @@ impl BufferMapCallback {
         }
     }
 
-    pub(crate) fn call(mut self, status: BufferMapAsyncStatus) {
+    pub(crate) fn call(mut self, result: BufferAccessResult) {
         match self.inner.take() {
             Some(BufferMapCallbackInner::Rust { callback }) => {
-                callback(status);
+                callback(result);
             }
             // SAFETY: the contract of the call to from_c says that this unsafe is sound.
             Some(BufferMapCallbackInner::C { inner }) => unsafe {
+                let status = match result {
+                    Ok(()) => BufferMapAsyncStatus::Success,
+                    Err(BufferAccessError::Device(_)) => BufferMapAsyncStatus::ContextLost,
+                    Err(BufferAccessError::Invalid) | Err(BufferAccessError::Destroyed) => {
+                        BufferMapAsyncStatus::Invalid
+                    }
+                    Err(BufferAccessError::AlreadyMapped) => BufferMapAsyncStatus::AlreadyMapped,
+                    Err(BufferAccessError::MapAlreadyPending) => {
+                        BufferMapAsyncStatus::MapAlreadyPending
+                    }
+                    Err(BufferAccessError::MissingBufferUsage(_)) => {
+                        BufferMapAsyncStatus::InvalidUsageFlags
+                    }
+                    Err(BufferAccessError::UnalignedRange)
+                    | Err(BufferAccessError::UnalignedRangeSize { .. })
+                    | Err(BufferAccessError::UnalignedOffset { .. }) => {
+                        BufferMapAsyncStatus::InvalidAlignment
+                    }
+                    Err(BufferAccessError::OutOfBoundsUnderrun { .. })
+                    | Err(BufferAccessError::OutOfBoundsOverrun { .. })
+                    | Err(BufferAccessError::NegativeRange { .. }) => {
+                        BufferMapAsyncStatus::InvalidRange
+                    }
+                    Err(_) => BufferMapAsyncStatus::Error,
+                };
+
                 (inner.callback)(status, inner.user_data);
             },
             None => {
@@ -144,6 +170,8 @@ pub struct BufferMapOperation {
 pub enum BufferAccessError {
     #[error(transparent)]
     Device(#[from] DeviceError),
+    #[error("buffer map failed")]
+    Failed,
     #[error("buffer is invalid")]
     Invalid,
     #[error("buffer is destroyed")]
@@ -181,8 +209,11 @@ pub enum BufferAccessError {
         start: wgt::BufferAddress,
         end: wgt::BufferAddress,
     },
+    #[error("buffer map aborted")]
+    MapAborted,
 }
 
+pub type BufferAccessResult = Result<(), BufferAccessError>;
 pub(crate) struct BufferPendingMapping {
     pub range: Range<wgt::BufferAddress>,
     pub op: BufferMapOperation,

diff --git a/wgpu/src/backend/direct.rs b/wgpu/src/backend/direct.rs
@@ -1701,10 +1701,7 @@ impl crate::Context for Context {
                 MapMode::Write => wgc::device::HostMap::Write,
             },
             callback: wgc::resource::BufferMapCallback::from_rust(Box::new(|status| {
-                let res = match status {
-                    wgc::resource::BufferMapAsyncStatus::Success => Ok(()),
-                    _ => Err(crate::BufferAsyncError),
-                };
+                let res = status.map_err(|_| crate::BufferAsyncError);
                 callback(res);
             })),
         };