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

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

Merged
merged 2 commits into from
Oct 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
10 changes: 3 additions & 7 deletions deno_webgpu/src/buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
{
Expand All @@ -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
Expand Down
7 changes: 3 additions & 4 deletions player/tests/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
6 changes: 3 additions & 3 deletions wgpu-core/src/device/life.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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));
}
Expand Down
42 changes: 9 additions & 33 deletions wgpu-core/src/device/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -129,7 +129,7 @@ impl RenderPassContext {
}
}

pub type BufferMapPendingClosure = (BufferMapOperation, BufferMapAsyncStatus);
pub type BufferMapPendingClosure = (BufferMapOperation, BufferAccessResult);

#[derive(Default)]
pub struct UserClosures {
Expand Down Expand Up @@ -3480,7 +3480,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);
Expand Down Expand Up @@ -3537,7 +3537,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);
Expand Down Expand Up @@ -5473,33 +5473,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);
}
Expand Down Expand Up @@ -5735,7 +5714,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 {
Expand Down Expand Up @@ -5766,10 +5745,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;
Expand Down
41 changes: 36 additions & 5 deletions wgpu-core/src/resource.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 }),
}
Expand All @@ -108,13 +108,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 => {
Expand All @@ -141,6 +167,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")]
Expand Down Expand Up @@ -178,8 +206,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,
Expand Down
5 changes: 1 addition & 4 deletions wgpu/src/backend/direct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
})),
};
Expand Down