From 69cdaf94dfb127fadc354883d423b63b4e4c3cb1 Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Thu, 12 Dec 2019 11:41:02 +0000 Subject: [PATCH] RenderPipeline: validate depth bias params are not NaN Also changes the sampler validation to allow INFINITY and only check for NaN. BUG=dawn:296 Change-Id: I2a61df807d37dcaf280b12a1ffe56dc670d0f455 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/14480 Commit-Queue: Corentin Wallez Reviewed-by: Austin Eng Reviewed-by: Kai Ninomiya --- src/dawn_native/RenderPipeline.cpp | 13 ++++++ src/dawn_native/Sampler.cpp | 14 +++--- .../RenderPipelineValidationTests.cpp | 46 +++++++++++++++++++ .../validation/SamplerValidationTests.cpp | 11 +++-- 4 files changed, 74 insertions(+), 10 deletions(-) diff --git a/src/dawn_native/RenderPipeline.cpp b/src/dawn_native/RenderPipeline.cpp index be87040387..ee28fcf9d0 100644 --- a/src/dawn_native/RenderPipeline.cpp +++ b/src/dawn_native/RenderPipeline.cpp @@ -115,8 +115,15 @@ namespace dawn_native { if (descriptor->nextInChain != nullptr) { return DAWN_VALIDATION_ERROR("nextInChain must be nullptr"); } + DAWN_TRY(ValidateFrontFace(descriptor->frontFace)); DAWN_TRY(ValidateCullMode(descriptor->cullMode)); + + if (std::isnan(descriptor->depthBiasSlopeScale) || + std::isnan(descriptor->depthBiasClamp)) { + return DAWN_VALIDATION_ERROR("Depth bias parameters must not be NaN."); + } + return {}; } @@ -709,6 +716,12 @@ namespace dawn_native { if (descA.frontFace != descB.frontFace || descA.cullMode != descB.cullMode) { return false; } + + ASSERT(!std::isnan(descA.depthBiasSlopeScale)); + ASSERT(!std::isnan(descB.depthBiasSlopeScale)); + ASSERT(!std::isnan(descA.depthBiasClamp)); + ASSERT(!std::isnan(descB.depthBiasClamp)); + if (descA.depthBias != descB.depthBias || descA.depthBiasSlopeScale != descB.depthBiasSlopeScale || descA.depthBiasClamp != descB.depthBiasClamp) { diff --git a/src/dawn_native/Sampler.cpp b/src/dawn_native/Sampler.cpp index 230da340e0..034a8c3eab 100644 --- a/src/dawn_native/Sampler.cpp +++ b/src/dawn_native/Sampler.cpp @@ -27,12 +27,12 @@ namespace dawn_native { return DAWN_VALIDATION_ERROR("nextInChain must be nullptr"); } - if (!std::isfinite(descriptor->lodMinClamp) || !std::isfinite(descriptor->lodMaxClamp)) { - return DAWN_VALIDATION_ERROR("LOD must be finite"); + if (std::isnan(descriptor->lodMinClamp) || std::isnan(descriptor->lodMaxClamp)) { + return DAWN_VALIDATION_ERROR("LOD clamp bounds must not be NaN"); } if (descriptor->lodMinClamp < 0 || descriptor->lodMaxClamp < 0) { - return DAWN_VALIDATION_ERROR("LOD must be positive"); + return DAWN_VALIDATION_ERROR("LOD clamp bounds must be positive"); } if (descriptor->lodMinClamp > descriptor->lodMaxClamp) { @@ -101,10 +101,10 @@ namespace dawn_native { return true; } - ASSERT(std::isfinite(a->mLodMinClamp)); - ASSERT(std::isfinite(b->mLodMinClamp)); - ASSERT(std::isfinite(a->mLodMaxClamp)); - ASSERT(std::isfinite(b->mLodMaxClamp)); + ASSERT(!std::isnan(a->mLodMinClamp)); + ASSERT(!std::isnan(b->mLodMinClamp)); + ASSERT(!std::isnan(a->mLodMaxClamp)); + ASSERT(!std::isnan(b->mLodMaxClamp)); return a->mAddressModeU == b->mAddressModeU && a->mAddressModeV == b->mAddressModeV && a->mAddressModeW == b->mAddressModeW && a->mMagFilter == b->mMagFilter && diff --git a/src/tests/unittests/validation/RenderPipelineValidationTests.cpp b/src/tests/unittests/validation/RenderPipelineValidationTests.cpp index 5349a3ae06..f8b3d42415 100644 --- a/src/tests/unittests/validation/RenderPipelineValidationTests.cpp +++ b/src/tests/unittests/validation/RenderPipelineValidationTests.cpp @@ -18,6 +18,7 @@ #include "utils/ComboRenderPipelineDescriptor.h" #include "utils/WGPUHelpers.h" +#include #include class RenderPipelineValidationTest : public ValidationTest { @@ -71,6 +72,51 @@ TEST_F(RenderPipelineValidationTest, CreationSuccess) { } } +// Tests that depth bias parameters must not be NaN. +TEST_F(RenderPipelineValidationTest, DepthBiasParameterNotBeNaN) { + // Control case, depth bias parameters in ComboRenderPipeline default to 0 which is finite + { + utils::ComboRenderPipelineDescriptor descriptor(device); + descriptor.vertexStage.module = vsModule; + descriptor.cFragmentStage.module = fsModule; + device.CreateRenderPipeline(&descriptor); + } + + // Infinite depth bias clamp is valid + { + utils::ComboRenderPipelineDescriptor descriptor(device); + descriptor.vertexStage.module = vsModule; + descriptor.cFragmentStage.module = fsModule; + descriptor.cRasterizationState.depthBiasClamp = INFINITY; + device.CreateRenderPipeline(&descriptor); + } + // NAN depth bias clamp is invalid + { + utils::ComboRenderPipelineDescriptor descriptor(device); + descriptor.vertexStage.module = vsModule; + descriptor.cFragmentStage.module = fsModule; + descriptor.cRasterizationState.depthBiasClamp = NAN; + ASSERT_DEVICE_ERROR(device.CreateRenderPipeline(&descriptor)); + } + + // Infinite depth bias slope is valid + { + utils::ComboRenderPipelineDescriptor descriptor(device); + descriptor.vertexStage.module = vsModule; + descriptor.cFragmentStage.module = fsModule; + descriptor.cRasterizationState.depthBiasSlopeScale = INFINITY; + device.CreateRenderPipeline(&descriptor); + } + // NAN depth bias slope is invalid + { + utils::ComboRenderPipelineDescriptor descriptor(device); + descriptor.vertexStage.module = vsModule; + descriptor.cFragmentStage.module = fsModule; + descriptor.cRasterizationState.depthBiasSlopeScale = NAN; + ASSERT_DEVICE_ERROR(device.CreateRenderPipeline(&descriptor)); + } +} + // Tests that at least one color state is required. TEST_F(RenderPipelineValidationTest, ColorStateRequired) { { diff --git a/src/tests/unittests/validation/SamplerValidationTests.cpp b/src/tests/unittests/validation/SamplerValidationTests.cpp index e262536500..96f9187708 100644 --- a/src/tests/unittests/validation/SamplerValidationTests.cpp +++ b/src/tests/unittests/validation/SamplerValidationTests.cpp @@ -24,6 +24,10 @@ namespace { // Test NaN and INFINITY values are not allowed TEST_F(SamplerValidationTest, InvalidLOD) { + { + wgpu::SamplerDescriptor samplerDesc = utils::GetDefaultSamplerDescriptor(); + device.CreateSampler(&samplerDesc); + } { wgpu::SamplerDescriptor samplerDesc = utils::GetDefaultSamplerDescriptor(); samplerDesc.lodMinClamp = NAN; @@ -36,13 +40,14 @@ namespace { } { wgpu::SamplerDescriptor samplerDesc = utils::GetDefaultSamplerDescriptor(); - samplerDesc.lodMinClamp = INFINITY; - ASSERT_DEVICE_ERROR(device.CreateSampler(&samplerDesc)); + samplerDesc.lodMaxClamp = INFINITY; + device.CreateSampler(&samplerDesc); } { wgpu::SamplerDescriptor samplerDesc = utils::GetDefaultSamplerDescriptor(); samplerDesc.lodMaxClamp = INFINITY; - ASSERT_DEVICE_ERROR(device.CreateSampler(&samplerDesc)); + samplerDesc.lodMinClamp = INFINITY; + device.CreateSampler(&samplerDesc); } }