From 51c347a231e36bb1c06e033931d3c420b4ae86d6 Mon Sep 17 00:00:00 2001 From: Jiawei Shao Date: Thu, 12 Dec 2019 01:29:01 +0000 Subject: [PATCH] Add two missing checks on render pass descriptor This patch adds two missing checks on the render pass descriptor: 1. NaN is not allowed to be a value of clearColor and clearDepth. 2. Ensure only valid values can be used as loadOp and storeOp. This patch also adds the unit tests to ensure INFINITY is a valid value for both clearColor and clearDepth. BUG=dawn:299 Change-Id: Ia5500701ccd99abf488a80c87adb809521d7873f Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/14460 Reviewed-by: Austin Eng Reviewed-by: Corentin Wallez Commit-Queue: Jiawei Shao --- src/dawn_native/CommandEncoder.cpp | 23 ++++++ .../RenderPassDescriptorValidationTests.cpp | 76 +++++++++++++++++++ 2 files changed, 99 insertions(+) diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp index e0de3a215a..9a84237d7d 100644 --- a/src/dawn_native/CommandEncoder.cpp +++ b/src/dawn_native/CommandEncoder.cpp @@ -26,6 +26,7 @@ #include "dawn_native/ErrorData.h" #include "dawn_native/RenderPassEncoder.h" #include "dawn_native/RenderPipeline.h" +#include "dawn_native/ValidationUtils_autogen.h" #include "dawn_platform/DawnPlatform.h" #include "dawn_platform/tracing/TraceEvent.h" @@ -376,6 +377,18 @@ namespace dawn_native { "renderable"); } + DAWN_TRY(ValidateLoadOp(colorAttachment.loadOp)); + DAWN_TRY(ValidateStoreOp(colorAttachment.storeOp)); + + if (colorAttachment.loadOp == wgpu::LoadOp::Clear) { + if (std::isnan(colorAttachment.clearColor.r) || + std::isnan(colorAttachment.clearColor.g) || + std::isnan(colorAttachment.clearColor.b) || + std::isnan(colorAttachment.clearColor.a)) { + return DAWN_VALIDATION_ERROR("Color clear value cannot contain NaN"); + } + } + DAWN_TRY(ValidateOrSetColorAttachmentSampleCount(attachment, sampleCount)); DAWN_TRY(ValidateResolveTarget(device, colorAttachment)); @@ -404,6 +417,16 @@ namespace dawn_native { "depth stencil format"); } + DAWN_TRY(ValidateLoadOp(depthStencilAttachment->depthLoadOp)); + DAWN_TRY(ValidateLoadOp(depthStencilAttachment->stencilLoadOp)); + DAWN_TRY(ValidateStoreOp(depthStencilAttachment->depthStoreOp)); + DAWN_TRY(ValidateStoreOp(depthStencilAttachment->stencilStoreOp)); + + if (depthStencilAttachment->depthLoadOp == wgpu::LoadOp::Clear && + std::isnan(depthStencilAttachment->clearDepth)) { + return DAWN_VALIDATION_ERROR("Depth clear value cannot be NaN"); + } + // This validates that the depth storeOp and stencil storeOps are the same if (depthStencilAttachment->depthStoreOp != depthStencilAttachment->stencilStoreOp) { return DAWN_VALIDATION_ERROR( diff --git a/src/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp b/src/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp index 94d770bfac..d25b246250 100644 --- a/src/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp +++ b/src/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp @@ -18,6 +18,8 @@ #include "utils/WGPUHelpers.h" +#include + namespace { class RenderPassDescriptorValidationTest : public ValidationTest { @@ -661,6 +663,80 @@ TEST_F(MultisampledRenderPassDescriptorValidationTest, DepthStencilAttachmentSam } } +// Tests that NaN cannot be accepted as a valid color or depth clear value and INFINITY is valid in +// both color and depth clear values. +TEST_F(RenderPassDescriptorValidationTest, UseNaNOrINFINITYAsColorOrDepthClearValue) { + wgpu::TextureView color = Create2DAttachment(device, 1, 1, wgpu::TextureFormat::RGBA8Unorm); + + // Tests that NaN cannot be used in clearColor. + { + utils::ComboRenderPassDescriptor renderPass({color}, nullptr); + renderPass.cColorAttachments[0].clearColor.r = NAN; + AssertBeginRenderPassError(&renderPass); + } + + { + utils::ComboRenderPassDescriptor renderPass({color}, nullptr); + renderPass.cColorAttachments[0].clearColor.g = NAN; + AssertBeginRenderPassError(&renderPass); + } + + { + utils::ComboRenderPassDescriptor renderPass({color}, nullptr); + renderPass.cColorAttachments[0].clearColor.b = NAN; + AssertBeginRenderPassError(&renderPass); + } + + { + utils::ComboRenderPassDescriptor renderPass({color}, nullptr); + renderPass.cColorAttachments[0].clearColor.a = NAN; + AssertBeginRenderPassError(&renderPass); + } + + // Tests that INFINITY can be used in clearColor. + { + utils::ComboRenderPassDescriptor renderPass({color}, nullptr); + renderPass.cColorAttachments[0].clearColor.r = INFINITY; + AssertBeginRenderPassSuccess(&renderPass); + } + + { + utils::ComboRenderPassDescriptor renderPass({color}, nullptr); + renderPass.cColorAttachments[0].clearColor.g = INFINITY; + AssertBeginRenderPassSuccess(&renderPass); + } + + { + utils::ComboRenderPassDescriptor renderPass({color}, nullptr); + renderPass.cColorAttachments[0].clearColor.b = INFINITY; + AssertBeginRenderPassSuccess(&renderPass); + } + + { + utils::ComboRenderPassDescriptor renderPass({color}, nullptr); + renderPass.cColorAttachments[0].clearColor.a = INFINITY; + AssertBeginRenderPassSuccess(&renderPass); + } + + // Tests that NaN cannot be used in clearDepth. + { + wgpu::TextureView depth = + Create2DAttachment(device, 1, 1, wgpu::TextureFormat::Depth24Plus); + utils::ComboRenderPassDescriptor renderPass({color}, depth); + renderPass.cDepthStencilAttachmentInfo.clearDepth = NAN; + AssertBeginRenderPassError(&renderPass); + } + + // Tests that INFINITY can be used in clearDepth. + { + wgpu::TextureView depth = + Create2DAttachment(device, 1, 1, wgpu::TextureFormat::Depth24Plus); + utils::ComboRenderPassDescriptor renderPass({color}, depth); + renderPass.cDepthStencilAttachmentInfo.clearDepth = INFINITY; + AssertBeginRenderPassSuccess(&renderPass); + } +} + // TODO(cwallez@chromium.org): Constraints on attachment aliasing? } // anonymous namespace