Unset vertex buffer: validations and unittests

If vertex buffer is null, SetVertexBuffer() actually unset that
buffer slot. This is a new feature in WebGPU. This change adds
validations and unittest in order to support this feature.

Bug: dawn:1675
Change-Id: Ia3e5d4196423590ff5b60ea78cc1e8cdd9c67d15
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/124842
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Yunchao He <yunchao.he@intel.com>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>

dawn.json

@@ -1903,7 +1903,7 @@
                 "name": "set vertex buffer",
                 "args": [
                     {"name": "slot", "type": "uint32_t"},
-                    {"name": "buffer", "type": "buffer"},
+                    {"name": "buffer", "type": "buffer", "optional": true},
                     {"name": "offset", "type": "uint64_t", "default": "0"},
                     {"name": "size", "type": "uint64_t", "default": "WGPU_WHOLE_SIZE"}
                 ]
@@ -2114,7 +2114,7 @@
                 "name": "set vertex buffer",
                 "args": [
                     {"name": "slot", "type": "uint32_t"},
-                    {"name": "buffer", "type": "buffer"},
+                    {"name": "buffer", "type": "buffer", "optional": true},
                     {"name": "offset", "type": "uint64_t", "default": "0"},
                     {"name": "size", "type": "uint64_t", "default": "WGPU_WHOLE_SIZE"}
                 ]

src/dawn/native/CommandBufferStateTracker.cpp

@@ -682,6 +682,11 @@ void CommandBufferStateTracker::SetIndexBuffer(wgpu::IndexFormat format, uint64_
     mIndexBufferSize = size;
 }
 
