From 6be313225eaae51e390fcc4db5aed12c4f306f89 Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Mon, 8 Jul 2019 10:12:16 +0000 Subject: [PATCH] Invert component names in RG11B10 and RGB10A2 The CL that introduced this formats reordered their components based on the name of the Vulkan formats, but it turns out that WebGPU lists components from low bits to high-bits while Vulkan PACK32 formats are listed from high to low. So the format component orders match, except for RGB10A2 which is actually BGR10A2 in Vulkan. Instead the Vulkan backend is updated to correctly use the RGB10A2 format. BUG=dawn:128 Change-Id: If7f045f020429c44c84b9aed34a4a80107208d5c Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/8601 Commit-Queue: Corentin Wallez Reviewed-by: Jiawei Shao --- dawn.json | 4 +-- src/dawn_native/Texture.cpp | 4 +-- src/dawn_native/metal/TextureMTL.mm | 7 ++--- src/dawn_native/vulkan/TextureVk.cpp | 6 ++-- src/tests/end2end/TextureFormatTests.cpp | 39 ++++++++++-------------- 5 files changed, 25 insertions(+), 35 deletions(-) diff --git a/dawn.json b/dawn.json index 44f1e7d451..34b5700e45 100644 --- a/dawn.json +++ b/dawn.json @@ -1075,8 +1075,8 @@ {"value": 25, "name": "RGBA8 sint"}, {"value": 26, "name": "BGRA8 unorm"}, {"value": 27, "name": "BGRA8 unorm srgb"}, - {"value": 28, "name": "A2 RGB10 unorm"}, - {"value": 29, "name": "B10 GR11 float"}, + {"value": 28, "name": "RGB10 A2 unorm"}, + {"value": 29, "name": "RG11 B10 float"}, {"value": 30, "name": "RG32 float"}, {"value": 31, "name": "RG32 uint"}, diff --git a/src/dawn_native/Texture.cpp b/src/dawn_native/Texture.cpp index f38c341882..2c3ffa5301 100644 --- a/src/dawn_native/Texture.cpp +++ b/src/dawn_native/Texture.cpp @@ -160,8 +160,8 @@ namespace dawn_native { case dawn::TextureFormat::RGBA8Sint: case dawn::TextureFormat::BGRA8Unorm: case dawn::TextureFormat::BGRA8UnormSrgb: - case dawn::TextureFormat::A2RGB10Unorm: - case dawn::TextureFormat::B10GR11Float: + case dawn::TextureFormat::RGB10A2Unorm: + case dawn::TextureFormat::RG11B10Float: return MakeColorFormat(format, true, 4); case dawn::TextureFormat::RG32Uint: diff --git a/src/dawn_native/metal/TextureMTL.mm b/src/dawn_native/metal/TextureMTL.mm index b049a5352f..0efe780e17 100644 --- a/src/dawn_native/metal/TextureMTL.mm +++ b/src/dawn_native/metal/TextureMTL.mm @@ -179,12 +179,9 @@ namespace dawn_native { namespace metal { return MTLPixelFormatBGRA8Unorm; case dawn::TextureFormat::BGRA8UnormSrgb: return MTLPixelFormatBGRA8Unorm_sRGB; - case dawn::TextureFormat::A2RGB10Unorm: - // TODO(cwallez@chromium.org): The format expectations are inverted compared to the - // implementation of the format in Metal. - UNREACHABLE(); + case dawn::TextureFormat::RGB10A2Unorm: return MTLPixelFormatRGB10A2Unorm; - case dawn::TextureFormat::B10GR11Float: + case dawn::TextureFormat::RG11B10Float: return MTLPixelFormatRG11B10Float; case dawn::TextureFormat::RG32Uint: diff --git a/src/dawn_native/vulkan/TextureVk.cpp b/src/dawn_native/vulkan/TextureVk.cpp index 0f92b1c1b5..ba1f4b5527 100644 --- a/src/dawn_native/vulkan/TextureVk.cpp +++ b/src/dawn_native/vulkan/TextureVk.cpp @@ -269,9 +269,9 @@ namespace dawn_native { namespace vulkan { return VK_FORMAT_B8G8R8A8_UNORM; case dawn::TextureFormat::BGRA8UnormSrgb: return VK_FORMAT_B8G8R8A8_SRGB; - case dawn::TextureFormat::A2RGB10Unorm: - return VK_FORMAT_A2R10G10B10_UNORM_PACK32; - case dawn::TextureFormat::B10GR11Float: + case dawn::TextureFormat::RGB10A2Unorm: + return VK_FORMAT_A2B10G10R10_UNORM_PACK32; + case dawn::TextureFormat::RG11B10Float: return VK_FORMAT_B10G11R11_UFLOAT_PACK32; case dawn::TextureFormat::RG32Uint: diff --git a/src/tests/end2end/TextureFormatTests.cpp b/src/tests/end2end/TextureFormatTests.cpp index d1ef7ffe34..b2232f4ac1 100644 --- a/src/tests/end2end/TextureFormatTests.cpp +++ b/src/tests/end2end/TextureFormatTests.cpp @@ -615,23 +615,18 @@ TEST_P(TextureFormatTest, BGRA8UnormSrgb) { 1.0e-3); } -// Test the A2RGB10Unorm format -// TODO(cwallez@chromium.org): This is actually RGB10A2 in WebGPU but Vulkan doesn't support that -// format. Do all platforms have A in the high bits? -TEST_P(TextureFormatTest, A2RGB10Unorm) { - // TODO(cwallez@chromium.org): The format R and B channel are inverted compared to what Metal - // does. - DAWN_SKIP_TEST_IF(IsMetal()); - auto MakeA2RGB10 = [](uint32_t r, uint32_t g, uint32_t b, uint32_t a) -> uint32_t { +// Test the RGB10A2Unorm format +TEST_P(TextureFormatTest, RGB10A2Unorm) { + auto MakeRGB10A2 = [](uint32_t r, uint32_t g, uint32_t b, uint32_t a) -> uint32_t { ASSERT((r & 0x3FF) == r); ASSERT((g & 0x3FF) == g); ASSERT((b & 0x3FF) == b); ASSERT((a & 0x3) == a); - return a << 30 | r << 20 | g << 10 | b; + return r | g << 10 | b << 20 | a << 30; }; - std::vector textureData = {MakeA2RGB10(0, 0, 0, 0), MakeA2RGB10(1023, 1023, 1023, 1), - MakeA2RGB10(243, 578, 765, 2), MakeA2RGB10(0, 0, 0, 3)}; + std::vector textureData = {MakeRGB10A2(0, 0, 0, 0), MakeRGB10A2(1023, 1023, 1023, 1), + MakeRGB10A2(243, 578, 765, 2), MakeRGB10A2(0, 0, 0, 3)}; // clang-format off std::vector expectedData = { 0.0f, 0.0f, 0.0f, 0.0f, @@ -641,14 +636,12 @@ TEST_P(TextureFormatTest, A2RGB10Unorm) { }; // clang-format on - DoSampleTest({dawn::TextureFormat::A2RGB10Unorm, 4, Float, 4}, textureData, expectedData, + DoSampleTest({dawn::TextureFormat::RGB10A2Unorm, 4, Float, 4}, textureData, expectedData, 2.0e-3); } -// Test the B10GR11Float format -// TODO(cwallez@chromium.org): This is actually GR11B10 in WebGPU but Vulkan doesn't support that -// format. Do all platforms have it reversed? -TEST_P(TextureFormatTest, B10GR11Float) { +// Test the RG11B10Float format +TEST_P(TextureFormatTest, RG11B10Float) { constexpr uint32_t kFloat11Zero = 0; constexpr uint32_t kFloat11Infinity = 0x7C0; constexpr uint32_t kFloat11Nan = 0x7C1; @@ -659,20 +652,20 @@ TEST_P(TextureFormatTest, B10GR11Float) { constexpr uint32_t kFloat10Nan = 0x3E1; constexpr uint32_t kFloat10One = 0x1E0; - auto MakeB10GR11 = [](uint32_t r, uint32_t g, uint32_t b) { + auto MakeRG11B10 = [](uint32_t r, uint32_t g, uint32_t b) { ASSERT((r & 0x7FF) == r); ASSERT((g & 0x7FF) == g); ASSERT((b & 0x3FF) == b); - return b << 22 | g << 11 | r; + return r | g << 11 | b << 22; }; // Test each of (0, 1, INFINITY, NaN) for each component but never two with the same value at a // time. std::vector textureData = { - MakeB10GR11(kFloat11Zero, kFloat11Infinity, kFloat10Nan), - MakeB10GR11(kFloat11Infinity, kFloat11Nan, kFloat10One), - MakeB10GR11(kFloat11Nan, kFloat11One, kFloat10Zero), - MakeB10GR11(kFloat11One, kFloat11Zero, kFloat10Infinity), + MakeRG11B10(kFloat11Zero, kFloat11Infinity, kFloat10Nan), + MakeRG11B10(kFloat11Infinity, kFloat11Nan, kFloat10One), + MakeRG11B10(kFloat11Nan, kFloat11One, kFloat10Zero), + MakeRG11B10(kFloat11One, kFloat11Zero, kFloat10Infinity), }; // This is one of the only 3-channel formats, so we don't have specific testing for them. Alpha @@ -686,7 +679,7 @@ TEST_P(TextureFormatTest, B10GR11Float) { }; // clang-format on - DoSampleTest({dawn::TextureFormat::B10GR11Float, 4, Float, 4}, textureData, expectedData); + DoSampleTest({dawn::TextureFormat::RG11B10Float, 4, Float, 4}, textureData, expectedData); } // TODO(cwallez@chromium.org): Add tests for depth-stencil formats when we know if they are copyable