From 232f4d183ce8ab93909b30af814e320c0f0db57c Mon Sep 17 00:00:00 2001 From: shrekshao Date: Tue, 23 Feb 2021 16:37:29 +0000 Subject: [PATCH] No-ops for out of bounds drawIndexed Store index buffer size in CommandBufferStateTracker and skip issuing drawIndexed call if it's out of bounds. Bug: dawn:622 Change-Id: I8f4bd8ba03dea931815dc0db87ffacb9936a123d Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/42000 Reviewed-by: Corentin Wallez Commit-Queue: Shrek Shao --- src/dawn_native/CommandBufferStateTracker.cpp | 3 +- src/dawn_native/CommandBufferStateTracker.h | 10 ++++++- src/dawn_native/RenderEncoderBase.cpp | 11 ++++++- src/tests/end2end/DrawIndexedTests.cpp | 29 +++++++++++++++++++ 4 files changed, 50 insertions(+), 3 deletions(-) diff --git a/src/dawn_native/CommandBufferStateTracker.cpp b/src/dawn_native/CommandBufferStateTracker.cpp index 44f821de71..00c8fd0483 100644 --- a/src/dawn_native/CommandBufferStateTracker.cpp +++ b/src/dawn_native/CommandBufferStateTracker.cpp @@ -210,9 +210,10 @@ namespace dawn_native { mAspects.reset(VALIDATION_ASPECT_BIND_GROUPS); } - void CommandBufferStateTracker::SetIndexBuffer(wgpu::IndexFormat format) { + void CommandBufferStateTracker::SetIndexBuffer(wgpu::IndexFormat format, uint64_t size) { mIndexBufferSet = true; mIndexFormat = format; + mIndexBufferSize = size; } void CommandBufferStateTracker::SetVertexBuffer(VertexBufferSlot slot) { diff --git a/src/dawn_native/CommandBufferStateTracker.h b/src/dawn_native/CommandBufferStateTracker.h index ac02330499..84a3e6dc53 100644 --- a/src/dawn_native/CommandBufferStateTracker.h +++ b/src/dawn_native/CommandBufferStateTracker.h @@ -38,12 +38,19 @@ namespace dawn_native { void SetComputePipeline(ComputePipelineBase* pipeline); void SetRenderPipeline(RenderPipelineBase* pipeline); void SetBindGroup(BindGroupIndex index, BindGroupBase* bindgroup); - void SetIndexBuffer(wgpu::IndexFormat format); + void SetIndexBuffer(wgpu::IndexFormat format, uint64_t size); void SetVertexBuffer(VertexBufferSlot slot); static constexpr size_t kNumAspects = 4; using ValidationAspects = std::bitset; + uint64_t GetIndexBufferSize() { + return mIndexBufferSize; + } + wgpu::IndexFormat GetIndexFormat() { + return mIndexFormat; + } + private: MaybeError ValidateOperation(ValidationAspects requiredAspects); void RecomputeLazyAspects(ValidationAspects aspects); @@ -57,6 +64,7 @@ namespace dawn_native { ityp::bitset mVertexBufferSlotsUsed; bool mIndexBufferSet = false; wgpu::IndexFormat mIndexFormat; + uint64_t mIndexBufferSize = 0; PipelineLayoutBase* mLastPipelineLayout = nullptr; RenderPipelineBase* mLastRenderPipeline = nullptr; diff --git a/src/dawn_native/RenderEncoderBase.cpp b/src/dawn_native/RenderEncoderBase.cpp index f309bb2664..a6be71cbc6 100644 --- a/src/dawn_native/RenderEncoderBase.cpp +++ b/src/dawn_native/RenderEncoderBase.cpp @@ -15,6 +15,7 @@ #include "dawn_native/RenderEncoderBase.h" #include "common/Constants.h" +#include "common/Log.h" #include "dawn_native/Buffer.h" #include "dawn_native/CommandEncoder.h" #include "dawn_native/CommandValidation.h" @@ -95,6 +96,14 @@ namespace dawn_native { } } + if (static_cast(firstIndex) + indexCount > + mCommandBufferState.GetIndexBufferSize() / + IndexFormatSize(mCommandBufferState.GetIndexFormat())) { + // Index range is out of bounds + // Treat as no-op and skip issuing draw call + dawn::WarningLog() << "Index range is out of bounds"; + return {}; + } DrawIndexedCmd* draw = allocator->Allocate(Command::DrawIndexed); draw->indexCount = indexCount; draw->instanceCount = instanceCount; @@ -235,7 +244,7 @@ namespace dawn_native { } } - mCommandBufferState.SetIndexBuffer(format); + mCommandBufferState.SetIndexBuffer(format, size); SetIndexBufferCmd* cmd = allocator->Allocate(Command::SetIndexBuffer); diff --git a/src/tests/end2end/DrawIndexedTests.cpp b/src/tests/end2end/DrawIndexedTests.cpp index c894624299..b4d7d59ffe 100644 --- a/src/tests/end2end/DrawIndexedTests.cpp +++ b/src/tests/end2end/DrawIndexedTests.cpp @@ -114,6 +114,35 @@ TEST_P(DrawIndexedTest, Uint32) { Test(6, 1, 0, 0, 0, 0, filled, filled); } +// Out of bounds drawIndexed are treated as no-ops instead of invalid operations +// Some agreements: https://github.com/gpuweb/gpuweb/issues/955 +TEST_P(DrawIndexedTest, OutOfBounds) { + RGBA8 filled(0, 255, 0, 255); + RGBA8 notFilled(0, 0, 0, 0); + + // a valid draw. + Test(6, 1, 0, 0, 0, 0, filled, filled); + // indexCount is 0 but firstIndex out of bound + Test(0, 1, 20, 0, 0, 0, notFilled, notFilled); + // indexCount + firstIndex out of bound + Test(6, 1, 7, 0, 0, 0, notFilled, notFilled); + // only firstIndex out of bound + Test(6, 1, 20, 0, 0, 0, notFilled, notFilled); + // firstIndex much larger than the bound + Test(6, 1, 10000, 0, 0, 0, notFilled, notFilled); + // only indexCount out of bound + Test(20, 1, 0, 0, 0, 0, notFilled, notFilled); + // indexCount much larger than the bound + Test(10000, 1, 0, 0, 0, 0, notFilled, notFilled); + // max uint32_t indexCount and firstIndex + Test(std::numeric_limits::max(), 1, std::numeric_limits::max(), 0, 0, 0, + notFilled, notFilled); + // max uint32_t indexCount and small firstIndex + Test(std::numeric_limits::max(), 1, 2, 0, 0, 0, notFilled, notFilled); + // small indexCount and max uint32_t firstIndex + Test(2, 1, std::numeric_limits::max(), 0, 0, 0, notFilled, notFilled); +} + // Test the parameter 'baseVertex' of DrawIndexed() works. TEST_P(DrawIndexedTest, BaseVertex) { DAWN_SKIP_TEST_IF(HasToggleEnabled("disable_base_vertex"));