From d1ff3838b807cd4a04fd68154d90a75ea423fa00 Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Thu, 13 Oct 2022 21:41:52 +0200 Subject: [PATCH] Expose the full Result type to the rust map_async callback (#2939) --- CHANGELOG.md | 1 + deno_webgpu/src/buffer.rs | 10 +++------ player/tests/test.rs | 7 +++--- wgpu-core/src/device/life.rs | 6 +++--- wgpu-core/src/device/mod.rs | 42 ++++++++---------------------------- wgpu-core/src/resource.rs | 41 ++++++++++++++++++++++++++++++----- wgpu/src/backend/direct.rs | 5 +---- 7 files changed, 56 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ba1c3cd815..5b243a6c4e 100644 --- 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 index 250950f4e1..738760606c 100644 --- 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 { - let (sender, receiver) = oneshot::channel::>(); + let (sender, receiver) = oneshot::channel::(); 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 index 0ced0fad8c..06da011a49 100644 --- a/player/tests/test.rs +++ b/player/tests/test.rs @@ -55,10 +55,9 @@ struct Test<'a> { actions: Vec>, } -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 index c06349dc4f..bf8b3c86b9 100644 --- a/wgpu-core/src/device/life.rs +++ b/wgpu-core/src/device/life.rs @@ -887,11 +887,11 @@ impl LifetimeTracker { 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 LifetimeTracker { 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 index ab209699f6..57c6b943fc 100644 --- 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 Global { 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 Global { 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 Global { buffer_id: id::BufferId, range: Range, 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::(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 Global { 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 Global { Ok(None) } - pub fn buffer_unmap( - &self, - buffer_id: id::BufferId, - ) -> Result<(), BufferAccessError> { + pub fn buffer_unmap(&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 index e4d7d90828..85d4d97157 100644 --- 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,7 +84,7 @@ pub struct BufferMapCallback { enum BufferMapCallbackInner { Rust { - callback: Box, + callback: Box, }, C { inner: BufferMapCallbackC, @@ -92,7 +92,7 @@ enum BufferMapCallbackInner { } impl BufferMapCallback { - pub fn from_rust(callback: Box) -> Self { + pub fn from_rust(callback: Box) -> 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, pub op: BufferMapOperation, diff --git a/wgpu/src/backend/direct.rs b/wgpu/src/backend/direct.rs index 3d33e2c768..06fcbb6f36 100644 --- 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); })), };