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>
diff --git a/src/dawn_native/CommandBufferStateTracker.cpp b/src/dawn_native/CommandBufferStateTracker.cpp
index 3418753..6372a6d 100644
--- a/src/dawn_native/CommandBufferStateTracker.cpp
+++ b/src/dawn_native/CommandBufferStateTracker.cpp
@@ -131,7 +131,7 @@
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 @@
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");
diff --git a/src/dawn_native/RenderEncoderBase.cpp b/src/dawn_native/RenderEncoderBase.cpp
index e662309..b25e6cc 100644
--- a/src/dawn_native/RenderEncoderBase.cpp
+++ b/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 @@
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) {
diff --git a/src/dawn_native/RenderPipeline.cpp b/src/dawn_native/RenderPipeline.cpp
index 1341ee5..2d91291 100644
--- a/src/dawn_native/RenderPipeline.cpp
+++ b/src/dawn_native/RenderPipeline.cpp
@@ -85,6 +85,7 @@
}
MaybeError ValidateVertexStateDescriptor(
+ DeviceBase* device,
const VertexStateDescriptor* descriptor,
wgpu::PrimitiveTopology primitiveTopology,
std::bitset<kMaxVertexAttributes>* attributesSetMask) {
@@ -94,12 +95,14 @@
DAWN_TRY(ValidateIndexFormat(descriptor->indexFormat));
// Pipeline descriptors using strip topologies must not have an undefined index format.
- if (descriptor->indexFormat == wgpu::IndexFormat::Undefined) {
- if (primitiveTopology == wgpu::PrimitiveTopology::LineStrip ||
- primitiveTopology == wgpu::PrimitiveTopology::TriangleStrip) {
+ if (IsStripPrimitiveTopology(primitiveTopology)) {
+ if (descriptor->indexFormat == wgpu::IndexFormat::Undefined) {
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 @@
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 @@
std::bitset<kMaxVertexAttributes> attributesSetMask;
if (descriptor->vertexState) {
- DAWN_TRY(ValidateVertexStateDescriptor(
+ DAWN_TRY(ValidateVertexStateDescriptor(device,
descriptor->vertexState, descriptor->primitiveTopology, &attributesSetMask));
}
@@ -516,12 +524,6 @@
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;
diff --git a/src/dawn_native/RenderPipeline.h b/src/dawn_native/RenderPipeline.h
index aee8d3d..f06abff 100644
--- a/src/dawn_native/RenderPipeline.h
+++ b/src/dawn_native/RenderPipeline.h
@@ -30,12 +30,13 @@
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 @@
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;
diff --git a/src/tests/unittests/validation/IndexBufferValidationTests.cpp b/src/tests/unittests/validation/IndexBufferValidationTests.cpp
index b2b68b5..d125d79 100644
--- a/src/tests/unittests/validation/IndexBufferValidationTests.cpp
+++ b/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 @@
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();
+ }
+}
diff --git a/src/tests/unittests/validation/RenderPipelineValidationTests.cpp b/src/tests/unittests/validation/RenderPipelineValidationTests.cpp
index cc353c1..da539d2f 100644
--- a/src/tests/unittests/validation/RenderPipelineValidationTests.cpp
+++ b/src/tests/unittests/validation/RenderPipelineValidationTests.cpp
@@ -577,9 +577,16 @@
descriptor.primitiveTopology = primitiveTopology;
descriptor.cVertexState.indexFormat = indexFormat;
- // Succeeds even when the index format is undefined because the
- // primitive topology isn't a strip type.
- device.CreateRenderPipeline(&descriptor);
+ if (indexFormat == wgpu::IndexFormat::Undefined) {
+ // Succeeds even when the index format is undefined because the
+ // 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);
+ }
}
}
}
