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 <brandon1.jones@intel.com>
Reviewed-by: Austin Eng <enga@chromium.org>

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<double, 4> ConvertToFloatToDoubleColor(dawn_native::Color color) {
-        const std::array<double, 4> outputValue = {
-            static_cast<double>(static_cast<float>(color.r)),
-            static_cast<double>(static_cast<float>(color.g)),
-            static_cast<double>(static_cast<float>(color.b)),
-            static_cast<double>(static_cast<float>(color.a))};
-        return outputValue;
-    }
-
     std::array<float, 4> ConvertToFloatColor(dawn_native::Color color) {
         const std::array<float, 4> outputValue = {
             static_cast<float>(color.r), static_cast<float>(color.g), static_cast<float>(color.b),
             static_cast<float>(color.a)};
         return outputValue;
     }
-    std::array<int32_t, 4> ConvertToFloatToSignedIntegerColor(dawn_native::Color color) {
+    std::array<int32_t, 4> ConvertToSignedIntegerColor(dawn_native::Color color) {
         const std::array<int32_t, 4> outputValue = {
-            static_cast<int32_t>(static_cast<float>(color.r)),
+            static_cast<int32_t>(color.r), static_cast<int32_t>(color.g),
-            static_cast<int32_t>(static_cast<float>(color.g)),
+            static_cast<int32_t>(color.b), static_cast<int32_t>(color.a)};
-            static_cast<int32_t>(static_cast<float>(color.b)),
-            static_cast<int32_t>(static_cast<float>(color.a))};
         return outputValue;
     }
 
-    std::array<uint32_t, 4> ConvertToFloatToUnsignedIntegerColor(dawn_native::Color color) {
+    std::array<uint32_t, 4> ConvertToUnsignedIntegerColor(dawn_native::Color color) {
         const std::array<uint32_t, 4> outputValue = {
-            static_cast<uint32_t>(static_cast<float>(color.r)),
+            static_cast<uint32_t>(color.r), static_cast<uint32_t>(color.g),
-            static_cast<uint32_t>(static_cast<float>(color.g)),
+            static_cast<uint32_t>(color.b), static_cast<uint32_t>(color.a)};
-            static_cast<uint32_t>(static_cast<float>(color.b)),
-            static_cast<uint32_t>(static_cast<float>(color.a))};
         return outputValue;
     }
 

src/dawn_native/CommandBuffer.h

