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>
diff --git a/src/dawn/native/CommandEncoder.cpp b/src/dawn/native/CommandEncoder.cpp
index 13c8431..95157ca 100644
--- a/src/dawn/native/CommandEncoder.cpp
+++ b/src/dawn/native/CommandEncoder.cpp
@@ -781,11 +781,14 @@
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 @@
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();
diff --git a/src/dawn/tests/end2end/DeprecatedAPITests.cpp b/src/dawn/tests/end2end/DeprecatedAPITests.cpp
index a015a58..20d3fdd 100644
--- a/src/dawn/tests/end2end/DeprecatedAPITests.cpp
+++ b/src/dawn/tests/end2end/DeprecatedAPITests.cpp
@@ -47,8 +47,7 @@
// 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, DISABLED_ReadOnlyDepthStencilStoreLoadOpsAttachment) {
+TEST_P(DeprecationTests, ReadOnlyDepthStencilStoreLoadOpsAttachment) {
utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, 1, 1);
wgpu::RenderPassEncoder pass;
@@ -71,6 +70,8 @@
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 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, DISABLED_AttachmentClearColor) {
+TEST_P(DeprecationTests, AttachmentClearColor) {
utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, 1, 1);
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
wgpu::RenderPassEncoder pass;
diff --git a/src/dawn/tests/end2end/SwapChainValidationTests.cpp b/src/dawn/tests/end2end/SwapChainValidationTests.cpp
index b02d760..20948ed 100644
--- a/src/dawn/tests/end2end/SwapChainValidationTests.cpp
+++ b/src/dawn/tests/end2end/SwapChainValidationTests.cpp
@@ -78,16 +78,16 @@
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();
- wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPassDesc);
- pass.End();
- if (errorAtFinish) {
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ if (errorAtBeginRenderPass) {
+ ASSERT_DEVICE_ERROR(encoder.BeginRenderPass(&renderPassDesc));
} else {
+ wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPassDesc);
+ pass.End();
wgpu::CommandBuffer commands = encoder.Finish();
if (errorAtSubmit) {
diff --git a/src/dawn/tests/unittests/validation/QueryValidationTests.cpp b/src/dawn/tests/unittests/validation/QueryValidationTests.cpp
index d0289e2..0712461 100644
--- a/src/dawn/tests/unittests/validation/QueryValidationTests.cpp
+++ b/src/dawn/tests/unittests/validation/QueryValidationTests.cpp
@@ -156,9 +156,7 @@
CreateQuerySet(otherDevice, wgpu::QueryType::Occlusion, 2);
renderPass.occlusionQuerySet = occlusionQuerySetOnOther;
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
- wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
- pass.End();
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ ASSERT_DEVICE_ERROR(encoder.BeginRenderPass(&renderPass));
// 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 @@
renderPass.occlusionQuerySet = querySet;
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
- encoder.BeginRenderPass(&renderPass);
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ ASSERT_DEVICE_ERROR(encoder.BeginRenderPass(&renderPass));
}
// Test timestampWrites in compute pass descriptor
@@ -357,9 +354,8 @@
wgpu::QuerySet occlusionQuerySet = CreateQuerySet(device, wgpu::QueryType::Occlusion, 1);
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
- EncodeComputePassWithTimestampWrites(
- encoder, {{occlusionQuerySet, 0, wgpu::ComputePassTimestampLocation::Beginning}});
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ ASSERT_DEVICE_ERROR(EncodeComputePassWithTimestampWrites(
+ encoder, {{occlusionQuerySet, 0, wgpu::ComputePassTimestampLocation::Beginning}}));
}
// Fail to write timestamps to a query set created from another device
@@ -369,18 +365,16 @@
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}});
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ {querySetFromOtherDevice, 1, wgpu::ComputePassTimestampLocation::End}}));
}
// Fail to write timestamps to the query index which exceeds the number of queries in query set
{
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
- EncodeComputePassWithTimestampWrites(
- encoder, {{querySet, 2, wgpu::ComputePassTimestampLocation::Beginning}});
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ ASSERT_DEVICE_ERROR(EncodeComputePassWithTimestampWrites(
+ encoder, {{querySet, 2, wgpu::ComputePassTimestampLocation::Beginning}}));
}
// Success to write timestamps to the same query index twice on same compute pass
@@ -405,18 +399,16 @@
// 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}});
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ {querySet, 1, wgpu::ComputePassTimestampLocation::Beginning}}));
}
// Fail to write timestamps at invalid location of compute pass
{
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
- EncodeComputePassWithTimestampWrites(
- encoder, {{querySet, 0, static_cast<wgpu::ComputePassTimestampLocation>(0xFFFFFFFF)}});
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ ASSERT_DEVICE_ERROR(EncodeComputePassWithTimestampWrites(
+ encoder, {{querySet, 0, static_cast<wgpu::ComputePassTimestampLocation>(0xFFFFFFFF)}}));
}
// Fail to write timestamps to a destroyed query set
@@ -451,9 +443,8 @@
wgpu::QuerySet occlusionQuerySet = CreateQuerySet(device, wgpu::QueryType::Occlusion, 1);
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
- EncodeRenderPassWithTimestampWrites(
- encoder, {{occlusionQuerySet, 0, wgpu::RenderPassTimestampLocation::Beginning}});
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites(
+ encoder, {{occlusionQuerySet, 0, wgpu::RenderPassTimestampLocation::Beginning}}));
}
// Fail to write timestamps to a query set created from another device
@@ -463,18 +454,16 @@
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}});
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ {querySetFromOtherDevice, 1, wgpu::RenderPassTimestampLocation::End}}));
}
// Fail to write timestamps to the query index which exceeds the number of queries in query set
{
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
- EncodeRenderPassWithTimestampWrites(
- encoder, {{querySet, 2, wgpu::RenderPassTimestampLocation::Beginning}});
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites(
+ encoder, {{querySet, 2, wgpu::RenderPassTimestampLocation::Beginning}}));
}
// Success to write timestamps to the same query index and location twice on different render
@@ -494,27 +483,24 @@
// 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}});
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ {querySet, 0, wgpu::RenderPassTimestampLocation::End}}));
}
// 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}});
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ {querySet, 1, wgpu::RenderPassTimestampLocation::Beginning}}));
}
// Fail to write timestamps at invalid location of render pass
{
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
- EncodeRenderPassWithTimestampWrites(
- encoder, {{querySet, 0, static_cast<wgpu::RenderPassTimestampLocation>(0xFFFFFFFF)}});
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites(
+ encoder, {{querySet, 0, static_cast<wgpu::RenderPassTimestampLocation>(0xFFFFFFFF)}}));
}
// Fail to write timestamps to a destroyed query set
@@ -785,8 +771,7 @@
renderPass.occlusionQuerySet = querySet;
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
- encoder.BeginRenderPass(&renderPass);
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ ASSERT_DEVICE_ERROR(encoder.BeginRenderPass(&renderPass));
}
class ResolveQuerySetValidationTest : public QuerySetValidationTest {
diff --git a/src/dawn/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp b/src/dawn/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp
index f0cfd3c..87b292f 100644
--- a/src/dawn/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp
+++ b/src/dawn/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp
@@ -27,20 +27,14 @@
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));
}
};
diff --git a/src/dawn/tests/unittests/validation/RenderPipelineValidationTests.cpp b/src/dawn/tests/unittests/validation/RenderPipelineValidationTests.cpp
index 675b1d9..927f05b 100644
--- a/src/dawn/tests/unittests/validation/RenderPipelineValidationTests.cpp
+++ b/src/dawn/tests/unittests/validation/RenderPipelineValidationTests.cpp
@@ -710,11 +710,7 @@
utils::ComboRenderPassDescriptor renderPassDescriptor({});
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
- wgpu::RenderPassEncoder renderPass = encoder.BeginRenderPass(&renderPassDescriptor);
- renderPass.SetPipeline(vertexOnlyPipeline);
- renderPass.End();
-
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ ASSERT_DEVICE_ERROR(encoder.BeginRenderPass(&renderPassDescriptor));
}
// Vertex-only render pipeline can not work with color target
diff --git a/webgpu-cts/expectations.txt b/webgpu-cts/expectations.txt
index 956ce88..66dc697 100644
--- a/webgpu-cts/expectations.txt
+++ b/webgpu-cts/expectations.txt
@@ -446,6 +446,19 @@
################################################################################
################################################################################
+# 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
################################################################################
