deduplicate vertex buffer size validation (#7124)

wgpu-core/src/command/bundle.rs

@@ -111,91 +111,6 @@ use super::{
     DrawKind,
 };
 
-/// <https://gpuweb.github.io/gpuweb/#dom-gpurendercommandsmixin-draw>
-fn validate_draw(
-    vertex: &[Option<VertexState>],
-    step: &[VertexStep],
-    first_vertex: u32,
-    vertex_count: u32,
-    first_instance: u32,
-    instance_count: u32,
-) -> Result<(), DrawError> {
-    let vertices_end = first_vertex as u64 + vertex_count as u64;
-    let instances_end = first_instance as u64 + instance_count as u64;
-
-    for (idx, (vbs, step)) in vertex.iter().zip(step).enumerate() {
-        let Some(vbs) = vbs else {
-            continue;
-        };
-
-        let stride_count = match step.mode {
-            wgt::VertexStepMode::Vertex => vertices_end,
-            wgt::VertexStepMode::Instance => instances_end,
-        };
-
-        if stride_count == 0 {
-            continue;
-        }
-
-        let offset = (stride_count - 1) * step.stride + step.last_stride;
-        let limit = vbs.range.end - vbs.range.start;
-        if offset > limit {
-            return Err(DrawError::VertexOutOfBounds {
-                step_mode: step.mode,
-                offset,
-                limit,
-                slot: idx as u32,
-            });
-        }
-    }
-
-    Ok(())
-}
-
-// See https://gpuweb.github.io/gpuweb/#dom-gpurendercommandsmixin-drawindexed
-fn validate_indexed_draw(
-    vertex: &[Option<VertexState>],
-    step: &[VertexStep],
-    index_state: &IndexState,
-    first_index: u32,
-    index_count: u32,
-    first_instance: u32,
-    instance_count: u32,
-) -> Result<(), DrawError> {
-    let last_index = first_index as u64 + index_count as u64;
-    let index_limit = index_state.limit();
-    if last_index > index_limit {
-        return Err(DrawError::IndexBeyondLimit {
-            last_index,
-            index_limit,
-        });
-    }
-
-    let stride_count = first_instance as u64 + instance_count as u64;
-    for (idx, (vbs, step)) in vertex.iter().zip(step).enumerate() {
-        let Some(vbs) = vbs else {
-            continue;
-        };
-
-        if stride_count == 0 || step.mode != wgt::VertexStepMode::Instance {
-            continue;
-        }
-
-        let offset = (stride_count - 1) * step.stride + step.last_stride;
-        let limit = vbs.range.end - vbs.range.start;
-        if offset > limit {
-            return Err(DrawError::VertexOutOfBounds {
-                step_mode: step.mode,
-                offset,
-                limit,
-                slot: idx as u32,
-            });
-        }
-    }
-
-    Ok(())
-}
-
 /// Describes a [`RenderBundleEncoder`].
 #[derive(Clone, Debug, Default, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
@@ -359,7 +274,7 @@ impl RenderBundleEncoder {
             trackers: RenderBundleScope::new(),
             pipeline: None,
             bind: (0..hal::MAX_BIND_GROUPS).map(|_| None).collect(),
-            vertex: (0..hal::MAX_VERTEX_BUFFERS).map(|_| None).collect(),
+            vertex: Default::default(),
             index: None,
             flat_dynamic_offsets: Vec::new(),
             device: device.clone(),
@@ -781,14 +696,9 @@ fn draw(
     let pipeline = state.pipeline()?;
     let used_bind_groups = pipeline.used_bind_groups;
 
-    validate_draw(
+    let vertex_limits = super::VertexLimits::new(state.vertex_buffer_sizes(), &pipeline.steps);
-        &state.vertex[..],
+    vertex_limits.validate_vertex_limit(first_vertex, vertex_count)?;
-        &pipeline.steps,
+    vertex_limits.validate_instance_limit(first_instance, instance_count)?;
-        first_vertex,
-        vertex_count,
-        first_instance,
-        instance_count,
-    )?;
 
     if instance_count > 0 && vertex_count > 0 {
         state.flush_vertices();
@@ -819,15 +729,18 @@ fn draw_indexed(
         None => return Err(DrawError::MissingIndexBuffer.into()),
     };
 
-    validate_indexed_draw(
+    let vertex_limits = super::VertexLimits::new(state.vertex_buffer_sizes(), &pipeline.steps);
-        &state.vertex[..],
+
-        &pipeline.steps,
+    let last_index = first_index as u64 + index_count as u64;
-        index,
+    let index_limit = index.limit();
-        first_index,
+    if last_index > index_limit {
-        index_count,
+        return Err(DrawError::IndexBeyondLimit {
-        first_instance,
+            last_index,
-        instance_count,
+            index_limit,
-    )?;
+        }
+        .into());
+    }
+    vertex_limits.validate_instance_limit(first_instance, instance_count)?;
 
     if instance_count > 0 && index_count > 0 {
         state.flush_index();
@@ -1330,7 +1243,7 @@ struct State {
     bind: ArrayVec<Option<BindState>, { hal::MAX_BIND_GROUPS }>,
 
     /// The state of each vertex buffer slot.
-    vertex: ArrayVec<Option<VertexState>, { hal::MAX_VERTEX_BUFFERS }>,
+    vertex: [Option<VertexState>; hal::MAX_VERTEX_BUFFERS],
 
     /// The current index buffer, if one has been set. We flush this state
     /// before indexed draw commands.
@@ -1513,6 +1426,12 @@ impl State {
 
         self.commands.extend(commands);
     }
+
+    fn vertex_buffer_sizes(&self) -> impl Iterator<Item = Option<wgt::BufferAddress>> + '_ {
+        self.vertex
+            .iter()
+            .map(|vbs| vbs.as_ref().map(|vbs| vbs.range.end - vbs.range.start))
+    }
 }
 
 /// Error encountered when finishing recording a render bundle.

wgpu-core/src/command/draw.rs

@@ -6,7 +6,6 @@ use crate::{
     },
     track::ResourceUsageCompatibilityError,
 };
-use wgt::VertexStepMode;
 
 use thiserror::Error;
 
@@ -35,13 +34,6 @@ pub enum DrawError {
         vertex_limit: u64,
         slot: u32,
     },
-    #[error("{step_mode:?} buffer out of bounds at slot {slot}. Offset {offset} beyond limit {limit}. Did you bind the correct `Vertex` step-rate vertex buffer?")]
-    VertexOutOfBounds {
-        step_mode: VertexStepMode,
-        offset: u64,
-        limit: u64,
-        slot: u32,
-    },
     #[error("Instance {last_instance} extends beyond limit {instance_limit} imposed by the buffer in slot {slot}. Did you bind the correct `Instance` step-rate vertex buffer?")]
     InstanceBeyondLimit {
         last_instance: u64,

wgpu-core/src/command/render.rs

@@ -3,7 +3,7 @@ use crate::command::{
     validate_and_begin_occlusion_query, validate_and_begin_pipeline_statistics_query,
 };
 use crate::init_tracker::BufferInitTrackerAction;
-use crate::pipeline::RenderPipeline;
+use crate::pipeline::{RenderPipeline, VertexStep};
 use crate::resource::InvalidResourceError;
 use crate::snatch::SnatchGuard;
 use crate::{
@@ -24,7 +24,7 @@ use crate::{
     global::Global,
     hal_label, id,
     init_tracker::{MemoryInitKind, TextureInitRange, TextureInitTrackerAction},
-    pipeline::{self, PipelineFlags},
+    pipeline::PipelineFlags,
     resource::{
         DestroyedResourceError, Labeled, MissingBufferUsageError, MissingTextureUsageError,
         ParentDevice, QuerySet, Texture, TextureView, TextureViewNotRenderableReason,
@@ -45,7 +45,7 @@ use serde::Deserialize;
 #[cfg(feature = "serde")]
 use serde::Serialize;
 
-use std::{borrow::Cow, fmt, iter, mem::size_of, num::NonZeroU32, ops::Range, str, sync::Arc};
+use std::{borrow::Cow, fmt, mem::size_of, num::NonZeroU32, ops::Range, str, sync::Arc};
 
 use super::render_command::ArcRenderCommand;
 use super::{
@@ -368,57 +368,45 @@ impl IndexState {
     }
 }
 
-#[derive(Clone, Copy, Debug)]
-struct VertexBufferState {
-    total_size: BufferAddress,
-    step: pipeline::VertexStep,
-    bound: bool,
-}
-
-impl VertexBufferState {
-    const EMPTY: Self = Self {
-        total_size: 0,
-        step: pipeline::VertexStep {
-            stride: 0,
-            last_stride: 0,
-            mode: VertexStepMode::Vertex,
-        },
-        bound: false,
-    };
-}
-
 #[derive(Debug, Default)]
-struct VertexState {
+pub(crate) struct VertexLimits {
-    inputs: ArrayVec<VertexBufferState, { hal::MAX_VERTEX_BUFFERS }>,
     /// Length of the shortest vertex rate vertex buffer
-    vertex_limit: u64,
+    pub(crate) vertex_limit: u64,
     /// Buffer slot which the shortest vertex rate vertex buffer is bound to
     vertex_limit_slot: u32,
     /// Length of the shortest instance rate vertex buffer
-    instance_limit: u64,
+    pub(crate) instance_limit: u64,
     /// Buffer slot which the shortest instance rate vertex buffer is bound to
     instance_limit_slot: u32,
 }
 
-impl VertexState {
+impl VertexLimits {
-    fn update_limits(&mut self) {
+    pub(crate) fn new(
+        buffer_sizes: impl Iterator<Item = Option<BufferAddress>>,
+        pipeline_steps: &[VertexStep],
+    ) -> Self {
         // Implements the validation from https://gpuweb.github.io/gpuweb/#dom-gpurendercommandsmixin-draw
         // Except that the formula is shuffled to extract the number of vertices in order
         // to carry the bulk of the computation when changing states instead of when producing
         // draws. Draw calls tend to happen at a higher frequency. Here we determine vertex
         // limits that can be cheaply checked for each draw call.
-        self.vertex_limit = u32::MAX as u64;
-        self.instance_limit = u32::MAX as u64;
-        for (idx, vbs) in self.inputs.iter().enumerate() {
-            if !vbs.bound {
-                continue;
-            }
 
-            let limit = if vbs.total_size < vbs.step.last_stride {
+        let mut vertex_limit = u64::MAX;
+        let mut vertex_limit_slot = 0;
+        let mut instance_limit = u64::MAX;
+        let mut instance_limit_slot = 0;
+
+        for (idx, (buffer_size, step)) in buffer_sizes.zip(pipeline_steps).enumerate() {
+            let Some(buffer_size) = buffer_size else {
+                // Missing required vertex buffer
+                return Self::default();
+            };
+
+            let limit = if buffer_size < step.last_stride {
                 // The buffer cannot fit the last vertex.
                 0
             } else {
-                if vbs.step.stride == 0 {
+                if step.stride == 0 {
                     // We already checked that the last stride fits, the same
                     // vertex will be repeated so this slot can accommodate any number of
                     // vertices.
@@ -426,30 +414,79 @@ impl VertexState {
                 }
 
                 // The general case.
-                (vbs.total_size - vbs.step.last_stride) / vbs.step.stride + 1
+                (buffer_size - step.last_stride) / step.stride + 1
             };
 
-            match vbs.step.mode {
+            match step.mode {
                 VertexStepMode::Vertex => {
-                    if limit < self.vertex_limit {
+                    if limit < vertex_limit {
-                        self.vertex_limit = limit;
+                        vertex_limit = limit;
-                        self.vertex_limit_slot = idx as _;
+                        vertex_limit_slot = idx as _;
                     }
                 }
                 VertexStepMode::Instance => {
-                    if limit < self.instance_limit {
+                    if limit < instance_limit {
-                        self.instance_limit = limit;
+                        instance_limit = limit;
-                        self.instance_limit_slot = idx as _;
+                        instance_limit_slot = idx as _;
                     }
                 }
             }
         }
+
+        Self {
+            vertex_limit,
+            vertex_limit_slot,
+            instance_limit,
+            instance_limit_slot,
+        }
     }
 
-    fn reset(&mut self) {
+    pub(crate) fn validate_vertex_limit(
-        self.inputs.clear();
+        &self,
-        self.vertex_limit = 0;
+        first_vertex: u32,
-        self.instance_limit = 0;
+        vertex_count: u32,
+    ) -> Result<(), DrawError> {
+        let last_vertex = first_vertex as u64 + vertex_count as u64;
+        let vertex_limit = self.vertex_limit;
+        if last_vertex > vertex_limit {
+            return Err(DrawError::VertexBeyondLimit {
+                last_vertex,
+                vertex_limit,
+                slot: self.vertex_limit_slot,
+            });
+        }
+
+        Ok(())
+    }
+
+    pub(crate) fn validate_instance_limit(
+        &self,
+        first_instance: u32,
+        instance_count: u32,
+    ) -> Result<(), DrawError> {
+        let last_instance = first_instance as u64 + instance_count as u64;
+        let instance_limit = self.instance_limit;
+        if last_instance > instance_limit {
+            return Err(DrawError::InstanceBeyondLimit {
+                last_instance,
+                instance_limit,
+                slot: self.instance_limit_slot,
+            });
+        }
+
+        Ok(())
+    }
+}
+
+#[derive(Debug, Default)]
+struct VertexState {
+    buffer_sizes: [Option<BufferAddress>; hal::MAX_VERTEX_BUFFERS],
+    limits: VertexLimits,
+}
+
+impl VertexState {
+    fn update_limits(&mut self, pipeline_steps: &[VertexStep]) {
+        self.limits = VertexLimits::new(self.buffer_sizes.iter().copied(), pipeline_steps);
     }
 }
 
@@ -496,8 +533,12 @@ impl<'scope, 'snatch_guard, 'cmd_buf, 'raw_encoder>
             }
 
             // Determine how many vertex buffers have already been bound
-            let vertex_buffer_count =
+            let vertex_buffer_count = self
-                self.vertex.inputs.iter().take_while(|v| v.bound).count() as u32;
+                .vertex
+                .buffer_sizes
+                .iter()
+                .take_while(|v| v.is_some())
+                .count() as u32;
             // Compare with the needed quantity
             if vertex_buffer_count < pipeline.vertex_steps.len() as u32 {
                 return Err(DrawError::MissingVertexBuffer {
@@ -536,7 +577,7 @@ impl<'scope, 'snatch_guard, 'cmd_buf, 'raw_encoder>
         self.binder.reset();
         self.pipeline = None;
         self.index.reset();
-        self.vertex.reset();
+        self.vertex = Default::default();
     }
 }
 
@@ -2119,23 +2160,8 @@ fn set_pipeline(
         }
     }
 
-    // Initialize each `vertex.inputs[i].step` from
-    // `pipeline.vertex_steps[i]`.  Enlarge `vertex.inputs`
-    // as necessary to accommodate all slots in the
-    // pipeline. If `vertex.inputs` is longer, fill the
-    // extra entries with default `VertexStep`s.
-    while state.vertex.inputs.len() < pipeline.vertex_steps.len() {
-        state.vertex.inputs.push(VertexBufferState::EMPTY);
-    }
-
-    // This is worse as a `zip`, but it's close.
-    let mut steps = pipeline.vertex_steps.iter();
-    for input in state.vertex.inputs.iter_mut() {
-        input.step = steps.next().cloned().unwrap_or_default();
-    }
-
     // Update vertex buffer limits.
-    state.vertex.update_limits();
+    state.vertex.update_limits(&pipeline.vertex_steps);
     Ok(())
 }
 
@@ -2218,24 +2244,18 @@ fn set_vertex_buffer(
     buffer.check_usage(BufferUsages::VERTEX)?;
     let buf_raw = buffer.try_raw(state.snatch_guard)?;
 
-    let empty_slots = (1 + slot as usize).saturating_sub(state.vertex.inputs.len());
-    state
-        .vertex
-        .inputs
-        .extend(iter::repeat(VertexBufferState::EMPTY).take(empty_slots));
-    let vertex_state = &mut state.vertex.inputs[slot as usize];
     //TODO: where are we checking that the offset is in bound?
-    vertex_state.total_size = match size {
+    let buffer_size = match size {
         Some(s) => s.get(),
         None => buffer.size - offset,
     };
-    vertex_state.bound = true;
+    state.vertex.buffer_sizes[slot as usize] = Some(buffer_size);
 
     state
         .buffer_memory_init_actions
         .extend(buffer.initialization_status.read().create_action(
             &buffer,
-            offset..(offset + vertex_state.total_size),
+            offset..(offset + buffer_size),
             MemoryInitKind::NeedsInitializedMemory,
         ));
 
@@ -2247,7 +2267,9 @@ fn set_vertex_buffer(
     unsafe {
         hal::DynCommandEncoder::set_vertex_buffer(state.raw_encoder, slot, bb);
     }
-    state.vertex.update_limits();
+    if let Some(pipeline) = state.pipeline.as_ref() {
+        state.vertex.update_limits(&pipeline.vertex_steps);
+    }
     Ok(())
 }
 
@@ -2374,24 +2396,14 @@ fn draw(
 
     state.is_ready(false)?;
 
-    let last_vertex = first_vertex as u64 + vertex_count as u64;
+    state
-    let vertex_limit = state.vertex.vertex_limit;
+        .vertex
-    if last_vertex > vertex_limit {
+        .limits
-        return Err(DrawError::VertexBeyondLimit {
+        .validate_vertex_limit(first_vertex, vertex_count)?;
-            last_vertex,
+    state
-            vertex_limit,
+        .vertex
-            slot: state.vertex.vertex_limit_slot,
+        .limits
-        });
+        .validate_instance_limit(first_instance, instance_count)?;
-    }
-    let last_instance = first_instance as u64 + instance_count as u64;
-    let instance_limit = state.vertex.instance_limit;
-    if last_instance > instance_limit {
-        return Err(DrawError::InstanceBeyondLimit {
-            last_instance,
-            instance_limit,
-            slot: state.vertex.instance_limit_slot,
-        });
-    }
 
     unsafe {
         if instance_count > 0 && vertex_count > 0 {
@@ -2423,15 +2435,10 @@ fn draw_indexed(
             index_limit,
         });
     }
-    let last_instance = first_instance as u64 + instance_count as u64;
+    state
-    let instance_limit = state.vertex.instance_limit;
+        .vertex
-    if last_instance > instance_limit {
+        .limits
-        return Err(DrawError::InstanceBeyondLimit {
+        .validate_instance_limit(first_instance, instance_count)?;
-            last_instance,
-            instance_limit,
-            slot: state.vertex.instance_limit_slot,
-        });
-    }
 
     unsafe {
         if instance_count > 0 && index_count > 0 {