From 17738b6d8c4f18b8a9045a18169cd499d9923647 Mon Sep 17 00:00:00 2001 From: Jiawei Shao Date: Wed, 12 Jun 2019 08:53:45 +0000 Subject: [PATCH] Fix wrong computation of texture copy buffer size This patch fixes a bug in the computation of texture copy buffer size. As the 'width' and 'height' in copy commands are all in pixels, while the buffer size is counted in bytes, we shoud first convert 'width' into bytes before calculating the buffer size. BUG=dawn:42 TEST=dawn_unittests Change-Id: Iebd5ed07a54eea762f4a653e295ecacb845ba32f Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/7940 Commit-Queue: Jiawei Shao Reviewed-by: Corentin Wallez --- src/dawn_native/CommandEncoder.cpp | 15 +- .../CopyCommandsValidationTests.cpp | 136 +++++++++++++++++- 2 files changed, 138 insertions(+), 13 deletions(-) diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp index acb17d34d1..3fa880d721 100644 --- a/src/dawn_native/CommandEncoder.cpp +++ b/src/dawn_native/CommandEncoder.cpp @@ -187,7 +187,8 @@ namespace dawn_native { return {}; } - MaybeError ComputeTextureCopyBufferSize(const Extent3D& copySize, + MaybeError ComputeTextureCopyBufferSize(dawn::TextureFormat textureFormat, + const Extent3D& copySize, uint32_t rowPitch, uint32_t imageHeight, uint32_t* bufferSize) { @@ -195,7 +196,8 @@ namespace dawn_native { // TODO(cwallez@chromium.org): check for overflows uint32_t slicePitch = rowPitch * imageHeight; - uint32_t sliceSize = rowPitch * (copySize.height - 1) + copySize.width; + uint32_t sliceSize = rowPitch * (copySize.height - 1) + + copySize.width * TextureFormatPixelSize(textureFormat); *bufferSize = (slicePitch * (copySize.depth - 1)) + sliceSize; return {}; @@ -906,9 +908,9 @@ namespace dawn_native { 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(ComputeTextureCopyBufferSize( + copy->destination.texture->GetFormat(), copy->copySize, + copy->source.rowPitch, copy->source.imageHeight, &bufferCopySize)); DAWN_TRY(ValidateCopySizeFitsInTexture(copy->destination, copy->copySize)); DAWN_TRY(ValidateCopySizeFitsInBuffer(copy->source, bufferCopySize)); @@ -933,7 +935,8 @@ namespace dawn_native { DAWN_TRY(ValidateRowPitch(copy->source.texture->GetFormat(), copy->copySize, copy->destination.rowPitch)); DAWN_TRY(ComputeTextureCopyBufferSize( - copy->copySize, copy->destination.rowPitch, copy->destination.imageHeight, + copy->source.texture->GetFormat(), copy->copySize, + copy->destination.rowPitch, copy->destination.imageHeight, &bufferCopySize)); DAWN_TRY(ValidateCopySizeFitsInTexture(copy->source, copy->copySize)); diff --git a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp index eccf3b6013..afad75c513 100644 --- a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp +++ b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include "common/Assert.h" #include "common/Constants.h" #include "common/Math.h" #include "tests/unittests/validation/ValidationTest.h" @@ -44,9 +45,27 @@ class CopyCommandTest : public ValidationTest { return tex; } - uint32_t BufferSizeForTextureCopy(uint32_t width, uint32_t height, uint32_t depth) { - uint32_t rowPitch = Align(width * 4, kTextureRowPitchAlignment); - return (rowPitch * (height - 1) + width) * depth; + // TODO(jiawei.shao@intel.com): support more pixel formats + uint32_t TextureFormatPixelSize(dawn::TextureFormat format) { + switch (format) { + case dawn::TextureFormat::R8G8Unorm: + return 2; + case dawn::TextureFormat::R8G8B8A8Unorm: + return 4; + default: + UNREACHABLE(); + return 0; + } + } + + uint32_t BufferSizeForTextureCopy( + uint32_t width, + uint32_t height, + uint32_t depth, + dawn::TextureFormat format = dawn::TextureFormat::R8G8B8A8Unorm) { + uint32_t bytesPerPixel = TextureFormatPixelSize(format); + uint32_t rowPitch = Align(width * bytesPerPixel, kTextureRowPitchAlignment); + return (rowPitch * (height - 1) + width * bytesPerPixel) * depth; } void ValidateExpectation(dawn::CommandEncoder encoder, utils::Expectation expectation) { @@ -315,12 +334,13 @@ TEST_F(CopyCommandTest_B2T, OutOfBoundsOnBuffer) { TestB2TCopy(utils::Expectation::Failure, source, 4, 256, 0, destination, 0, 0, {0, 0, 0}, {4, 4, 1}); - // OOB on the buffer because (row pitch * (height - 1) + width) * depth overflows + // OOB on the buffer because (row pitch * (height - 1) + width * bytesPerPixel) * depth + // overflows TestB2TCopy(utils::Expectation::Failure, source, 0, 512, 0, destination, 0, 0, {0, 0, 0}, {4, 3, 1}); // Not OOB on the buffer although row pitch * height overflows - // but (row pitch * (height - 1) + width) * depth does not overlow + // but (row pitch * (height - 1) + width * bytesPerPixel) * depth does not overflow { uint32_t sourceBufferSize = BufferSizeForTextureCopy(7, 3, 1); ASSERT_TRUE(256 * 3 > sourceBufferSize) << "row pitch * height should overflow buffer"; @@ -508,6 +528,56 @@ TEST_F(CopyCommandTest_B2T, BufferOrTextureInErrorState) { } } +// Regression tests for a bug in the computation of texture copy buffer size in Dawn. +TEST_F(CopyCommandTest_B2T, TextureCopyBufferSizeLastRowComputation) { + constexpr uint32_t kRowPitch = 256; + constexpr uint32_t kWidth = 4; + constexpr uint32_t kHeight = 4; + + constexpr std::array kFormats = {dawn::TextureFormat::R8G8B8A8Unorm, + dawn::TextureFormat::R8G8Unorm}; + + { + // kRowPitch * (kHeight - 1) + kWidth is not large enough to be the valid buffer size in + // this test because the buffer sizes in B2T copies are not in texels but in bytes. + constexpr uint32_t kInvalidBufferSize = kRowPitch * (kHeight - 1) + kWidth; + + for (dawn::TextureFormat format : kFormats) { + dawn::Buffer source = + CreateBuffer(kInvalidBufferSize, dawn::BufferUsageBit::TransferSrc); + dawn::Texture destination = + Create2DTexture(kWidth, kHeight, 1, 1, format, dawn::TextureUsageBit::TransferDst); + TestB2TCopy(utils::Expectation::Failure, source, 0, kRowPitch, 0, destination, 0, 0, + {0, 0, 0}, {kWidth, kHeight, 1}); + } + } + + { + for (dawn::TextureFormat format : kFormats) { + uint32_t validBufferSize = BufferSizeForTextureCopy(kWidth, kHeight, 1, format); + dawn::Texture destination = + Create2DTexture(kWidth, kHeight, 1, 1, format, dawn::TextureUsageBit::TransferDst); + + // Verify the return value of BufferSizeForTextureCopy() is exactly the minimum valid + // buffer size in this test. + { + uint32_t invalidBuffferSize = validBufferSize - 1; + dawn::Buffer source = + CreateBuffer(invalidBuffferSize, dawn::BufferUsageBit::TransferSrc); + TestB2TCopy(utils::Expectation::Failure, source, 0, kRowPitch, 0, destination, 0, 0, + {0, 0, 0}, {kWidth, kHeight, 1}); + } + + { + dawn::Buffer source = + CreateBuffer(validBufferSize, dawn::BufferUsageBit::TransferSrc); + TestB2TCopy(utils::Expectation::Success, source, 0, kRowPitch, 0, destination, 0, 0, + {0, 0, 0}, {kWidth, kHeight, 1}); + } + } + } +} + class CopyCommandTest_T2B : public CopyCommandTest { }; @@ -603,12 +673,13 @@ TEST_F(CopyCommandTest_T2B, OutOfBoundsOnBuffer) { TestT2BCopy(utils::Expectation::Failure, source, 0, 0, {0, 0, 0}, destination, 4, 256, 0, {4, 4, 1}); - // OOB on the buffer because (row pitch * (height - 1) + width) * depth overflows + // OOB on the buffer because (row pitch * (height - 1) + width * bytesPerPixel) * depth + // overflows TestT2BCopy(utils::Expectation::Failure, source, 0, 0, {0, 0, 0}, destination, 0, 512, 0, {4, 3, 1}); // Not OOB on the buffer although row pitch * height overflows - // but (row pitch * (height - 1) + width) * depth does not overlow + // but (row pitch * (height - 1) + width * bytesPerPixel) * depth does not overflow { uint32_t destinationBufferSize = BufferSizeForTextureCopy(7, 3, 1); ASSERT_TRUE(256 * 3 > destinationBufferSize) << "row pitch * height should overflow buffer"; @@ -766,6 +837,57 @@ TEST_F(CopyCommandTest_T2B, BufferOrTextureInErrorState) { } } +// Regression tests for a bug in the computation of texture copy buffer size in Dawn. +TEST_F(CopyCommandTest_T2B, TextureCopyBufferSizeLastRowComputation) { + constexpr uint32_t kRowPitch = 256; + constexpr uint32_t kWidth = 4; + constexpr uint32_t kHeight = 4; + + constexpr std::array kFormats = {dawn::TextureFormat::R8G8B8A8Unorm, + dawn::TextureFormat::R8G8Unorm}; + + { + // kRowPitch * (kHeight - 1) + kWidth is not large enough to be the valid buffer size in + // this test because the buffer sizes in T2B copies are not in texels but in bytes. + constexpr uint32_t kInvalidBufferSize = kRowPitch * (kHeight - 1) + kWidth; + + for (dawn::TextureFormat format : kFormats) { + dawn::Texture source = + Create2DTexture(kWidth, kHeight, 1, 1, format, dawn::TextureUsageBit::TransferDst); + + dawn::Buffer destination = + CreateBuffer(kInvalidBufferSize, dawn::BufferUsageBit::TransferSrc); + TestT2BCopy(utils::Expectation::Failure, source, 0, 0, {0, 0, 0}, destination, 0, + kRowPitch, 0, {kWidth, kHeight, 1}); + } + } + + { + for (dawn::TextureFormat format : kFormats) { + uint32_t validBufferSize = BufferSizeForTextureCopy(kWidth, kHeight, 1, format); + dawn::Texture source = + Create2DTexture(kWidth, kHeight, 1, 1, format, dawn::TextureUsageBit::TransferSrc); + + // Verify the return value of BufferSizeForTextureCopy() is exactly the minimum valid + // buffer size in this test. + { + uint32_t invalidBufferSize = validBufferSize - 1; + dawn::Buffer destination = + CreateBuffer(invalidBufferSize, dawn::BufferUsageBit::TransferDst); + TestT2BCopy(utils::Expectation::Failure, source, 0, 0, {0, 0, 0}, destination, 0, + kRowPitch, 0, {kWidth, kHeight, 1}); + } + + { + dawn::Buffer destination = + CreateBuffer(validBufferSize, dawn::BufferUsageBit::TransferDst); + TestT2BCopy(utils::Expectation::Success, source, 0, 0, {0, 0, 0}, destination, 0, + kRowPitch, 0, {kWidth, kHeight, 1}); + } + } + } +} + class CopyCommandTest_T2T : public CopyCommandTest {}; TEST_F(CopyCommandTest_T2T, Success) {