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.

Sign up for GitHub

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

Jump to bottom

Fix Javascript exception on repeated BufferSlice::get_mapped_range calls #4726

Merged
merged 7 commits into from
Dec 6, 2023
Merged

Fix Javascript exception on repeated BufferSlice::get_mapped_range calls #4726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8d3d7ec
Add reusable buffer mappings for WASM
DouglasDwyer Nov 20, 2023
620ee5d
Run cargo fmt
DouglasDwyer Nov 20, 2023
2cff52a
Update CHANGELOG.md
DouglasDwyer Nov 20, 2023
9d5a386
Merge remote-tracking branch 'upstream/trunk' into trunk
DouglasDwyer Dec 2, 2023
cb2f904
Update web.rs
DouglasDwyer Dec 2, 2023
dca0aea
Add documentation for WebBuffer struct
DouglasDwyer Dec 3, 2023
5dcdd3d
Merge remote-tracking branch 'upstream/trunk' into trunk
DouglasDwyer Dec 6, 2023
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,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
Open in desktop
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 @@ -1615,7 +1615,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 @@ -1818,7 +1819,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 @@ -2026,12 +2030,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 @@ -2041,9 +2047,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 @@ -2057,14 +2061,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 @@ -2104,7 +2106,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 @@ -2240,9 +2242,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 @@ -2456,14 +2458,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 @@ -2525,7 +2527,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 @@ -2567,7 +2569,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 @@ -2584,7 +2586,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 @@ -2605,8 +2607,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 @@ -2860,9 +2862,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 @@ -2914,15 +2917,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 @@ -2944,15 +2947,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 @@ -3016,7 +3019,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 @@ -3029,7 +3032,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 @@ -3135,15 +3138,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 @@ -3165,15 +3168,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 @@ -3235,7 +3240,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 @@ -3248,7 +3253,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 @@ -3466,6 +3471,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 {
DouglasDwyer marked this conversation as resolved.
Show resolved Hide resolved
/// 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
Loading