Validate strip_index_format equals index buffer format for indexed drawing with strip topology

Author: beicause

CHANGELOG.md

@@ -85,6 +85,7 @@ Bottom level categories:
 - Tracing support has been restored. By @andyleiserson in [#8429](https://github.com/gfx-rs/wgpu/pull/8429).
 - Pipelines using passthrough shaders now correctly require explicit pipeline layout. By @inner-daemons in #8881.
 - Allow using a shader that defines I/O for dual-source blending in a pipeline that does not make use of it. By @andyleiserson in [#8856](https://github.com/gfx-rs/wgpu/pull/8856).
+- Validate `strip_index_format` isn't None and equals index buffer format for indexed drawing with strip topology. By @beicause in [#8850](https://github.com/gfx-rs/wgpu/pull/8850).
 
 #### naga
 

wgpu-core/src/command/bundle.rs

@@ -718,7 +718,7 @@ fn draw(
     first_vertex: u32,
     first_instance: u32,
 ) -> Result<(), RenderBundleErrorInner> {
-    state.is_ready()?;
+    state.is_ready(DrawCommandFamily::Draw)?;
     let pipeline = state.pipeline()?;
 
     let vertex_limits = super::VertexLimits::new(state.vertex_buffer_sizes(), &pipeline.steps);
@@ -746,13 +746,10 @@ fn draw_indexed(
     base_vertex: i32,
     first_instance: u32,
 ) -> Result<(), RenderBundleErrorInner> {
-    state.is_ready()?;
+    state.is_ready(DrawCommandFamily::DrawIndexed)?;
     let pipeline = state.pipeline()?;
 
-    let index = match state.index {
-        Some(ref index) => index,
-        None => return Err(DrawError::MissingIndexBuffer.into()),
-    };
+    let index = state.index.as_ref().unwrap();
 
     let vertex_limits = super::VertexLimits::new(state.vertex_buffer_sizes(), &pipeline.steps);
 
@@ -788,7 +785,7 @@ fn draw_mesh_tasks(
     group_count_y: u32,
     group_count_z: u32,
 ) -> Result<(), RenderBundleErrorInner> {
-    state.is_ready()?;
+    state.is_ready(DrawCommandFamily::DrawMeshTasks)?;
 
     let groups_size_limit = state.device.limits.max_task_mesh_workgroups_per_dimension;
     let max_groups = state.device.limits.max_task_mesh_workgroup_total_count;
@@ -822,7 +819,7 @@ fn multi_draw_indirect(
     offset: u64,
     family: DrawCommandFamily,
 ) -> Result<(), RenderBundleErrorInner> {
-    state.is_ready()?;
+    state.is_ready(family)?;
     state
         .device
         .require_downlevel_flags(wgt::DownlevelFlags::INDIRECT_EXECUTION)?;
@@ -846,10 +843,7 @@ fn multi_draw_indirect(
         ));
 
     let vertex_or_index_limit = if family == DrawCommandFamily::DrawIndexed {
-        let index = match state.index {
-            Some(ref mut index) => index,
-            None => return Err(DrawError::MissingIndexBuffer.into()),
-        };
+        let index = state.index.as_mut().unwrap();
         state.commands.extend(index.flush());
         index.limit()
     } else {
@@ -1408,11 +1402,29 @@ impl State {
     /// Validation for a draw command.
     ///
     /// This should be further deduplicated with similar validation on render/compute passes.
-    fn is_ready(&mut self) -> Result<(), DrawError> {
+    fn is_ready(&mut self, family: DrawCommandFamily) -> Result<(), DrawError> {
         if let Some(pipeline) = self.pipeline.as_ref() {
             self.binder
                 .check_compatibility(pipeline.pipeline.as_ref())?;
             self.binder.check_late_buffer_bindings()?;
+
+            if family == DrawCommandFamily::DrawIndexed {
+                let pipeline = &pipeline.pipeline;
+                let index_format = match &self.index {
+                    Some(index) => index.format,
+                    None => return Err(DrawError::MissingIndexBuffer),
+                };
+
+                if pipeline.topology.is_strip() && pipeline.strip_index_format != Some(index_format)
+                {
+                    return Err(DrawError::UnmatchedStripIndexFormat {
+                        pipeline: pipeline.error_ident(),
+                        strip_index_format: pipeline.strip_index_format,
+                        buffer_format: index_format,
+                    });
+                }
+            }
+
             Ok(())
         } else {
             Err(DrawError::MissingPipeline(pass::MissingPipeline))

wgpu-core/src/command/draw.rs

@@ -46,17 +46,14 @@ pub enum DrawError {
     },
     #[error("Index {last_index} extends beyond limit {index_limit}. Did you bind the correct index buffer?")]
     IndexBeyondLimit { last_index: u64, index_limit: u64 },
-    #[error(
-        "Index buffer format {buffer_format:?} doesn't match {pipeline}'s index format {pipeline_format:?}"
-    )]
-    UnmatchedIndexFormats {
+    #[error("For indexed drawing with strip topology, {pipeline}'s strip index format {strip_index_format:?} must match index buffer format {buffer_format:?}")]
+    UnmatchedStripIndexFormat {
         pipeline: ResourceErrorIdent,
-        pipeline_format: wgt::IndexFormat,
+        strip_index_format: Option<wgt::IndexFormat>,
         buffer_format: wgt::IndexFormat,
     },
     #[error(transparent)]
     BindingSizeTooSmall(#[from] LateMinBufferBindingSizeMismatch),
-
     #[error(
         "Wrong pipeline type for this draw command. Attempted to call {} draw command on {} pipeline",
         if *wanted_mesh_pipeline {"mesh shader"} else {"standard"},

wgpu-core/src/command/render.rs

@@ -559,21 +559,20 @@ impl<'scope, 'snatch_guard, 'cmd_enc> State<'scope, 'snatch_guard, 'cmd_enc> {
 
             if family == DrawCommandFamily::DrawIndexed {
                 // Pipeline expects an index buffer
-                if let Some(pipeline_index_format) = pipeline.strip_index_format {
-                    // We have a buffer bound
-                    let buffer_index_format = self
-                        .index
-                        .buffer_format
-                        .ok_or(DrawError::MissingIndexBuffer)?;
-
-                    // The buffers are different formats
-                    if pipeline_index_format != buffer_index_format {
-                        return Err(DrawError::UnmatchedIndexFormats {
-                            pipeline: pipeline.error_ident(),
-                            pipeline_format: pipeline_index_format,
-                            buffer_format: buffer_index_format,
-                        });
-                    }
+                // We have a buffer bound
+                let buffer_index_format = self
+                    .index
+                    .buffer_format
+                    .ok_or(DrawError::MissingIndexBuffer)?;
+
+                if pipeline.topology.is_strip()
+                    && pipeline.strip_index_format != Some(buffer_index_format)
+                {
+                    return Err(DrawError::UnmatchedStripIndexFormat {
+                        pipeline: pipeline.error_ident(),
+                        strip_index_format: pipeline.strip_index_format,
+                        buffer_format: buffer_index_format,
+                    });
                 }
             }
             if (family == DrawCommandFamily::DrawMeshTasks) != pipeline.is_mesh {

wgpu-core/src/device/resource.rs

@@ -4632,6 +4632,7 @@ impl Device {
             pass_context,
             _shader_modules: shader_modules,
             flags,
+            topology: desc.primitive.topology,
             strip_index_format: desc.primitive.strip_index_format,
             vertex_steps,
             late_sized_buffer_groups,

wgpu-core/src/pipeline.rs

@@ -807,6 +807,7 @@ pub struct RenderPipeline {
     pub(crate) _shader_modules: ArrayVec<Arc<ShaderModule>, { hal::MAX_CONCURRENT_SHADER_STAGES }>,
     pub(crate) pass_context: RenderPassContext,
     pub(crate) flags: PipelineFlags,
+    pub(crate) topology: wgt::PrimitiveTopology,
     pub(crate) strip_index_format: Option<wgt::IndexFormat>,
     pub(crate) vertex_steps: Vec<VertexStep>,
     pub(crate) late_sized_buffer_groups: ArrayVec<LateSizedBufferGroup, { hal::MAX_BIND_GROUPS }>,

wgpu-types/src/render.rs

@@ -374,7 +374,8 @@ pub struct PrimitiveState {
     /// When drawing strip topologies with indices, this is the required format for the index buffer.
     /// This has no effect on non-indexed or non-strip draws.
     ///
-    /// Specifying this value enables primitive restart, allowing individual strips to be separated
+    /// This is required for indexed drawing with strip topology and must match index buffer format, as primitive restart is always enabled
+    /// in all backends and individual strips will be separated
     /// with the index value `0xFFFF` when using `Uint16`, or `0xFFFFFFFF` when using `Uint32`.
     #[cfg_attr(feature = "serde", serde(default))]
     pub strip_index_format: Option<IndexFormat>,