From e42d6a4f8fde860fe2f04ab769ec968b6e73cde7 Mon Sep 17 00:00:00 2001 From: Connor Fitzgerald Date: Tue, 7 Feb 2023 12:29:42 -0500 Subject: [PATCH 1/4] Add regression test --- wgpu/tests/regression/issue_3457.rs | 190 ++++++++++++++++++++++++++ wgpu/tests/regression/issue_3457.wgsl | 28 ++++ wgpu/tests/root.rs | 3 + 3 files changed, 221 insertions(+) create mode 100644 wgpu/tests/regression/issue_3457.rs create mode 100644 wgpu/tests/regression/issue_3457.wgsl diff --git a/wgpu/tests/regression/issue_3457.rs b/wgpu/tests/regression/issue_3457.rs new file mode 100644 index 0000000000..5d5923c31e --- /dev/null +++ b/wgpu/tests/regression/issue_3457.rs @@ -0,0 +1,190 @@ +use crate::common::{initialize_test, TestParameters}; + +use wasm_bindgen_test::wasm_bindgen_test; +use wgpu::*; + +#[wasm_bindgen_test] +#[test] +fn pass_reset_vertex_buffer() { + initialize_test(TestParameters::default(), |ctx| { + let module = ctx + .device + .create_shader_module(include_wgsl!("issue_3457.wgsl")); + + let vertex_buffer1 = ctx.device.create_buffer(&BufferDescriptor { + label: Some("vertex buffer 1"), + size: 6 * 16, + usage: BufferUsages::VERTEX, + mapped_at_creation: false, + }); + + let vertex_buffer2 = ctx.device.create_buffer(&BufferDescriptor { + label: Some("vertex buffer 2"), + size: 6 * 4, + usage: BufferUsages::VERTEX, + mapped_at_creation: false, + }); + + let pipeline_layout = ctx + .device + .create_pipeline_layout(&PipelineLayoutDescriptor { + label: Some("Pipeline Layout"), + bind_group_layouts: &[], + push_constant_ranges: &[], + }); + + let double_pipeline = ctx + .device + .create_render_pipeline(&RenderPipelineDescriptor { + label: Some("Double Pipeline"), + layout: Some(&pipeline_layout), + vertex: VertexState { + module: &module, + entry_point: "double_buffer_vert", + buffers: &[ + VertexBufferLayout { + array_stride: 16, + step_mode: VertexStepMode::Vertex, + attributes: &vertex_attr_array![0 => Float32x4], + }, + VertexBufferLayout { + array_stride: 4, + step_mode: VertexStepMode::Vertex, + attributes: &vertex_attr_array![1 => Float32], + }, + ], + }, + primitive: PrimitiveState::default(), + depth_stencil: None, + multisample: MultisampleState::default(), + fragment: Some(FragmentState { + module: &module, + entry_point: "double_buffer_frag", + targets: &[Some(ColorTargetState { + format: TextureFormat::Rgba8Unorm, + blend: None, + write_mask: ColorWrites::all(), + })], + }), + multiview: None, + }); + + let single_pipeline = ctx + .device + .create_render_pipeline(&RenderPipelineDescriptor { + label: Some("Single Pipeline"), + layout: Some(&pipeline_layout), + vertex: VertexState { + module: &module, + entry_point: "single_buffer_vert", + buffers: &[VertexBufferLayout { + array_stride: 16, + step_mode: VertexStepMode::Vertex, + attributes: &vertex_attr_array![0 => Float32x4], + }], + }, + primitive: PrimitiveState::default(), + depth_stencil: Some(DepthStencilState { + format: TextureFormat::Depth32Float, + depth_write_enabled: false, + depth_compare: CompareFunction::Always, + stencil: StencilState::default(), + bias: DepthBiasState::default(), + }), + multisample: MultisampleState::default(), + fragment: None, + multiview: None, + }); + + let view = ctx + .device + .create_texture(&TextureDescriptor { + label: Some("Render texture"), + size: Extent3d { + width: 4, + height: 4, + depth_or_array_layers: 1, + }, + mip_level_count: 1, + sample_count: 1, + dimension: TextureDimension::D2, + format: TextureFormat::Rgba8Unorm, + usage: TextureUsages::RENDER_ATTACHMENT, + view_formats: &[], + }) + .create_view(&TextureViewDescriptor::default()); + + let mut encoder1 = ctx + .device + .create_command_encoder(&CommandEncoderDescriptor::default()); + + let mut double_rpass = encoder1.begin_render_pass(&RenderPassDescriptor { + label: Some("double renderpass"), + color_attachments: &[Some(RenderPassColorAttachment { + view: &view, + resolve_target: None, + ops: Operations { + load: LoadOp::Clear(Color::BLACK), + store: false, + }, + })], + depth_stencil_attachment: None, + }); + + double_rpass.set_pipeline(&double_pipeline); + double_rpass.set_vertex_buffer(0, vertex_buffer1.slice(..)); + double_rpass.set_vertex_buffer(1, vertex_buffer2.slice(..)); + double_rpass.draw(0..3, 0..1); + + drop(double_rpass); + + ctx.queue.submit(Some(encoder1.finish())); + + drop((vertex_buffer2, view)); + + ctx.device.poll(Maintain::Wait); + + let mut encoder2 = ctx + .device + .create_command_encoder(&CommandEncoderDescriptor::default()); + + let depth_view = ctx + .device + .create_texture(&TextureDescriptor { + label: Some("Depth texture"), + size: Extent3d { + width: 4, + height: 4, + depth_or_array_layers: 1, + }, + mip_level_count: 1, + sample_count: 1, + dimension: TextureDimension::D2, + format: TextureFormat::Depth32Float, + usage: TextureUsages::RENDER_ATTACHMENT, + view_formats: &[], + }) + .create_view(&TextureViewDescriptor::default()); + + let mut single_rpass = encoder2.begin_render_pass(&RenderPassDescriptor { + label: Some("single renderpass"), + color_attachments: &[], + depth_stencil_attachment: Some(RenderPassDepthStencilAttachment { + view: &depth_view, + depth_ops: Some(Operations { + load: LoadOp::Clear(0.0), + store: false, + }), + stencil_ops: None, + }), + }); + + single_rpass.set_pipeline(&single_pipeline); + single_rpass.set_vertex_buffer(0, vertex_buffer1.slice(..)); + single_rpass.draw(0..6, 0..1); + + drop(single_rpass); + + ctx.queue.submit(Some(encoder2.finish())); + }) +} diff --git a/wgpu/tests/regression/issue_3457.wgsl b/wgpu/tests/regression/issue_3457.wgsl new file mode 100644 index 0000000000..042985f029 --- /dev/null +++ b/wgpu/tests/regression/issue_3457.wgsl @@ -0,0 +1,28 @@ +struct DoubleVertexIn { + @location(0) position: vec4, + @location(1) value: f32, +} + +struct DoubleVertexOut { + @builtin(position) position: vec4, + @location(0) value: f32, +} + +@vertex +fn double_buffer_vert(v_in: DoubleVertexIn) -> DoubleVertexOut { + return DoubleVertexOut(v_in.position, v_in.value); +} + +@fragment +fn double_buffer_frag(v_out: DoubleVertexOut) -> @location(0) vec4 { + return vec4(v_out.value); +} + +struct SingleVertexIn { + @location(0) position: vec4, +} + +@vertex +fn single_buffer_vert(v_in: SingleVertexIn) -> @builtin(position) vec4 { + return v_in.position; +} diff --git a/wgpu/tests/root.rs b/wgpu/tests/root.rs index a8676fb959..277b34eb65 100644 --- a/wgpu/tests/root.rs +++ b/wgpu/tests/root.rs @@ -3,6 +3,9 @@ use wasm_bindgen_test::wasm_bindgen_test_configure; // All files containing tests mod common; +mod regression { + mod issue_3457; +} mod buffer; mod buffer_copy; mod buffer_usages; From ca08915a65981ab7244f8d7ccd0f643bd7f0ec97 Mon Sep 17 00:00:00 2001 From: Connor Fitzgerald Date: Wed, 8 Feb 2023 17:37:17 -0500 Subject: [PATCH 2/4] Fix --- wgpu-hal/src/gles/command.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/wgpu-hal/src/gles/command.rs b/wgpu-hal/src/gles/command.rs index 7c1eb1c841..8076a13aeb 100644 --- a/wgpu-hal/src/gles/command.rs +++ b/wgpu-hal/src/gles/command.rs @@ -620,6 +620,11 @@ impl crate::CommandEncoder for super::CommandEncoder { self.state.dirty_vbuf_mask = 0; self.state.active_first_instance = 0; self.state.color_targets.clear(); + for index in 0..self.state.vertex_attributes.len() { + self.cmd_buffer + .commands + .push(C::UnsetVertexAttribute(index as u32)); + } self.state.vertex_attributes.clear(); self.state.primitive = super::PrimitiveState::default(); } From 0ae32bc032f7561eb8c016d26bccc0de800ad710 Mon Sep 17 00:00:00 2001 From: Connor Fitzgerald Date: Wed, 8 Feb 2023 19:13:36 -0500 Subject: [PATCH 3/4] Docs for regression test --- wgpu/tests/regression/issue_3457.rs | 69 +++++++++++++-------------- wgpu/tests/regression/issue_3457.wgsl | 5 ++ 2 files changed, 38 insertions(+), 36 deletions(-) diff --git a/wgpu/tests/regression/issue_3457.rs b/wgpu/tests/regression/issue_3457.rs index 5d5923c31e..c8a02984de 100644 --- a/wgpu/tests/regression/issue_3457.rs +++ b/wgpu/tests/regression/issue_3457.rs @@ -3,6 +3,15 @@ use crate::common::{initialize_test, TestParameters}; use wasm_bindgen_test::wasm_bindgen_test; use wgpu::*; +/// The core issue here was that we weren't properly disabling vertex attributes on GL +/// when a renderpass ends. This ended up being rather tricky to test for as GL is remarkably +/// tolerant of errors. This test, with the fix not-applied, only fails on WebGL. +/// +/// We need to setup a situation where it's invalid to issue a draw call without the fix. +/// To do this we first make a renderpass using two vertex buffers and draw on it. Then we +/// submit, delete the second vertex buffer and `poll(Wait)`. Because we maintained the device, +/// the actual underlying buffer for the second vertex buffer is deleted, causing a draw call +/// that is invalid if the second attribute is still enabled. #[wasm_bindgen_test] #[test] fn pass_reset_vertex_buffer() { @@ -11,6 +20,7 @@ fn pass_reset_vertex_buffer() { .device .create_shader_module(include_wgsl!("issue_3457.wgsl")); + // We use two separate vertex buffers so we can delete one in between submisions let vertex_buffer1 = ctx.device.create_buffer(&BufferDescriptor { label: Some("vertex buffer 1"), size: 6 * 16, @@ -84,15 +94,17 @@ fn pass_reset_vertex_buffer() { }], }, primitive: PrimitiveState::default(), - depth_stencil: Some(DepthStencilState { - format: TextureFormat::Depth32Float, - depth_write_enabled: false, - depth_compare: CompareFunction::Always, - stencil: StencilState::default(), - bias: DepthBiasState::default(), - }), + depth_stencil: None, multisample: MultisampleState::default(), - fragment: None, + fragment: Some(FragmentState { + module: &module, + entry_point: "single_buffer_frag", + targets: &[Some(ColorTargetState { + format: TextureFormat::Rgba8Unorm, + blend: None, + write_mask: ColorWrites::all(), + })], + }), multiview: None, }); @@ -138,45 +150,30 @@ fn pass_reset_vertex_buffer() { drop(double_rpass); + // Submit the first pass using both buffers ctx.queue.submit(Some(encoder1.finish())); + // Drop the second buffer, meaning it's invalid to use draw + // unless it's unbound. + drop(vertex_buffer2); - drop((vertex_buffer2, view)); - + // Make sure the buffers are actually deleted. ctx.device.poll(Maintain::Wait); let mut encoder2 = ctx .device .create_command_encoder(&CommandEncoderDescriptor::default()); - let depth_view = ctx - .device - .create_texture(&TextureDescriptor { - label: Some("Depth texture"), - size: Extent3d { - width: 4, - height: 4, - depth_or_array_layers: 1, - }, - mip_level_count: 1, - sample_count: 1, - dimension: TextureDimension::D2, - format: TextureFormat::Depth32Float, - usage: TextureUsages::RENDER_ATTACHMENT, - view_formats: &[], - }) - .create_view(&TextureViewDescriptor::default()); - let mut single_rpass = encoder2.begin_render_pass(&RenderPassDescriptor { label: Some("single renderpass"), - color_attachments: &[], - depth_stencil_attachment: Some(RenderPassDepthStencilAttachment { - view: &depth_view, - depth_ops: Some(Operations { - load: LoadOp::Clear(0.0), + color_attachments: &[Some(RenderPassColorAttachment { + view: &view, + resolve_target: None, + ops: Operations { + load: LoadOp::Clear(Color::BLACK), store: false, - }), - stencil_ops: None, - }), + }, + })], + depth_stencil_attachment: None, }); single_rpass.set_pipeline(&single_pipeline); diff --git a/wgpu/tests/regression/issue_3457.wgsl b/wgpu/tests/regression/issue_3457.wgsl index 042985f029..2f9671d673 100644 --- a/wgpu/tests/regression/issue_3457.wgsl +++ b/wgpu/tests/regression/issue_3457.wgsl @@ -26,3 +26,8 @@ struct SingleVertexIn { fn single_buffer_vert(v_in: SingleVertexIn) -> @builtin(position) vec4 { return v_in.position; } + +@fragment +fn single_buffer_vert() -> @location(0) vec4 { + return vec4(0.0); +} From 759986798893fd2c7e38fe256381aef779bb2866 Mon Sep 17 00:00:00 2001 From: Connor Fitzgerald Date: Wed, 8 Feb 2023 19:22:00 -0500 Subject: [PATCH 4/4] Whoops --- wgpu/tests/regression/issue_3457.rs | 6 +++--- wgpu/tests/regression/issue_3457.wgsl | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/wgpu/tests/regression/issue_3457.rs b/wgpu/tests/regression/issue_3457.rs index c8a02984de..f4359fb937 100644 --- a/wgpu/tests/regression/issue_3457.rs +++ b/wgpu/tests/regression/issue_3457.rs @@ -23,14 +23,14 @@ fn pass_reset_vertex_buffer() { // We use two separate vertex buffers so we can delete one in between submisions let vertex_buffer1 = ctx.device.create_buffer(&BufferDescriptor { label: Some("vertex buffer 1"), - size: 6 * 16, + size: 3 * 16, usage: BufferUsages::VERTEX, mapped_at_creation: false, }); let vertex_buffer2 = ctx.device.create_buffer(&BufferDescriptor { label: Some("vertex buffer 2"), - size: 6 * 4, + size: 3 * 4, usage: BufferUsages::VERTEX, mapped_at_creation: false, }); @@ -178,7 +178,7 @@ fn pass_reset_vertex_buffer() { single_rpass.set_pipeline(&single_pipeline); single_rpass.set_vertex_buffer(0, vertex_buffer1.slice(..)); - single_rpass.draw(0..6, 0..1); + single_rpass.draw(0..3, 0..1); drop(single_rpass); diff --git a/wgpu/tests/regression/issue_3457.wgsl b/wgpu/tests/regression/issue_3457.wgsl index 2f9671d673..f02aaa1bb1 100644 --- a/wgpu/tests/regression/issue_3457.wgsl +++ b/wgpu/tests/regression/issue_3457.wgsl @@ -28,6 +28,6 @@ fn single_buffer_vert(v_in: SingleVertexIn) -> @builtin(position) vec4 { } @fragment -fn single_buffer_vert() -> @location(0) vec4 { +fn single_buffer_frag() -> @location(0) vec4 { return vec4(0.0); }