Validate stripIndexFormat at draw time.

Updates validation logic to match the recent changes in
https://github.com/gpuweb/gpuweb/pull/2385 that allows stripIndexFormat
to be undefined at pipeline creation time, even for strip topologies.
Non indexed draw calls are valid with such pipelines. Indexed draw calls
fail validation at draw time.

Bug: dawn:1224
Change-Id: I28ff78eac726d46f99a099ffb2338b5da81a4a88
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/72000
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Brandon Jones <bajones@chromium.org>

src/dawn_native/CommandBufferStateTracker.cpp

@@ -245,11 +245,20 @@ namespace dawn_native {
 
             RenderPipelineBase* lastRenderPipeline = GetRenderPipeline();
             wgpu::IndexFormat pipelineIndexFormat = lastRenderPipeline->GetStripIndexFormat();
-            DAWN_INVALID_IF(
+
-                IsStripPrimitiveTopology(lastRenderPipeline->GetPrimitiveTopology()) &&
+            if (IsStripPrimitiveTopology(lastRenderPipeline->GetPrimitiveTopology())) {
+                DAWN_INVALID_IF(
+                    pipelineIndexFormat == wgpu::IndexFormat::Undefined,
+                    "%s has a strip primitive topology (%s) but a strip index format of %s, which "
+                    "prevents it for being used for indexed draw calls.",
+                    lastRenderPipeline, lastRenderPipeline->GetPrimitiveTopology(),
+                    pipelineIndexFormat);
+
+                DAWN_INVALID_IF(
                     mIndexFormat != pipelineIndexFormat,
-                "Strip index format (%s) of %s does not match index buffer format (%s).",
+                    "Strip index format (%s) of %s does not match index buffer format (%s).",
-                pipelineIndexFormat, lastRenderPipeline, mIndexFormat);
+                    pipelineIndexFormat, lastRenderPipeline, mIndexFormat);
+            }
 
             // The chunk of code above should be similar to the one in |RecomputeLazyAspects|.
             // It returns the first invalid state found. We shouldn't be able to reach this line

src/dawn_native/RenderPipeline.cpp

@@ -248,14 +248,9 @@ namespace dawn_native {
             DAWN_TRY(ValidateFrontFace(descriptor->frontFace));
             DAWN_TRY(ValidateCullMode(descriptor->cullMode));
 
-            // Pipeline descriptors must have stripIndexFormat != undefined IFF they are using strip
+            // Pipeline descriptors must have stripIndexFormat == undefined if they are using
-            // topologies.
+            // non-strip topologies.
-            if (IsStripPrimitiveTopology(descriptor->topology)) {
+            if (!IsStripPrimitiveTopology(descriptor->topology)) {
-                DAWN_INVALID_IF(
-                    descriptor->stripIndexFormat == wgpu::IndexFormat::Undefined,
-                    "StripIndexFormat is undefined when using a strip primitive topology (%s).",
-                    descriptor->topology);
-            } else {
                 DAWN_INVALID_IF(
                     descriptor->stripIndexFormat != wgpu::IndexFormat::Undefined,
                     "StripIndexFormat (%s) is not undefined when using a non-strip primitive "

src/tests/unittests/validation/IndexBufferValidationTests.cpp

@@ -155,6 +155,8 @@ TEST_F(IndexBufferValidationTest, IndexBufferFormatMatchesPipelineStripFormat) {
                                                        wgpu::PrimitiveTopology::TriangleStrip);
     wgpu::RenderPipeline pipeline16 = MakeTestPipeline(wgpu::IndexFormat::Uint16,
                                                        wgpu::PrimitiveTopology::LineStrip);
+    wgpu::RenderPipeline pipelineUndef =
+        MakeTestPipeline(wgpu::IndexFormat::Undefined, wgpu::PrimitiveTopology::LineStrip);
 
     wgpu::Buffer indexBuffer =
         utils::CreateBufferFromData<uint32_t>(device, wgpu::BufferUsage::Index, {0, 1, 2});
@@ -196,6 +198,31 @@ TEST_F(IndexBufferValidationTest, IndexBufferFormatMatchesPipelineStripFormat) {
         encoder.DrawIndexed(3);
         encoder.Finish();
     }
+
+    // Expected to fail because pipeline doesn't specify an index format.
+    {
+        wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc);
+        encoder.SetIndexBuffer(indexBuffer, wgpu::IndexFormat::Uint16);
+        encoder.SetPipeline(pipelineUndef);
+        encoder.DrawIndexed(3);
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+
+    {
+        wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc);
+        encoder.SetIndexBuffer(indexBuffer, wgpu::IndexFormat::Uint32);
+        encoder.SetPipeline(pipelineUndef);
+        encoder.DrawIndexed(3);
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+
+    // Expected to succeed because non-indexed draw calls don't require a pipeline index format.
+    {
+        wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc);
+        encoder.SetPipeline(pipelineUndef);
+        encoder.Draw(3);
+        encoder.Finish();
+    }
 }
 
 // Check that the index buffer must have the Index usage.

src/tests/unittests/validation/RenderPipelineValidationTests.cpp

@@ -727,8 +727,8 @@ TEST_F(RenderPipelineValidationTest, StorageBufferInVertexShaderNoLayout) {
     ASSERT_DEVICE_ERROR(device.CreateRenderPipeline(&descriptor));
 }
 
-// Tests that strip primitive topologies require an index format
+// Tests that only strip primitive topologies allow an index format
-TEST_F(RenderPipelineValidationTest, StripIndexFormatRequired) {
+TEST_F(RenderPipelineValidationTest, StripIndexFormatAllowed) {
     constexpr uint32_t kNumStripType = 2u;
     constexpr uint32_t kNumListType = 3u;
     constexpr uint32_t kNumIndexFormat = 3u;
@@ -751,14 +751,8 @@ TEST_F(RenderPipelineValidationTest, StripIndexFormatRequired) {
             descriptor.primitive.topology = primitiveTopology;
             descriptor.primitive.stripIndexFormat = indexFormat;
 
-            if (indexFormat == wgpu::IndexFormat::Undefined) {
+            // Always succeeds, regardless of if an index format is given.
-                // Fail because the index format is undefined and the primitive
+            device.CreateRenderPipeline(&descriptor);
-                // topology is a strip type.
-                ASSERT_DEVICE_ERROR(device.CreateRenderPipeline(&descriptor));
-            } else {
-                // Succeeds because the index format is given.
-                device.CreateRenderPipeline(&descriptor);
-            }
         }
     }