From d86edb310a70f1ef054b7e0aa85f0ba1d9031ba2 Mon Sep 17 00:00:00 2001 From: Brandon Jones Date: Tue, 11 Dec 2018 09:48:36 +0000 Subject: [PATCH] Implement BufferCopyView.imageHeight Parameter Implement BufferCopyView.imageHeight Parameter. Adds validation tests. Adequate functional testing not possible without 3D texture support. Bug: dawn:48 Change-Id: I00e4464eb451c948dee2890a11fb0984eff681a0 Reviewed-on: https://dawn-review.googlesource.com/c/2980 Commit-Queue: Corentin Wallez Reviewed-by: Austin Eng Reviewed-by: Corentin Wallez --- src/dawn_native/CommandBuffer.cpp | 30 +++++++- src/dawn_native/d3d12/CommandBufferD3D12.cpp | 5 +- src/dawn_native/d3d12/TextureCopySplitter.cpp | 17 ++--- src/dawn_native/d3d12/TextureCopySplitter.h | 4 +- src/dawn_native/metal/CommandBufferMTL.mm | 4 +- src/dawn_native/opengl/CommandBufferGL.cpp | 4 + src/dawn_native/vulkan/CommandBufferVk.cpp | 2 +- src/tests/unittests/d3d12/CopySplitTests.cpp | 76 +++++++++++++------ .../CopyCommandsValidationTests.cpp | 46 +++++++++++ 9 files changed, 144 insertions(+), 44 deletions(-) diff --git a/src/dawn_native/CommandBuffer.cpp b/src/dawn_native/CommandBuffer.cpp index 2f12197f0a..c9e6c254d8 100644 --- a/src/dawn_native/CommandBuffer.cpp +++ b/src/dawn_native/CommandBuffer.cpp @@ -88,11 +88,24 @@ namespace dawn_native { return {}; } + MaybeError ValidateImageHeight(uint32_t imageHeight, uint32_t copyHeight) { + if (imageHeight < copyHeight) { + return DAWN_VALIDATION_ERROR("Image height must not be less than the copy height."); + } + + return {}; + } + MaybeError ComputeTextureCopyBufferSize(const Extent3D& copySize, uint32_t rowPitch, + uint32_t imageHeight, uint32_t* bufferSize) { + DAWN_TRY(ValidateImageHeight(imageHeight, copySize.height)); + // TODO(cwallez@chromium.org): check for overflows - *bufferSize = (rowPitch * (copySize.height - 1) + copySize.width) * copySize.depth; + uint32_t slicePitch = rowPitch * imageHeight; + uint32_t sliceSize = rowPitch * (copySize.height - 1) + copySize.width; + *bufferSize = (slicePitch * (copySize.depth - 1)) + sliceSize; return {}; } @@ -388,7 +401,9 @@ namespace dawn_native { uint32_t bufferCopySize = 0; DAWN_TRY(ValidateRowPitch(copy->destination.texture->GetFormat(), copy->copySize, copy->source.rowPitch)); + DAWN_TRY(ComputeTextureCopyBufferSize(copy->copySize, copy->source.rowPitch, + copy->source.imageHeight, &bufferCopySize)); DAWN_TRY(ValidateCopySizeFitsInTexture(copy->destination, copy->copySize)); @@ -412,7 +427,8 @@ namespace dawn_native { DAWN_TRY(ValidateRowPitch(copy->source.texture->GetFormat(), copy->copySize, copy->destination.rowPitch)); DAWN_TRY(ComputeTextureCopyBufferSize( - copy->copySize, copy->destination.rowPitch, &bufferCopySize)); + copy->copySize, copy->destination.rowPitch, copy->destination.imageHeight, + &bufferCopySize)); DAWN_TRY(ValidateCopySizeFitsInTexture(copy->source, copy->copySize)); DAWN_TRY(ValidateCopySizeFitsInBuffer(copy->destination, bufferCopySize)); @@ -695,6 +711,11 @@ namespace dawn_native { } else { copy->source.rowPitch = source->rowPitch; } + if (source->imageHeight == 0) { + copy->source.imageHeight = copySize->height; + } else { + copy->source.imageHeight = source->imageHeight; + } } void CommandBufferBuilder::CopyTextureToBuffer(const TextureCopyView* source, @@ -729,6 +750,11 @@ namespace dawn_native { } else { copy->destination.rowPitch = destination->rowPitch; } + if (destination->imageHeight == 0) { + copy->destination.imageHeight = copySize->height; + } else { + copy->destination.imageHeight = destination->imageHeight; + } } } // namespace dawn_native diff --git a/src/dawn_native/d3d12/CommandBufferD3D12.cpp b/src/dawn_native/d3d12/CommandBufferD3D12.cpp index 20440974dc..37bce7385c 100644 --- a/src/dawn_native/d3d12/CommandBufferD3D12.cpp +++ b/src/dawn_native/d3d12/CommandBufferD3D12.cpp @@ -331,7 +331,7 @@ namespace dawn_native { namespace d3d12 { auto copySplit = ComputeTextureCopySplit( copy->destination.origin, copy->copySize, static_cast(TextureFormatPixelSize(texture->GetFormat())), - copy->source.offset, copy->source.rowPitch); + copy->source.offset, copy->source.rowPitch, copy->source.imageHeight); D3D12_TEXTURE_COPY_LOCATION textureLocation; textureLocation.pResource = texture->GetD3D12Resource(); @@ -378,7 +378,8 @@ namespace dawn_native { namespace d3d12 { auto copySplit = ComputeTextureCopySplit( copy->source.origin, copy->copySize, static_cast(TextureFormatPixelSize(texture->GetFormat())), - copy->destination.offset, copy->destination.rowPitch); + copy->destination.offset, copy->destination.rowPitch, + copy->destination.imageHeight); D3D12_TEXTURE_COPY_LOCATION textureLocation; textureLocation.pResource = texture->GetD3D12Resource(); diff --git a/src/dawn_native/d3d12/TextureCopySplitter.cpp b/src/dawn_native/d3d12/TextureCopySplitter.cpp index 7c58e89bbe..5e58cbcba9 100644 --- a/src/dawn_native/d3d12/TextureCopySplitter.cpp +++ b/src/dawn_native/d3d12/TextureCopySplitter.cpp @@ -40,15 +40,10 @@ namespace dawn_native { namespace d3d12 { Extent3D copySize, uint32_t texelSize, uint32_t offset, - uint32_t rowPitch) { + uint32_t rowPitch, + uint32_t imageHeight) { TextureCopySplit copy; - if (origin.z != 0 || copySize.depth > 1) { - // TODO(enga@google.com): Handle 3D - ASSERT(false); - return copy; - } - ASSERT(rowPitch % texelSize == 0); uint32_t alignedOffset = offset & ~(D3D12_TEXTURE_DATA_PLACEMENT_ALIGNMENT - 1); @@ -74,7 +69,7 @@ namespace dawn_native { namespace d3d12 { ASSERT(alignedOffset < offset); Origin3D texelOffset; - ComputeTexelOffsets(offset - alignedOffset, rowPitch, rowPitch * copySize.height, texelSize, + ComputeTexelOffsets(offset - alignedOffset, rowPitch, rowPitch * imageHeight, texelSize, &texelOffset); uint32_t rowPitchInTexels = rowPitch / texelSize; @@ -111,7 +106,7 @@ namespace dawn_native { namespace d3d12 { copy.copies[0].bufferOffset = texelOffset; copy.copies[0].bufferSize.width = copySize.width + texelOffset.x; - copy.copies[0].bufferSize.height = copySize.height + texelOffset.y; + copy.copies[0].bufferSize.height = imageHeight + texelOffset.y; copy.copies[0].bufferSize.depth = copySize.depth + texelOffset.z; return copy; @@ -162,7 +157,7 @@ namespace dawn_native { namespace d3d12 { copy.copies[0].bufferOffset = texelOffset; copy.copies[0].bufferSize.width = rowPitchInTexels; - copy.copies[0].bufferSize.height = copySize.height + texelOffset.y; + copy.copies[0].bufferSize.height = imageHeight + texelOffset.y; copy.copies[0].bufferSize.depth = copySize.depth + texelOffset.z; copy.copies[1].textureOffset.x = origin.x + copy.copies[0].copySize.width; @@ -178,7 +173,7 @@ namespace dawn_native { namespace d3d12 { copy.copies[1].bufferOffset.y = texelOffset.y + 1; copy.copies[1].bufferOffset.z = texelOffset.z; copy.copies[1].bufferSize.width = copy.copies[1].copySize.width; - copy.copies[1].bufferSize.height = copySize.height + texelOffset.y + 1; + copy.copies[1].bufferSize.height = imageHeight + texelOffset.y + 1; copy.copies[1].bufferSize.depth = copySize.depth + texelOffset.z; return copy; diff --git a/src/dawn_native/d3d12/TextureCopySplitter.h b/src/dawn_native/d3d12/TextureCopySplitter.h index f82da87858..e468fc6799 100644 --- a/src/dawn_native/d3d12/TextureCopySplitter.h +++ b/src/dawn_native/d3d12/TextureCopySplitter.h @@ -41,8 +41,8 @@ namespace dawn_native { namespace d3d12 { Extent3D copySize, uint32_t texelSize, uint32_t offset, - uint32_t rowPitch); - + uint32_t rowPitch, + uint32_t imageHeight); }} // namespace dawn_native::d3d12 #endif // DAWNNATIVE_D3D12_TEXTURECOPYSPLITTER_H_ diff --git a/src/dawn_native/metal/CommandBufferMTL.mm b/src/dawn_native/metal/CommandBufferMTL.mm index 03066ede61..7726fcc0a3 100644 --- a/src/dawn_native/metal/CommandBufferMTL.mm +++ b/src/dawn_native/metal/CommandBufferMTL.mm @@ -265,7 +265,7 @@ namespace dawn_native { namespace metal { [encoders.blit copyFromBuffer:buffer->GetMTLBuffer() sourceOffset:src.offset sourceBytesPerRow:src.rowPitch - sourceBytesPerImage:(src.rowPitch * copySize.height) + sourceBytesPerImage:(src.rowPitch * src.imageHeight) sourceSize:size toTexture:texture->GetMTLTexture() destinationSlice:dst.slice @@ -300,7 +300,7 @@ namespace dawn_native { namespace metal { toBuffer:buffer->GetMTLBuffer() destinationOffset:dst.offset destinationBytesPerRow:dst.rowPitch - destinationBytesPerImage:dst.rowPitch * copySize.height]; + destinationBytesPerImage:(dst.rowPitch * dst.imageHeight)]; } break; default: { UNREACHABLE(); } break; diff --git a/src/dawn_native/opengl/CommandBufferGL.cpp b/src/dawn_native/opengl/CommandBufferGL.cpp index 7ac7bdff19..6602db879b 100644 --- a/src/dawn_native/opengl/CommandBufferGL.cpp +++ b/src/dawn_native/opengl/CommandBufferGL.cpp @@ -333,6 +333,7 @@ namespace dawn_native { namespace opengl { glPixelStorei(GL_UNPACK_ROW_LENGTH, src.rowPitch / TextureFormatPixelSize(texture->GetFormat())); + glPixelStorei(GL_UNPACK_IMAGE_HEIGHT, src.imageHeight); switch (texture->GetDimension()) { case dawn::TextureDimension::e2D: if (texture->GetArrayLayers() > 1) { @@ -352,6 +353,7 @@ namespace dawn_native { namespace opengl { UNREACHABLE(); } glPixelStorei(GL_UNPACK_ROW_LENGTH, 0); + glPixelStorei(GL_UNPACK_IMAGE_HEIGHT, 0); glBindBuffer(GL_PIXEL_UNPACK_BUFFER, 0); } break; @@ -393,11 +395,13 @@ namespace dawn_native { namespace opengl { glBindBuffer(GL_PIXEL_PACK_BUFFER, buffer->GetHandle()); glPixelStorei(GL_PACK_ROW_LENGTH, dst.rowPitch / TextureFormatPixelSize(texture->GetFormat())); + glPixelStorei(GL_PACK_IMAGE_HEIGHT, dst.imageHeight); ASSERT(copySize.depth == 1 && src.origin.z == 0); void* offset = reinterpret_cast(static_cast(dst.offset)); glReadPixels(src.origin.x, src.origin.y, copySize.width, copySize.height, format.format, format.type, offset); glPixelStorei(GL_PACK_ROW_LENGTH, 0); + glPixelStorei(GL_PACK_IMAGE_HEIGHT, 0); glBindBuffer(GL_PIXEL_PACK_BUFFER, 0); glDeleteFramebuffers(1, &readFBO); diff --git a/src/dawn_native/vulkan/CommandBufferVk.cpp b/src/dawn_native/vulkan/CommandBufferVk.cpp index 9ebdb70dc7..ab165b0d14 100644 --- a/src/dawn_native/vulkan/CommandBufferVk.cpp +++ b/src/dawn_native/vulkan/CommandBufferVk.cpp @@ -50,7 +50,7 @@ namespace dawn_native { namespace vulkan { // In Vulkan the row length is in texels while it is in bytes for Dawn region.bufferRowLength = bufferCopy.rowPitch / TextureFormatPixelSize(texture->GetFormat()); - region.bufferImageHeight = bufferCopy.rowPitch * copySize.height; + region.bufferImageHeight = bufferCopy.imageHeight; region.imageSubresource.aspectMask = texture->GetVkAspectMask(); region.imageSubresource.mipLevel = textureCopy.level; diff --git a/src/tests/unittests/d3d12/CopySplitTests.cpp b/src/tests/unittests/d3d12/CopySplitTests.cpp index e3d6688f7f..ff55415efe 100644 --- a/src/tests/unittests/d3d12/CopySplitTests.cpp +++ b/src/tests/unittests/d3d12/CopySplitTests.cpp @@ -37,6 +37,7 @@ namespace { struct BufferSpec { uint32_t offset; uint32_t rowPitch; + uint32_t imageHeight; }; // Check that each copy region fits inside the buffer footprint @@ -119,7 +120,7 @@ namespace { const auto& copy = copySplit.copies[i]; uint32_t rowPitchInTexels = bufferSpec.rowPitch / textureSpec.texelSize; - uint32_t slicePitchInTexels = rowPitchInTexels * copy.copySize.height; + uint32_t slicePitchInTexels = rowPitchInTexels * bufferSpec.imageHeight; uint32_t absoluteTexelOffset = copySplit.offset / textureSpec.texelSize + copy.bufferOffset.x + copy.bufferOffset.y * rowPitchInTexels + copy.bufferOffset.z * slicePitchInTexels; ASSERT(absoluteTexelOffset >= bufferSpec.offset / textureSpec.texelSize); @@ -153,7 +154,8 @@ namespace { } std::ostream& operator<<(std::ostream& os, const BufferSpec& bufferSpec) { - os << "BufferSpec(" << bufferSpec.offset << ", " << bufferSpec.rowPitch << ")"; + os << "BufferSpec(" << bufferSpec.offset << ", " << bufferSpec.rowPitch << ", " + << bufferSpec.imageHeight << ")"; return os; } @@ -169,22 +171,25 @@ namespace { // Define base texture sizes and offsets to test with: some aligned, some unaligned constexpr TextureSpec kBaseTextureSpecs[] = { - { 0, 0, 0, 1, 1, 1, 4 }, - { 31, 16, 0, 1, 1, 1, 4 }, - { 64, 16, 0, 1, 1, 1, 4 }, + {0, 0, 0, 1, 1, 1, 4}, + {31, 16, 0, 1, 1, 1, 4}, + {64, 16, 0, 1, 1, 1, 4}, + {64, 16, 8, 1, 1, 1, 4}, - { 0, 0, 0, 1024, 1024, 1, 4 }, - { 256, 512, 0, 1024, 1024, 1, 4 }, - { 64, 48, 0, 1024, 1024, 1, 4 }, + {0, 0, 0, 1024, 1024, 1, 4}, + {256, 512, 0, 1024, 1024, 1, 4}, + {64, 48, 0, 1024, 1024, 1, 4}, + {64, 48, 16, 1024, 1024, 1024, 4}, - { 0, 0, 0, 257, 31, 1, 4 }, - { 0, 0, 0, 17, 93, 1, 4 }, - { 59, 13, 0, 257, 31, 1, 4 }, - { 17, 73, 0, 17, 93, 1, 4 }, + {0, 0, 0, 257, 31, 1, 4}, + {0, 0, 0, 17, 93, 1, 4}, + {59, 13, 0, 257, 31, 1, 4}, + {17, 73, 0, 17, 93, 1, 4}, + {17, 73, 59, 17, 93, 99, 4}, }; // Define base buffer sizes to work with: some offsets aligned, some unaligned. rowPitch is the minimum required - std::array BaseBufferSpecs(const TextureSpec& textureSpec) { + std::array BaseBufferSpecs(const TextureSpec& textureSpec) { uint32_t rowPitch = Align(textureSpec.texelSize * textureSpec.width, kTextureRowPitchAlignment); auto alignNonPow2 = [](uint32_t value, uint32_t size) -> uint32_t { @@ -192,18 +197,21 @@ namespace { }; return { - BufferSpec{alignNonPow2(0, textureSpec.texelSize), rowPitch}, - BufferSpec{alignNonPow2(512, textureSpec.texelSize), rowPitch}, - BufferSpec{alignNonPow2(1024, textureSpec.texelSize), rowPitch}, + BufferSpec{alignNonPow2(0, textureSpec.texelSize), rowPitch, textureSpec.height}, + BufferSpec{alignNonPow2(512, textureSpec.texelSize), rowPitch, textureSpec.height}, + BufferSpec{alignNonPow2(1024, textureSpec.texelSize), rowPitch, textureSpec.height}, + BufferSpec{alignNonPow2(1024, textureSpec.texelSize), rowPitch, textureSpec.height * 2}, - BufferSpec{alignNonPow2(32, textureSpec.texelSize), rowPitch}, - BufferSpec{alignNonPow2(64, textureSpec.texelSize), rowPitch}, + BufferSpec{alignNonPow2(32, textureSpec.texelSize), rowPitch, textureSpec.height}, + BufferSpec{alignNonPow2(64, textureSpec.texelSize), rowPitch, textureSpec.height}, + BufferSpec{alignNonPow2(64, textureSpec.texelSize), rowPitch, textureSpec.height * 2}, - BufferSpec{alignNonPow2(31, textureSpec.texelSize), rowPitch}, - BufferSpec{alignNonPow2(257, textureSpec.texelSize), rowPitch}, - BufferSpec{alignNonPow2(511, textureSpec.texelSize), rowPitch}, - BufferSpec{alignNonPow2(513, textureSpec.texelSize), rowPitch}, - BufferSpec{alignNonPow2(1023, textureSpec.texelSize), rowPitch}, + BufferSpec{alignNonPow2(31, textureSpec.texelSize), rowPitch, textureSpec.height}, + BufferSpec{alignNonPow2(257, textureSpec.texelSize), rowPitch, textureSpec.height}, + BufferSpec{alignNonPow2(511, textureSpec.texelSize), rowPitch, textureSpec.height}, + BufferSpec{alignNonPow2(513, textureSpec.texelSize), rowPitch, textureSpec.height}, + BufferSpec{alignNonPow2(1023, textureSpec.texelSize), rowPitch, textureSpec.height}, + BufferSpec{alignNonPow2(1023, textureSpec.texelSize), rowPitch, textureSpec.height * 2}, }; } @@ -223,7 +231,7 @@ class CopySplitTest : public testing::Test { TextureCopySplit copySplit = ComputeTextureCopySplit( {textureSpec.x, textureSpec.y, textureSpec.z}, {textureSpec.width, textureSpec.height, textureSpec.depth}, textureSpec.texelSize, - bufferSpec.offset, bufferSpec.rowPitch); + bufferSpec.offset, bufferSpec.rowPitch, bufferSpec.imageHeight); ValidateCopySplit(textureSpec, bufferSpec, copySplit); return copySplit; } @@ -370,3 +378,23 @@ TEST_F(CopySplitTest, RowPitch) { } } } + +TEST_F(CopySplitTest, ImageHeight) { + for (TextureSpec textureSpec : kBaseTextureSpecs) { + for (BufferSpec bufferSpec : BaseBufferSpecs(textureSpec)) { + uint32_t baseImageHeight = bufferSpec.imageHeight; + for (uint32_t i = 0; i < 5; ++i) { + bufferSpec.imageHeight = baseImageHeight + i * 256; + + TextureCopySplit copySplit = DoTest(textureSpec, bufferSpec); + if (HasFatalFailure()) { + std::ostringstream message; + message << "Failed generating splits: " << textureSpec << ", " << bufferSpec + << std::endl + << copySplit << std::endl; + FAIL() << message.str(); + } + } + } + } +} diff --git a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp index 636df884cb..6f4145090f 100644 --- a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp +++ b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp @@ -340,6 +340,29 @@ TEST_F(CopyCommandTest_B2T, IncorrectRowPitch) { dawn::TextureAspect::Color, {65, 1, 1}); } +TEST_F(CopyCommandTest_B2T, ImageHeightConstraint) { + uint32_t bufferSize = BufferSizeForTextureCopy(5, 5, 1); + dawn::Buffer source = CreateBuffer(bufferSize, dawn::BufferUsageBit::TransferSrc); + dawn::Texture destination = Create2DTexture(16, 16, 1, 1, dawn::TextureFormat::R8G8B8A8Unorm, + dawn::TextureUsageBit::TransferDst); + + // Image height is zero (Valid) + TestB2TCopy(utils::Expectation::Success, source, 0, 256, 0, destination, 0, 0, {0, 0, 0}, + dawn::TextureAspect::Color, {4, 4, 1}); + + // Image height is equal to copy height (Valid) + TestB2TCopy(utils::Expectation::Success, source, 0, 256, 0, destination, 0, 0, {0, 0, 0}, + dawn::TextureAspect::Color, {4, 4, 1}); + + // Image height is larger than copy height (Valid) + TestB2TCopy(utils::Expectation::Success, source, 0, 256, 4, destination, 0, 0, {0, 0, 0}, + dawn::TextureAspect::Color, {4, 4, 1}); + + // Image height is less than copy height (Invalid) + TestB2TCopy(utils::Expectation::Failure, source, 0, 256, 3, destination, 0, 0, {0, 0, 0}, + dawn::TextureAspect::Color, {4, 4, 1}); +} + // Test B2T copies with incorrect buffer offset usage TEST_F(CopyCommandTest_B2T, IncorrectBufferOffset) { uint32_t bufferSize = BufferSizeForTextureCopy(4, 4, 1); @@ -523,6 +546,29 @@ TEST_F(CopyCommandTest_T2B, IncorrectRowPitch) { destination, 0, 256, 0, {65, 1, 1}); } +TEST_F(CopyCommandTest_T2B, ImageHeightConstraint) { + uint32_t bufferSize = BufferSizeForTextureCopy(5, 5, 1); + dawn::Texture source = Create2DTexture(16, 16, 1, 1, dawn::TextureFormat::R8G8B8A8Unorm, + dawn::TextureUsageBit::TransferSrc); + dawn::Buffer destination = CreateBuffer(bufferSize, dawn::BufferUsageBit::TransferDst); + + // Image height is zero (Valid) + TestT2BCopy(utils::Expectation::Success, source, 0, 0, {0, 0, 0}, dawn::TextureAspect::Color, + destination, 0, 256, 0, {4, 4, 1}); + + // Image height is equal to copy height (Valid) + TestT2BCopy(utils::Expectation::Success, source, 0, 0, {0, 0, 0}, dawn::TextureAspect::Color, + destination, 0, 256, 4, {4, 4, 1}); + + // Image height exceeds copy height (Valid) + TestT2BCopy(utils::Expectation::Success, source, 0, 0, {0, 0, 0}, dawn::TextureAspect::Color, + destination, 0, 256, 5, {4, 4, 1}); + + // Image height is less than copy height (Invalid) + TestT2BCopy(utils::Expectation::Failure, source, 0, 0, {0, 0, 0}, dawn::TextureAspect::Color, + destination, 0, 256, 3, {4, 4, 1}); +} + // Test T2B copies with incorrect buffer offset usage TEST_F(CopyCommandTest_T2B, IncorrectBufferOffset) { uint32_t bufferSize = BufferSizeForTextureCopy(128, 16, 1);