Make properties required

This commit makes depthWriteEnabled and depthCompare required and
makes depthClearValue conditionally required for the spec change
in WebGPU V1.

https://github.com/gpuweb/gpuweb/pull/3849

depthClearValue is required if depthLoadOp is clear and the
attachment has a depth aspect. To simulate it, this commit lets
NAN represent unspecified depthClearValue and lets the default
value of depthClearValue be NAN.

Bug: dawn:1669
Change-Id: I469338e909b1d3c345bc2642ee47daee858909ca
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/120620
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>

dawn.json

@@ -1989,7 +1989,7 @@
             {"name": "view", "type": "texture view"},
             {"name": "depth load op", "type": "load op", "default": "undefined"},
             {"name": "depth store op", "type": "store op", "default": "undefined"},
-            {"name": "depth clear value", "type": "float", "default": "0"},
+            {"name": "depth clear value", "type": "float", "default": "NAN"},
             {"name": "depth read only", "type": "bool", "default": "false"},
             {"name": "stencil load op", "type": "load op", "default": "undefined"},
             {"name": "stencil store op", "type": "store op", "default": "undefined"},
@@ -2282,8 +2282,8 @@
         "extensible": "in",
         "members": [
             {"name": "format", "type": "texture format"},
-            {"name": "depth write enabled", "type": "bool", "default": "false"},
+            {"name": "depth write enabled", "type": "bool"},
-            {"name": "depth compare", "type": "compare function", "default": "always"},
+            {"name": "depth compare", "type": "compare function"},
             {"name": "stencil front", "type": "stencil face state"},
             {"name": "stencil back", "type": "stencil face state"},
             {"name": "stencil read mask", "type": "uint32_t", "default": "0xFFFFFFFF"},

src/dawn/native/BlitBufferToDepthStencil.cpp

@@ -137,6 +137,7 @@ ResultOrError<Ref<RenderPipelineBase>> GetOrCreateRG8ToDepth16UnormPipeline(Devi
     DepthStencilState dsState = {};
     dsState.format = wgpu::TextureFormat::Depth16Unorm;
     dsState.depthWriteEnabled = true;
+    dsState.depthCompare = wgpu::CompareFunction::Always;
 
     RenderPipelineDescriptor renderPipelineDesc = {};
     renderPipelineDesc.vertex.module = shaderModule.Get();
@@ -186,6 +187,7 @@ ResultOrError<InternalPipelineStore::BlitR8ToStencilPipelines> GetOrCreateR8ToSt
     DepthStencilState dsState = {};
     dsState.format = format;
     dsState.depthWriteEnabled = false;
+    dsState.depthCompare = wgpu::CompareFunction::Always;
     dsState.stencilFront.passOp = wgpu::StencilOperation::Replace;
 
     RenderPipelineDescriptor renderPipelineDesc = {};
@@ -288,6 +290,7 @@ MaybeError BlitRG8ToDepth16Unorm(DeviceBase* device,
 
         RenderPassDepthStencilAttachment dsAttachment;
         dsAttachment.view = dstView.Get();
+        dsAttachment.depthClearValue = 0.0;
         dsAttachment.depthLoadOp = wgpu::LoadOp::Load;
         dsAttachment.depthStoreOp = wgpu::StoreOp::Store;
 
@@ -400,6 +403,7 @@ MaybeError BlitR8ToStencil(DeviceBase* device,
         }
 
         RenderPassDepthStencilAttachment dsAttachment;
+        dsAttachment.depthClearValue = 0.0;
         dsAttachment.view = dstView.Get();
         if (format.HasDepth()) {
             dsAttachment.depthLoadOp = wgpu::LoadOp::Load;

src/dawn/native/BlitDepthToDepth.cpp

@@ -75,6 +75,7 @@ ResultOrError<Ref<RenderPipelineBase>> GetOrCreateDepthBlitPipeline(DeviceBase*
     DepthStencilState dsState = {};
     dsState.format = format;
     dsState.depthWriteEnabled = true;
+    dsState.depthCompare = wgpu::CompareFunction::Always;
 
     RenderPipelineDescriptor renderPipelineDesc = {};
     renderPipelineDesc.vertex.module = shaderModule.Get();
@@ -203,6 +204,7 @@ MaybeError BlitDepthToDepth(DeviceBase* device,
 
         RenderPassDepthStencilAttachment dsAttachment = {};
         dsAttachment.view = dstView.Get();
+        dsAttachment.depthClearValue = 0.0;
         dsAttachment.depthLoadOp = wgpu::LoadOp::Load;
         dsAttachment.depthStoreOp = wgpu::StoreOp::Store;
         if (dst.texture->GetFormat().HasStencil()) {

src/dawn/native/CommandEncoder.cpp

@@ -371,12 +371,18 @@ MaybeError ValidateRenderPassDepthStencilAttachment(
                         depthStencilAttachment->stencilReadOnly);
     }
 
-    if (depthStencilAttachment->depthLoadOp == wgpu::LoadOp::Clear) {
+    if (depthStencilAttachment->depthLoadOp == wgpu::LoadOp::Clear &&
-        DAWN_INVALID_IF(std::isnan(depthStencilAttachment->depthClearValue),
+        IsSubset(Aspect::Depth, attachment->GetAspects())) {
-                        "depthClearValue is NaN.");
+        DAWN_INVALID_IF(
+            std::isnan(depthStencilAttachment->depthClearValue),
+            "depthClearValue (%f) must be set and must not be a NaN value if the attachment "
+            "(%s) has a depth aspect and depthLoadOp is clear.",
+            depthStencilAttachment->depthClearValue, attachment);
         DAWN_INVALID_IF(depthStencilAttachment->depthClearValue < 0.0f ||
                             depthStencilAttachment->depthClearValue > 1.0f,
-                        "depthClearValue is not between 0.0 and 1.0");
+                        "depthClearValue (%f) must be between 0.0 and 1.0 if the attachment (%s) "
+                        "has a depth aspect and depthLoadOp is clear.",
+                        depthStencilAttachment->depthClearValue, attachment);
     }
 
     // *sampleCount == 0 must only happen when there is no color attachment. In that case we

src/dawn/tests/end2end/DepthStencilStateTests.cpp

@@ -781,6 +781,8 @@ TEST_P(DepthStencilStateTest, CreatePipelineWithAllFormats) {
 // Test that the front and back stencil states are set correctly (and take frontFace into account)
 TEST_P(DepthStencilStateTest, StencilFrontAndBackFace) {
     wgpu::DepthStencilState state;
+    state.depthWriteEnabled = false;
+    state.depthCompare = wgpu::CompareFunction::Always;
     state.stencilFront.compare = wgpu::CompareFunction::Always;
     state.stencilBack.compare = wgpu::CompareFunction::Never;
 
@@ -794,12 +796,16 @@ TEST_P(DepthStencilStateTest, StencilFrontAndBackFace) {
 // Test that the depth reference of a new render pass is initialized to default value 0
 TEST_P(DepthStencilStateTest, StencilReferenceInitialized) {
     wgpu::DepthStencilState stencilAlwaysReplaceState;
+    stencilAlwaysReplaceState.depthWriteEnabled = false;
+    stencilAlwaysReplaceState.depthCompare = wgpu::CompareFunction::Always;
     stencilAlwaysReplaceState.stencilFront.compare = wgpu::CompareFunction::Always;
     stencilAlwaysReplaceState.stencilFront.passOp = wgpu::StencilOperation::Replace;
     stencilAlwaysReplaceState.stencilBack.compare = wgpu::CompareFunction::Always;
     stencilAlwaysReplaceState.stencilBack.passOp = wgpu::StencilOperation::Replace;
 
     wgpu::DepthStencilState stencilEqualKeepState;
+    stencilEqualKeepState.depthWriteEnabled = false;
+    stencilEqualKeepState.depthCompare = wgpu::CompareFunction::Always;
     stencilEqualKeepState.stencilFront.compare = wgpu::CompareFunction::Equal;
     stencilEqualKeepState.stencilFront.passOp = wgpu::StencilOperation::Keep;
     stencilEqualKeepState.stencilBack.compare = wgpu::CompareFunction::Equal;

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

@@ -1052,6 +1052,7 @@ TEST_F(RenderPassDescriptorValidationTest, UseNaNOrINFINITYAsColorOrDepthClearVa
         wgpu::TextureView depth =
             Create2DAttachment(device, 1, 1, wgpu::TextureFormat::Depth24Plus);
         utils::ComboRenderPassDescriptor renderPass({color}, depth);
+        renderPass.cDepthStencilAttachmentInfo.depthLoadOp = wgpu::LoadOp::Clear;
         renderPass.cDepthStencilAttachmentInfo.stencilLoadOp = wgpu::LoadOp::Undefined;
         renderPass.cDepthStencilAttachmentInfo.stencilStoreOp = wgpu::StoreOp::Undefined;
         renderPass.cDepthStencilAttachmentInfo.depthClearValue = NAN;
@@ -1063,6 +1064,7 @@ TEST_F(RenderPassDescriptorValidationTest, UseNaNOrINFINITYAsColorOrDepthClearVa
         wgpu::TextureView depth =
             Create2DAttachment(device, 1, 1, wgpu::TextureFormat::Depth24Plus);
         utils::ComboRenderPassDescriptor renderPass({color}, depth);
+        renderPass.cDepthStencilAttachmentInfo.depthLoadOp = wgpu::LoadOp::Clear;
         renderPass.cDepthStencilAttachmentInfo.stencilLoadOp = wgpu::LoadOp::Undefined;
         renderPass.cDepthStencilAttachmentInfo.stencilStoreOp = wgpu::StoreOp::Undefined;
         renderPass.cDepthStencilAttachmentInfo.depthClearValue = INFINITY;
@@ -1126,6 +1128,45 @@ TEST_F(RenderPassDescriptorValidationTest, ValidateDepthClearValueRange) {
     AssertBeginRenderPassSuccess(&renderPass);
 }
 
+// Tests that default depthClearValue is required if attachment has a depth aspect and depthLoadOp
+// is clear.
+TEST_F(RenderPassDescriptorValidationTest, DefaultDepthClearValue) {
+    wgpu::TextureView depthView =
+        Create2DAttachment(device, 1, 1, wgpu::TextureFormat::Depth24Plus);
+    wgpu::TextureView stencilView = Create2DAttachment(device, 1, 1, wgpu::TextureFormat::Stencil8);
+
+    wgpu::RenderPassDepthStencilAttachment depthStencilAttachment;
+
+    wgpu::RenderPassDescriptor renderPassDescriptor;
+    renderPassDescriptor.colorAttachmentCount = 0;
+    renderPassDescriptor.colorAttachments = nullptr;
+    renderPassDescriptor.depthStencilAttachment = &depthStencilAttachment;
+
+    // Default depthClearValue should be accepted if attachment doesn't have
+    // a depth aspect.
+    depthStencilAttachment.view = stencilView;
+    depthStencilAttachment.stencilLoadOp = wgpu::LoadOp::Load;
+    depthStencilAttachment.stencilStoreOp = wgpu::StoreOp::Store;
+    AssertBeginRenderPassSuccess(&renderPassDescriptor);
+
+    // Default depthClearValue should be accepted if depthLoadOp is not clear.
+    depthStencilAttachment.view = depthView;
+    depthStencilAttachment.stencilLoadOp = wgpu::LoadOp::Undefined;
+    depthStencilAttachment.stencilStoreOp = wgpu::StoreOp::Undefined;
+    depthStencilAttachment.depthLoadOp = wgpu::LoadOp::Load;
+    depthStencilAttachment.depthStoreOp = wgpu::StoreOp::Store;
+    AssertBeginRenderPassSuccess(&renderPassDescriptor);
+
+    // Default depthClearValue should fail the validation
+    // if attachment has a depth aspect and depthLoadOp is clear.
+    depthStencilAttachment.depthLoadOp = wgpu::LoadOp::Clear;
+    AssertBeginRenderPassError(&renderPassDescriptor);
+
+    // The validation should pass if valid depthClearValue is provided.
+    depthStencilAttachment.depthClearValue = 0.0f;
+    AssertBeginRenderPassSuccess(&renderPassDescriptor);
+}
+
 TEST_F(RenderPassDescriptorValidationTest, ValidateDepthStencilReadOnly) {
     wgpu::TextureView colorView = Create2DAttachment(device, 1, 1, wgpu::TextureFormat::RGBA8Unorm);
     wgpu::TextureView depthStencilView =