@@ -64,9 +64,8 @@ namespace dawn_native {
     bool IsFullBufferOverwrittenInTextureToBufferCopy(const CopyTextureToBufferCmd* copy);
 
     std::array<float, 4> ConvertToFloatColor(dawn_native::Color color);
-    std::array<double, 4> ConvertToFloatToDoubleColor(dawn_native::Color color);
+    std::array<int32_t, 4> ConvertToSignedIntegerColor(dawn_native::Color color);
-    std::array<int32_t, 4> ConvertToFloatToSignedIntegerColor(dawn_native::Color color);
+    std::array<uint32_t, 4> ConvertToUnsignedIntegerColor(dawn_native::Color color);
-    std::array<uint32_t, 4> ConvertToFloatToUnsignedIntegerColor(dawn_native::Color color);
 
 }  // namespace dawn_native
 

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));

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<double, 4> 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<SetBlendColorCmd>();
-                    const std::array<double, 4> blendColor =
+                    [encoder setBlendColorRed:cmd->color.r
-                        ConvertToFloatToDoubleColor(cmd->color);
+                                        green:cmd->color.g
-                    [encoder setBlendColorRed:blendColor[0]
+                                         blue:cmd->color.b
-                                        green:blendColor[1]
+                                        alpha:cmd->color.a];
-                                         blue:blendColor[2]
-                                        alpha:blendColor[3]];
                     break;
                 }
 

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<uint32_t, 4> appliedClearColor =
-                            ConvertToFloatToUnsignedIntegerColor(attachmentInfo->clearColor);
+                            ConvertToUnsignedIntegerColor(attachmentInfo->clearColor);
                         gl.ClearBufferuiv(GL_COLOR, i, appliedClearColor.data());
                     } else if (attachmentFormat.HasComponentType(Format::Type::Sint)) {
                         const std::array<int32_t, 4> appliedClearColor =
-                            ConvertToFloatToSignedIntegerColor(attachmentInfo->clearColor);
+                            ConvertToSignedIntegerColor(attachmentInfo->clearColor);
                         gl.ClearBufferiv(GL_COLOR, i, appliedClearColor.data());
                     } else {
                         UNREACHABLE();

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<uint32_t, 4> 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<int32_t, 4> appliedClearColor =
-                            ConvertToFloatToSignedIntegerColor(attachmentInfo.clearColor);
+                            ConvertToSignedIntegerColor(attachmentInfo.clearColor);
                         for (uint32_t i = 0; i < 4; ++i) {
                             clearValues[attachmentCount].color.int32[i] = appliedClearColor[i];
                         }

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) {
+TEST_P(RenderPassLoadOpTests, LoadOpClear2ToThe24IntegerFormats) {
-    // Intel GPUs fail when we attempt to clear to a value that exceeds 2147483647 on a RGBA32Uint
+    constexpr double k2ToThe24Double = 16777216.0;
-    // texture.
+    constexpr uint32_t k2ToThe24Uint = static_cast<uint32_t>(k2ToThe24Double);
-    // Bug: dawn:530
-    DAWN_SKIP_TEST_IF(IsIntel() && IsD3D12());
 
     // RGBA32Uint
     {
-        constexpr wgpu::Color kClearColor = {4194966911.0, 3555555555.0, 2555555555.0,
+        constexpr wgpu::Color kClearColor = {k2ToThe24Double, k2ToThe24Double, k2ToThe24Double,
-                                             1555555555.0};
+                                             k2ToThe24Double};
-        constexpr std::array<uint32_t, 4> kExpectedPixelValue = {4194966784, 3555555584, 2555555584,
+        constexpr std::array<uint32_t, 4> kExpectedPixelValue = {k2ToThe24Uint, k2ToThe24Uint,
-                                                                 1555555584};
+                                                                 k2ToThe24Uint, k2ToThe24Uint};
         TestIntegerClearColor<uint32_t>(wgpu::TextureFormat::RGBA32Uint, kClearColor,
                                         kExpectedPixelValue);
     }
 
-    // RGBA32Sint
+    constexpr int32_t k2ToThe24Sint = static_cast<int32_t>(k2ToThe24Double);
+    // RGBA32Sint For +2^24
     {
-        constexpr wgpu::Color kClearColor = {2147483447.0, -2147483447.0, 1000000555.0,
+        constexpr wgpu::Color kClearColor = {k2ToThe24Double, k2ToThe24Double, k2ToThe24Double,
-                                             -1000000555.0};
+                                             k2ToThe24Double};
-        constexpr std::array<int32_t, 4> kExpectedPixelValue = {2147483392, -2147483392, 1000000576,
+        constexpr std::array<int32_t, 4> kExpectedPixelValue = {k2ToThe24Sint, k2ToThe24Sint,
-                                                                -1000000576};
+                                                                k2ToThe24Sint, k2ToThe24Sint};
+        TestIntegerClearColor<int32_t>(wgpu::TextureFormat::RGBA32Sint, kClearColor,
+                                       kExpectedPixelValue);
+    }
+
+    // RGBA32Sint For -2^24
+    {
+        constexpr wgpu::Color kClearColor = {-k2ToThe24Double, -k2ToThe24Double, -k2ToThe24Double,
+                                             -k2ToThe24Double};
+        constexpr std::array<int32_t, 4> kExpectedPixelValue = {-k2ToThe24Sint, -k2ToThe24Sint,
+                                                                -k2ToThe24Sint, -k2ToThe24Sint};
         TestIntegerClearColor<int32_t>(wgpu::TextureFormat::RGBA32Sint, kClearColor,
                                        kExpectedPixelValue);
     }

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<wgpu::TextureFormat, 2> 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);