Skip to content

Commit

Permalink
Fix Javascript exception on repeated BufferSlice::get_mapped_range …
Browse files Browse the repository at this point in the history 
…calls (gfx-rs#4726)

* Add reusable buffer mappings for WASM

* Run cargo fmt

* Update CHANGELOG.md

* Update web.rs

* Add documentation for WebBuffer struct
  • Loading branch information
@DouglasDwyer @bradwerth
DouglasDwyer authored and bradwerth committed Dec 8, 2023
1 parent 6f75c54 commit 17ed636
Show file tree
Hide file tree
Showing 2 changed files with 119 additions and 45 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,10 @@ Passing an owned value `window` to `Surface` will return a `Surface<'static>`. S

### Bug Fixes

#### WebGPU

- Allow calling `BufferSlice::get_mapped_range` multiple times on the same buffer slice (instead of throwing a Javascript exception): By @DouglasDwyer in [#4726](https://github.com/gfx-rs/wgpu/pull/4726)

#### WGL

- Create a hidden window per `wgpu::Instance` instead of sharing a global one.
Expand Down
160 changes: 115 additions & 45 deletions wgpu/src/backend/web.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ fn map_texture_view_dimension(

fn map_buffer_copy_view(view: crate::ImageCopyBuffer<'_>) -> web_sys::GpuImageCopyBuffer {
let buffer: &<Context as crate::Context>::BufferData = downcast_ref(view.buffer.data.as_ref());
let mut mapped = web_sys::GpuImageCopyBuffer::new(&buffer.0);
let mut mapped = web_sys::GpuImageCopyBuffer::new(&buffer.0.buffer);
if let Some(bytes_per_row) = view.layout.bytes_per_row {
mapped.bytes_per_row(bytes_per_row);
}
Expand Down Expand Up @@ -1022,8 +1022,8 @@ impl crate::context::Context for Context {
type TextureViewData = Sendable<web_sys::GpuTextureView>;
type SamplerId = Identified<web_sys::GpuSampler>;
type SamplerData = Sendable<web_sys::GpuSampler>;
type BufferId = Identified<web_sys::GpuBuffer>;
type BufferData = Sendable<web_sys::GpuBuffer>;
type BufferId = Identified<WebBuffer>;
type BufferData = Sendable<WebBuffer>;
type TextureId = Identified<web_sys::GpuTexture>;
type TextureData = Sendable<web_sys::GpuTexture>;
type QuerySetId = Identified<web_sys::GpuQuerySet>;
Expand Down Expand Up @@ -1616,7 +1616,8 @@ impl crate::context::Context for Context {
}) => {
let buffer: &<Context as crate::Context>::BufferData =
downcast_ref(buffer.data.as_ref());
let mut mapped_buffer_binding = web_sys::GpuBufferBinding::new(&buffer.0);
let mut mapped_buffer_binding =
web_sys::GpuBufferBinding::new(&buffer.0.buffer);
mapped_buffer_binding.offset(offset as f64);
if let Some(s) = size {
mapped_buffer_binding.size(s.get() as f64);
Expand Down Expand Up @@ -1819,7 +1820,10 @@ impl crate::context::Context for Context {
if let Some(label) = desc.label {
mapped_desc.label(label);
}
create_identified(device_data.0.create_buffer(&mapped_desc))
create_identified(WebBuffer::new(
device_data.0.create_buffer(&mapped_desc),
desc,
))
}

fn device_create_texture(
Expand Down Expand Up @@ -2027,12 +2031,14 @@ impl crate::context::Context for Context {
range: Range<wgt::BufferAddress>,
callback: crate::context::BufferMapCallback,
) {
let map_promise = buffer_data.0.map_async_with_f64_and_f64(
let map_promise = buffer_data.0.buffer.map_async_with_f64_and_f64(
map_map_mode(mode),
range.start as f64,
(range.end - range.start) as f64,
);

buffer_data.0.set_mapped_range(range);

register_then_closures(&map_promise, callback, Ok(()), Err(crate::BufferAsyncError));
}

Expand All @@ -2042,9 +2048,7 @@ impl crate::context::Context for Context {
buffer_data: &Self::BufferData,
sub_range: Range<wgt::BufferAddress>,
) -> Box<dyn crate::context::BufferMappedRange> {
let array_buffer =
self.buffer_get_mapped_range_as_array_buffer(_buffer, buffer_data, sub_range);
let actual_mapping = js_sys::Uint8Array::new(&array_buffer);
let actual_mapping = buffer_data.0.get_mapped_range(sub_range);
let temporary_mapping = actual_mapping.to_vec();
Box::new(BufferMappedRange {
actual_mapping,
Expand All @@ -2058,14 +2062,12 @@ impl crate::context::Context for Context {
buffer_data: &Self::BufferData,
sub_range: Range<wgt::BufferAddress>,
) -> js_sys::ArrayBuffer {
buffer_data.0.get_mapped_range_with_f64_and_f64(
sub_range.start as f64,
(sub_range.end - sub_range.start) as f64,
)
buffer_data.0.get_mapped_array_buffer(sub_range)
}

fn buffer_unmap(&self, _buffer: &Self::BufferId, buffer_data: &Self::BufferData) {
buffer_data.0.unmap();
buffer_data.0.buffer.unmap();
buffer_data.0.mapping.borrow_mut().mapped_buffer = None;
}

fn texture_create_view(
Expand Down Expand Up @@ -2105,7 +2107,7 @@ impl crate::context::Context for Context {
}

fn buffer_destroy(&self, _buffer: &Self::BufferId, buffer_data: &Self::BufferData) {
buffer_data.0.destroy();
buffer_data.0.buffer.destroy();
}

fn buffer_drop(&self, _buffer: &Self::BufferId, _buffer_data: &Self::BufferData) {
Expand Down Expand Up @@ -2241,9 +2243,9 @@ impl crate::context::Context for Context {
encoder_data
.0
.copy_buffer_to_buffer_with_f64_and_f64_and_f64(
&source_data.0,
&source_data.0.buffer,
source_offset as f64,
&destination_data.0,
&destination_data.0.buffer,
destination_offset as f64,
copy_size as f64,
)
Expand Down Expand Up @@ -2457,14 +2459,14 @@ impl crate::context::Context for Context {
) {
let buffer: &<Context as crate::Context>::BufferData = downcast_ref(buffer.data.as_ref());
match size {
Some(size) => {
encoder_data
.0
.clear_buffer_with_f64_and_f64(&buffer.0, offset as f64, size as f64)
}
Some(size) => encoder_data.0.clear_buffer_with_f64_and_f64(
&buffer.0.buffer,
offset as f64,
size as f64,
),
None => encoder_data
.0
.clear_buffer_with_f64(&buffer.0, offset as f64),
.clear_buffer_with_f64(&buffer.0.buffer, offset as f64),
}
}

Expand Down Expand Up @@ -2526,7 +2528,7 @@ impl crate::context::Context for Context {
&query_set_data.0,
first_query,
query_count,
&destination_data.0,
&destination_data.0.buffer,
destination_offset as u32,
);
}
Expand Down Expand Up @@ -2568,7 +2570,7 @@ impl crate::context::Context for Context {
queue_data
.0
.write_buffer_with_f64_and_buffer_source_and_f64_and_f64(
&buffer_data.0,
&buffer_data.0.buffer,
offset as f64,
&js_sys::Uint8Array::from(data).buffer(),
0f64,
Expand All @@ -2585,7 +2587,7 @@ impl crate::context::Context for Context {
offset: wgt::BufferAddress,
size: wgt::BufferSize,
) -> Option<()> {
let usage = wgt::BufferUsages::from_bits_truncate(buffer_data.0.usage());
let usage = wgt::BufferUsages::from_bits_truncate(buffer_data.0.buffer.usage());
// TODO: actually send this down the error scope
if !usage.contains(wgt::BufferUsages::COPY_DST) {
log::error!("Destination buffer is missing the `COPY_DST` usage flag");
Expand All @@ -2606,8 +2608,8 @@ impl crate::context::Context for Context {
);
return None;
}
if write_size + offset > buffer_data.0.size() as u64 {
log::error!("copy of {}..{} would end up overrunning the bounds of the destination buffer of size {}", offset, offset + write_size, buffer_data.0.size());
if write_size + offset > buffer_data.0.buffer.size() as u64 {
log::error!("copy of {}..{} would end up overrunning the bounds of the destination buffer of size {}", offset, offset + write_size, buffer_data.0.buffer.size());
return None;
}
Some(())
Expand Down Expand Up @@ -2861,9 +2863,10 @@ impl crate::context::Context for Context {
indirect_buffer_data: &Self::BufferData,
indirect_offset: wgt::BufferAddress,
) {
pass_data
.0
.dispatch_workgroups_indirect_with_f64(&indirect_buffer_data.0, indirect_offset as f64);
pass_data.0.dispatch_workgroups_indirect_with_f64(
&indirect_buffer_data.0.buffer,
indirect_offset as f64,
);
}

fn render_bundle_encoder_set_pipeline(
Expand Down Expand Up @@ -2915,15 +2918,15 @@ impl crate::context::Context for Context {
match size {
Some(s) => {
encoder_data.0.set_index_buffer_with_f64_and_f64(
&buffer_data.0,
&buffer_data.0.buffer,
map_index_format(index_format),
offset as f64,
s.get() as f64,
);
}
None => {
encoder_data.0.set_index_buffer_with_f64(
&buffer_data.0,
&buffer_data.0.buffer,
map_index_format(index_format),
offset as f64,
);
Expand All @@ -2945,15 +2948,15 @@ impl crate::context::Context for Context {
Some(s) => {
encoder_data.0.set_vertex_buffer_with_f64_and_f64(
slot,
Some(&buffer_data.0),
Some(&buffer_data.0.buffer),
offset as f64,
s.get() as f64,
);
}
None => {
encoder_data.0.set_vertex_buffer_with_f64(
slot,
Some(&buffer_data.0),
Some(&buffer_data.0.buffer),
offset as f64,
);
}
Expand Down Expand Up @@ -3017,7 +3020,7 @@ impl crate::context::Context for Context {
) {
encoder_data
.0
.draw_indirect_with_f64(&indirect_buffer_data.0, indirect_offset as f64);
.draw_indirect_with_f64(&indirect_buffer_data.0.buffer, indirect_offset as f64);
}

fn render_bundle_encoder_draw_indexed_indirect(
Expand All @@ -3030,7 +3033,7 @@ impl crate::context::Context for Context {
) {
encoder_data
.0
.draw_indexed_indirect_with_f64(&indirect_buffer_data.0, indirect_offset as f64);
.draw_indexed_indirect_with_f64(&indirect_buffer_data.0.buffer, indirect_offset as f64);
}

fn render_bundle_encoder_multi_draw_indirect(
Expand Down Expand Up @@ -3136,15 +3139,15 @@ impl crate::context::Context for Context {
match size {
Some(s) => {
pass_data.0.set_index_buffer_with_f64_and_f64(
&buffer_data.0,
&buffer_data.0.buffer,
map_index_format(index_format),
offset as f64,
s.get() as f64,
);
}
None => {
pass_data.0.set_index_buffer_with_f64(
&buffer_data.0,
&buffer_data.0.buffer,
map_index_format(index_format),
offset as f64,
);
Expand All @@ -3166,15 +3169,17 @@ impl crate::context::Context for Context {
Some(s) => {
pass_data.0.set_vertex_buffer_with_f64_and_f64(
slot,
Some(&buffer_data.0),
Some(&buffer_data.0.buffer),
offset as f64,
s.get() as f64,
);
}
None => {
pass_data
.0
.set_vertex_buffer_with_f64(slot, Some(&buffer_data.0), offset as f64);
pass_data.0.set_vertex_buffer_with_f64(
slot,
Some(&buffer_data.0.buffer),
offset as f64,
);
}
};
}
Expand Down Expand Up @@ -3236,7 +3241,7 @@ impl crate::context::Context for Context {
) {
pass_data
.0
.draw_indirect_with_f64(&indirect_buffer_data.0, indirect_offset as f64);
.draw_indirect_with_f64(&indirect_buffer_data.0.buffer, indirect_offset as f64);
}

fn render_pass_draw_indexed_indirect(
Expand All @@ -3249,7 +3254,7 @@ impl crate::context::Context for Context {
) {
pass_data
.0
.draw_indexed_indirect_with_f64(&indirect_buffer_data.0, indirect_offset as f64);
.draw_indexed_indirect_with_f64(&indirect_buffer_data.0.buffer, indirect_offset as f64);
}

fn render_pass_multi_draw_indirect(
Expand Down Expand Up @@ -3467,6 +3472,71 @@ impl QueueWriteBuffer for WebQueueWriteBuffer {
}
}

/// Stores the state of a GPU buffer and a reference to its mapped `ArrayBuffer` (if any).
/// The WebGPU specification forbids calling `getMappedRange` on a `web_sys::GpuBuffer` more than
/// once, so this struct stores the initial mapped range and re-uses it, allowing for multiple `get_mapped_range`
/// calls on the Rust-side.
#[derive(Debug)]
pub struct WebBuffer {
/// The associated GPU buffer.
buffer: web_sys::GpuBuffer,
/// The mapped array buffer and mapped range.
mapping: RefCell<WebBufferMapState>,
}

impl WebBuffer {
/// Creates a new web buffer for the given Javascript object and description.
fn new(buffer: web_sys::GpuBuffer, desc: &crate::BufferDescriptor<'_>) -> Self {
Self {
buffer,
mapping: RefCell::new(WebBufferMapState {
mapped_buffer: None,
range: 0..desc.size,
}),
}
}

/// Creates a raw Javascript array buffer over the provided range.
fn get_mapped_array_buffer(&self, sub_range: Range<wgt::BufferAddress>) -> js_sys::ArrayBuffer {
self.buffer.get_mapped_range_with_f64_and_f64(
sub_range.start as f64,
(sub_range.end - sub_range.start) as f64,
)
}

/// Obtains a reference to the re-usable buffer mapping as a Javascript array view.
fn get_mapped_range(&self, sub_range: Range<wgt::BufferAddress>) -> js_sys::Uint8Array {
let mut mapping = self.mapping.borrow_mut();
let range = mapping.range.clone();
let array_buffer = mapping.mapped_buffer.get_or_insert_with(|| {
self.buffer.get_mapped_range_with_f64_and_f64(
range.start as f64,
(range.end - range.start) as f64,
)
});
js_sys::Uint8Array::new_with_byte_offset_and_length(
array_buffer,
(sub_range.start - range.start) as u32,
(sub_range.end - sub_range.start) as u32,
)
}

/// Sets the range of the buffer which is presently mapped.
fn set_mapped_range(&self, range: Range<wgt::BufferAddress>) {
self.mapping.borrow_mut().range = range;
}
}

/// Remembers which portion of a buffer has been mapped, along with a reference
/// to the mapped portion.
#[derive(Debug)]
struct WebBufferMapState {
/// The mapped memory of the buffer.
pub mapped_buffer: Option<js_sys::ArrayBuffer>,
/// The total range which has been mapped in the buffer overall.
pub range: Range<wgt::BufferAddress>,
}

#[derive(Debug)]
pub struct BufferMappedRange {
actual_mapping: js_sys::Uint8Array,
Expand Down

0 comments on commit 17ed636

Please sign in to comment.