From 16ebcf601d7f814ec1e017f499276b7da92d61b5 Mon Sep 17 00:00:00 2001 From: Brandon Jones Date: Thu, 1 Oct 2020 19:56:49 +0000 Subject: [PATCH] Limit Clear Color Values to 2^24 For Integer Formats Adds validation to ensure clear colors do not exceed 2^24 and a corresponding unit test. Also removes intermediate float conversions that are no longer necessary. Bug: dawn:525 Change-Id: I020b98de85384c20da51158de79eab87f60dcf6d Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/29040 Commit-Queue: Brandon Jones Reviewed-by: Austin Eng --- src/dawn_native/CommandBuffer.cpp | 29 ++---- src/dawn_native/CommandBuffer.h | 5 +- src/dawn_native/CommandEncoder.cpp | 20 +++++ src/dawn_native/metal/CommandBufferMTL.mm | 18 ++-- src/dawn_native/opengl/CommandBufferGL.cpp | 4 +- src/dawn_native/vulkan/CommandBufferVk.cpp | 4 +- src/tests/end2end/RenderPassLoadOpTests.cpp | 37 +++++--- .../RenderPassDescriptorValidationTests.cpp | 88 +++++++++++++++++++ 8 files changed, 150 insertions(+), 55 deletions(-) diff --git a/src/dawn_native/CommandBuffer.cpp b/src/dawn_native/CommandBuffer.cpp index 095a0d93cf..e4ec07756f 100644 --- a/src/dawn_native/CommandBuffer.cpp +++ b/src/dawn_native/CommandBuffer.cpp @@ -189,40 +189,23 @@ namespace dawn_native { return true; } - // The below functions convert a dawn_native::Color (doubles) to floats, then to the desired - // data type. This intermediate conversion to float must be done to ensure all backends produce - // consistent results. - - std::array ConvertToFloatToDoubleColor(dawn_native::Color color) { - const std::array outputValue = { - static_cast(static_cast(color.r)), - static_cast(static_cast(color.g)), - static_cast(static_cast(color.b)), - static_cast(static_cast(color.a))}; - return outputValue; - } - std::array ConvertToFloatColor(dawn_native::Color color) { const std::array outputValue = { static_cast(color.r), static_cast(color.g), static_cast(color.b), static_cast(color.a)}; return outputValue; } - std::array ConvertToFloatToSignedIntegerColor(dawn_native::Color color) { + std::array ConvertToSignedIntegerColor(dawn_native::Color color) { const std::array outputValue = { - static_cast(static_cast(color.r)), - static_cast(static_cast(color.g)), - static_cast(static_cast(color.b)), - static_cast(static_cast(color.a))}; + static_cast(color.r), static_cast(color.g), + static_cast(color.b), static_cast(color.a)}; return outputValue; } - std::array ConvertToFloatToUnsignedIntegerColor(dawn_native::Color color) { + std::array ConvertToUnsignedIntegerColor(dawn_native::Color color) { const std::array outputValue = { - static_cast(static_cast(color.r)), - static_cast(static_cast(color.g)), - static_cast(static_cast(color.b)), - static_cast(static_cast(color.a))}; + static_cast(color.r), static_cast(color.g), + static_cast(color.b), static_cast(color.a)}; return outputValue; } diff --git a/src/dawn_native/CommandBuffer.h b/src/dawn_native/CommandBuffer.h index 6d8a549e09..e90d320280 100644 --- a/src/dawn_native/CommandBuffer.h +++ b/src/dawn_native/CommandBuffer.h @@ -64,9 +64,8 @@ namespace dawn_native { bool IsFullBufferOverwrittenInTextureToBufferCopy(const CopyTextureToBufferCmd* copy); std::array ConvertToFloatColor(dawn_native::Color color); - std::array ConvertToFloatToDoubleColor(dawn_native::Color color); - std::array ConvertToFloatToSignedIntegerColor(dawn_native::Color color); - std::array ConvertToFloatToUnsignedIntegerColor(dawn_native::Color color); + std::array ConvertToSignedIntegerColor(dawn_native::Color color); + std::array ConvertToUnsignedIntegerColor(dawn_native::Color color); } // namespace dawn_native diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp index 0a8cfec3a6..fa56db03b6 100644 --- a/src/dawn_native/CommandEncoder.cpp +++ b/src/dawn_native/CommandEncoder.cpp @@ -292,6 +292,26 @@ namespace dawn_native { std::isnan(colorAttachment.clearColor.a)) { return DAWN_VALIDATION_ERROR("Color clear value cannot contain NaN"); } + + if (attachment->GetFormat().HasComponentType(Format::Type::Uint) && + (colorAttachment.clearColor.r < 0 || colorAttachment.clearColor.g < 0 || + colorAttachment.clearColor.b < 0 || colorAttachment.clearColor.a < 0)) { + return DAWN_VALIDATION_ERROR( + "Clear values less than zero are invalid for unsigned integer formats."); + } + + constexpr double k2ToThe24 = 16777216.0; + + if ((attachment->GetFormat().HasComponentType(Format::Type::Uint) || + attachment->GetFormat().HasComponentType(Format::Type::Sint)) && + (std::abs(colorAttachment.clearColor.r) > k2ToThe24 || + std::abs(colorAttachment.clearColor.g) > k2ToThe24 || + std::abs(colorAttachment.clearColor.b) > k2ToThe24 || + std::abs(colorAttachment.clearColor.a) > k2ToThe24)) { + return DAWN_VALIDATION_ERROR( + "Clear values with an absolute value of more than 16777216.0 (2^24) " + "are invalid for integer formats."); + } } DAWN_TRY(ValidateOrSetColorAttachmentSampleCount(attachment, sampleCount)); diff --git a/src/dawn_native/metal/CommandBufferMTL.mm b/src/dawn_native/metal/CommandBufferMTL.mm index b84fe13327..2a9d4aab8b 100644 --- a/src/dawn_native/metal/CommandBufferMTL.mm +++ b/src/dawn_native/metal/CommandBufferMTL.mm @@ -62,14 +62,12 @@ namespace dawn_native { namespace metal { auto& attachmentInfo = renderPass->colorAttachments[attachment]; switch (attachmentInfo.loadOp) { - case wgpu::LoadOp::Clear: { + case wgpu::LoadOp::Clear: descriptor.colorAttachments[i].loadAction = MTLLoadActionClear; - const std::array clearColor = - ConvertToFloatToDoubleColor(attachmentInfo.clearColor); descriptor.colorAttachments[i].clearColor = MTLClearColorMake( - clearColor[0], clearColor[1], clearColor[2], clearColor[3]); + attachmentInfo.clearColor.r, attachmentInfo.clearColor.g, + attachmentInfo.clearColor.b, attachmentInfo.clearColor.a); break; - } case wgpu::LoadOp::Load: descriptor.colorAttachments[i].loadAction = MTLLoadActionLoad; @@ -1240,12 +1238,10 @@ namespace dawn_native { namespace metal { case Command::SetBlendColor: { SetBlendColorCmd* cmd = mCommands.NextCommand(); - const std::array blendColor = - ConvertToFloatToDoubleColor(cmd->color); - [encoder setBlendColorRed:blendColor[0] - green:blendColor[1] - blue:blendColor[2] - alpha:blendColor[3]]; + [encoder setBlendColorRed:cmd->color.r + green:cmd->color.g + blue:cmd->color.b + alpha:cmd->color.a]; break; } diff --git a/src/dawn_native/opengl/CommandBufferGL.cpp b/src/dawn_native/opengl/CommandBufferGL.cpp index c7ce9a9f53..77d424a1ec 100644 --- a/src/dawn_native/opengl/CommandBufferGL.cpp +++ b/src/dawn_native/opengl/CommandBufferGL.cpp @@ -935,11 +935,11 @@ namespace dawn_native { namespace opengl { gl.ClearBufferfv(GL_COLOR, i, appliedClearColor.data()); } else if (attachmentFormat.HasComponentType(Format::Type::Uint)) { const std::array appliedClearColor = - ConvertToFloatToUnsignedIntegerColor(attachmentInfo->clearColor); + ConvertToUnsignedIntegerColor(attachmentInfo->clearColor); gl.ClearBufferuiv(GL_COLOR, i, appliedClearColor.data()); } else if (attachmentFormat.HasComponentType(Format::Type::Sint)) { const std::array appliedClearColor = - ConvertToFloatToSignedIntegerColor(attachmentInfo->clearColor); + ConvertToSignedIntegerColor(attachmentInfo->clearColor); gl.ClearBufferiv(GL_COLOR, i, appliedClearColor.data()); } else { UNREACHABLE(); diff --git a/src/dawn_native/vulkan/CommandBufferVk.cpp b/src/dawn_native/vulkan/CommandBufferVk.cpp index 9c431acc16..db080a8f09 100644 --- a/src/dawn_native/vulkan/CommandBufferVk.cpp +++ b/src/dawn_native/vulkan/CommandBufferVk.cpp @@ -317,13 +317,13 @@ namespace dawn_native { namespace vulkan { } } else if (attachmentFormat.HasComponentType(Format::Type::Uint)) { const std::array appliedClearColor = - ConvertToFloatToUnsignedIntegerColor(attachmentInfo.clearColor); + ConvertToUnsignedIntegerColor(attachmentInfo.clearColor); for (uint32_t i = 0; i < 4; ++i) { clearValues[attachmentCount].color.uint32[i] = appliedClearColor[i]; } } else if (attachmentFormat.HasComponentType(Format::Type::Sint)) { const std::array appliedClearColor = - ConvertToFloatToSignedIntegerColor(attachmentInfo.clearColor); + ConvertToSignedIntegerColor(attachmentInfo.clearColor); for (uint32_t i = 0; i < 4; ++i) { clearValues[attachmentCount].color.int32[i] = appliedClearColor[i]; } diff --git a/src/tests/end2end/RenderPassLoadOpTests.cpp b/src/tests/end2end/RenderPassLoadOpTests.cpp index 4e4bd88c5e..d75afac6fc 100644 --- a/src/tests/end2end/RenderPassLoadOpTests.cpp +++ b/src/tests/end2end/RenderPassLoadOpTests.cpp @@ -238,28 +238,37 @@ TEST_P(RenderPassLoadOpTests, LoadOpClearOnIntegerFormats) { // This test verifies that input double values are being rounded to floats internally when // clearing. -TEST_P(RenderPassLoadOpTests, LoadOpClearLargeIntegerValueRounding) { - // Intel GPUs fail when we attempt to clear to a value that exceeds 2147483647 on a RGBA32Uint - // texture. - // Bug: dawn:530 - DAWN_SKIP_TEST_IF(IsIntel() && IsD3D12()); +TEST_P(RenderPassLoadOpTests, LoadOpClear2ToThe24IntegerFormats) { + constexpr double k2ToThe24Double = 16777216.0; + constexpr uint32_t k2ToThe24Uint = static_cast(k2ToThe24Double); // RGBA32Uint { - constexpr wgpu::Color kClearColor = {4194966911.0, 3555555555.0, 2555555555.0, - 1555555555.0}; - constexpr std::array kExpectedPixelValue = {4194966784, 3555555584, 2555555584, - 1555555584}; + constexpr wgpu::Color kClearColor = {k2ToThe24Double, k2ToThe24Double, k2ToThe24Double, + k2ToThe24Double}; + constexpr std::array kExpectedPixelValue = {k2ToThe24Uint, k2ToThe24Uint, + k2ToThe24Uint, k2ToThe24Uint}; TestIntegerClearColor(wgpu::TextureFormat::RGBA32Uint, kClearColor, kExpectedPixelValue); } - // RGBA32Sint + constexpr int32_t k2ToThe24Sint = static_cast(k2ToThe24Double); + // RGBA32Sint For +2^24 { - constexpr wgpu::Color kClearColor = {2147483447.0, -2147483447.0, 1000000555.0, - -1000000555.0}; - constexpr std::array kExpectedPixelValue = {2147483392, -2147483392, 1000000576, - -1000000576}; + constexpr wgpu::Color kClearColor = {k2ToThe24Double, k2ToThe24Double, k2ToThe24Double, + k2ToThe24Double}; + constexpr std::array kExpectedPixelValue = {k2ToThe24Sint, k2ToThe24Sint, + k2ToThe24Sint, k2ToThe24Sint}; + TestIntegerClearColor(wgpu::TextureFormat::RGBA32Sint, kClearColor, + kExpectedPixelValue); + } + + // RGBA32Sint For -2^24 + { + constexpr wgpu::Color kClearColor = {-k2ToThe24Double, -k2ToThe24Double, -k2ToThe24Double, + -k2ToThe24Double}; + constexpr std::array kExpectedPixelValue = {-k2ToThe24Sint, -k2ToThe24Sint, + -k2ToThe24Sint, -k2ToThe24Sint}; TestIntegerClearColor(wgpu::TextureFormat::RGBA32Sint, kClearColor, kExpectedPixelValue); } diff --git a/src/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp b/src/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp index 277c12e32f..73e53c1406 100644 --- a/src/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp +++ b/src/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp @@ -762,6 +762,94 @@ namespace { } } + // Tests that values that require more than 24 bits to express are not allowed for integer + // formats. + TEST_F(RenderPassDescriptorValidationTest, ExceedValidColorClearRange) { + std::array formats = {wgpu::TextureFormat::RGBA32Sint, + wgpu::TextureFormat::RGBA32Uint}; + + constexpr double k2toThe24 = 16777216.0; + double kLargerThan2toPower24 = nextafter(k2toThe24, k2toThe24 + 1); + + for (wgpu::TextureFormat format : formats) { + wgpu::TextureView color = Create2DAttachment(device, 1, 1, format); + + // Tests that 16777216.0 is a valid clear color. + { + utils::ComboRenderPassDescriptor renderPass({color}, nullptr); + renderPass.cColorAttachments[0].clearColor.r = k2toThe24; + renderPass.cColorAttachments[0].clearColor.g = k2toThe24; + renderPass.cColorAttachments[0].clearColor.b = k2toThe24; + renderPass.cColorAttachments[0].clearColor.a = k2toThe24; + AssertBeginRenderPassSuccess(&renderPass); + } + + // Tests that 16777216.01 cannot be used as a clear color. + { + utils::ComboRenderPassDescriptor renderPass({color}, nullptr); + renderPass.cColorAttachments[0].clearColor.r = kLargerThan2toPower24; + AssertBeginRenderPassError(&renderPass); + } + + { + utils::ComboRenderPassDescriptor renderPass({color}, nullptr); + renderPass.cColorAttachments[0].clearColor.g = kLargerThan2toPower24; + AssertBeginRenderPassError(&renderPass); + } + + { + utils::ComboRenderPassDescriptor renderPass({color}, nullptr); + renderPass.cColorAttachments[0].clearColor.b = kLargerThan2toPower24; + AssertBeginRenderPassError(&renderPass); + } + + { + utils::ComboRenderPassDescriptor renderPass({color}, nullptr); + renderPass.cColorAttachments[0].clearColor.a = kLargerThan2toPower24; + AssertBeginRenderPassError(&renderPass); + } + + // Tests that -16777216.0 is a valid clear color for Sint, but not for Uint + { + utils::ComboRenderPassDescriptor renderPass({color}, nullptr); + renderPass.cColorAttachments[0].clearColor.r = -k2toThe24; + renderPass.cColorAttachments[0].clearColor.g = -k2toThe24; + renderPass.cColorAttachments[0].clearColor.b = -k2toThe24; + renderPass.cColorAttachments[0].clearColor.a = -k2toThe24; + if (format == wgpu::TextureFormat::RGBA32Sint) { + AssertBeginRenderPassSuccess(&renderPass); + } else if (format == wgpu::TextureFormat::RGBA32Uint) { + AssertBeginRenderPassError(&renderPass); + } + } + + // Tests that -16777216.01 cannot be used as a clear color. + { + utils::ComboRenderPassDescriptor renderPass({color}, nullptr); + renderPass.cColorAttachments[0].clearColor.r = -kLargerThan2toPower24; + AssertBeginRenderPassError(&renderPass); + } + + { + utils::ComboRenderPassDescriptor renderPass({color}, nullptr); + renderPass.cColorAttachments[0].clearColor.g = -kLargerThan2toPower24; + AssertBeginRenderPassError(&renderPass); + } + + { + utils::ComboRenderPassDescriptor renderPass({color}, nullptr); + renderPass.cColorAttachments[0].clearColor.b = -kLargerThan2toPower24; + AssertBeginRenderPassError(&renderPass); + } + + { + utils::ComboRenderPassDescriptor renderPass({color}, nullptr); + renderPass.cColorAttachments[0].clearColor.a = -kLargerThan2toPower24; + AssertBeginRenderPassError(&renderPass); + } + } + } + TEST_F(RenderPassDescriptorValidationTest, ValidateDepthStencilReadOnly) { wgpu::TextureView colorView = Create2DAttachment(device, 1, 1, wgpu::TextureFormat::RGBA8Unorm);