Allow unaligned source offset in writeTexture

With some refactoring of the relevant validation code.

Bug: dawn:520
Change-Id: Iedda0f7b1b67c20d3a88f2c4183dcc8eeae2096f
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/30742
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Kai Ninomiya <kainino@chromium.org>

src/dawn_native/CommandEncoder.cpp

@@ -101,36 +101,20 @@ namespace dawn_native {
             return {};
         }
 
-        MaybeError ValidateTextureToBufferCopyRestrictions(const TextureCopyView& src) {
+        MaybeError ValidateLinearTextureCopyOffset(const TextureDataLayout& layout,
-            const Format& format = src.texture->GetFormat();
+                                                   const TexelBlockInfo& blockInfo) {
-
+            if (layout.offset % blockInfo.byteSize != 0) {
-            bool depthSelected = false;
-            switch (src.aspect) {
-                case wgpu::TextureAspect::All:
-                    switch (format.aspects) {
-                        case Aspect::Color:
-                        case Aspect::Stencil:
-                            break;
-                        case Aspect::Depth:
-                            depthSelected = true;
-                            break;
-                        default:
                 return DAWN_VALIDATION_ERROR(
-                                "A single aspect must be selected for multi planar formats in "
+                    "offset must be a multiple of the texel block byte size.");
-                                "texture to buffer copies");
             }
-                    break;
+            return {};
-                case wgpu::TextureAspect::DepthOnly:
-                    ASSERT(format.aspects & Aspect::Depth);
-                    depthSelected = true;
-                    break;
-                case wgpu::TextureAspect::StencilOnly:
-                    ASSERT(format.aspects & Aspect::Stencil);
-                    break;
         }
 
-            if (depthSelected) {
+        MaybeError ValidateTextureDepthStencilToBufferCopyRestrictions(const TextureCopyView& src) {
-                switch (format.format) {
+            Aspect aspectUsed;
+            DAWN_TRY_ASSIGN(aspectUsed, SingleAspectUsedByTextureCopyView(src));
+            if (aspectUsed == Aspect::Depth) {
+                switch (src.texture->GetFormat().format) {
                     case wgpu::TextureFormat::Depth24Plus:
                     case wgpu::TextureFormat::Depth24PlusStencil8:
                         return DAWN_VALIDATION_ERROR(
@@ -640,7 +624,7 @@ namespace dawn_native {
                 DAWN_TRY(ValidateCanUseAs(destination->texture, wgpu::TextureUsage::CopyDst));
                 DAWN_TRY(ValidateTextureSampleCountInBufferCopyCommands(destination->texture));
 
-                DAWN_TRY(ValidateBufferToTextureCopyRestrictions(*destination));
+                DAWN_TRY(ValidateLinearToDepthStencilCopyRestrictions(*destination));
                 // We validate texture copy range before validating linear texture data,
                 // because in the latter we divide copyExtent.width by blockWidth and
                 // copyExtent.height by blockHeight while the divisibility conditions are
@@ -650,6 +634,7 @@ namespace dawn_native {
             const TexelBlockInfo& blockInfo =
                 destination->texture->GetFormat().GetAspectInfo(destination->aspect).block;
             if (GetDevice()->IsValidationEnabled()) {
+                DAWN_TRY(ValidateLinearTextureCopyOffset(source->layout, blockInfo));
                 DAWN_TRY(ValidateLinearTextureData(source->layout, source->buffer->GetSize(),
                                                    blockInfo, *copySize));
 
@@ -700,11 +685,11 @@ namespace dawn_native {
                 DAWN_TRY(ValidateTextureCopyView(GetDevice(), *source, *copySize));
                 DAWN_TRY(ValidateCanUseAs(source->texture, wgpu::TextureUsage::CopySrc));
                 DAWN_TRY(ValidateTextureSampleCountInBufferCopyCommands(source->texture));
+                DAWN_TRY(ValidateTextureDepthStencilToBufferCopyRestrictions(*source));
 
                 DAWN_TRY(ValidateBufferCopyView(GetDevice(), *destination));
                 DAWN_TRY(ValidateCanUseAs(destination->buffer, wgpu::BufferUsage::CopyDst));
 
-                DAWN_TRY(ValidateTextureToBufferCopyRestrictions(*source));
                 // We validate texture copy range before validating linear texture data,
                 // because in the latter we divide copyExtent.width by blockWidth and
                 // copyExtent.height by blockHeight while the divisibility conditions are
@@ -714,6 +699,7 @@ namespace dawn_native {
             const TexelBlockInfo& blockInfo =
                 source->texture->GetFormat().GetAspectInfo(source->aspect).block;
             if (GetDevice()->IsValidationEnabled()) {
+                DAWN_TRY(ValidateLinearTextureCopyOffset(destination->layout, blockInfo));
                 DAWN_TRY(ValidateLinearTextureData(
                     destination->layout, destination->buffer->GetSize(), blockInfo, *copySize));
 

src/dawn_native/CommandValidation.cpp

@@ -446,11 +446,6 @@ namespace dawn_native {
                                          uint64_t byteSize,
                                          const TexelBlockInfo& blockInfo,
                                          const Extent3D& copyExtent) {
-        // Validation for the texel block alignments:
-        if (layout.offset % blockInfo.byteSize != 0) {
-            return DAWN_VALIDATION_ERROR("Offset must be a multiple of the texel or block size");
-        }
-
         ASSERT(copyExtent.width % blockInfo.width == 0);
         uint32_t widthInBlocks = copyExtent.width / blockInfo.width;
         ASSERT(copyExtent.height % blockInfo.height == 0);
@@ -589,36 +584,35 @@ namespace dawn_native {
         return {};
     }
 
-    MaybeError ValidateBufferToTextureCopyRestrictions(const TextureCopyView& dst) {
+    // Always returns a single aspect (color, stencil, or depth).
-        const Format& format = dst.texture->GetFormat();
+    ResultOrError<Aspect> SingleAspectUsedByTextureCopyView(const TextureCopyView& view) {
-
+        const Format& format = view.texture->GetFormat();
-        bool depthSelected = false;
+        switch (view.aspect) {
-        switch (dst.aspect) {
             case wgpu::TextureAspect::All:
-                switch (format.aspects) {
+                if (HasOneBit(format.aspects)) {
-                    case Aspect::Color:
+                    return Aspect{format.aspects};
-                    case Aspect::Stencil:
+                } else {
-                        break;
-                    case Aspect::Depth:
-                        depthSelected = true;
-                        break;
-                    default:
                     return DAWN_VALIDATION_ERROR(
-                            "A single aspect must be selected for multi planar formats in buffer "
+                        "A single aspect must be selected for multi-planar formats in "
-                            "to texture copies");
+                        "texture <-> linear data copies");
                 }
                 break;
             case wgpu::TextureAspect::DepthOnly:
                 ASSERT(format.aspects & Aspect::Depth);
-                depthSelected = true;
+                return Aspect::Depth;
-                break;
             case wgpu::TextureAspect::StencilOnly:
                 ASSERT(format.aspects & Aspect::Stencil);
-                break;
+                return Aspect::Stencil;
         }
-        if (depthSelected) {
+    }
+
+    MaybeError ValidateLinearToDepthStencilCopyRestrictions(const TextureCopyView& dst) {
+        Aspect aspectUsed;
+        DAWN_TRY_ASSIGN(aspectUsed, SingleAspectUsedByTextureCopyView(dst));
+        if (aspectUsed == Aspect::Depth) {
             return DAWN_VALIDATION_ERROR("Cannot copy into the depth aspect of a texture");
         }
+
         return {};
     }
 

src/dawn_native/CommandValidation.h

@@ -57,7 +57,8 @@ namespace dawn_native {
                                          const Extent3D& copyExtent);
     MaybeError ValidateTextureCopyRange(const TextureCopyView& textureCopyView,
                                         const Extent3D& copySize);
-    MaybeError ValidateBufferToTextureCopyRestrictions(const TextureCopyView& dst);
+    ResultOrError<Aspect> SingleAspectUsedByTextureCopyView(const TextureCopyView& view);
+    MaybeError ValidateLinearToDepthStencilCopyRestrictions(const TextureCopyView& dst);
 
     MaybeError ValidateBufferCopyView(DeviceBase const* device,
                                       const BufferCopyView& bufferCopyView);

src/dawn_native/Queue.cpp

@@ -415,7 +415,7 @@ namespace dawn_native {
             return DAWN_VALIDATION_ERROR("The sample count of textures must be 1");
         }
 
-        DAWN_TRY(ValidateBufferToTextureCopyRestrictions(*destination));
+        DAWN_TRY(ValidateLinearToDepthStencilCopyRestrictions(*destination));
         // We validate texture copy range before validating linear texture data,
         // because in the latter we divide copyExtent.width by blockWidth and
         // copyExtent.height by blockHeight while the divisibility conditions are

src/tests/end2end/QueueTests.cpp

@@ -446,9 +446,8 @@ TEST_P(QueueWriteTextureTests, VaryingDataOffset) {
     textureSpec.textureSize = {kWidth, kHeight, 1};
     textureSpec.level = 0;
 
-    for (unsigned int i : {1, 2, 4, 17, 64, 128, 300}) {
+    for (uint64_t offset : {1, 2, 4, 17, 64, 128, 300}) {
         DataSpec dataSpec = MinimumDataSpec({kWidth, kHeight, 1});
-        uint64_t offset = i * utils::GetTexelBlockSizeInBytes(kTextureFormat);
         dataSpec.size += offset;
         dataSpec.offset += offset;
         DoTest(textureSpec, dataSpec, {kWidth, kHeight, 1});

src/tests/unittests/validation/QueueWriteTextureValidationTests.cpp

@@ -278,18 +278,20 @@ namespace {
             TestWriteTexture(dataSize, 0, 256, 3, destination, 0, {0, 0, 0}, {4, 4, 1}));
     }
 
-    // Test WriteTexture with incorrect data offset usage
+    // Test WriteTexture with data offset
-    TEST_F(QueueWriteTextureValidationTest, IncorrectDataOffset) {
+    TEST_F(QueueWriteTextureValidationTest, DataOffset) {
         uint64_t dataSize =
             utils::RequiredBytesInCopy(256, 0, {4, 4, 1}, wgpu::TextureFormat::RGBA8Unorm);
         wgpu::Texture destination = Create2DTexture({16, 16, 1}, 5, wgpu::TextureFormat::RGBA8Unorm,
                                                     wgpu::TextureUsage::CopyDst);
 
-        // Correct usage
+        // Offset aligned
         TestWriteTexture(dataSize, dataSize - 4, 256, 0, destination, 0, {0, 0, 0}, {1, 1, 1});
-
+        // Offset not aligned
+        TestWriteTexture(dataSize, dataSize - 5, 256, 0, destination, 0, {0, 0, 0}, {1, 1, 1});
+        // Offset+size too large
         ASSERT_DEVICE_ERROR(
-            TestWriteTexture(dataSize, dataSize - 6, 256, 0, destination, 0, {0, 0, 0}, {1, 1, 1}));
+            TestWriteTexture(dataSize, dataSize - 3, 256, 0, destination, 0, {0, 0, 0}, {1, 1, 1}));
     }
 
     // Test multisampled textures can be used in WriteTexture.
@@ -551,23 +553,21 @@ namespace {
         static constexpr uint32_t kHeight = 16;
     };
 
-    // Tests to verify that data offset must be a multiple of the compressed texture blocks in bytes
+    // Tests to verify that data offset may not be a multiple of the compressed texture block size
     TEST_F(WriteTextureTest_CompressedTextureFormats, DataOffset) {
         for (wgpu::TextureFormat bcFormat : utils::kBCFormats) {
             wgpu::Texture texture = Create2DTexture(bcFormat);
 
-            // Valid usages of data offset.
+            // Valid if aligned.
             {
-                uint32_t validDataOffset = utils::GetTexelBlockSizeInBytes(bcFormat);
+                uint32_t kAlignedOffset = utils::GetTexelBlockSizeInBytes(bcFormat);
-                QueueWriteTextureValidationTest::TestWriteTexture(512, validDataOffset, 256, 4,
+                TestWriteTexture(1024, kAlignedOffset, 256, 4, texture, 0, {0, 0, 0}, {4, 16, 1});
-                                                                  texture, 0, {0, 0, 0}, {4, 4, 1});
             }
 
-            // Failures on invalid data offset.
+            // Still valid if not aligned.
             {
-                uint32_t kInvalidDataOffset = utils::GetTexelBlockSizeInBytes(bcFormat) / 2;
+                uint32_t kUnalignedOffset = utils::GetTexelBlockSizeInBytes(bcFormat) - 1;
-                ASSERT_DEVICE_ERROR(TestWriteTexture(512, kInvalidDataOffset, 256, 4, texture, 0,
+                TestWriteTexture(1024, kUnalignedOffset, 256, 4, texture, 0, {0, 0, 0}, {4, 16, 1});
-                                                     {0, 0, 0}, {4, 4, 1}));
             }
         }
     }