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 <jiawei.shao@intel.com>
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Tomek Ponitka <tommek@google.com>
diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp
index 2450edc..b1a6348 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 @@
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<CopyBufferToTextureCmd>(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 @@
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<CopyTextureToBufferCmd>(Command::CopyTextureToBuffer);
@@ -782,7 +798,7 @@
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 095cf61..034585a 100644
--- a/src/dawn_native/CommandValidation.cpp
+++ b/src/dawn_native/CommandValidation.cpp
@@ -440,8 +440,9 @@
}
// 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 28728bd..434e248 100644
--- a/src/tests/end2end/CopyTests.cpp
+++ b/src/tests/end2end/CopyTests.cpp
@@ -667,6 +667,32 @@
}
}
+// 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 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 6542c54..7d65e83 100644
--- a/src/tests/end2end/DeprecatedAPITests.cpp
+++ b/src/tests/end2end/DeprecatedAPITests.cpp
@@ -323,18 +323,18 @@
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 @@
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 4e5febd..edf9ccf 100644
--- a/src/tests/end2end/QueueTests.cpp
+++ b/src/tests/end2end/QueueTests.cpp
@@ -462,6 +462,36 @@
}
}
+// 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 1edb396..3d4f65c 100644
--- a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp
+++ b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp
@@ -507,16 +507,45 @@
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, 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 @@
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 a9dca68..d0aad61 100644
--- a/src/tests/unittests/validation/QueueWriteTextureValidationTests.cpp
+++ b/src/tests/unittests/validation/QueueWriteTextureValidationTests.cpp
@@ -221,11 +221,33 @@
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});
