Renderpass take resource ownership (gfx-rs#5884)

* share timestamp write struct * Make name of set_push_constants methods consistently plural * remove lifetime bounds of resources passed into render pass * first render pass resource ownership test * introduce dynrenderpass & immediately create ArcCommands and take ownership of resources passed on pass creation * Use of dynrenderpass in deno * Separate active occlusion & pipeline statitics query * resolve render/compute command is now behind `replay` feature * add vertex & index buffer to ownership test * test for pipeline statistics query * add occlusion query set to pass resource test * add tests for resource ownership of render pass query timestamps * RenderPass can now be made 'static just like ComputePass. Add respective test * Extend encoder_operations_fail_while_pass_alive test to also check encoder locking errors with render passes * improve changelog entry on lifetime bounds
Vecvec · Jul 1, 2024 · 0a76c0f · 0a76c0f
1 parent c9a2d97
commit 0a76c0f
Show file tree

Hide file tree

Showing 21 changed files with 2,135 additions and 786 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -41,24 +41,38 @@ Bottom level categories:
 
 ### Major Changes
 
-#### Remove lifetime bounds on `wgpu::ComputePass`
+#### Lifetime bounds on `wgpu::RenderPass` & `wgpu::ComputePass`
 
-TODO(wumpf): This is still work in progress. Should write a bit more about it. Also will very likely extend to `wgpu::RenderPass` before release.
+`wgpu::RenderPass` & `wgpu::ComputePass` recording methods (e.g. `wgpu::RenderPass:set_render_pipeline`) no longer impose a lifetime constraint to objects passed to a pass (like pipelines/buffers/bindgroups/query-sets etc.).
+
+This means the following pattern works now as expected:
+```rust
+let mut pipelines: Vec<wgpu::RenderPipeline> = ...;
+// ...
+let mut cpass = encoder.begin_compute_pass(&wgpu::ComputePassDescriptor::default());
+cpass.set_pipeline(&pipelines[123]);
+// Change pipeline container - this requires mutable access to `pipelines` while one of the pipelines is in use.
+pipelines.push(/* ... */);
+// Continue pass recording.
+cpass.set_bindgroup(...);
+```
+Previously, a set pipeline (or other resource) had to outlive pass recording which often affected wider systems,
+meaning that users needed to prove to the borrow checker that `Vec<wgpu::RenderPipeline>` (or similar constructs)
+aren't accessed mutably for the duration of pass recording.
 
-`wgpu::ComputePass` recording methods (e.g. `wgpu::ComputePass:set_render_pipeline`) no longer impose a lifetime constraint passed in resources.
 
-Furthermore, you can now opt out of `wgpu::ComputePass`'s lifetime dependency on its parent `wgpu::CommandEncoder` using `wgpu::ComputePass::forget_lifetime`:
+Furthermore, you can now opt out of `wgpu::RenderPass`/`wgpu::ComputePass`'s lifetime dependency on its parent `wgpu::CommandEncoder` using `wgpu::RenderPass::forget_lifetime`/`wgpu::ComputePass::forget_lifetime`:
 ```rust
 fn independent_cpass<'enc>(encoder: &'enc mut wgpu::CommandEncoder) -> wgpu::ComputePass<'static> {
     let cpass: wgpu::ComputePass<'enc> = encoder.begin_compute_pass(&wgpu::ComputePassDescriptor::default());
     cpass.forget_lifetime()
 }
 ```
-⚠️ As long as a `wgpu::ComputePass` is pending for a given `wgpu::CommandEncoder`, creation of a compute or render pass is an error and invalidates the `wgpu::CommandEncoder`.
-This is very useful for library authors, but opens up an easy way for incorrect use, so use with care.
-`forget_lifetime` is zero overhead and has no side effects on pass recording.
+⚠️ As long as a `wgpu::RenderPass`/`wgpu::ComputePass` is pending for a given `wgpu::CommandEncoder`, creation of a compute or render pass is an error and invalidates the `wgpu::CommandEncoder`.
+`forget_lifetime` can be very useful for library authors, but opens up an easy way for incorrect use, so use with care.
+This method doesn't add any additional overhead and has no side effects on pass recording.
 
-By @wumpf in [#5569](https://github.com/gfx-rs/wgpu/pull/5569), [#5575](https://github.com/gfx-rs/wgpu/pull/5575), [#5620](https://github.com/gfx-rs/wgpu/pull/5620), [#5768](https://github.com/gfx-rs/wgpu/pull/5768) (together with @kpreid), [#5671](https://github.com/gfx-rs/wgpu/pull/5671).
+By @wumpf in [#5569](https://github.com/gfx-rs/wgpu/pull/5569), [#5575](https://github.com/gfx-rs/wgpu/pull/5575), [#5620](https://github.com/gfx-rs/wgpu/pull/5620), [#5768](https://github.com/gfx-rs/wgpu/pull/5768) (together with @kpreid), [#5671](https://github.com/gfx-rs/wgpu/pull/5671), [#5794](https://github.com/gfx-rs/wgpu/pull/5794), [#5884](https://github.com/gfx-rs/wgpu/pull/5884).
 
 #### Querying shader compilation errors
 

diff --git a/deno_webgpu/command_encoder.rs b/deno_webgpu/command_encoder.rs
@@ -186,7 +186,7 @@ pub fn op_webgpu_command_encoder_begin_render_pass(
             .get::<WebGpuQuerySet>(timestamp_writes.query_set)?;
         let query_set = query_set_resource.1;
 
-        Some(wgpu_core::command::RenderPassTimestampWrites {
+        Some(wgpu_core::command::PassTimestampWrites {
             query_set,
             beginning_of_pass_write_index: timestamp_writes.beginning_of_pass_write_index,
             end_of_pass_write_index: timestamp_writes.end_of_pass_write_index,
@@ -200,6 +200,8 @@ pub fn op_webgpu_command_encoder_begin_render_pass(
         .transpose()?
         .map(|query_set| query_set.1);
 
+    let instance = state.borrow::<super::Instance>();
+    let command_encoder = &command_encoder_resource.1;
     let descriptor = wgpu_core::command::RenderPassDescriptor {
         label: Some(label),
         color_attachments: Cow::from(color_attachments),
@@ -208,15 +210,14 @@ pub fn op_webgpu_command_encoder_begin_render_pass(
         occlusion_query_set: occlusion_query_set_resource,
     };
 
-    let render_pass = wgpu_core::command::RenderPass::new(command_encoder_resource.1, &descriptor);
-
+    let (render_pass, error) = gfx_select!(command_encoder => instance.command_encoder_create_render_pass_dyn(*command_encoder, &descriptor));
     let rid = state
         .resource_table
         .add(super::render_pass::WebGpuRenderPass(RefCell::new(
             render_pass,
         )));
 
-    Ok(WebGpuResult::rid(rid))
+    Ok(WebGpuResult::rid_err(rid, error))
 }
 
 #[derive(Deserialize)]
@@ -245,7 +246,7 @@ pub fn op_webgpu_command_encoder_begin_compute_pass(
             .get::<WebGpuQuerySet>(timestamp_writes.query_set)?;
         let query_set = query_set_resource.1;
 
-        Some(wgpu_core::command::ComputePassTimestampWrites {
+        Some(wgpu_core::command::PassTimestampWrites {
             query_set,
             beginning_of_pass_write_index: timestamp_writes.beginning_of_pass_write_index,
             end_of_pass_write_index: timestamp_writes.end_of_pass_write_index,

diff --git a/deno_webgpu/render_pass.rs b/deno_webgpu/render_pass.rs
@@ -9,11 +9,10 @@ use deno_core::ResourceId;
 use serde::Deserialize;
 use std::borrow::Cow;
 use std::cell::RefCell;
-use wgpu_core::global::Global;
 
 use super::error::WebGpuResult;
 
-pub(crate) struct WebGpuRenderPass(pub(crate) RefCell<wgpu_core::command::RenderPass>);
+pub(crate) struct WebGpuRenderPass(pub(crate) RefCell<Box<dyn wgpu_core::command::DynRenderPass>>);
 impl Resource for WebGpuRenderPass {
     fn name(&self) -> Cow<str> {
         "webGPURenderPass".into()
@@ -42,8 +41,8 @@ pub fn op_webgpu_render_pass_set_viewport(
         .resource_table
         .get::<WebGpuRenderPass>(args.render_pass_rid)?;
 
-    state.borrow::<Global>().render_pass_set_viewport(
-        &mut render_pass_resource.0.borrow_mut(),
+    render_pass_resource.0.borrow_mut().set_viewport(
+        state.borrow(),
         args.x,
         args.y,
         args.width,
@@ -69,13 +68,10 @@ pub fn op_webgpu_render_pass_set_scissor_rect(
         .resource_table
         .get::<WebGpuRenderPass>(render_pass_rid)?;
 
-    state.borrow::<Global>().render_pass_set_scissor_rect(
-        &mut render_pass_resource.0.borrow_mut(),
-        x,
-        y,
-        width,
-        height,
-    )?;
+    render_pass_resource
+        .0
+        .borrow_mut()
+        .set_scissor_rect(state.borrow(), x, y, width, height)?;
 
     Ok(WebGpuResult::empty())
 }
@@ -91,9 +87,10 @@ pub fn op_webgpu_render_pass_set_blend_constant(
         .resource_table
         .get::<WebGpuRenderPass>(render_pass_rid)?;
 
-    state
-        .borrow::<Global>()
-        .render_pass_set_blend_constant(&mut render_pass_resource.0.borrow_mut(), &color)?;
+    render_pass_resource
+        .0
+        .borrow_mut()
+        .set_blend_constant(state.borrow(), color)?;
 
     Ok(WebGpuResult::empty())
 }
@@ -109,9 +106,10 @@ pub fn op_webgpu_render_pass_set_stencil_reference(
         .resource_table
         .get::<WebGpuRenderPass>(render_pass_rid)?;
 
-    state
-        .borrow::<Global>()
-        .render_pass_set_stencil_reference(&mut render_pass_resource.0.borrow_mut(), reference)?;
+    render_pass_resource
+        .0
+        .borrow_mut()
+        .set_stencil_reference(state.borrow(), reference)?;
 
     Ok(WebGpuResult::empty())
 }
@@ -127,9 +125,10 @@ pub fn op_webgpu_render_pass_begin_occlusion_query(
         .resource_table
         .get::<WebGpuRenderPass>(render_pass_rid)?;
 
-    state
-        .borrow::<Global>()
-        .render_pass_begin_occlusion_query(&mut render_pass_resource.0.borrow_mut(), query_index)?;
+    render_pass_resource
+        .0
+        .borrow_mut()
+        .begin_occlusion_query(state.borrow(), query_index)?;
 
     Ok(WebGpuResult::empty())
 }
@@ -144,9 +143,10 @@ pub fn op_webgpu_render_pass_end_occlusion_query(
         .resource_table
         .get::<WebGpuRenderPass>(render_pass_rid)?;
 
-    state
-        .borrow::<Global>()
-        .render_pass_end_occlusion_query(&mut render_pass_resource.0.borrow_mut())?;
+    render_pass_resource
+        .0
+        .borrow_mut()
+        .end_occlusion_query(state.borrow())?;
 
     Ok(WebGpuResult::empty())
 }
@@ -172,9 +172,10 @@ pub fn op_webgpu_render_pass_execute_bundles(
         .resource_table
         .get::<WebGpuRenderPass>(render_pass_rid)?;
 
-    state
-        .borrow::<Global>()
-        .render_pass_execute_bundles(&mut render_pass_resource.0.borrow_mut(), &bundles)?;
+    render_pass_resource
+        .0
+        .borrow_mut()
+        .execute_bundles(state.borrow(), &bundles)?;
 
     Ok(WebGpuResult::empty())
 }
@@ -189,12 +190,7 @@ pub fn op_webgpu_render_pass_end(
         .resource_table
         .take::<WebGpuRenderPass>(render_pass_rid)?;
 
-    // TODO: Just like parent_id ComputePass, there's going to be DynComputePass soon which will eliminate the need of doing gfx_select here.
-    let instance = state.borrow::<Global>();
-    let parent_id = render_pass_resource.0.borrow().parent_id();
-    gfx_select!(parent_id => instance.render_pass_end(
-        &mut render_pass_resource.0.borrow_mut()
-    ))?;
+    render_pass_resource.0.borrow_mut().end(state.borrow())?;
 
     Ok(WebGpuResult::empty())
 }
@@ -226,8 +222,8 @@ pub fn op_webgpu_render_pass_set_bind_group(
 
     let dynamic_offsets_data: &[u32] = &dynamic_offsets_data[start..start + len];
 
-    state.borrow::<Global>().render_pass_set_bind_group(
-        &mut render_pass_resource.0.borrow_mut(),
+    render_pass_resource.0.borrow_mut().set_bind_group(
+        state.borrow(),
         index,
         bind_group_resource.1,
         dynamic_offsets_data,
@@ -247,8 +243,8 @@ pub fn op_webgpu_render_pass_push_debug_group(
         .resource_table
         .get::<WebGpuRenderPass>(render_pass_rid)?;
 
-    state.borrow::<Global>().render_pass_push_debug_group(
-        &mut render_pass_resource.0.borrow_mut(),
+    render_pass_resource.0.borrow_mut().push_debug_group(
+        state.borrow(),
         group_label,
         0, // wgpu#975
     )?;
@@ -266,9 +262,10 @@ pub fn op_webgpu_render_pass_pop_debug_group(
         .resource_table
         .get::<WebGpuRenderPass>(render_pass_rid)?;
 
-    state
-        .borrow::<Global>()
-        .render_pass_pop_debug_group(&mut render_pass_resource.0.borrow_mut())?;
+    render_pass_resource
+        .0
+        .borrow_mut()
+        .pop_debug_group(state.borrow())?;
 
     Ok(WebGpuResult::empty())
 }
@@ -284,8 +281,8 @@ pub fn op_webgpu_render_pass_insert_debug_marker(
         .resource_table
         .get::<WebGpuRenderPass>(render_pass_rid)?;
 
-    state.borrow::<Global>().render_pass_insert_debug_marker(
-        &mut render_pass_resource.0.borrow_mut(),
+    render_pass_resource.0.borrow_mut().insert_debug_marker(
+        state.borrow(),
         marker_label,
         0, // wgpu#975
     )?;
@@ -307,10 +304,10 @@ pub fn op_webgpu_render_pass_set_pipeline(
         .resource_table
         .get::<WebGpuRenderPass>(render_pass_rid)?;
 
-    state.borrow::<Global>().render_pass_set_pipeline(
-        &mut render_pass_resource.0.borrow_mut(),
-        render_pipeline_resource.1,
-    )?;
+    render_pass_resource
+        .0
+        .borrow_mut()
+        .set_pipeline(state.borrow(), render_pipeline_resource.1)?;
 
     Ok(WebGpuResult::empty())
 }
@@ -341,8 +338,8 @@ pub fn op_webgpu_render_pass_set_index_buffer(
         None
     };
 
-    state.borrow::<Global>().render_pass_set_index_buffer(
-        &mut render_pass_resource.0.borrow_mut(),
+    render_pass_resource.0.borrow_mut().set_index_buffer(
+        state.borrow(),
         buffer_resource.1,
         index_format,
         offset,
@@ -378,8 +375,8 @@ pub fn op_webgpu_render_pass_set_vertex_buffer(
         None
     };
 
-    state.borrow::<Global>().render_pass_set_vertex_buffer(
-        &mut render_pass_resource.0.borrow_mut(),
+    render_pass_resource.0.borrow_mut().set_vertex_buffer(
+        state.borrow(),
         slot,
         buffer_resource.1,
         offset,
@@ -403,8 +400,8 @@ pub fn op_webgpu_render_pass_draw(
         .resource_table
         .get::<WebGpuRenderPass>(render_pass_rid)?;
 
-    state.borrow::<Global>().render_pass_draw(
-        &mut render_pass_resource.0.borrow_mut(),
+    render_pass_resource.0.borrow_mut().draw(
+        state.borrow(),
         vertex_count,
         instance_count,
         first_vertex,
@@ -429,8 +426,8 @@ pub fn op_webgpu_render_pass_draw_indexed(
         .resource_table
         .get::<WebGpuRenderPass>(render_pass_rid)?;
 
-    state.borrow::<Global>().render_pass_draw_indexed(
-        &mut render_pass_resource.0.borrow_mut(),
+    render_pass_resource.0.borrow_mut().draw_indexed(
+        state.borrow(),
         index_count,
         instance_count,
         first_index,
@@ -456,8 +453,8 @@ pub fn op_webgpu_render_pass_draw_indirect(
         .resource_table
         .get::<WebGpuRenderPass>(render_pass_rid)?;
 
-    state.borrow::<Global>().render_pass_draw_indirect(
-        &mut render_pass_resource.0.borrow_mut(),
+    render_pass_resource.0.borrow_mut().draw_indirect(
+        state.borrow(),
         buffer_resource.1,
         indirect_offset,
     )?;
@@ -480,8 +477,8 @@ pub fn op_webgpu_render_pass_draw_indexed_indirect(
         .resource_table
         .get::<WebGpuRenderPass>(render_pass_rid)?;
 
-    state.borrow::<Global>().render_pass_draw_indexed_indirect(
-        &mut render_pass_resource.0.borrow_mut(),
+    render_pass_resource.0.borrow_mut().draw_indexed_indirect(
+        state.borrow(),
         buffer_resource.1,
         indirect_offset,
     )?;

diff --git a/tests/tests/compute_pass_ownership.rs b/tests/tests/compute_pass_ownership.rs
@@ -111,14 +111,14 @@ async fn compute_pass_query_set_ownership_pipeline_statistics(ctx: TestingContex
 }
 
 #[gpu_test]
-static COMPUTE_PASS_QUERY_TIMESTAMPS: GpuTestConfiguration =
+static COMPUTE_PASS_QUERY_SET_OWNERSHIP_TIMESTAMPS: GpuTestConfiguration =
     GpuTestConfiguration::new()
         .parameters(TestParameters::default().test_features_limits().features(
             wgpu::Features::TIMESTAMP_QUERY | wgpu::Features::TIMESTAMP_QUERY_INSIDE_PASSES,
         ))
-        .run_async(compute_pass_query_timestamps);
+        .run_async(compute_pass_query_set_ownership_timestamps);
 
-async fn compute_pass_query_timestamps(ctx: TestingContext) {
+async fn compute_pass_query_set_ownership_timestamps(ctx: TestingContext) {
     let ResourceSetup {
         gpu_buffer,
         cpu_buffer,