From 7ce4924a35eac58c0c44bd1b561b76d094ff6dee Mon Sep 17 00:00:00 2001 From: Tomek Ponitka Date: Wed, 5 Aug 2020 16:43:24 +0000 Subject: [PATCH] Fixing linear texture data validation on bytesPerRow This makes the validation match the spec more. Since the change makes the validation throw less errors than it used to, most of the tests should still be fine, except for those I fixed. Bug: dawn:482 Change-Id: I1d01356df66c897191a2305df419f088b45c8467 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/26302 Reviewed-by: Jiawei Shao Reviewed-by: Austin Eng Commit-Queue: Tomek Ponitka --- src/dawn_native/CommandEncoder.cpp | 20 ++++- src/dawn_native/CommandValidation.cpp | 5 +- src/tests/end2end/CopyTests.cpp | 52 +++++++++++ src/tests/end2end/DeprecatedAPITests.cpp | 14 +-- src/tests/end2end/QueueTests.cpp | 30 +++++++ .../CopyCommandsValidationTests.cpp | 90 +++++++++++++++---- .../QueueWriteTextureValidationTests.cpp | 28 +++++- 7 files changed, 209 insertions(+), 30 deletions(-) diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp index 2450edc919..b1a63488eb 100644 --- a/src/dawn_native/CommandEncoder.cpp +++ b/src/dawn_native/CommandEncoder.cpp @@ -15,6 +15,7 @@ #include "dawn_native/CommandEncoder.h" #include "common/BitSetIterator.h" +#include "common/Math.h" #include "dawn_native/BindGroup.h" #include "dawn_native/Buffer.h" #include "dawn_native/CommandBuffer.h" @@ -713,12 +714,20 @@ namespace dawn_native { defaultedRowsPerImage = copySize->height; } + // In the case of one row copy bytesPerRow might not contain enough bytes + uint32_t bytesPerRow = source->layout.bytesPerRow; + if (copySize->height <= 1 && copySize->depth <= 1) { + bytesPerRow = + Align(copySize->width * destination->texture->GetFormat().blockByteSize, + kTextureBytesPerRowAlignment); + } + // Record the copy command. CopyBufferToTextureCmd* copy = allocator->Allocate(Command::CopyBufferToTexture); copy->source.buffer = source->buffer; copy->source.offset = source->layout.offset; - copy->source.bytesPerRow = source->layout.bytesPerRow; + copy->source.bytesPerRow = bytesPerRow; copy->source.rowsPerImage = defaultedRowsPerImage; copy->destination.texture = destination->texture; copy->destination.origin = destination->origin; @@ -773,6 +782,13 @@ namespace dawn_native { defaultedRowsPerImage = copySize->height; } + // In the case of one row copy bytesPerRow might not contain enough bytes + uint32_t bytesPerRow = destination->layout.bytesPerRow; + if (copySize->height <= 1 && copySize->depth <= 1) { + bytesPerRow = Align(copySize->width * source->texture->GetFormat().blockByteSize, + kTextureBytesPerRowAlignment); + } + // Record the copy command. CopyTextureToBufferCmd* copy = allocator->Allocate(Command::CopyTextureToBuffer); @@ -782,7 +798,7 @@ namespace dawn_native { copy->source.aspect = source->aspect; copy->destination.buffer = destination->buffer; copy->destination.offset = destination->layout.offset; - copy->destination.bytesPerRow = destination->layout.bytesPerRow; + copy->destination.bytesPerRow = bytesPerRow; copy->destination.rowsPerImage = defaultedRowsPerImage; copy->copySize = *copySize; diff --git a/src/dawn_native/CommandValidation.cpp b/src/dawn_native/CommandValidation.cpp index 095cf61a83..034585a527 100644 --- a/src/dawn_native/CommandValidation.cpp +++ b/src/dawn_native/CommandValidation.cpp @@ -440,8 +440,9 @@ namespace dawn_native { } // Validation for other members in layout: - if (layout.bytesPerRow < - copyExtent.width / blockInfo.blockWidth * blockInfo.blockByteSize) { + if ((copyExtent.height > 1 || copyExtent.depth > 1) && + layout.bytesPerRow < + copyExtent.width / blockInfo.blockWidth * blockInfo.blockByteSize) { return DAWN_VALIDATION_ERROR( "bytesPerRow must not be less than the number of bytes per row"); } diff --git a/src/tests/end2end/CopyTests.cpp b/src/tests/end2end/CopyTests.cpp index 28728bdfaa..434e248453 100644 --- a/src/tests/end2end/CopyTests.cpp +++ b/src/tests/end2end/CopyTests.cpp @@ -667,6 +667,32 @@ TEST_P(CopyTests_T2B, RowPitchUnaligned) { } } +// Test that copying with bytesPerRow = 0 and bytesPerRow < bytesInACompleteRow works +// when we're copying one row only +TEST_P(CopyTests_T2B, BytesPerRowWithOneRowCopy) { + constexpr uint32_t kWidth = 259; + constexpr uint32_t kHeight = 127; + + TextureSpec textureSpec; + textureSpec.copyOrigin = {0, 0, 0}; + textureSpec.textureSize = {kWidth, kHeight, 1}; + textureSpec.level = 0; + + // bytesPerRow = 0 + { + BufferSpec bufferSpec = MinimumBufferSpec(5, 1); + bufferSpec.bytesPerRow = 0; + DoTest(textureSpec, bufferSpec, {5, 1, 1}); + } + + // bytesPerRow < bytesInACompleteRow + { + BufferSpec bufferSpec = MinimumBufferSpec(259, 1); + bufferSpec.bytesPerRow = 256; + DoTest(textureSpec, bufferSpec, {259, 1, 1}); + } +} + // Test that copying whole texture 2D array layers in one texture-to-buffer-copy works. TEST_P(CopyTests_T2B, Texture2DArrayRegion) { constexpr uint32_t kWidth = 256; @@ -1076,6 +1102,32 @@ TEST_P(CopyTests_B2T, RowPitchUnaligned) { } } +// Test that copying with bytesPerRow = 0 and bytesPerRow < bytesInACompleteRow works +// when we're copying one row only +TEST_P(CopyTests_B2T, BytesPerRowWithOneRowCopy) { + constexpr uint32_t kWidth = 259; + constexpr uint32_t kHeight = 127; + + TextureSpec textureSpec; + textureSpec.copyOrigin = {0, 0, 0}; + textureSpec.textureSize = {kWidth, kHeight, 1}; + textureSpec.level = 0; + + // bytesPerRow = 0 + { + BufferSpec bufferSpec = MinimumBufferSpec(5, 1); + bufferSpec.bytesPerRow = 0; + DoTest(textureSpec, bufferSpec, {5, 1, 1}); + } + + // bytesPerRow < bytesInACompleteRow + { + BufferSpec bufferSpec = MinimumBufferSpec(259, 1); + bufferSpec.bytesPerRow = 256; + DoTest(textureSpec, bufferSpec, {259, 1, 1}); + } +} + // Test that copying whole texture 2D array layers in one texture-to-buffer-copy works. TEST_P(CopyTests_B2T, Texture2DArrayRegion) { constexpr uint32_t kWidth = 256; diff --git a/src/tests/end2end/DeprecatedAPITests.cpp b/src/tests/end2end/DeprecatedAPITests.cpp index 6542c54f70..7d65e83131 100644 --- a/src/tests/end2end/DeprecatedAPITests.cpp +++ b/src/tests/end2end/DeprecatedAPITests.cpp @@ -323,18 +323,18 @@ TEST_P(BufferCopyViewDeprecationTests, DeprecationWarning) { bufCopy.offset = 4; EXPECT_DEPRECATION_WARNING(encoder.CopyBufferToTexture(&bufCopy, &texCopy, ©Size)); EXPECT_DEPRECATION_WARNING(encoder.CopyTextureToBuffer(&texCopy, &bufCopy, ©Size)); - // Since bytesPerRow is 0 - ASSERT_DEVICE_ERROR(encoder.Finish()); + wgpu::CommandBuffer command = encoder.Finish(); + queue.Submit(1, &command); } { wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); wgpu::BufferCopyView bufCopy = {}; bufCopy.buffer = buffer; - bufCopy.bytesPerRow = kTextureBytesPerRowAlignment; + bufCopy.bytesPerRow = 128; EXPECT_DEPRECATION_WARNING(encoder.CopyBufferToTexture(&bufCopy, &texCopy, ©Size)); EXPECT_DEPRECATION_WARNING(encoder.CopyTextureToBuffer(&texCopy, &bufCopy, ©Size)); - wgpu::CommandBuffer command = encoder.Finish(); - queue.Submit(1, &command); + // Since bytesPerRow is not 256-aligned. + ASSERT_DEVICE_ERROR(encoder.Finish()); } { wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); @@ -343,8 +343,8 @@ TEST_P(BufferCopyViewDeprecationTests, DeprecationWarning) { bufCopy.rowsPerImage = 1; EXPECT_DEPRECATION_WARNING(encoder.CopyBufferToTexture(&bufCopy, &texCopy, ©Size)); EXPECT_DEPRECATION_WARNING(encoder.CopyTextureToBuffer(&texCopy, &bufCopy, ©Size)); - // Since bytesPerRow is 0 - ASSERT_DEVICE_ERROR(encoder.Finish()); + wgpu::CommandBuffer command = encoder.Finish(); + queue.Submit(1, &command); } } diff --git a/src/tests/end2end/QueueTests.cpp b/src/tests/end2end/QueueTests.cpp index 4e5febdc0b..edf9ccf980 100644 --- a/src/tests/end2end/QueueTests.cpp +++ b/src/tests/end2end/QueueTests.cpp @@ -462,6 +462,36 @@ TEST_P(QueueWriteTextureTests, VaryingBytesPerRow) { } } +// Test that writing with bytesPerRow = 0 and bytesPerRow < bytesInACompleteRow works +// when we're copying one row only +TEST_P(QueueWriteTextureTests, BytesPerRowWithOneRowCopy) { + constexpr uint32_t kWidth = 259; + constexpr uint32_t kHeight = 127; + + TextureSpec textureSpec; + textureSpec.copyOrigin = {0, 0, 0}; + textureSpec.textureSize = {kWidth, kHeight, 1}; + textureSpec.level = 0; + + // bytesPerRow = 0 + { + constexpr wgpu::Extent3D copyExtent = {5, 1, 1}; + + DataSpec dataSpec = MinimumDataSpec(copyExtent); + dataSpec.bytesPerRow = 0; + DoTest(textureSpec, dataSpec, copyExtent); + } + + // bytesPerRow < bytesInACompleteRow + { + constexpr wgpu::Extent3D copyExtent = {259, 1, 1}; + + DataSpec dataSpec = MinimumDataSpec(copyExtent); + dataSpec.bytesPerRow = 256; + DoTest(textureSpec, dataSpec, copyExtent); + } +} + // Test with bytesPerRow greater than needed in a write to a texture array. TEST_P(QueueWriteTextureTests, VaryingArrayBytesPerRow) { constexpr uint32_t kWidth = 257; diff --git a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp index 1edb39650b..3d4f65c871 100644 --- a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp +++ b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp @@ -507,16 +507,45 @@ TEST_F(CopyCommandTest_B2T, IncorrectBytesPerRow) { wgpu::TextureUsage::CopyDst); // bytes per row is 0 - TestB2TCopy(utils::Expectation::Failure, source, 0, 0, 0, destination, 0, {0, 0, 0}, - {64, 4, 1}); + { + // copyHeight > 1 + TestB2TCopy(utils::Expectation::Failure, source, 0, 0, 0, destination, 0, {0, 0, 0}, + {64, 4, 1}); + + // copyDepth > 1 + TestB2TCopy(utils::Expectation::Failure, source, 0, 0, 1, destination, 0, {0, 0, 0}, + {64, 1, 4}); + + // copyHeight = 1 and copyDepth = 1 + TestB2TCopy(utils::Expectation::Success, source, 0, 0, 0, destination, 0, {0, 0, 0}, + {64, 1, 1}); + } // bytes per row is not 256-byte aligned - TestB2TCopy(utils::Expectation::Failure, source, 0, 128, 0, destination, 0, {0, 0, 0}, - {4, 4, 1}); + { + // copyHeight > 1 + TestB2TCopy(utils::Expectation::Failure, source, 0, 128, 0, destination, 0, {0, 0, 0}, + {4, 4, 1}); + + // copyHeight = 1 and copyDepth = 1 + TestB2TCopy(utils::Expectation::Failure, source, 0, 128, 0, destination, 0, {0, 0, 0}, + {4, 1, 1}); + } // bytes per row is less than width * bytesPerPixel - TestB2TCopy(utils::Expectation::Failure, source, 0, 256, 0, destination, 0, {0, 0, 0}, - {65, 1, 1}); + { + // copyHeight > 1 + TestB2TCopy(utils::Expectation::Failure, source, 0, 256, 0, destination, 0, {0, 0, 0}, + {65, 2, 1}); + + // copyDepth > 1 + TestB2TCopy(utils::Expectation::Failure, source, 0, 256, 1, destination, 0, {0, 0, 0}, + {65, 1, 2}); + + // copyHeight = 1 and copyDepth = 1 + TestB2TCopy(utils::Expectation::Success, source, 0, 256, 0, destination, 0, {0, 0, 0}, + {65, 1, 1}); + } } TEST_F(CopyCommandTest_B2T, ImageHeightConstraint) { @@ -907,20 +936,49 @@ TEST_F(CopyCommandTest_T2B, IncorrectUsage) { TEST_F(CopyCommandTest_T2B, IncorrectBytesPerRow) { uint64_t bufferSize = BufferSizeForTextureCopy(128, 16, 1); wgpu::Texture source = Create2DTexture(128, 16, 5, 1, wgpu::TextureFormat::RGBA8Unorm, - wgpu::TextureUsage::CopyDst); - wgpu::Buffer destination = CreateBuffer(bufferSize, wgpu::BufferUsage::CopySrc); + wgpu::TextureUsage::CopySrc); + wgpu::Buffer destination = CreateBuffer(bufferSize, wgpu::BufferUsage::CopyDst); // bytes per row is 0 - TestT2BCopy(utils::Expectation::Failure, source, 0, {0, 0, 0}, destination, 0, 256, 0, - {64, 4, 1}); + { + // copyHeight > 1 + TestT2BCopy(utils::Expectation::Failure, source, 0, {0, 0, 0}, destination, 0, 0, 0, + {64, 4, 1}); + + // copyDepth > 1 + TestT2BCopy(utils::Expectation::Failure, source, 0, {0, 0, 0}, destination, 0, 0, 1, + {64, 1, 4}); + + // copyHeight = 1 and copyDepth = 1 + TestT2BCopy(utils::Expectation::Success, source, 0, {0, 0, 0}, destination, 0, 0, 0, + {64, 1, 1}); + } // bytes per row is not 256-byte aligned - TestT2BCopy(utils::Expectation::Failure, source, 0, {0, 0, 0}, destination, 0, 257, 0, - {4, 4, 1}); + { + // copyHeight > 1 + TestT2BCopy(utils::Expectation::Failure, source, 0, {0, 0, 0}, destination, 0, 128, 0, + {4, 4, 1}); + + // copyHeight = 1 and copyDepth = 1 + TestT2BCopy(utils::Expectation::Failure, source, 0, {0, 0, 0}, destination, 0, 128, 0, + {4, 1, 1}); + } // bytes per row is less than width * bytesPerPixel - TestT2BCopy(utils::Expectation::Failure, source, 0, {0, 0, 0}, destination, 0, 256, 0, - {65, 1, 1}); + { + // copyHeight > 1 + TestT2BCopy(utils::Expectation::Failure, source, 0, {0, 0, 0}, destination, 0, 256, 0, + {65, 2, 1}); + + // copyDepth > 1 + TestT2BCopy(utils::Expectation::Failure, source, 0, {0, 0, 0}, destination, 0, 256, 1, + {65, 1, 2}); + + // copyHeight = 1 and copyDepth = 1 + TestT2BCopy(utils::Expectation::Success, source, 0, {0, 0, 0}, destination, 0, 256, 0, + {65, 1, 1}); + } } TEST_F(CopyCommandTest_T2B, ImageHeightConstraint) { @@ -1033,9 +1091,9 @@ TEST_F(CopyCommandTest_T2B, TextureCopyBufferSizeLastRowComputation) { for (wgpu::TextureFormat format : kFormats) { wgpu::Texture source = - Create2DTexture(kWidth, kHeight, 1, 1, format, wgpu::TextureUsage::CopyDst); + Create2DTexture(kWidth, kHeight, 1, 1, format, wgpu::TextureUsage::CopySrc); - wgpu::Buffer destination = CreateBuffer(kInvalidBufferSize, wgpu::BufferUsage::CopySrc); + wgpu::Buffer destination = CreateBuffer(kInvalidBufferSize, wgpu::BufferUsage::CopyDst); TestT2BCopy(utils::Expectation::Failure, source, 0, {0, 0, 0}, destination, 0, kBytesPerRow, 0, {kWidth, kHeight, 1}); } diff --git a/src/tests/unittests/validation/QueueWriteTextureValidationTests.cpp b/src/tests/unittests/validation/QueueWriteTextureValidationTests.cpp index a9dca68d30..d0aad6170e 100644 --- a/src/tests/unittests/validation/QueueWriteTextureValidationTests.cpp +++ b/src/tests/unittests/validation/QueueWriteTextureValidationTests.cpp @@ -221,11 +221,33 @@ namespace { wgpu::Texture destination = Create2DTexture({3, 7, 1}, 1, wgpu::TextureFormat::RGBA8Unorm, wgpu::TextureUsage::CopyDst); - // bytesPerRow = 0 is invalid - ASSERT_DEVICE_ERROR(TestWriteTexture(128, 0, 0, 0, destination, 0, {0, 0, 0}, {3, 7, 1})); + // bytesPerRow = 0 + { + // copyHeight > 1 + ASSERT_DEVICE_ERROR( + TestWriteTexture(128, 0, 0, 0, destination, 0, {0, 0, 0}, {3, 7, 1})); + + // copyDepth > 1 + ASSERT_DEVICE_ERROR( + TestWriteTexture(128, 0, 0, 1, destination, 0, {0, 0, 0}, {3, 1, 2})); + + // copyHeight = 1 and copyDepth = 1 + TestWriteTexture(128, 0, 0, 0, destination, 0, {0, 0, 0}, {3, 1, 1}); + } // bytesPerRow = 11 is invalid since a row takes 12 bytes. - ASSERT_DEVICE_ERROR(TestWriteTexture(128, 0, 11, 0, destination, 0, {0, 0, 0}, {3, 7, 1})); + { + // copyHeight > 1 + ASSERT_DEVICE_ERROR( + TestWriteTexture(128, 0, 11, 0, destination, 0, {0, 0, 0}, {3, 7, 1})); + + // copyDepth > 1 + ASSERT_DEVICE_ERROR( + TestWriteTexture(128, 0, 11, 1, destination, 0, {0, 0, 0}, {3, 1, 2})); + + // copyHeight = 1 and copyDepth = 1 + TestWriteTexture(128, 0, 11, 0, destination, 0, {0, 0, 0}, {3, 1, 1}); + } // bytesPerRow = 12 is valid since a row takes 12 bytes. TestWriteTexture(128, 0, 12, 0, destination, 0, {0, 0, 0}, {3, 7, 1});