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>

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<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 @@ 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<CopyTextureToBufferCmd>(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;
 

src/dawn_native/CommandValidation.cpp

@@ -440,8 +440,9 @@ namespace dawn_native {
         }
 
         // Validation for other members in layout:
-        if (layout.bytesPerRow <
+        if ((copyExtent.height > 1 || copyExtent.depth > 1) &&
-            copyExtent.width / blockInfo.blockWidth * blockInfo.blockByteSize) {
+            layout.bytesPerRow <
+                copyExtent.width / blockInfo.blockWidth * blockInfo.blockByteSize) {
             return DAWN_VALIDATION_ERROR(
                 "bytesPerRow must not be less than the number of bytes per row");
         }

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;

src/tests/end2end/DeprecatedAPITests.cpp

@@ -323,18 +323,18 @@ TEST_P(BufferCopyViewDeprecationTests, DeprecationWarning) {
         bufCopy.offset = 4;
         EXPECT_DEPRECATION_WARNING(encoder.CopyBufferToTexture(&bufCopy, &texCopy, &copySize));
         EXPECT_DEPRECATION_WARNING(encoder.CopyTextureToBuffer(&texCopy, &bufCopy, &copySize));
-        // Since bytesPerRow is 0
+        wgpu::CommandBuffer command = encoder.Finish();
-        ASSERT_DEVICE_ERROR(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, &copySize));
         EXPECT_DEPRECATION_WARNING(encoder.CopyTextureToBuffer(&texCopy, &bufCopy, &copySize));
-        wgpu::CommandBuffer command = encoder.Finish();
+        // Since bytesPerRow is not 256-aligned.
-        queue.Submit(1, &command);
+        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, &copySize));
         EXPECT_DEPRECATION_WARNING(encoder.CopyTextureToBuffer(&texCopy, &bufCopy, &copySize));
-        // Since bytesPerRow is 0
+        wgpu::CommandBuffer command = encoder.Finish();
-        ASSERT_DEVICE_ERROR(encoder.Finish());
+        queue.Submit(1, &command);
     }
 }
 

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;

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::TextureUsage::CopySrc);
-    wgpu::Buffer destination = CreateBuffer(bufferSize, wgpu::BufferUsage::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});
         }

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
+        // bytesPerRow = 0
-        ASSERT_DEVICE_ERROR(TestWriteTexture(128, 0, 0, 0, destination, 0, {0, 0, 0}, {3, 7, 1}));
+        {
+            // 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});