commit ef90079944eb1c7b94f70320203997dced1658b6
author Corentin Wallez <cwallez@chromium.org> Thu Apr 25 20:14:32 2019 +0000
committer Commit Bot service account <commit-bot@chromium.org> Thu Apr 25 20:14:32 2019 +0000
tree b099a27694da33cfec3002a4ab82a53adda4126b
parent 3826880f81c3a96a39213f665c0094c79271d93e

CommandEncoder: promote errors to device errors after finish()

BUG=dawn:138

Change-Id: Iddb1aab886558c706d0188adc21c7ee8f68533d4
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/6720
Commit-Queue: Kai Ninomiya <kainino@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>

src/dawn_native/CommandEncoder.cpp

diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp
index 080990a..7b50777 100644
--- a/src/dawn_native/CommandEncoder.cpp
+++ b/src/dawn_native/CommandEncoder.cpp
@@ -824,9 +824,13 @@
     // Implementation of functions to interact with sub-encoders
 
     void CommandEncoderBase::HandleError(const char* message) {
-        if (!mGotError) {
-            mGotError = true;
-            mErrorMessage = message;
+        if (mEncodingState != EncodingState::Finished) {
+            if (!mGotError) {
+                mGotError = true;
+                mErrorMessage = message;
+            }
+        } else {
+            GetDevice()->HandleError(message);
         }
     }
 

src/tests/unittests/validation/CommandBufferValidationTests.cpp

diff --git a/src/tests/unittests/validation/CommandBufferValidationTests.cpp b/src/tests/unittests/validation/CommandBufferValidationTests.cpp
index 882fd03..c679cf7 100644
--- a/src/tests/unittests/validation/CommandBufferValidationTests.cpp
+++ b/src/tests/unittests/validation/CommandBufferValidationTests.cpp
@@ -155,8 +155,28 @@
     }
 }
 
-// Test that beginning a compute pass before ending the previous pass causes an error.
+// Test that encoding command after a successful finish produces an error
+TEST_F(CommandBufferValidationTest, CallsAfterASuccessfulFinish) {
+    // A buffer that can be used in CopyBufferToBuffer
+    dawn::BufferDescriptor copyBufferDesc;
+    copyBufferDesc.size = 16;
+    copyBufferDesc.usage = dawn::BufferUsageBit::TransferSrc | dawn::BufferUsageBit::TransferDst;
+    dawn::Buffer copyBuffer = device.CreateBuffer(&copyBufferDesc);
+
+    dawn::CommandEncoder encoder = device.CreateCommandEncoder();
+    encoder.Finish();
+
+    ASSERT_DEVICE_ERROR(encoder.CopyBufferToBuffer(copyBuffer, 0, copyBuffer, 0, 0));
+}
+
+// Test that encoding command after a failed finish produces an error
 TEST_F(CommandBufferValidationTest, CallsAfterAFailedFinish) {
+    // A buffer that can be used in CopyBufferToBuffer
+    dawn::BufferDescriptor copyBufferDesc;
+    copyBufferDesc.size = 16;
+    copyBufferDesc.usage = dawn::BufferUsageBit::TransferSrc | dawn::BufferUsageBit::TransferDst;
+    dawn::Buffer copyBuffer = device.CreateBuffer(&copyBufferDesc);
+
     // A buffer that can't be used in CopyBufferToBuffer
     dawn::BufferDescriptor bufferDesc;
     bufferDesc.size = 16;
@@ -167,8 +187,7 @@
     encoder.CopyBufferToBuffer(buffer, 0, buffer, 0, 0);
     ASSERT_DEVICE_ERROR(encoder.Finish());
 
-    // TODO(cwallez@chromium.org): Currently this does nothing, should it be a device error?
-    encoder.CopyBufferToBuffer(buffer, 0, buffer, 0, 0);
+    ASSERT_DEVICE_ERROR(encoder.CopyBufferToBuffer(copyBuffer, 0, copyBuffer, 0, 0));
 }
 
 // Test that using a single buffer in multiple read usages in the same pass is allowed.

src/tests/unittests/validation/DynamicStateCommandValidationTests.cpp

diff --git a/src/tests/unittests/validation/DynamicStateCommandValidationTests.cpp b/src/tests/unittests/validation/DynamicStateCommandValidationTests.cpp
index 1485434..d76a84c 100644
--- a/src/tests/unittests/validation/DynamicStateCommandValidationTests.cpp
+++ b/src/tests/unittests/validation/DynamicStateCommandValidationTests.cpp
@@ -34,31 +34,32 @@
 TEST_F(SetScissorRectTest, EmptyScissor) {
     DummyRenderPass renderPass(device);
 
-    dawn::CommandEncoder encoder = device.CreateCommandEncoder();
-
     // Width of scissor rect is zero.
     {
+        dawn::CommandEncoder encoder = device.CreateCommandEncoder();
         dawn::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
         pass.SetScissorRect(0, 0, 0, 1);
         pass.EndPass();
+        ASSERT_DEVICE_ERROR(encoder.Finish());
     }
-    ASSERT_DEVICE_ERROR(encoder.Finish());
 
     // Height of scissor rect is zero.
     {
+        dawn::CommandEncoder encoder = device.CreateCommandEncoder();
         dawn::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
         pass.SetScissorRect(0, 0, 1, 0);
         pass.EndPass();
+        ASSERT_DEVICE_ERROR(encoder.Finish());
     }
-    ASSERT_DEVICE_ERROR(encoder.Finish());
 
     // Both width and height of scissor rect are zero.
     {
+        dawn::CommandEncoder encoder = device.CreateCommandEncoder();
         dawn::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
         pass.SetScissorRect(0, 0, 0, 0);
         pass.EndPass();
+        ASSERT_DEVICE_ERROR(encoder.Finish());
     }
-    ASSERT_DEVICE_ERROR(encoder.Finish());
 }
 
 // Test to check that a scissor larger than the framebuffer is allowed