diff --git a/src/dawn_native/CommandBufferStateTracker.cpp b/src/dawn_native/CommandBufferStateTracker.cpp index 3418753d53..6372a6de66 100644 --- a/src/dawn_native/CommandBufferStateTracker.cpp +++ b/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"); diff --git a/src/dawn_native/RenderEncoderBase.cpp b/src/dawn_native/RenderEncoderBase.cpp index e662309445..b25e6cc7a1 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 #include @@ -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) { diff --git a/src/dawn_native/RenderPipeline.cpp b/src/dawn_native/RenderPipeline.cpp index 1341ee5331..2d91291a75 100644 --- a/src/dawn_native/RenderPipeline.cpp +++ b/src/dawn_native/RenderPipeline.cpp @@ -85,6 +85,7 @@ namespace dawn_native { } MaybeError ValidateVertexStateDescriptor( + DeviceBase* device, const VertexStateDescriptor* descriptor, wgpu::PrimitiveTopology primitiveTopology, std::bitset* 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 (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 @@ 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 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; diff --git a/src/dawn_native/RenderPipeline.h b/src/dawn_native/RenderPipeline.h index aee8d3dbaf..f06abff5f8 100644 --- a/src/dawn_native/RenderPipeline.h +++ b/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; diff --git a/src/tests/unittests/validation/IndexBufferValidationTests.cpp b/src/tests/unittests/validation/IndexBufferValidationTests.cpp index b2b68b5a9a..d125d7942f 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 @@ 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(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 cc353c16e5..da539d2f09 100644 --- a/src/tests/unittests/validation/RenderPipelineValidationTests.cpp +++ b/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 - // 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); + } } } }