Update validation for pass encoding beginRenderPass/beginComputePass

Make validation for pass encoding aligned to spec, where
descriptor validation failure will make pass invalid and stop
immediately instead of defer to CommandEncoder::Finish()

Bug: dawn:1602
Change-Id: I7892009e31f7565e4da43c38d365b056c9ecc22f
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/112448
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Shrek Shao <shrekshao@google.com>

src/dawn/native/CommandEncoder.cpp

@@ -781,11 +781,14 @@ ComputePassEncoder* CommandEncoder::APIBeginComputePass(const ComputePassDescrip
 Ref<ComputePassEncoder> CommandEncoder::BeginComputePass(const ComputePassDescriptor* descriptor) {
     DeviceBase* device = GetDevice();
 
+    // If descriptor is invalid, make pass invalid and stop immediately
+    if (device->ConsumedError(ValidateComputePassDescriptor(device, descriptor))) {
+        return ComputePassEncoder::MakeError(device, this, &mEncodingContext);
+    }
+
     bool success = mEncodingContext.TryEncode(
         this,
         [&](CommandAllocator* allocator) -> MaybeError {
-            DAWN_TRY(ValidateComputePassDescriptor(device, descriptor));
-
             BeginComputePassCmd* cmd =
                 allocator->Allocate<BeginComputePassCmd>(Command::BeginComputePass);
 
@@ -844,17 +847,20 @@ Ref<RenderPassEncoder> CommandEncoder::BeginRenderPass(const RenderPassDescripto
 
     uint32_t width = 0;
     uint32_t height = 0;
+    uint32_t sampleCount = 0;
     bool depthReadOnly = false;
     bool stencilReadOnly = false;
     Ref<AttachmentState> attachmentState;
+
+    // If descriptor is invalid, make pass invalid and stop immediately
+    if (device->ConsumedError(ValidateRenderPassDescriptor(device, descriptor, &width, &height,
+                                                           &sampleCount, mUsageValidationMode))) {
+        return RenderPassEncoder::MakeError(device, this, &mEncodingContext);
+    }
+
     bool success = mEncodingContext.TryEncode(
         this,
         [&](CommandAllocator* allocator) -> MaybeError {
-            uint32_t sampleCount = 0;
-
-            DAWN_TRY(ValidateRenderPassDescriptor(device, descriptor, &width, &height, &sampleCount,
-                                                  mUsageValidationMode));
-
             ASSERT(width > 0 && height > 0 && sampleCount > 0);
 
             mEncodingContext.WillBeginRenderPass();

src/dawn/tests/end2end/DeprecatedAPITests.cpp

@@ -47,8 +47,7 @@ class DeprecationTests : public DawnTest {
 
 // Test that setting attachment rather than view for render pass color and depth/stencil attachments
 // is deprecated.
-// TODO(dawn:1602): validation implementations need updating
+TEST_P(DeprecationTests, ReadOnlyDepthStencilStoreLoadOpsAttachment) {
-TEST_P(DeprecationTests, DISABLED_ReadOnlyDepthStencilStoreLoadOpsAttachment) {
     utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, 1, 1);
     wgpu::RenderPassEncoder pass;
 
@@ -71,6 +70,8 @@ TEST_P(DeprecationTests, DISABLED_ReadOnlyDepthStencilStoreLoadOpsAttachment) {
 
     depthAttachment->depthLoadOp = wgpu::LoadOp::Load;
     depthAttachment->depthStoreOp = wgpu::StoreOp::Store;
+    depthAttachment->stencilLoadOp = wgpu::LoadOp::Undefined;
+    depthAttachment->stencilStoreOp = wgpu::StoreOp::Undefined;
 
     {
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
@@ -92,8 +93,7 @@ TEST_P(DeprecationTests, DISABLED_ReadOnlyDepthStencilStoreLoadOpsAttachment) {
 
 // Test that setting the clearColor, clearDepth, or clearStencil values for render pass attachments
 // is deprecated. (dawn:1269)
-// TODO(dawn:1602): validation implementations need updating
+TEST_P(DeprecationTests, AttachmentClearColor) {
-TEST_P(DeprecationTests, DISABLED_AttachmentClearColor) {
     utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, 1, 1);
     wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
     wgpu::RenderPassEncoder pass;

src/dawn/tests/end2end/SwapChainValidationTests.cpp

@@ -78,16 +78,16 @@ class SwapChainValidationTests : public DawnTest {
     void CheckTextureViewIsValid(wgpu::TextureView view) { CheckTextureView(view, false, false); }
 
   private:
-    void CheckTextureView(wgpu::TextureView view, bool errorAtFinish, bool errorAtSubmit) {
+    void CheckTextureView(wgpu::TextureView view, bool errorAtBeginRenderPass, bool errorAtSubmit) {
         utils::ComboRenderPassDescriptor renderPassDesc({view});
 
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+
+        if (errorAtBeginRenderPass) {
+            ASSERT_DEVICE_ERROR(encoder.BeginRenderPass(&renderPassDesc));
+        } else {
             wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPassDesc);
             pass.End();
-
-        if (errorAtFinish) {
-            ASSERT_DEVICE_ERROR(encoder.Finish());
-        } else {
             wgpu::CommandBuffer commands = encoder.Finish();
 
             if (errorAtSubmit) {

src/dawn/tests/unittests/validation/QueryValidationTests.cpp

@@ -156,9 +156,7 @@ TEST_F(OcclusionQueryValidationTest, InvalidOcclusionQuerySet) {
             CreateQuerySet(otherDevice, wgpu::QueryType::Occlusion, 2);
         renderPass.occlusionQuerySet = occlusionQuerySetOnOther;
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+        ASSERT_DEVICE_ERROR(encoder.BeginRenderPass(&renderPass));
-        pass.End();
-        ASSERT_DEVICE_ERROR(encoder.Finish());
 
         // Clear this out so we don't hold a reference. The query set
         // must be destroyed before the device local to this test case.
@@ -335,8 +333,7 @@ TEST_F(TimestampQueryValidationTest, SetOcclusionQueryWithTimestampQuerySet) {
     renderPass.occlusionQuerySet = querySet;
 
     wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-    encoder.BeginRenderPass(&renderPass);
+    ASSERT_DEVICE_ERROR(encoder.BeginRenderPass(&renderPass));
-    ASSERT_DEVICE_ERROR(encoder.Finish());
 }
 
 // Test timestampWrites in compute pass descriptor
@@ -357,9 +354,8 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnComputePass) {
         wgpu::QuerySet occlusionQuerySet = CreateQuerySet(device, wgpu::QueryType::Occlusion, 1);
 
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        EncodeComputePassWithTimestampWrites(
+        ASSERT_DEVICE_ERROR(EncodeComputePassWithTimestampWrites(
-            encoder, {{occlusionQuerySet, 0, wgpu::ComputePassTimestampLocation::Beginning}});
+            encoder, {{occlusionQuerySet, 0, wgpu::ComputePassTimestampLocation::Beginning}}));
-        ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 
     // Fail to write timestamps to a query set created from another device
@@ -369,18 +365,16 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnComputePass) {
             CreateQuerySet(otherDevice, wgpu::QueryType::Timestamp, 2);
 
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        EncodeComputePassWithTimestampWrites(
+        ASSERT_DEVICE_ERROR(EncodeComputePassWithTimestampWrites(
             encoder, {{querySet, 0, wgpu::ComputePassTimestampLocation::Beginning},
-                      {querySetFromOtherDevice, 1, wgpu::ComputePassTimestampLocation::End}});
+                      {querySetFromOtherDevice, 1, wgpu::ComputePassTimestampLocation::End}}));
-        ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 
     // Fail to write timestamps to the query index which exceeds the number of queries in query set
     {
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        EncodeComputePassWithTimestampWrites(
+        ASSERT_DEVICE_ERROR(EncodeComputePassWithTimestampWrites(
-            encoder, {{querySet, 2, wgpu::ComputePassTimestampLocation::Beginning}});
+            encoder, {{querySet, 2, wgpu::ComputePassTimestampLocation::Beginning}}));
-        ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 
     // Success to write timestamps to the same query index twice on same compute pass
@@ -405,18 +399,16 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnComputePass) {
     // Fail to write timestamps at same location of a compute pass
     {
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        EncodeComputePassWithTimestampWrites(
+        ASSERT_DEVICE_ERROR(EncodeComputePassWithTimestampWrites(
             encoder, {{querySet, 0, wgpu::ComputePassTimestampLocation::Beginning},
-                      {querySet, 1, wgpu::ComputePassTimestampLocation::Beginning}});
+                      {querySet, 1, wgpu::ComputePassTimestampLocation::Beginning}}));
-        ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 
     // Fail to write timestamps at invalid location of compute pass
     {
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        EncodeComputePassWithTimestampWrites(
+        ASSERT_DEVICE_ERROR(EncodeComputePassWithTimestampWrites(
-            encoder, {{querySet, 0, static_cast<wgpu::ComputePassTimestampLocation>(0xFFFFFFFF)}});
+            encoder, {{querySet, 0, static_cast<wgpu::ComputePassTimestampLocation>(0xFFFFFFFF)}}));
-        ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 
     // Fail to write timestamps to a destroyed query set
@@ -451,9 +443,8 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnRenderPass) {
         wgpu::QuerySet occlusionQuerySet = CreateQuerySet(device, wgpu::QueryType::Occlusion, 1);
 
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        EncodeRenderPassWithTimestampWrites(
+        ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites(
-            encoder, {{occlusionQuerySet, 0, wgpu::RenderPassTimestampLocation::Beginning}});
+            encoder, {{occlusionQuerySet, 0, wgpu::RenderPassTimestampLocation::Beginning}}));
-        ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 
     // Fail to write timestamps to a query set created from another device
@@ -463,18 +454,16 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnRenderPass) {
             CreateQuerySet(otherDevice, wgpu::QueryType::Timestamp, 2);
 
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        EncodeRenderPassWithTimestampWrites(
+        ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites(
             encoder, {{querySet, 0, wgpu::RenderPassTimestampLocation::Beginning},
-                      {querySetFromOtherDevice, 1, wgpu::RenderPassTimestampLocation::End}});
+                      {querySetFromOtherDevice, 1, wgpu::RenderPassTimestampLocation::End}}));
-        ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 
     // Fail to write timestamps to the query index which exceeds the number of queries in query set
     {
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        EncodeRenderPassWithTimestampWrites(
+        ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites(
-            encoder, {{querySet, 2, wgpu::RenderPassTimestampLocation::Beginning}});
+            encoder, {{querySet, 2, wgpu::RenderPassTimestampLocation::Beginning}}));
-        ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 
     // Success to write timestamps to the same query index and location twice on different render
@@ -494,27 +483,24 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnRenderPass) {
     // Fail to write timestamps to the same query index twice on same render pass
     {
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        EncodeRenderPassWithTimestampWrites(
+        ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites(
             encoder, {{querySet, 0, wgpu::RenderPassTimestampLocation::Beginning},
-                      {querySet, 0, wgpu::RenderPassTimestampLocation::End}});
+                      {querySet, 0, wgpu::RenderPassTimestampLocation::End}}));
-        ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 
     // Fail to write timestamps at same location of a render pass
     {
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        EncodeRenderPassWithTimestampWrites(
+        ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites(
             encoder, {{querySet, 0, wgpu::RenderPassTimestampLocation::Beginning},
-                      {querySet, 1, wgpu::RenderPassTimestampLocation::Beginning}});
+                      {querySet, 1, wgpu::RenderPassTimestampLocation::Beginning}}));
-        ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 
     // Fail to write timestamps at invalid location of render pass
     {
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        EncodeRenderPassWithTimestampWrites(
+        ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites(
-            encoder, {{querySet, 0, static_cast<wgpu::RenderPassTimestampLocation>(0xFFFFFFFF)}});
+            encoder, {{querySet, 0, static_cast<wgpu::RenderPassTimestampLocation>(0xFFFFFFFF)}}));
-        ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 
     // Fail to write timestamps to a destroyed query set
@@ -785,8 +771,7 @@ TEST_F(PipelineStatisticsQueryValidationTest, BeginRenderPassWithPipelineStatist
     renderPass.occlusionQuerySet = querySet;
 
     wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-    encoder.BeginRenderPass(&renderPass);
+    ASSERT_DEVICE_ERROR(encoder.BeginRenderPass(&renderPass));
-    ASSERT_DEVICE_ERROR(encoder.Finish());
 }
 
 class ResolveQuerySetValidationTest : public QuerySetValidationTest {

src/dawn/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp

@@ -27,20 +27,14 @@ namespace {
 class RenderPassDescriptorValidationTest : public ValidationTest {
   public:
     void AssertBeginRenderPassSuccess(const wgpu::RenderPassDescriptor* descriptor) {
-        wgpu::CommandEncoder commandEncoder = TestBeginRenderPass(descriptor);
-        commandEncoder.Finish();
-    }
-    void AssertBeginRenderPassError(const wgpu::RenderPassDescriptor* descriptor) {
-        wgpu::CommandEncoder commandEncoder = TestBeginRenderPass(descriptor);
-        ASSERT_DEVICE_ERROR(commandEncoder.Finish());
-    }
-
-  private:
-    wgpu::CommandEncoder TestBeginRenderPass(const wgpu::RenderPassDescriptor* descriptor) {
         wgpu::CommandEncoder commandEncoder = device.CreateCommandEncoder();
         wgpu::RenderPassEncoder renderPassEncoder = commandEncoder.BeginRenderPass(descriptor);
         renderPassEncoder.End();
-        return commandEncoder;
+        commandEncoder.Finish();
+    }
+    void AssertBeginRenderPassError(const wgpu::RenderPassDescriptor* descriptor) {
+        wgpu::CommandEncoder commandEncoder = device.CreateCommandEncoder();
+        ASSERT_DEVICE_ERROR(commandEncoder.BeginRenderPass(descriptor));
     }
 };
 

src/dawn/tests/unittests/validation/RenderPipelineValidationTests.cpp

@@ -710,11 +710,7 @@ TEST_F(RenderPipelineValidationTest, VertexOnlyPipelineRequireDepthStencilAttach
         utils::ComboRenderPassDescriptor renderPassDescriptor({});
 
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        wgpu::RenderPassEncoder renderPass = encoder.BeginRenderPass(&renderPassDescriptor);
+        ASSERT_DEVICE_ERROR(encoder.BeginRenderPass(&renderPassDescriptor));
-        renderPass.SetPipeline(vertexOnlyPipeline);
-        renderPass.End();
-
-        ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 
     // Vertex-only render pipeline can not work with color target

webgpu-cts/expectations.txt

@@ -445,6 +445,19 @@ crbug.com/dawn/1599 [ webgpu-adapter-swiftshader ] webgpu:web_platform,copyToTex
 crbug.com/dawn/1082 webgpu:web_platform,external_texture,video:* [ Skip ]
 ################################################################################
 
+################################################################################
+# commandEncoder validation tests need updates in CTS
+################################################################################
+crbug.com/dawn/1602 webgpu:api,validation,encoding,beginComputePass:* [ Skip ]
+crbug.com/dawn/1602 webgpu:api,validation,encoding,beginRenderPass:* [ Skip ]
+crbug.com/dawn/1602 webgpu:api,validation,encoding,encoder_open_state:non_pass_commands:* [ Skip ]
+crbug.com/dawn/1602 webgpu:api,validation,encoding,queries,general:occlusion_query,* [ Skip ]
+crbug.com/dawn/1602 webgpu:api,validation,render_pass,render_pass_descriptor:* [ Skip ]
+crbug.com/dawn/1602 webgpu:api,validation,render_pass,resolve:resolve_attachment:* [ Skip ]
+crbug.com/dawn/1602 webgpu:api,validation,render_pass,storeOp:* [ Skip ]
+crbug.com/dawn/1602 webgpu:api,validation,resource_usages,texture,in_render_misc:subresources,set_bind_group_on_same_index_depth_stencil_texture:* [ Skip ]
+################################################################################
+
 ################################################################################
 # untriaged failures
 # KEEP