From 64ed7d183bef79ae6035d8566836692487ac6799 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Fri, 11 Oct 2024 22:51:47 +0200 Subject: [PATCH] Vk: properly fix the ImageVkTest. Not sure what I did in 3e4e1bde69dde03e24da98935f9d56bc9cd1eec9 but that updated XFAIL is now an XPASS on NVidia. Because, apparently, the clear clears the whole memory, not just the image area, so even though the row pitch is different, the comparison of the initial N bytes passes. So I'm ditching the silly XFAILs and doing a proper image comparison that includes the actual driver-dependent row pitch for the images. Finally, the image-to-image copy was flat out wrong because it didn't take the *input* row pitch into account, so it copied garbage and then compared to a different kind of garbage. --- package/ci/unix-desktop-vulkan.sh | 3 +- src/Magnum/Vk/Test/CMakeLists.txt | 2 +- src/Magnum/Vk/Test/ImageVkTest.cpp | 144 +++++++++++++++++++---------- 3 files changed, 97 insertions(+), 52 deletions(-) diff --git a/package/ci/unix-desktop-vulkan.sh b/package/ci/unix-desktop-vulkan.sh index 66e2a752b..2931dc452 100755 --- a/package/ci/unix-desktop-vulkan.sh +++ b/package/ci/unix-desktop-vulkan.sh @@ -32,7 +32,8 @@ cmake .. \ -DCMAKE_INSTALL_PREFIX=$HOME/deps \ -DCMAKE_BUILD_TYPE=Debug \ -DMAGNUM_WITH_AUDIO=OFF \ - `# Needed by VkMeshVkTest, together with TgaImporter and AnyImageImporter` \ + `# Needed by VkImageVkTest, and by VkMeshVkTest, together with` \ + `# TgaImporter and AnyImageImporter` \ -DMAGNUM_WITH_DEBUGTOOLS=ON \ -DMAGNUM_WITH_GL=OFF \ -DMAGNUM_WITH_MATERIALTOOLS=OFF \ diff --git a/src/Magnum/Vk/Test/CMakeLists.txt b/src/Magnum/Vk/Test/CMakeLists.txt index 8402f1027..4625b124d 100644 --- a/src/Magnum/Vk/Test/CMakeLists.txt +++ b/src/Magnum/Vk/Test/CMakeLists.txt @@ -167,7 +167,7 @@ if(MAGNUM_BUILD_VK_TESTS) corrade_add_test(VkFenceVkTest FenceVkTest.cpp LIBRARIES MagnumVk MagnumVulkanTester) corrade_add_test(VkFramebufferVkTest FramebufferVkTest.cpp LIBRARIES MagnumVk MagnumVulkanTester) corrade_add_test(VkLayerPropertiesVkTest LayerPropertiesVkTest.cpp LIBRARIES MagnumVkTestLib) - corrade_add_test(VkImageVkTest ImageVkTest.cpp LIBRARIES MagnumVkTestLib MagnumVulkanTester) + corrade_add_test(VkImageVkTest ImageVkTest.cpp LIBRARIES MagnumVkTestLib MagnumDebugTools MagnumVulkanTester) corrade_add_test(VkImageViewVkTest ImageViewVkTest.cpp LIBRARIES MagnumVk MagnumVulkanTester) corrade_add_test(VkInstanceVkTest InstanceVkTest.cpp LIBRARIES MagnumVkTestLib) corrade_add_test(VkMemoryVkTest MemoryVkTest.cpp LIBRARIES MagnumVk MagnumVulkanTester) diff --git a/src/Magnum/Vk/Test/ImageVkTest.cpp b/src/Magnum/Vk/Test/ImageVkTest.cpp index 803241c13..a9d6f363c 100644 --- a/src/Magnum/Vk/Test/ImageVkTest.cpp +++ b/src/Magnum/Vk/Test/ImageVkTest.cpp @@ -32,8 +32,11 @@ #include #include +#include "Magnum/ImageView.h" +#include "Magnum/PixelFormat.h" #include "Magnum/Math/Color.h" #include "Magnum/Math/Range.h" +#include "Magnum/DebugTools/CompareImage.h" #include "Magnum/Vk/BufferCreateInfo.h" #include "Magnum/Vk/CommandPoolCreateInfo.h" #include "Magnum/Vk/CommandBuffer.h" @@ -376,8 +379,11 @@ void ImageVkTest::cmdClearColorImageFloat() { .end(); queue().submit({SubmitInfo{}.setCommandBuffers({cmd})}).wait(); - /* Check if the image is actually tightly packed for the comparison */ - /** @todo make this a builtin, returning a StridedArrayView for pixels */ + /* The image memory may be linear but still with some extra row padding, or + with extra data at the end. Fetch its layout properties and use the + image comparator for robustness. */ + /** @todo make this a builtin, returning an ImageView or StridedArrayView + for pixels */ VkSubresourceLayout layout; { VkImageSubresource subresource{}; @@ -386,17 +392,19 @@ void ImageVkTest::cmdClearColorImageFloat() { subresource.mipLevel = 0; device()->GetImageSubresourceLayout(device(), a, &subresource, &layout); } - CORRADE_EXPECT_FAIL_IF(layout.rowPitch != 4*4, - "The image doesn't have a tightly-packed memory, the following check won't work."); - - /* The image memory may be tightly packed but still actually larger than - what was requested. Compare just the prefix. */ - CORRADE_COMPARE_AS(Containers::arrayCast(a.dedicatedMemory().mapRead().prefix(4*4*4)), Containers::arrayView({ + const Containers::Array actual = a.dedicatedMemory().mapRead(); + const Color4ub expected[]{ 0xdeadc0de_rgba, 0xdeadc0de_rgba, 0xdeadc0de_rgba, 0xdeadc0de_rgba, 0xdeadc0de_rgba, 0xdeadc0de_rgba, 0xdeadc0de_rgba, 0xdeadc0de_rgba, 0xdeadc0de_rgba, 0xdeadc0de_rgba, 0xdeadc0de_rgba, 0xdeadc0de_rgba, 0xdeadc0de_rgba, 0xdeadc0de_rgba, 0xdeadc0de_rgba, 0xdeadc0de_rgba - }), TestSuite::Compare::Container); + }; + CORRADE_COMPARE_AS( + (ImageView2D{ + PixelStorage{}.setRowLength(layout.rowPitch/4), + Magnum::PixelFormat::RGBA8Unorm, {4, 4}, actual}), + (ImageView2D{Magnum::PixelFormat::RGBA8Unorm, {4, 4}, expected}), + DebugTools::CompareImage); } void ImageVkTest::cmdClearColorImageSignedIntegral() { @@ -429,8 +437,11 @@ void ImageVkTest::cmdClearColorImageSignedIntegral() { .end(); queue().submit({SubmitInfo{}.setCommandBuffers({cmd})}).wait(); - /* Check if the image is actually tightly packed for the comparison */ - /** @todo make this a builtin, returning a StridedArrayView for pixels */ + /* The image memory may be linear but still with some extra row padding, or + with extra data at the end. Fetch its layout properties and use the + image comparator for robustness. */ + /** @todo make this a builtin, returning an ImageView or StridedArrayView + for pixels */ VkSubresourceLayout layout; { VkImageSubresource subresource{}; @@ -439,17 +450,19 @@ void ImageVkTest::cmdClearColorImageSignedIntegral() { subresource.mipLevel = 0; device()->GetImageSubresourceLayout(device(), a, &subresource, &layout); } - CORRADE_EXPECT_FAIL_IF(layout.rowPitch != 4*4, - "The image doesn't have a tightly-packed memory, the following check won't work."); - - /* The image memory may be tightly packed but still actually larger than - what was requested. Compare just the prefix. */ - CORRADE_COMPARE_AS(Containers::arrayCast(a.dedicatedMemory().mapRead().prefix(4*4*4)), Containers::arrayView({ + const Containers::Array actual = a.dedicatedMemory().mapRead(); + const Vector4b expected[]{ {15, -7, 2, -1}, {15, -7, 2, -1}, {15, -7, 2, -1}, {15, -7, 2, -1}, {15, -7, 2, -1}, {15, -7, 2, -1}, {15, -7, 2, -1}, {15, -7, 2, -1}, {15, -7, 2, -1}, {15, -7, 2, -1}, {15, -7, 2, -1}, {15, -7, 2, -1}, {15, -7, 2, -1}, {15, -7, 2, -1}, {15, -7, 2, -1}, {15, -7, 2, -1}, - }), TestSuite::Compare::Container); + }; + CORRADE_COMPARE_AS( + (ImageView2D{ + PixelStorage{}.setRowLength(layout.rowPitch/4), + Magnum::PixelFormat::RGBA8I, {4, 4}, actual}), + (ImageView2D{Magnum::PixelFormat::RGBA8I, {4, 4}, expected}), + DebugTools::CompareImage); } void ImageVkTest::cmdClearColorImageUnsignedIntegral() { @@ -492,17 +505,19 @@ void ImageVkTest::cmdClearColorImageUnsignedIntegral() { subresource.mipLevel = 0; device()->GetImageSubresourceLayout(device(), a, &subresource, &layout); } - CORRADE_EXPECT_FAIL_IF(layout.rowPitch != 4*4, - "The image doesn't have a tightly-packed memory, the following check won't work."); - - /* The image memory may be tightly packed but still actually larger than - what was requested. Compare just the prefix. */ - CORRADE_COMPARE_AS(Containers::arrayCast(a.dedicatedMemory().mapRead().prefix(4*4*4)), Containers::arrayView({ + const Containers::Array actual = a.dedicatedMemory().mapRead(); + const Vector4ub expected[]{ {15, 37, 2, 1}, {15, 37, 2, 1}, {15, 37, 2, 1}, {15, 37, 2, 1}, {15, 37, 2, 1}, {15, 37, 2, 1}, {15, 37, 2, 1}, {15, 37, 2, 1}, {15, 37, 2, 1}, {15, 37, 2, 1}, {15, 37, 2, 1}, {15, 37, 2, 1}, {15, 37, 2, 1}, {15, 37, 2, 1}, {15, 37, 2, 1}, {15, 37, 2, 1}, - }), TestSuite::Compare::Container); + }; + CORRADE_COMPARE_AS( + (ImageView2D{ + PixelStorage{}.setRowLength(layout.rowPitch/4), + Magnum::PixelFormat::RGBA8I, {4, 4}, actual}), + (ImageView2D{Magnum::PixelFormat::RGBA8I, {4, 4}, expected}), + DebugTools::CompareImage); } void ImageVkTest::cmdClearDepthStencilImage() { @@ -661,28 +676,40 @@ void ImageVkTest::cmdCopyImage2D() { device().properties().pickQueueFamily(QueueFlag::Graphics)}}; CommandBuffer cmd = pool.allocate(); - /* To avoid going through a buffer which can guarantee the packing we want, - the tests uses a linear tiling image. These are poorly supported, have - weird paddings and the required allocation size is usually much larger - than expected. To prevent issues as much as possible, we'll thus create - images with non-insane sizes (so not 6 or 7 pixels wide, but 8), 4-byte - pixel format and explicitly slice the mapped memory. */ - /* Source image */ ImageCreateInfo2D aInfo{ImageUsage::TransferSource, PixelFormat::RGBA8UI, {8, 10}, 1, 1, ImageLayout::Preinitialized}; aInfo->tiling = VK_IMAGE_TILING_LINEAR; Image a{device(), aInfo, MemoryFlag::HostVisible}; - Utility::copy("________________________________" - "________________________________" - "________________________________" - "________________________________" - "____________AaaaAaaaAaaaAaaa____" - "____________BbbbBbbbBbbbBbbb____" - "____________CcccCcccCcccCccc____" - "____________DdddDdddDdddDddd____" - "________________________________" - "________________________________"_s, a.dedicatedMemory().map().prefix(8*10*4)); + + /* The image memory may be linear but still with some extra row padding, or + with extra data at the end. Fetch its layout properties and create an + appropriate strided array view for the copy. */ + /** @todo make this a builtin, returning an ImageView or StridedArrayView + for pixels */ + VkSubresourceLayout aLayout; + { + VkImageSubresource subresource{}; + subresource.arrayLayer = 0; + subresource.aspectMask = VK_IMAGE_ASPECT_COLOR_BIT; + subresource.mipLevel = 0; + device()->GetImageSubresourceLayout(device(), a, &subresource, &aLayout); + } + Utility::copy(Containers::stridedArrayView( + "________________________________" + "________________________________" + "________________________________" + "________________________________" + "____________AaaaAaaaAaaaAaaa____" + "____________BbbbBbbbBbbbBbbb____" + "____________CcccCcccCcccCccc____" + "____________DdddDdddDdddDddd____" + "________________________________" + "________________________________"_s).expanded<0, 2>({10, 8*4}), + Containers::StridedArrayView2D{a.dedicatedMemory().map(), + {10, 8*4}, + {std::ptrdiff_t(aLayout.rowPitch), 1}, + }); /* Destination image */ ImageCreateInfo2D bInfo{ImageUsage::TransferDestination, @@ -712,14 +739,31 @@ void ImageVkTest::cmdCopyImage2D() { .end(); queue().submit({SubmitInfo{}.setCommandBuffers({cmd})}).wait(); - CORRADE_EXPECT_FAIL_IF(b.dedicatedMemory().size() != 8*5*4 && !device().properties().name().hasPrefix("SwiftShader"), - "The image doesn't have a tightly-packed memory, the following check won't work."); - CORRADE_COMPARE(b.dedicatedMemory().mapRead().prefix(8*5*4), - "--------------------------------" - "----AaaaAaaaAaaaAaaa------------" - "----BbbbBbbbBbbbBbbb------------" - "----CcccCcccCcccCccc------------" - "----DdddDdddDdddDddd------------"_s); + /* The image memory may be linear but still with some extra row padding, or + with extra data at the end. Fetch its layout properties and use the + image comparator for robustness. */ + /** @todo make this a builtin, returning an ImageView or StridedArrayView + for pixels */ + VkSubresourceLayout bLayout; + { + VkImageSubresource subresource{}; + subresource.arrayLayer = 0; + subresource.aspectMask = VK_IMAGE_ASPECT_COLOR_BIT; + subresource.mipLevel = 0; + device()->GetImageSubresourceLayout(device(), b, &subresource, &bLayout); + } + const Containers::Array data = b.dedicatedMemory().mapRead(); + CORRADE_COMPARE_AS( + (ImageView2D{ + PixelStorage{}.setRowLength(bLayout.rowPitch/4), + Magnum::PixelFormat::RGBA8UI, {8, 5}, data}), + (ImageView2D{Magnum::PixelFormat::RGBA8UI, {8, 5}, + "--------------------------------" + "----AaaaAaaaAaaaAaaa------------" + "----BbbbBbbbBbbbBbbb------------" + "----CcccCcccCcccCccc------------" + "----DdddDdddDdddDddd------------"}), + DebugTools::CompareImage); } void ImageVkTest::cmdCopyImage2DArrayTo3D() {