+void CommandBufferStateTracker::UnsetVertexBuffer(VertexBufferSlot slot) {
+    mVertexBufferSlotsUsed.set(slot, false);
+    mVertexBufferSizes[slot] = 0;
+}
+
 void CommandBufferStateTracker::SetVertexBuffer(VertexBufferSlot slot, uint64_t size) {
     mVertexBufferSlotsUsed.set(slot);
     mVertexBufferSizes[slot] = size;

src/dawn/native/CommandBufferStateTracker.h

@@ -53,6 +53,7 @@ class CommandBufferStateTracker {
                       uint32_t dynamicOffsetCount,
                       const uint32_t* dynamicOffsets);
     void SetIndexBuffer(wgpu::IndexFormat format, uint64_t size);
+    void UnsetVertexBuffer(VertexBufferSlot slot);
     void SetVertexBuffer(VertexBufferSlot slot, uint64_t size);
 
     static constexpr size_t kNumAspects = 4;

src/dawn/native/RenderEncoderBase.cpp

@@ -381,50 +381,60 @@ void RenderEncoderBase::APISetVertexBuffer(uint32_t slot,
         this,
         [&](CommandAllocator* allocator) -> MaybeError {
             if (IsValidationEnabled()) {
-                DAWN_TRY(GetDevice()->ValidateObject(buffer));
-                DAWN_TRY(ValidateCanUseAs(buffer, wgpu::BufferUsage::Vertex));
-
                 DAWN_INVALID_IF(slot >= kMaxVertexBuffers,
                                 "Vertex buffer slot (%u) is larger the maximum (%u)", slot,
                                 kMaxVertexBuffers - 1);
 
-                DAWN_INVALID_IF(offset % 4 != 0, "Vertex buffer offset (%u) is not a multiple of 4",
+                if (buffer == nullptr) {
-                                offset);
+                    DAWN_INVALID_IF(offset != 0, "Offset (%u) must be 0 if buffer is null", offset);
-
+                    DAWN_INVALID_IF(
-                uint64_t bufferSize = buffer->GetSize();
+                        size != 0 && size != wgpu::kWholeSize,
-                DAWN_INVALID_IF(offset > bufferSize,
+                        "Size (%u) must be either 0 or wgpu::kWholeSize if buffer is null", size);
-                                "Vertex buffer offset (%u) is larger than the size (%u) of %s.",
-                                offset, bufferSize, buffer);
-
-                uint64_t remainingSize = bufferSize - offset;
-
-                if (size == wgpu::kWholeSize) {
-                    size = remainingSize;
                 } else {
-                    DAWN_INVALID_IF(size > remainingSize,
+                    DAWN_TRY(GetDevice()->ValidateObject(buffer));
-                                    "Vertex buffer range (offset: %u, size: %u) doesn't fit in "
+                    DAWN_TRY(ValidateCanUseAs(buffer, wgpu::BufferUsage::Vertex));
-                                    "the size (%u) "
+                    DAWN_INVALID_IF(offset % 4 != 0,
-                                    "of %s.",
+                                    "Vertex buffer offset (%u) is not a multiple of 4", offset);
-                                    offset, size, bufferSize, buffer);
+
+                    uint64_t bufferSize = buffer->GetSize();
+                    DAWN_INVALID_IF(offset > bufferSize,
+                                    "Vertex buffer offset (%u) is larger than the size (%u) of %s.",
+                                    offset, bufferSize, buffer);
+
+                    uint64_t remainingSize = bufferSize - offset;
+
+                    if (size == wgpu::kWholeSize) {
+                        size = remainingSize;
+                    } else {
+                        DAWN_INVALID_IF(size > remainingSize,
+                                        "Vertex buffer range (offset: %u, size: %u) doesn't fit in "
+                                        "the size (%u) "
+                                        "of %s.",
+                                        offset, size, bufferSize, buffer);
+                    }
                 }
             } else {
-                if (size == wgpu::kWholeSize) {
+                if (size == wgpu::kWholeSize && buffer != nullptr) {
                     DAWN_ASSERT(buffer->GetSize() >= offset);
                     size = buffer->GetSize() - offset;
                 }
             }
 
-            mCommandBufferState.SetVertexBuffer(VertexBufferSlot(uint8_t(slot)), size);
+            VertexBufferSlot vbSlot = VertexBufferSlot(static_cast<uint8_t>(slot));
+            if (buffer == nullptr) {
+                mCommandBufferState.UnsetVertexBuffer(vbSlot);
+            } else {
+                mCommandBufferState.SetVertexBuffer(vbSlot, size);
 
-            SetVertexBufferCmd* cmd =
+                SetVertexBufferCmd* cmd =
-                allocator->Allocate<SetVertexBufferCmd>(Command::SetVertexBuffer);
+                    allocator->Allocate<SetVertexBufferCmd>(Command::SetVertexBuffer);
-            cmd->slot = VertexBufferSlot(static_cast<uint8_t>(slot));
+                cmd->slot = vbSlot;
-            cmd->buffer = buffer;
+                cmd->buffer = buffer;
-            cmd->offset = offset;
+                cmd->offset = offset;
-            cmd->size = size;
+                cmd->size = size;
-
-            mUsageTracker.BufferUsedAs(buffer, wgpu::BufferUsage::Vertex);
 
+                mUsageTracker.BufferUsedAs(buffer, wgpu::BufferUsage::Vertex);
+            }
             return {};
         },
         "encoding %s.SetVertexBuffer(%u, %s, %u, %u).", this, slot, buffer, offset, size);

src/dawn/tests/unittests/validation/VertexBufferValidationTests.cpp

@@ -104,6 +104,61 @@ class VertexBufferValidationTest : public ValidationTest {
     wgpu::ShaderModule fsModule;
 };
 
+// Check that unset vertex buffer works.
+TEST_F(VertexBufferValidationTest, UnsetVertexBuffer) {
+    PlaceholderRenderPass renderPass(device);
+    wgpu::ShaderModule vsModule = MakeVertexShader(1);
+
+    wgpu::RenderPipeline pipeline = MakeRenderPipeline(vsModule, 1);
+
+    wgpu::Buffer vertexBuffer = MakeVertexBuffer();
+    wgpu::Buffer vb;
+
+    // Control case: set the vertex buffer needed by a pipeline in render pass is valid.
+    {
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+        pass.SetPipeline(pipeline);
+        pass.SetVertexBuffer(0, vertexBuffer);
+        pass.Draw(3);
+        pass.End();
+        encoder.Finish();
+    }
+    // Error case: unset the vertex buffer needed by a pipeline in render pass is an error.
+    {
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+        pass.SetPipeline(pipeline);
+        pass.SetVertexBuffer(0, vertexBuffer);
+        pass.SetVertexBuffer(0, vb);
+        pass.Draw(3);
+        pass.End();
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+
+    utils::ComboRenderBundleEncoderDescriptor renderBundleDesc = {};
+    renderBundleDesc.colorFormatsCount = 1;
+    renderBundleDesc.cColorFormats[0] = wgpu::TextureFormat::RGBA8Unorm;
+    // Control case: set the vertex buffer needed by a pipeline in render bundle encoder is valid.
+    {
+        wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc);
+        encoder.SetPipeline(pipeline);
+        encoder.SetVertexBuffer(0, vertexBuffer);
+        encoder.Draw(3);
+        encoder.Finish();
+    }
+    // Error case: unset the vertex buffer needed by a pipeline in render bundle encoder is an
+    // error.
+    {
+        wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc);
+        encoder.SetPipeline(pipeline);
+        encoder.SetVertexBuffer(0, vertexBuffer);
+        encoder.SetVertexBuffer(0, vb);
+        encoder.Draw(3);
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+}
+
 // Check that vertex buffers still count as bound if we switch the pipeline.
 TEST_F(VertexBufferValidationTest, VertexBuffersInheritedBetweenPipelines) {
     PlaceholderRenderPass renderPass(device);
@@ -141,6 +196,55 @@ TEST_F(VertexBufferValidationTest, VertexBuffersInheritedBetweenPipelines) {
     encoder.Finish();
 }
 
+// Check that inherited vertex buffers can be unset, and the unset operation does not impact
+// previous pipeline.
+TEST_F(VertexBufferValidationTest, UnsetInheritedVertexBuffers) {
+    PlaceholderRenderPass renderPass(device);
+    wgpu::ShaderModule vsModule2 = MakeVertexShader(2);
+    wgpu::ShaderModule vsModule1 = MakeVertexShader(1);
+
+    wgpu::RenderPipeline pipeline2 = MakeRenderPipeline(vsModule2, 2);
+    wgpu::RenderPipeline pipeline1 = MakeRenderPipeline(vsModule1, 1);
+
+    wgpu::Buffer vertexBuffer1 = MakeVertexBuffer();
+    wgpu::Buffer vertexBuffer2 = MakeVertexBuffer();
+    wgpu::Buffer vb;
+
+    // Control case: inherited vertex buffers can be unset, and the unset operation does not impact
+    // previous pipeline.
+    wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+    {
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+        pass.SetPipeline(pipeline2);
+        pass.SetVertexBuffer(0, vertexBuffer1);
+        pass.SetVertexBuffer(1, vertexBuffer2);
+        pass.Draw(3);
+        pass.SetPipeline(pipeline1);
+        pass.SetVertexBuffer(1, vb);
+        pass.Draw(3);
+        pass.End();
+        encoder.Finish();
+    }
+
+    // Error case: inherited vertex buffers can be unset, incorrect unset operation can make the
+    // pipeline lack of vertex buffer.
+    {
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+        pass.SetPipeline(pipeline2);
+        pass.SetVertexBuffer(0, vertexBuffer1);
+        pass.SetVertexBuffer(1, vertexBuffer2);
+        pass.Draw(3);
+        pass.SetPipeline(pipeline1);
+        pass.SetVertexBuffer(1, vb);
+        pass.SetVertexBuffer(0, vb);
+        pass.Draw(3);
+        pass.End();
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+}
+
 // Check that vertex buffers that are set are reset between render passes.
 TEST_F(VertexBufferValidationTest, VertexBuffersNotInheritedBetweenRenderPasses) {
     PlaceholderRenderPass renderPass(device);
@@ -194,6 +298,7 @@ TEST_F(VertexBufferValidationTest, VertexBuffersNotInheritedBetweenRenderPasses)
 // Check validation of the vertex buffer slot for OOB.
 TEST_F(VertexBufferValidationTest, VertexBufferSlotValidation) {
     wgpu::Buffer buffer = MakeVertexBuffer();
+    wgpu::Buffer vb;
 
     PlaceholderRenderPass renderPass(device);
 
@@ -215,6 +320,24 @@ TEST_F(VertexBufferValidationTest, VertexBufferSlotValidation) {
         ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 
+    // Control case: unset the last vertex buffer slot in render passes is ok.
+    {
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+        pass.SetVertexBuffer(kMaxVertexBuffers - 1, vb);
+        pass.End();
+        encoder.Finish();
+    }
+
+    // Error case: unset past the last vertex buffer slot in render pass fails.
+    {
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+        pass.SetVertexBuffer(kMaxVertexBuffers, vb);
+        pass.End();
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+
     utils::ComboRenderBundleEncoderDescriptor renderBundleDesc = {};
     renderBundleDesc.colorFormatsCount = 1;
     renderBundleDesc.cColorFormats[0] = wgpu::TextureFormat::RGBA8Unorm;
@@ -232,6 +355,32 @@ TEST_F(VertexBufferValidationTest, VertexBufferSlotValidation) {
         encoder.SetVertexBuffer(kMaxVertexBuffers, buffer, 0);
         ASSERT_DEVICE_ERROR(encoder.Finish());
     }
+
+    // Control case: unset the last vertex buffer slot in render bundles is ok.
+    {
+        wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc);
+        encoder.SetVertexBuffer(kMaxVertexBuffers - 1, vb);
+        encoder.Finish();
+    }
+
+    // Error case: unset past the last vertex buffer slot in render bundle fails.
+    {
+        wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc);
+        encoder.SetVertexBuffer(kMaxVertexBuffers, vb);
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+}
+
+// Test that it is valid to unset a slot which is not set before.
+TEST_F(VertexBufferValidationTest, UnsetANonSetSlot) {
+    wgpu::Buffer vb;
+
+    PlaceholderRenderPass renderPass(device);
+    wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+    wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+    pass.SetVertexBuffer(0, vb);
+    pass.End();
+    encoder.Finish();
 }
 
 // Test that for OOB validation of vertex buffer offset and size.
@@ -306,6 +455,54 @@ TEST_F(VertexBufferValidationTest, VertexBufferOffsetOOBValidation) {
     }
 }
 
+// Test that both offset and size must be 0 when unset vertex buffer.
+TEST_F(VertexBufferValidationTest, UnsetVertexBufferWithInvalidOffsetAndSize) {
+    wgpu::Buffer buffer = MakeVertexBuffer();
+    wgpu::Buffer vb;
+
+    PlaceholderRenderPass renderPass(device);
+    // Control case, it valid when both offset and size are 0.
+    {
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+        pass.SetVertexBuffer(0, buffer, 0, 256);
+        pass.SetVertexBuffer(0, vb, 0, 0);
+        pass.End();
+        encoder.Finish();
+    }
+
+    // It's valid to set size to wgpu::kWholeSize when unset vertex buffer and offset is 0, because
+    // kWholeSize of a null buffer is considered to be 0.
+    {
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+        pass.SetVertexBuffer(0, buffer, 0, 256);
+        pass.SetVertexBuffer(0, vb, 0, wgpu::kWholeSize);
+        pass.End();
+        encoder.Finish();
+    }
+
+    // Invalid offset
+    {
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+        pass.SetVertexBuffer(0, buffer, 0, 256);
+        pass.SetVertexBuffer(0, vb, 4, 0);
+        pass.End();
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+
+    // Invalid size
+    {
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+        pass.SetVertexBuffer(0, buffer, 0, 256);
+        pass.SetVertexBuffer(0, vb, 0, 256);
+        pass.End();
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+}
+
 // Check that the vertex buffer must have the Vertex usage.
 TEST_F(VertexBufferValidationTest, InvalidUsage) {
     wgpu::Buffer vertexBuffer = MakeVertexBuffer();