Remove lifetime dependency of ComputePass to its parent command enc…

…oder (#5620)

* lift encoder->computepass lifetime constraint and add now failing test
* compute passes now take an arc to their parent command encoder, thus removing compile time dependency to it
* Command encoder goes now into locked state while compute pass is open
* changelog entry
* share most of the code between get_encoder and lock_encoder

CHANGELOG.md

@@ -47,7 +47,13 @@ TODO(wumpf): This is still work in progress. Should write a bit more about it. A
 
 `wgpu::ComputePass` recording methods (e.g. `wgpu::ComputePass:set_render_pipeline`) no longer impose a lifetime constraint passed in resources.
 
-By @wumpf in [#5569](https://github.com/gfx-rs/wgpu/pull/5569), [#5575](https://github.com/gfx-rs/wgpu/pull/5575).
+Furthermore, `wgpu::ComputePass` no longer has a life time dependency on its parent `wgpu::CommandEncoder`.
+⚠️ 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`.
+Previously, this was statically enforced by a lifetime constraint.
+TODO(wumpf): There was some discussion on whether to make this life time constraint opt-in or opt-out (entirely on `wgpu` side, no changes to `wgpu-core`).
+Lifting this lifetime dependencies is very useful for library authors, but opens up an easy way for incorrect use.
+
+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).
 
 #### Querying shader compilation errors
 

deno_webgpu/command_encoder.rs

@@ -261,15 +261,14 @@ pub fn op_webgpu_command_encoder_begin_compute_pass(
         timestamp_writes: timestamp_writes.as_ref(),
     };
 
-    let compute_pass = gfx_select!(command_encoder => instance.command_encoder_create_compute_pass_dyn(*command_encoder, &descriptor));
-
+    let (compute_pass, error) = gfx_select!(command_encoder => instance.command_encoder_create_compute_pass_dyn(*command_encoder, &descriptor));
     let rid = state
         .resource_table
         .add(super::compute_pass::WebGpuComputePass(RefCell::new(
             compute_pass,
         )));
 
-    Ok(WebGpuResult::rid(rid))
+    Ok(WebGpuResult::rid_err(rid, error))
 }
 
 #[op2]

tests/tests/compute_pass_resource_ownership.rs → tests/tests/compute_pass_ownership.rs

@@ -1,17 +1,14 @@
 //! Tests that compute passes take ownership of resources that are associated with.
 //! I.e. once a resource is passed in to a compute pass, it can be dropped.
 //!
-//! TODO: Test doesn't check on timestamp writes & pipeline statistics queries yet.
-//!       (Not important as long as they are lifetime constrained to the command encoder,
-//!       but once we lift this constraint, we should add tests for this as well!)
 //! TODO: Also should test resource ownership for:
 //!       * write_timestamp
 //!       * begin_pipeline_statistics_query
 
 use std::num::NonZeroU64;
 
 use wgpu::util::DeviceExt as _;
-use wgpu_test::{gpu_test, GpuTestConfiguration, TestParameters, TestingContext};
+use wgpu_test::{gpu_test, valid, GpuTestConfiguration, TestParameters, TestingContext};
 
 const SHADER_SRC: &str = "
 @group(0) @binding(0)
@@ -75,6 +72,50 @@ async fn compute_pass_resource_ownership(ctx: TestingContext) {
     assert_eq!(floats, [2.0, 4.0, 6.0, 8.0]);
 }
 
+#[gpu_test]
+static COMPUTE_PASS_KEEP_ENCODER_ALIVE: GpuTestConfiguration = GpuTestConfiguration::new()
+    .parameters(TestParameters::default().test_features_limits())
+    .run_async(compute_pass_keep_encoder_alive);
+
+async fn compute_pass_keep_encoder_alive(ctx: TestingContext) {
+    let ResourceSetup {
+        gpu_buffer: _,
+        cpu_buffer: _,
+        buffer_size: _,
+        indirect_buffer,
+        bind_group,
+        pipeline,
+    } = resource_setup(&ctx);
+
+    let mut encoder = ctx
+        .device
+        .create_command_encoder(&wgpu::CommandEncoderDescriptor {
+            label: Some("encoder"),
+        });
+
+    let mut cpass = encoder.begin_compute_pass(&wgpu::ComputePassDescriptor {
+        label: Some("compute_pass"),
+        timestamp_writes: None,
+    });
+
+    // Now drop the encoder - it is kept alive by the compute pass.
+    drop(encoder);
+    ctx.async_poll(wgpu::Maintain::wait())
+        .await
+        .panic_on_timeout();
+
+    // Record some draw commands.
+    cpass.set_pipeline(&pipeline);
+    cpass.set_bind_group(0, &bind_group, &[]);
+    cpass.dispatch_workgroups_indirect(&indirect_buffer, 0);
+
+    // Dropping the pass will still execute the pass, even though there's no way to submit it.
+    // Ideally, this would log an error, but the encoder is not dropped until the compute pass is dropped,
+    // making this a valid operation.
+    // (If instead the encoder was explicitly destroyed or finished, this would be an error.)
+    valid(&ctx.device, || drop(cpass));
+}
+
 // Setup ------------------------------------------------------------
 
 struct ResourceSetup {

tests/tests/encoder.rs

@@ -1,4 +1,8 @@
-use wgpu_test::{fail, gpu_test, FailureCase, GpuTestConfiguration, TestParameters};
+use wgpu::util::DeviceExt;
+use wgpu::CommandEncoder;
+use wgpu_test::{
+    fail, gpu_test, FailureCase, GpuTestConfiguration, TestParameters, TestingContext,
+};
 
 #[gpu_test]
 static DROP_ENCODER: GpuTestConfiguration = GpuTestConfiguration::new().run_sync(|ctx| {
@@ -72,3 +76,227 @@ static DROP_ENCODER_AFTER_ERROR: GpuTestConfiguration = GpuTestConfiguration::ne
         // The encoder is still open!
         drop(encoder);
     });
+
+// TODO: This should also apply to render passes once the lifetime bound is lifted.
+#[gpu_test]
+static ENCODER_OPERATIONS_FAIL_WHILE_COMPUTE_PASS_ALIVE: GpuTestConfiguration =
+    GpuTestConfiguration::new()
+        .parameters(TestParameters::default().features(
+            wgpu::Features::CLEAR_TEXTURE
+                | wgpu::Features::TIMESTAMP_QUERY
+                | wgpu::Features::TIMESTAMP_QUERY_INSIDE_ENCODERS,
+        ))
+        .run_sync(encoder_operations_fail_while_compute_pass_alive);
+
+fn encoder_operations_fail_while_compute_pass_alive(ctx: TestingContext) {
+    let buffer_source = ctx
+        .device
+        .create_buffer_init(&wgpu::util::BufferInitDescriptor {
+            label: None,
+            contents: &[0u8; 4],
+            usage: wgpu::BufferUsages::COPY_SRC,
+        });
+    let buffer_dest = ctx
+        .device
+        .create_buffer_init(&wgpu::util::BufferInitDescriptor {
+            label: None,
+            contents: &[0u8; 4],
+            usage: wgpu::BufferUsages::COPY_DST,
+        });
+
+    let texture_desc = wgpu::TextureDescriptor {
+        label: None,
+        size: wgpu::Extent3d {
+            width: 1,
+            height: 1,
+            depth_or_array_layers: 1,
+        },
+        mip_level_count: 1,
+        sample_count: 1,
+        dimension: wgpu::TextureDimension::D2,
+        format: wgpu::TextureFormat::Rgba8Unorm,
+        usage: wgpu::TextureUsages::COPY_DST,
+        view_formats: &[],
+    };
+    let texture_dst = ctx.device.create_texture(&texture_desc);
+    let texture_src = ctx.device.create_texture(&wgpu::TextureDescriptor {
+        usage: wgpu::TextureUsages::COPY_SRC,
+        ..texture_desc
+    });
+    let query_set = ctx.device.create_query_set(&wgpu::QuerySetDescriptor {
+        count: 1,
+        ty: wgpu::QueryType::Timestamp,
+        label: None,
+    });
+
+    #[allow(clippy::type_complexity)]
+    let recording_ops: Vec<(_, Box<dyn Fn(&mut CommandEncoder)>)> = vec![
+        (
+            "begin_compute_pass",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.begin_compute_pass(&wgpu::ComputePassDescriptor::default());
+            }),
+        ),
+        (
+            "begin_render_pass",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.begin_render_pass(&wgpu::RenderPassDescriptor::default());
+            }),
+        ),
+        (
+            "copy_buffer_to_buffer",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.copy_buffer_to_buffer(&buffer_source, 0, &buffer_dest, 0, 4);
+            }),
+        ),
+        (
+            "copy_buffer_to_texture",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.copy_buffer_to_texture(
+                    wgpu::ImageCopyBuffer {
+                        buffer: &buffer_source,
+                        layout: wgpu::ImageDataLayout {
+                            offset: 0,
+                            bytes_per_row: Some(4),
+                            rows_per_image: None,
+                        },
+                    },
+                    texture_dst.as_image_copy(),
+                    texture_dst.size(),
+                );
+            }),
+        ),
+        (
+            "copy_texture_to_buffer",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.copy_texture_to_buffer(
+                    wgpu::ImageCopyTexture {
+                        texture: &texture_src,
+                        mip_level: 0,
+                        origin: wgpu::Origin3d::ZERO,
+                        aspect: wgpu::TextureAspect::All,
+                    },
+                    wgpu::ImageCopyBuffer {
+                        buffer: &buffer_dest,
+                        layout: wgpu::ImageDataLayout {
+                            offset: 0,
+                            bytes_per_row: Some(4),
+                            rows_per_image: None,
+                        },
+                    },
+                    texture_dst.size(),
+                );
+            }),
+        ),
+        (
+            "copy_texture_to_texture",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.copy_texture_to_texture(
+                    wgpu::ImageCopyTexture {
+                        texture: &texture_src,
+                        mip_level: 0,
+                        origin: wgpu::Origin3d::ZERO,
+                        aspect: wgpu::TextureAspect::All,
+                    },
+                    wgpu::ImageCopyTexture {
+                        texture: &texture_dst,
+                        mip_level: 0,
+                        origin: wgpu::Origin3d::ZERO,
+                        aspect: wgpu::TextureAspect::All,
+                    },
+                    texture_dst.size(),
+                );
+            }),
+        ),
+        (
+            "clear_texture",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.clear_texture(&texture_dst, &wgpu::ImageSubresourceRange::default());
+            }),
+        ),
+        (
+            "clear_buffer",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.clear_buffer(&buffer_dest, 0, None);
+            }),
+        ),
+        (
+            "insert_debug_marker",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.insert_debug_marker("marker");
+            }),
+        ),
+        (
+            "push_debug_group",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.push_debug_group("marker");
+            }),
+        ),
+        (
+            "pop_debug_group",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.pop_debug_group();
+            }),
+        ),
+        (
+            "resolve_query_set",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.resolve_query_set(&query_set, 0..1, &buffer_dest, 0);
+            }),
+        ),
+        (
+            "write_timestamp",
+            Box::new(|encoder: &mut wgpu::CommandEncoder| {
+                encoder.write_timestamp(&query_set, 0);
+            }),
+        ),
+    ];
+
+    for (op_name, op) in recording_ops.iter() {
+        let mut encoder = ctx
+            .device
+            .create_command_encoder(&wgpu::CommandEncoderDescriptor::default());
+
+        let pass = encoder.begin_compute_pass(&wgpu::ComputePassDescriptor::default());
+
+        ctx.device.push_error_scope(wgpu::ErrorFilter::Validation);
+
+        log::info!("Testing operation {} on a locked command encoder", op_name);
+        fail(
+            &ctx.device,
+            || op(&mut encoder),
+            Some("Command encoder is locked"),
+        );
+
+        // Drop the pass - this also fails now since the encoder is invalid:
+        fail(
+            &ctx.device,
+            || drop(pass),
+            Some("Command encoder is invalid"),
+        );
+        // Also, it's not possible to create a new pass on the encoder:
+        fail(
+            &ctx.device,
+            || encoder.begin_compute_pass(&wgpu::ComputePassDescriptor::default()),
+            Some("Command encoder is invalid"),
+        );
+    }
+
+    // Test encoder finishing separately since it consumes the encoder and doesn't fit above pattern.
+    {
+        let mut encoder = ctx
+            .device
+            .create_command_encoder(&wgpu::CommandEncoderDescriptor::default());
+        let pass = encoder.begin_compute_pass(&wgpu::ComputePassDescriptor::default());
+        fail(
+            &ctx.device,
+            || encoder.finish(),
+            Some("Command encoder is locked"),
+        );
+        fail(
+            &ctx.device,
+            || drop(pass),
+            Some("Command encoder is invalid"),
+        );
+    }
+}

tests/tests/root.rs

@@ -11,7 +11,7 @@ mod buffer;
 mod buffer_copy;
 mod buffer_usages;
 mod clear_texture;
-mod compute_pass_resource_ownership;
+mod compute_pass_ownership;
 mod create_surface_error;
 mod device;
 mod encoder;

wgpu-core/src/command/clear.rs

@@ -26,8 +26,6 @@ use wgt::{math::align_to, BufferAddress, BufferUsages, ImageSubresourceRange, Te
 pub enum ClearError {
     #[error("To use clear_texture the CLEAR_TEXTURE feature needs to be enabled")]
     MissingClearTextureFeature,
-    #[error("Command encoder {0:?} is invalid")]
-    InvalidCommandEncoder(CommandEncoderId),
     #[error("Device {0:?} is invalid")]
     InvalidDevice(DeviceId),
     #[error("Buffer {0:?} is invalid or destroyed")]
@@ -74,6 +72,8 @@ whereas subesource range specified start {subresource_base_array_layer} and coun
     },
     #[error(transparent)]
     Device(#[from] DeviceError),
+    #[error(transparent)]
+    CommandEncoderError(#[from] super::CommandEncoderError),
 }
 
 impl Global {
@@ -89,8 +89,7 @@ impl Global {
 
         let hub = A::hub(self);
 
-        let cmd_buf = CommandBuffer::get_encoder(hub, command_encoder_id)
-            .map_err(|_| ClearError::InvalidCommandEncoder(command_encoder_id))?;
+        let cmd_buf = CommandBuffer::get_encoder(hub, command_encoder_id)?;
         let mut cmd_buf_data = cmd_buf.data.lock();
         let cmd_buf_data = cmd_buf_data.as_mut().unwrap();
 
@@ -183,8 +182,7 @@ impl Global {
 
         let hub = A::hub(self);
 
-        let cmd_buf = CommandBuffer::get_encoder(hub, command_encoder_id)
-            .map_err(|_| ClearError::InvalidCommandEncoder(command_encoder_id))?;
+        let cmd_buf = CommandBuffer::get_encoder(hub, command_encoder_id)?;
         let mut cmd_buf_data = cmd_buf.data.lock();
         let cmd_buf_data = cmd_buf_data.as_mut().unwrap();