From 8c9858e9b8e36b8c13606b1053133ab395c03418 Mon Sep 17 00:00:00 2001 From: Jiawei Shao Date: Sat, 1 Aug 2020 04:18:17 +0000 Subject: [PATCH] Fix clearing sint/uint color attachments on Vulkan and OpenGL This patch fixes a bug on the clear of color attachments with signed or unsigned integer formats on Vulkan and OpenGL by using the correct APIs to set the clear color for signed/unsigned integer formats. BUG=dawn:497 Change-Id: If1bc9858875e6384e71c15bb6770fbbb10045037 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/26041 Reviewed-by: Austin Eng Commit-Queue: Jiawei Shao --- src/dawn_native/CommandBuffer.cpp | 14 ++++ src/dawn_native/CommandBuffer.h | 3 + src/dawn_native/opengl/CommandBufferGL.cpp | 20 +++-- src/dawn_native/vulkan/CommandBufferVk.cpp | 25 +++++- src/tests/end2end/RenderPassLoadOpTests.cpp | 89 +++++++++++++++++++++ 5 files changed, 142 insertions(+), 9 deletions(-) diff --git a/src/dawn_native/CommandBuffer.cpp b/src/dawn_native/CommandBuffer.cpp index e1446ab756..8329e95d9a 100644 --- a/src/dawn_native/CommandBuffer.cpp +++ b/src/dawn_native/CommandBuffer.cpp @@ -175,4 +175,18 @@ namespace dawn_native { return true; } + + std::array ConvertToSignedIntegerColor(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 ConvertToUnsignedIntegerColor(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; + } } // namespace dawn_native diff --git a/src/dawn_native/CommandBuffer.h b/src/dawn_native/CommandBuffer.h index 375f102810..11076a16b4 100644 --- a/src/dawn_native/CommandBuffer.h +++ b/src/dawn_native/CommandBuffer.h @@ -51,6 +51,9 @@ namespace dawn_native { bool IsFullBufferOverwrittenInTextureToBufferCopy(const CopyTextureToBufferCmd* copy); + std::array ConvertToSignedIntegerColor(dawn_native::Color color); + std::array ConvertToUnsignedIntegerColor(dawn_native::Color color); + } // namespace dawn_native #endif // DAWNNATIVE_COMMANDBUFFER_H_ diff --git a/src/dawn_native/opengl/CommandBufferGL.cpp b/src/dawn_native/opengl/CommandBufferGL.cpp index 8bb099c070..c15b943eb8 100644 --- a/src/dawn_native/opengl/CommandBufferGL.cpp +++ b/src/dawn_native/opengl/CommandBufferGL.cpp @@ -913,13 +913,23 @@ namespace dawn_native { namespace opengl { auto* attachmentInfo = &renderPass->colorAttachments[i]; // Load op - color - // TODO(cwallez@chromium.org): Choose the clear function depending on the - // componentType: things work for now because the clear color is always a float, but - // when that's fixed will lose precision on integer formats when converting to - // float. if (attachmentInfo->loadOp == wgpu::LoadOp::Clear) { gl.ColorMaski(i, true, true, true, true); - gl.ClearBufferfv(GL_COLOR, i, &attachmentInfo->clearColor.r); + + const Format& attachmentFormat = attachmentInfo->view->GetFormat(); + if (attachmentFormat.HasComponentType(Format::Type::Float)) { + gl.ClearBufferfv(GL_COLOR, i, &attachmentInfo->clearColor.r); + } else if (attachmentFormat.HasComponentType(Format::Type::Uint)) { + const std::array appliedClearColor = + ConvertToUnsignedIntegerColor(attachmentInfo->clearColor); + gl.ClearBufferuiv(GL_COLOR, i, appliedClearColor.data()); + } else if (attachmentFormat.HasComponentType(Format::Type::Sint)) { + const std::array appliedClearColor = + ConvertToSignedIntegerColor(attachmentInfo->clearColor); + gl.ClearBufferiv(GL_COLOR, i, appliedClearColor.data()); + } else { + UNREACHABLE(); + } } if (attachmentInfo->storeOp == wgpu::StoreOp::Clear) { diff --git a/src/dawn_native/vulkan/CommandBufferVk.cpp b/src/dawn_native/vulkan/CommandBufferVk.cpp index 9e281dce9e..8315b1b787 100644 --- a/src/dawn_native/vulkan/CommandBufferVk.cpp +++ b/src/dawn_native/vulkan/CommandBufferVk.cpp @@ -234,10 +234,27 @@ namespace dawn_native { namespace vulkan { attachments[attachmentCount] = view->GetHandle(); - clearValues[attachmentCount].color.float32[0] = attachmentInfo.clearColor.r; - clearValues[attachmentCount].color.float32[1] = attachmentInfo.clearColor.g; - clearValues[attachmentCount].color.float32[2] = attachmentInfo.clearColor.b; - clearValues[attachmentCount].color.float32[3] = attachmentInfo.clearColor.a; + const Format& attachmentFormat = view->GetFormat(); + if (attachmentFormat.HasComponentType(Format::Type::Float)) { + clearValues[attachmentCount].color.float32[0] = attachmentInfo.clearColor.r; + clearValues[attachmentCount].color.float32[1] = attachmentInfo.clearColor.g; + clearValues[attachmentCount].color.float32[2] = attachmentInfo.clearColor.b; + clearValues[attachmentCount].color.float32[3] = attachmentInfo.clearColor.a; + } else if (attachmentFormat.HasComponentType(Format::Type::Uint)) { + const std::array appliedClearColor = + 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 = + ConvertToSignedIntegerColor(attachmentInfo.clearColor); + for (uint32_t i = 0; i < 4; ++i) { + clearValues[attachmentCount].color.int32[i] = appliedClearColor[i]; + } + } else { + UNREACHABLE(); + } attachmentCount++; } diff --git a/src/tests/end2end/RenderPassLoadOpTests.cpp b/src/tests/end2end/RenderPassLoadOpTests.cpp index 011c6e0fe6..42a5c79a50 100644 --- a/src/tests/end2end/RenderPassLoadOpTests.cpp +++ b/src/tests/end2end/RenderPassLoadOpTests.cpp @@ -95,6 +95,44 @@ class RenderPassLoadOpTests : public DawnTest { blueQuad = DrawQuad(device, vsSource, fsSource); } + template + void TestIntegerClearColor(wgpu::TextureFormat format, + const wgpu::Color& clearColor, + const std::array& expectedPixelValue) { + constexpr wgpu::Extent3D kTextureSize = {1, 1, 1}; + + wgpu::TextureDescriptor textureDescriptor; + textureDescriptor.dimension = wgpu::TextureDimension::e2D; + textureDescriptor.size = kTextureSize; + textureDescriptor.usage = + wgpu::TextureUsage::OutputAttachment | wgpu::TextureUsage::CopySrc; + textureDescriptor.format = format; + wgpu::Texture texture = device.CreateTexture(&textureDescriptor); + + utils::ComboRenderPassDescriptor renderPassDescriptor({texture.CreateView()}); + renderPassDescriptor.cColorAttachments[0].clearColor = clearColor; + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder renderPass = encoder.BeginRenderPass(&renderPassDescriptor); + renderPass.EndPass(); + + const uint64_t bufferSize = sizeof(T) * expectedPixelValue.size(); + wgpu::BufferDescriptor bufferDescriptor; + bufferDescriptor.size = bufferSize; + bufferDescriptor.usage = wgpu::BufferUsage::CopySrc | wgpu::BufferUsage::CopyDst; + wgpu::Buffer buffer = device.CreateBuffer(&bufferDescriptor); + + wgpu::TextureCopyView textureCopyView = utils::CreateTextureCopyView(texture, 0, {0, 0, 0}); + wgpu::BufferCopyView bufferCopyView = + utils::CreateBufferCopyView(buffer, 0, kTextureBytesPerRowAlignment, 0); + encoder.CopyTextureToBuffer(&textureCopyView, &bufferCopyView, &kTextureSize); + + wgpu::CommandBuffer commandBuffer = encoder.Finish(); + queue.Submit(1, &commandBuffer); + + EXPECT_BUFFER_U32_RANGE_EQ(reinterpret_cast(expectedPixelValue.data()), + buffer, 0, bufferSize / sizeof(uint32_t)); + } + wgpu::Texture renderTarget; wgpu::TextureView renderTargetView; @@ -147,6 +185,57 @@ TEST_P(RenderPassLoadOpTests, ColorClearThenLoadAndDraw) { 0, 0); } +// Test clearing a color attachment with signed and unsigned integer formats. +TEST_P(RenderPassLoadOpTests, LoadOpClearOnIntegerFormats) { + // RGBA8Uint + { + constexpr wgpu::Color kClearColor = {2.f, 3.3f, 254.8f, 255.0f}; + constexpr std::array kExpectedPixelValue = {2, 3, 254, 255}; + TestIntegerClearColor(wgpu::TextureFormat::RGBA8Uint, kClearColor, + kExpectedPixelValue); + } + + // RGBA8Sint + { + constexpr wgpu::Color kClearColor = {2.f, -3.3f, 126.8f, -128.0f}; + constexpr std::array kExpectedPixelValue = {2, -3, 126, -128}; + TestIntegerClearColor(wgpu::TextureFormat::RGBA8Sint, kClearColor, + kExpectedPixelValue); + } + + // RGBA16Uint + { + constexpr wgpu::Color kClearColor = {2.f, 3.3f, 512.7f, 65535.f}; + constexpr std::array kExpectedPixelValue = {2, 3, 512, 65535u}; + TestIntegerClearColor(wgpu::TextureFormat::RGBA16Uint, kClearColor, + kExpectedPixelValue); + } + + // RGBA16Sint + { + constexpr wgpu::Color kClearColor = {2.f, -3.3f, 32767.8f, -32768.0f}; + constexpr std::array kExpectedPixelValue = {2, -3, 32767, -32768}; + TestIntegerClearColor(wgpu::TextureFormat::RGBA16Sint, kClearColor, + kExpectedPixelValue); + } + + // RGBA32Uint + { + constexpr wgpu::Color kClearColor = {2.f, 3.3f, 65534.8f, 65537.f}; + constexpr std::array kExpectedPixelValue = {2, 3, 65534, 65537}; + TestIntegerClearColor(wgpu::TextureFormat::RGBA32Uint, kClearColor, + kExpectedPixelValue); + } + + // RGBA32Sint + { + constexpr wgpu::Color kClearColor = {2.f, -3.3f, 65534.8f, -65537.f}; + constexpr std::array kExpectedPixelValue = {2, -3, 65534, -65537}; + TestIntegerClearColor(wgpu::TextureFormat::RGBA32Sint, kClearColor, + kExpectedPixelValue); + } +} + DAWN_INSTANTIATE_TEST(RenderPassLoadOpTests, D3D12Backend(), MetalBackend(),