Various cleanups for updated indexFormat handling

Addresses post-merge comments left by cwallez@ on
https://dawn-review.googlesource.com/c/dawn/+/27182

BUG=dawn:502

Change-Id: I9bce09da9bb46e92a4c613df2279bdefdd06d747
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/27761
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Brandon Jones <bajones@chromium.org>

src/dawn_native/CommandBufferStateTracker.cpp

@@ -131,7 +131,7 @@ namespace dawn_native {
                 wgpu::IndexFormat pipelineIndexFormat =
                     mLastRenderPipeline->GetVertexStateDescriptor()->indexFormat;
                 if (mIndexFormat != wgpu::IndexFormat::Undefined) {
-                    if (!mLastRenderPipeline->IsStripPrimitiveTopology() ||
+                    if (!IsStripPrimitiveTopology(mLastRenderPipeline->GetPrimitiveTopology()) ||
                         mIndexFormat == pipelineIndexFormat) {
                         mAspects.set(VALIDATION_ASPECT_INDEX_BUFFER);
                     }
@@ -155,7 +155,7 @@ namespace dawn_native {
             if (!mIndexBufferSet) {
                 return DAWN_VALIDATION_ERROR("Missing index buffer");
             } else if (mIndexFormat != wgpu::IndexFormat::Undefined &&
-                mLastRenderPipeline->IsStripPrimitiveTopology() &&
+                IsStripPrimitiveTopology(mLastRenderPipeline->GetPrimitiveTopology()) &&
                 mIndexFormat != pipelineIndexFormat) {
                 return DAWN_VALIDATION_ERROR(
                     "Pipeline strip index format does not match index buffer format");

src/dawn_native/RenderEncoderBase.cpp

@@ -20,6 +20,7 @@
 #include "dawn_native/Commands.h"
 #include "dawn_native/Device.h"
 #include "dawn_native/RenderPipeline.h"
+#include "dawn_native/ValidationUtils_autogen.h"
 
 #include <math.h>
 #include <cstring>
@@ -152,6 +153,7 @@ namespace dawn_native {
                                                  bool requireFormat) {
         mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError {
             DAWN_TRY(GetDevice()->ValidateObject(buffer));
+            DAWN_TRY(ValidateIndexFormat(format));
 
             uint64_t bufferSize = buffer->GetSize();
             if (offset > bufferSize) {

src/dawn_native/RenderPipeline.cpp

@@ -85,6 +85,7 @@ namespace dawn_native {
         }
 
         MaybeError ValidateVertexStateDescriptor(
+            DeviceBase* device,
             const VertexStateDescriptor* descriptor,
             wgpu::PrimitiveTopology primitiveTopology,
             std::bitset<kMaxVertexAttributes>* attributesSetMask) {
@@ -94,12 +95,14 @@ namespace dawn_native {
             DAWN_TRY(ValidateIndexFormat(descriptor->indexFormat));
 
             // Pipeline descriptors using strip topologies must not have an undefined index format.
-            if (descriptor->indexFormat == wgpu::IndexFormat::Undefined) {
+            if (IsStripPrimitiveTopology(primitiveTopology)) {
-                if (primitiveTopology == wgpu::PrimitiveTopology::LineStrip ||
+                if (descriptor->indexFormat == wgpu::IndexFormat::Undefined) {
-                    primitiveTopology == wgpu::PrimitiveTopology::TriangleStrip) {
                     return DAWN_VALIDATION_ERROR(
                         "indexFormat must not be undefined when using strip primitive topologies");
                 }
+            } else if (descriptor->indexFormat != wgpu::IndexFormat::Undefined) {
+                device->EmitDeprecationWarning(
+                    "Specifying an indexFormat when using list primitive topologies is deprecated");
             }
 
             if (descriptor->vertexBufferCount > kMaxVertexBuffers) {
@@ -293,7 +296,12 @@ namespace dawn_native {
         return VertexFormatNumComponents(format) * VertexFormatComponentSize(format);
     }
 
-    MaybeError ValidateRenderPipelineDescriptor(const DeviceBase* device,
+    bool IsStripPrimitiveTopology(wgpu::PrimitiveTopology primitiveTopology) {
+        return primitiveTopology == wgpu::PrimitiveTopology::LineStrip ||
+               primitiveTopology == wgpu::PrimitiveTopology::TriangleStrip;
+    }
+
+    MaybeError ValidateRenderPipelineDescriptor(DeviceBase* device,
                                                 const RenderPipelineDescriptor* descriptor) {
         if (descriptor->nextInChain != nullptr) {
             return DAWN_VALIDATION_ERROR("nextInChain must be nullptr");
@@ -312,7 +320,7 @@ namespace dawn_native {
 
         std::bitset<kMaxVertexAttributes> attributesSetMask;
         if (descriptor->vertexState) {
-            DAWN_TRY(ValidateVertexStateDescriptor(
+            DAWN_TRY(ValidateVertexStateDescriptor(device,
                 descriptor->vertexState, descriptor->primitiveTopology, &attributesSetMask));
         }
 
@@ -516,12 +524,6 @@ namespace dawn_native {
         return mPrimitiveTopology;
     }
 
-    bool RenderPipelineBase::IsStripPrimitiveTopology() const {
-        ASSERT(!IsError());
-        return mPrimitiveTopology == wgpu::PrimitiveTopology::LineStrip ||
-               mPrimitiveTopology == wgpu::PrimitiveTopology::TriangleStrip;
-    }
-
     wgpu::CullMode RenderPipelineBase::GetCullMode() const {
         ASSERT(!IsError());
         return mRasterizationState.cullMode;

src/dawn_native/RenderPipeline.h

@@ -30,12 +30,13 @@ namespace dawn_native {
     class DeviceBase;
     class RenderBundleEncoder;
 
-    MaybeError ValidateRenderPipelineDescriptor(const DeviceBase* device,
+    MaybeError ValidateRenderPipelineDescriptor(DeviceBase* device,
                                                 const RenderPipelineDescriptor* descriptor);
     size_t IndexFormatSize(wgpu::IndexFormat format);
     uint32_t VertexFormatNumComponents(wgpu::VertexFormat format);
     size_t VertexFormatComponentSize(wgpu::VertexFormat format);
     size_t VertexFormatSize(wgpu::VertexFormat format);
+    bool IsStripPrimitiveTopology(wgpu::PrimitiveTopology primitiveTopology);
 
     bool StencilTestEnabled(const DepthStencilStateDescriptor* mDepthStencilState);
     bool BlendEnabled(const ColorStateDescriptor* mColorState);
@@ -68,7 +69,6 @@ namespace dawn_native {
         const ColorStateDescriptor* GetColorStateDescriptor(uint32_t attachmentSlot) const;
         const DepthStencilStateDescriptor* GetDepthStencilStateDescriptor() const;
         wgpu::PrimitiveTopology GetPrimitiveTopology() const;
-        bool IsStripPrimitiveTopology() const;
         wgpu::CullMode GetCullMode() const;
         wgpu::FrontFace GetFrontFace() const;
 

src/tests/unittests/validation/IndexBufferValidationTests.cpp

@@ -15,8 +15,38 @@
 #include "tests/unittests/validation/ValidationTest.h"
 
 #include "utils/ComboRenderBundleEncoderDescriptor.h"
+#include "utils/ComboRenderPipelineDescriptor.h"
+#include "utils/WGPUHelpers.h"
 
-class IndexBufferValidationTest : public ValidationTest {};
+class IndexBufferValidationTest : public ValidationTest {
+    protected:
+    wgpu::RenderPipeline MakeTestPipeline(wgpu::IndexFormat format,
+        wgpu::PrimitiveTopology primitiveTopology) {
+        wgpu::ShaderModule vsModule =
+            utils::CreateShaderModule(device, utils::SingleShaderStage::Vertex, R"(
+                #version 450
+                void main() {
+                    gl_Position = vec4(0, 0, 0, 1);
+                })");
+
+        wgpu::ShaderModule fsModule =
+            utils::CreateShaderModule(device, utils::SingleShaderStage::Fragment, R"(
+                #version 450
+                layout(location = 0) out vec4 fragColor;
+                void main() {
+                    fragColor = vec4(0.0, 1.0, 0.0, 1.0);
+                })");
+
+        utils::ComboRenderPipelineDescriptor descriptor(device);
+        descriptor.vertexStage.module = vsModule;
+        descriptor.cFragmentStage.module = fsModule;
+        descriptor.primitiveTopology = primitiveTopology;
+        descriptor.cVertexState.indexFormat = format;
+        descriptor.cColorStates[0].format = wgpu::TextureFormat::RGBA8Unorm;
+
+        return device.CreateRenderPipeline(&descriptor);
+    }
+};
 
 // Test that for OOB validation of index buffer offset and size.
 TEST_F(IndexBufferValidationTest, IndexBufferOffsetOOBValidation) {
@@ -92,3 +122,53 @@ TEST_F(IndexBufferValidationTest, IndexBufferOffsetOOBValidation) {
         ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 }
+
+// Test that formats given when setting an index buffers must match the format specified on the
+// pipeline for strip primitive topologies.
+TEST_F(IndexBufferValidationTest, IndexBufferFormatMatchesPipelineStripFormat) {
+    wgpu::RenderPipeline pipeline32 = MakeTestPipeline(wgpu::IndexFormat::Uint32,
+                                                       wgpu::PrimitiveTopology::TriangleStrip);
+    wgpu::RenderPipeline pipeline16 = MakeTestPipeline(wgpu::IndexFormat::Uint16,
+                                                       wgpu::PrimitiveTopology::LineStrip);
+
+    wgpu::Buffer indexBuffer =
+        utils::CreateBufferFromData<uint32_t>(device, wgpu::BufferUsage::Index, {0, 1, 2});
+
+    utils::ComboRenderBundleEncoderDescriptor renderBundleDesc = {};
+    renderBundleDesc.colorFormatsCount = 1;
+    renderBundleDesc.cColorFormats[0] = wgpu::TextureFormat::RGBA8Unorm;
+
+    // Expected to fail because pipeline and index formats don't match.
+    {
+        wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc);
+        encoder.SetIndexBufferWithFormat(indexBuffer, wgpu::IndexFormat::Uint16);
+        encoder.SetPipeline(pipeline32);
+        encoder.DrawIndexed(3);
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+
+    {
+        wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc);
+        encoder.SetIndexBufferWithFormat(indexBuffer, wgpu::IndexFormat::Uint32);
+        encoder.SetPipeline(pipeline16);
+        encoder.DrawIndexed(3);
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+
+    // Expected to succeed because pipeline and index formats match.
+    {
+        wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc);
+        encoder.SetIndexBufferWithFormat(indexBuffer, wgpu::IndexFormat::Uint16);
+        encoder.SetPipeline(pipeline16);
+        encoder.DrawIndexed(3);
+        encoder.Finish();
+    }
+
+    {
+        wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc);
+        encoder.SetIndexBufferWithFormat(indexBuffer, wgpu::IndexFormat::Uint32);
+        encoder.SetPipeline(pipeline32);
+        encoder.DrawIndexed(3);
+        encoder.Finish();
+    }
+}

src/tests/unittests/validation/RenderPipelineValidationTests.cpp

@@ -577,9 +577,16 @@ TEST_F(RenderPipelineValidationTest, StripIndexFormatRequired) {
             descriptor.primitiveTopology = primitiveTopology;
             descriptor.cVertexState.indexFormat = indexFormat;
 
-            // Succeeds even when the index format is undefined because the
+            if (indexFormat == wgpu::IndexFormat::Undefined) {
-            // primitive topology isn't a strip type.
+                // Succeeds even when the index format is undefined because the
-            device.CreateRenderPipeline(&descriptor);
+                // primitive topology isn't a strip type.
+                device.CreateRenderPipeline(&descriptor);
+            } else {
+                // TODO(crbug.com/dawn/502): Once setIndexBuffer requires an
+                // indexFormat. this should fail. For now it succeeds to allow
+                // backwards compatibility during the deprecation period.
+                device.CreateRenderPipeline(&descriptor);
+            }
         }
     }
 }