Refix validation for command encoder pass encoding

According to latest spec, beginRenderPass/beginComputePass
don't throw validation error. Error still defers to
CommandEncoder::Finish()

Bug: dawn:1602
Change-Id: I5fd76f2c8951273a8dd82b02e20f076079354b60
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/115120
Reviewed-by: Austin Eng <enga@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Shrek Shao <shrekshao@google.com>

src/dawn/native/CommandEncoder.cpp

@@ -796,14 +796,11 @@ 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);
 
@@ -867,18 +864,14 @@ Ref<RenderPassEncoder> CommandEncoder::BeginRenderPass(const RenderPassDescripto
     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),
-                              "validating render pass descriptor.")) {
-        return RenderPassEncoder::MakeError(device, this, &mEncodingContext);
-    }
-
     std::function<void()> passEndCallback = nullptr;
 
     bool success = mEncodingContext.TryEncode(
         this,
         [&](CommandAllocator* allocator) -> MaybeError {
+            DAWN_TRY(ValidateRenderPassDescriptor(device, descriptor, &width, &height, &sampleCount,
+                                                  mUsageValidationMode));
+
             ASSERT(width > 0 && height > 0 && sampleCount > 0);
 
             mEncodingContext.WillBeginRenderPass();

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 errorAtBeginRenderPass, bool errorAtSubmit) {
+    void CheckTextureView(wgpu::TextureView view, bool errorAtFinish, 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/DeprecatedAPITests.cpp

@@ -64,8 +64,9 @@ TEST_P(DeprecationTests, ReadOnlyDepthStencilStoreLoadOpsAttachment) {
 
     {
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        EXPECT_DEPRECATION_ERROR_OR_WARNING(
+        EXPECT_DEPRECATION_WARNING_ONLY(pass = encoder.BeginRenderPass(&renderPass.renderPassInfo));
-            pass = encoder.BeginRenderPass(&renderPass.renderPassInfo));
+        pass.End();
+        EXPECT_DEPRECATION_ERROR_ONLY(encoder.Finish(););
     }
 
     depthAttachment->depthLoadOp = wgpu::LoadOp::Undefined;
@@ -75,8 +76,9 @@ TEST_P(DeprecationTests, ReadOnlyDepthStencilStoreLoadOpsAttachment) {
 
     {
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        EXPECT_DEPRECATION_ERROR_OR_WARNING(
+        EXPECT_DEPRECATION_WARNING_ONLY(pass = encoder.BeginRenderPass(&renderPass.renderPassInfo));
-            pass = encoder.BeginRenderPass(&renderPass.renderPassInfo));
+        pass.End();
+        EXPECT_DEPRECATION_ERROR_ONLY(encoder.Finish(););
     }
 }
 
@@ -110,19 +112,22 @@ TEST_P(DeprecationTests, AttachmentClearColor) {
 
     depthAttachment->clearStencil = 1;
 
-    EXPECT_DEPRECATION_ERROR_OR_WARNING(pass = encoder.BeginRenderPass(&renderPass.renderPassInfo));
+    EXPECT_DEPRECATION_WARNING_ONLY(pass = encoder.BeginRenderPass(&renderPass.renderPassInfo));
+    pass.End();
 
     depthAttachment->clearStencil = 0;
     depthAttachment->depthClearValue = 0.0f;
     depthAttachment->clearDepth = 1.0f;
 
-    EXPECT_DEPRECATION_ERROR_OR_WARNING(pass = encoder.BeginRenderPass(&renderPass.renderPassInfo));
+    EXPECT_DEPRECATION_WARNING_ONLY(pass = encoder.BeginRenderPass(&renderPass.renderPassInfo));
+    pass.End();
 
     renderPass.renderPassInfo.depthStencilAttachment = nullptr;
     renderPass.renderPassInfo.cColorAttachments[0].clearColor = {1.0, 2.0, 3.0, 4.0};
     renderPass.renderPassInfo.cColorAttachments[0].clearValue = {5.0, 4.0, 3.0, 2.0};
 
-    EXPECT_DEPRECATION_ERROR_OR_WARNING(pass = encoder.BeginRenderPass(&renderPass.renderPassInfo));
+    EXPECT_DEPRECATION_WARNING_ONLY(pass = encoder.BeginRenderPass(&renderPass.renderPassInfo));
+    pass.End();
 }
 
 // Test that endPass() is deprecated for both render and compute passes.

src/dawn/tests/unittests/validation/DeprecatedAPITests.h

@@ -35,6 +35,24 @@ static constexpr char kDisallowDeprecatedAPIsToggleName[] = "disallow_deprecated
     for (;;)                                                   \
     break
 
+#define EXPECT_DEPRECATION_WARNING_ONLY(statement)              \
+    if (!HasToggleEnabled(kDisallowDeprecatedAPIsToggleName)) { \
+        EXPECT_DEPRECATION_WARNING(statement);                  \
+    } else {                                                    \
+        statement;                                              \
+    }                                                           \
+    for (;;)                                                    \
+    break
+
+#define EXPECT_DEPRECATION_ERROR_ONLY(statement)               \
+    if (HasToggleEnabled(kDisallowDeprecatedAPIsToggleName)) { \
+        ASSERT_DEVICE_ERROR(statement);                        \
+    } else {                                                   \
+        statement;                                             \
+    }                                                          \
+    for (;;)                                                   \
+    break
+
 // Parameter is a single bool. When true, deprecated APIs are strictly disallowed (i.e. generate
 // errors). Otherwise, deprecated APIs only generate a warning message.
 class DeprecationTests : public ValidationTest, public testing::WithParamInterface<bool> {

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

@@ -156,7 +156,9 @@ TEST_F(OcclusionQueryValidationTest, InvalidOcclusionQuerySet) {
             CreateQuerySet(otherDevice, wgpu::QueryType::Occlusion, 2);
         renderPass.occlusionQuerySet = occlusionQuerySetOnOther;
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        ASSERT_DEVICE_ERROR(encoder.BeginRenderPass(&renderPass));
+        wgpu::RenderPassEncoder pass = 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.
@@ -333,7 +335,8 @@ TEST_F(TimestampQueryValidationTest, SetOcclusionQueryWithTimestampQuerySet) {
     renderPass.occlusionQuerySet = querySet;
 
     wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-    ASSERT_DEVICE_ERROR(encoder.BeginRenderPass(&renderPass));
+    encoder.BeginRenderPass(&renderPass);
+    ASSERT_DEVICE_ERROR(encoder.Finish());
 }
 
 // Test timestampWrites in compute pass descriptor
@@ -354,8 +357,9 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnComputePass) {
         wgpu::QuerySet occlusionQuerySet = CreateQuerySet(device, wgpu::QueryType::Occlusion, 1);
 
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        ASSERT_DEVICE_ERROR(EncodeComputePassWithTimestampWrites(
+        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
@@ -365,16 +369,18 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnComputePass) {
             CreateQuerySet(otherDevice, wgpu::QueryType::Timestamp, 2);
 
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        ASSERT_DEVICE_ERROR(EncodeComputePassWithTimestampWrites(
+        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();
-        ASSERT_DEVICE_ERROR(EncodeComputePassWithTimestampWrites(
+        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
@@ -399,16 +405,18 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnComputePass) {
     // Fail to write timestamps at same location of a compute pass
     {
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        ASSERT_DEVICE_ERROR(EncodeComputePassWithTimestampWrites(
+        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();
-        ASSERT_DEVICE_ERROR(EncodeComputePassWithTimestampWrites(
+        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
@@ -443,8 +451,9 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnRenderPass) {
         wgpu::QuerySet occlusionQuerySet = CreateQuerySet(device, wgpu::QueryType::Occlusion, 1);
 
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites(
+        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
@@ -454,16 +463,18 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnRenderPass) {
             CreateQuerySet(otherDevice, wgpu::QueryType::Timestamp, 2);
 
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites(
+        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();
-        ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites(
+        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
@@ -483,24 +494,27 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnRenderPass) {
     // Fail to write timestamps to the same query index twice on same render pass
     {
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites(
+        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();
-        ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites(
+        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();
-        ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites(
+        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
@@ -771,7 +785,8 @@ TEST_F(PipelineStatisticsQueryValidationTest, BeginRenderPassWithPipelineStatist
     renderPass.occlusionQuerySet = querySet;
 
     wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-    ASSERT_DEVICE_ERROR(encoder.BeginRenderPass(&renderPass));
+    encoder.BeginRenderPass(&renderPass);
+    ASSERT_DEVICE_ERROR(encoder.Finish());
 }
 
 class ResolveQuerySetValidationTest : public QuerySetValidationTest {

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

@@ -29,14 +29,20 @@ namespace {
 class RenderPassDescriptorValidationTest : public ValidationTest {
   public:
     void AssertBeginRenderPassSuccess(const wgpu::RenderPassDescriptor* descriptor) {
-        wgpu::CommandEncoder commandEncoder = device.CreateCommandEncoder();
+        wgpu::CommandEncoder commandEncoder = TestBeginRenderPass(descriptor);
-        wgpu::RenderPassEncoder renderPassEncoder = commandEncoder.BeginRenderPass(descriptor);
-        renderPassEncoder.End();
         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();
-        ASSERT_DEVICE_ERROR(commandEncoder.BeginRenderPass(descriptor));
+        wgpu::RenderPassEncoder renderPassEncoder = commandEncoder.BeginRenderPass(descriptor);
+        renderPassEncoder.End();
+        return commandEncoder;
     }
 };
 
@@ -1464,7 +1470,7 @@ TEST_P(DeprecationTests, RenderPassColorAttachmentBytesPerSample) {
             renderPassEncoder.End();
             commandEncoder.Finish();
         } else {
-            EXPECT_DEPRECATION_ERROR_OR_WARNING(commandEncoder.BeginRenderPass(&descriptor));
+            EXPECT_DEPRECATION_WARNING_ONLY(commandEncoder.BeginRenderPass(&descriptor));
         }
     }
 }

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

@@ -754,7 +754,11 @@ TEST_F(RenderPipelineValidationTest, VertexOnlyPipelineRequireDepthStencilAttach
         utils::ComboRenderPassDescriptor renderPassDescriptor;
 
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        ASSERT_DEVICE_ERROR(encoder.BeginRenderPass(&renderPassDescriptor));
+        wgpu::RenderPassEncoder renderPass = 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

@@ -442,20 +442,6 @@ crbug.com/dawn/1599 [ linux webgpu-adapter-swiftshader ] webgpu:web_platform,cop
 crbug.com/dawn/1599 [ webgpu-adapter-swiftshader ] webgpu:web_platform,copyToTexture,canvas:color_space_conversion:* [ Failure ]
 ################################################################################
 
-################################################################################
-# commandEncoder validation tests need updates in CTS
-# KEEP
-################################################################################
-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 ]
-################################################################################
-
 ################################################################################
 # Buffer.MapAsync pending map error tests need updates in CTS
 ################################################################################