Validate texture aspect on TextureView creation

Bug: dawn:439
Change-Id: Iba8c283e2f4551d9600410ff958d5a304a49ae2c
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/30724
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Stephen White <senorblanco@chromium.org>
Commit-Queue: Austin Eng <enga@chromium.org>

src/dawn_native/CommandValidation.cpp

@@ -501,24 +501,8 @@ namespace dawn_native {
             return DAWN_VALIDATION_ERROR("mipLevel out of range");
         }
 
-        switch (textureCopy.aspect) {
+        if (TryConvertAspect(texture->GetFormat(), textureCopy.aspect) == Aspect::None) {
-            case wgpu::TextureAspect::All:
+            return DAWN_VALIDATION_ERROR("Texture does not have selected aspect for texture copy.");
-                break;
-            case wgpu::TextureAspect::DepthOnly:
-                if ((texture->GetFormat().aspects & Aspect::Depth) == 0) {
-                    return DAWN_VALIDATION_ERROR(
-                        "Texture does not have depth aspect for texture copy");
-                }
-                break;
-            case wgpu::TextureAspect::StencilOnly:
-                if ((texture->GetFormat().aspects & Aspect::Stencil) == 0) {
-                    return DAWN_VALIDATION_ERROR(
-                        "Texture does not have stencil aspect for texture copy");
-                }
-                break;
-            default:
-                UNREACHABLE();
-                break;
         }
 
         if (texture->GetSampleCount() > 1 || texture->GetFormat().HasDepthOrStencil()) {

src/dawn_native/Texture.cpp

@@ -271,8 +271,8 @@ namespace dawn_native {
         DAWN_TRY(ValidateTextureFormat(descriptor->format));
 
         DAWN_TRY(ValidateTextureAspect(descriptor->aspect));
-        if (descriptor->aspect != wgpu::TextureAspect::All) {
+        if (TryConvertAspect(texture->GetFormat(), descriptor->aspect) == Aspect::None) {
-            return DAWN_VALIDATION_ERROR("Texture aspect must be 'all'");
+            return DAWN_VALIDATION_ERROR("Texture does not have selected aspect for texture view.");
         }
 
         // TODO(jiawei.shao@intel.com): check stuff based on resource limits
@@ -358,15 +358,19 @@ namespace dawn_native {
     }
 
     Aspect ConvertAspect(const Format& format, wgpu::TextureAspect aspect) {
+        Aspect aspectMask = TryConvertAspect(format, aspect);
+        ASSERT(aspectMask != Aspect::None);
+        return aspectMask;
+    }
+
+    Aspect TryConvertAspect(const Format& format, wgpu::TextureAspect aspect) {
         switch (aspect) {
             case wgpu::TextureAspect::All:
                 return format.aspects;
             case wgpu::TextureAspect::DepthOnly:
-                ASSERT(format.aspects & Aspect::Depth);
+                return format.aspects & Aspect::Depth;
-                return Aspect::Depth;
             case wgpu::TextureAspect::StencilOnly:
-                ASSERT(format.aspects & Aspect::Stencil);
+                return format.aspects & Aspect::Stencil;
-                return Aspect::Stencil;
         }
     }
 

src/dawn_native/Texture.h

@@ -73,9 +73,19 @@ namespace dawn_native {
         wgpu::TextureUsage::CopyDst | wgpu::TextureUsage::Storage |
         wgpu::TextureUsage::OutputAttachment;
 
+    // Convert the TextureAspect to an Aspect mask for the format. ASSERTs if the aspect
+    // does not exist in the format.
+    // Also ASSERTs if "All" is selected and results in more than one aspect.
     Aspect ConvertSingleAspect(const Format& format, wgpu::TextureAspect aspect);
+
+    // Convert the TextureAspect to an Aspect mask for the format. ASSERTs if the aspect
+    // does not exist in the format.
     Aspect ConvertAspect(const Format& format, wgpu::TextureAspect aspect);
 
+    // Try to convert the TextureAspect to an Aspect mask for the format. May return
+    // Aspect::None.
+    Aspect TryConvertAspect(const Format& format, wgpu::TextureAspect aspect);
+
     struct SubresourceRange {
         uint32_t baseMipLevel;
         uint32_t levelCount;

src/tests/unittests/validation/TextureViewValidationTests.cpp

@@ -344,12 +344,15 @@ namespace {
         ASSERT_DEVICE_ERROR(texture.CreateView(&descriptor));
     }
 
-    // Test that only TextureAspect::All is supported
+    // Test that the selected TextureAspects must exist in the texture format
-    TEST_F(TextureViewValidationTest, AspectMustBeAll) {
+    TEST_F(TextureViewValidationTest, AspectMustExist) {
         wgpu::TextureDescriptor descriptor = {};
         descriptor.size = {1, 1, 1};
-        descriptor.format = wgpu::TextureFormat::Depth32Float;
         descriptor.usage = wgpu::TextureUsage::Sampled | wgpu::TextureUsage::OutputAttachment;
+
+        // Can select: All and DepthOnly from Depth32Float, but not StencilOnly
+        {
+            descriptor.format = wgpu::TextureFormat::Depth32Float;
             wgpu::Texture texture = device.CreateTexture(&descriptor);
 
             wgpu::TextureViewDescriptor viewDescriptor = {};
@@ -357,7 +360,27 @@ namespace {
             texture.CreateView(&viewDescriptor);
 
             viewDescriptor.aspect = wgpu::TextureAspect::DepthOnly;
+            texture.CreateView(&viewDescriptor);
+
+            viewDescriptor.aspect = wgpu::TextureAspect::StencilOnly;
             ASSERT_DEVICE_ERROR(texture.CreateView(&viewDescriptor));
         }
 
+        // Can select: All, DepthOnly, and StencilOnly from Depth24PlusStencil8
+        {
+            descriptor.format = wgpu::TextureFormat::Depth24PlusStencil8;
+            wgpu::Texture texture = device.CreateTexture(&descriptor);
+
+            wgpu::TextureViewDescriptor viewDescriptor = {};
+            viewDescriptor.aspect = wgpu::TextureAspect::All;
+            texture.CreateView(&viewDescriptor);
+
+            viewDescriptor.aspect = wgpu::TextureAspect::DepthOnly;
+            texture.CreateView(&viewDescriptor);
+
+            viewDescriptor.aspect = wgpu::TextureAspect::StencilOnly;
+            texture.CreateView(&viewDescriptor);
+        }
+    }
+
 }  // anonymous namespace