Make SetScissorRect match upstream WebGPU specification.

This allows empty scissors, so add a test for it.
This disallows scissor boxes that are bigger than the renderpass
attachment so remove an end2end test for that behavior.

Update the SetScissorRect validation tests.

Bug: dawn:542
Change-Id: I5b8578a4df1b94510a9356bd4007efddf2711588
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/29820
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Stephen White <senorblanco@chromium.org>

src/dawn_native/RenderPassEncoder.cpp

@@ -131,8 +131,10 @@ namespace dawn_native {
                                            uint32_t width,
                                            uint32_t height) {
         mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError {
-            if (width == 0 || height == 0) {
+            if (width > mRenderTargetWidth || height > mRenderTargetHeight ||
-                return DAWN_VALIDATION_ERROR("Width and height must be greater than 0.");
+                x > mRenderTargetWidth - width || y > mRenderTargetHeight - height) {
+                return DAWN_VALIDATION_ERROR(
+                    "The scissor rect must be contained in the render targets");
             }
 
             SetScissorRectCmd* cmd =

src/dawn_native/d3d12/AdapterD3D12.cpp

@@ -139,6 +139,9 @@ namespace dawn_native { namespace d3d12 {
             // https://crbug.com/dawn/422
             D3D12_MESSAGE_ID_EXECUTECOMMANDLISTS_GPU_WRITTEN_READBACK_RESOURCE_MAPPED,
 
+            // WebGPU allows empty scissors without empty viewports.
+            D3D12_MESSAGE_ID_DRAW_EMPTY_SCISSOR_RECTANGLE,
+
             //
             // Temporary IDs: list of warnings that should be fixed or promoted
             //

src/dawn_native/metal/CommandBufferMTL.mm

@@ -1265,15 +1265,6 @@ namespace dawn_native { namespace metal {
                     rect.width = cmd->width;
                     rect.height = cmd->height;
 
-                    // The scissor rect x + width must be <= render pass width
-                    if ((rect.x + rect.width) > width) {
-                        rect.width = width - rect.x;
-                    }
-                    // The scissor rect y + height must be <= render pass height
-                    if ((rect.y + rect.height > height)) {
-                        rect.height = height - rect.y;
-                    }
-
                     [encoder setScissorRect:rect];
                     break;
                 }

src/tests/end2end/ScissorTests.cpp

@@ -70,29 +70,6 @@ TEST_P(ScissorTest, DefaultsToWholeRenderTarget) {
     EXPECT_PIXEL_RGBA8_EQ(RGBA8::kGreen, renderPass.color, 99, 99);
 }
 
-// Test setting the scissor to something larger than the attachments.
-TEST_P(ScissorTest, LargerThanAttachment) {
-    utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, 100, 100);
-    wgpu::RenderPipeline pipeline = CreateQuadPipeline(renderPass.colorFormat);
-
-    wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-    {
-        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass.renderPassInfo);
-        pass.SetPipeline(pipeline);
-        pass.SetScissorRect(0, 0, 200, 200);
-        pass.Draw(6);
-        pass.EndPass();
-    }
-
-    wgpu::CommandBuffer commands = encoder.Finish();
-    queue.Submit(1, &commands);
-
-    EXPECT_PIXEL_RGBA8_EQ(RGBA8::kGreen, renderPass.color, 0, 0);
-    EXPECT_PIXEL_RGBA8_EQ(RGBA8::kGreen, renderPass.color, 0, 99);
-    EXPECT_PIXEL_RGBA8_EQ(RGBA8::kGreen, renderPass.color, 99, 0);
-    EXPECT_PIXEL_RGBA8_EQ(RGBA8::kGreen, renderPass.color, 99, 99);
-}
-
 // Test setting a partial scissor (not empty, not full attachment)
 TEST_P(ScissorTest, PartialRect) {
     utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, 100, 100);
@@ -123,6 +100,29 @@ TEST_P(ScissorTest, PartialRect) {
     EXPECT_PIXEL_RGBA8_EQ(RGBA8::kGreen, renderPass.color, kX + kW - 1, kY + kH - 1);
 }
 
+// Test setting an empty scissor
+TEST_P(ScissorTest, EmptyRect) {
+    utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, 2, 2);
+    wgpu::RenderPipeline pipeline = CreateQuadPipeline(renderPass.colorFormat);
+
+    wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+    {
+        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass.renderPassInfo);
+        pass.SetPipeline(pipeline);
+        pass.SetScissorRect(1, 1, 0, 0);
+        pass.Draw(6);
+        pass.EndPass();
+    }
+
+    wgpu::CommandBuffer commands = encoder.Finish();
+    queue.Submit(1, &commands);
+
+    // Test that no pixel was written.
+    EXPECT_PIXEL_RGBA8_EQ(RGBA8::kZero, renderPass.color, 0, 0);
+    EXPECT_PIXEL_RGBA8_EQ(RGBA8::kZero, renderPass.color, 0, 1);
+    EXPECT_PIXEL_RGBA8_EQ(RGBA8::kZero, renderPass.color, 1, 0);
+    EXPECT_PIXEL_RGBA8_EQ(RGBA8::kZero, renderPass.color, 1, 1);
+}
 // Test that the scissor setting doesn't get inherited between renderpasses
 TEST_P(ScissorTest, NoInheritanceBetweenRenderPass) {
     utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, 100, 100);

src/tests/unittests/validation/DynamicStateCommandValidationTests.cpp

@@ -133,64 +133,74 @@ TEST_F(SetViewportTest, MinDepthEqualOrGreaterThanMaxDepth) {
     TestViewportCall(false, 0.0, 0.0, 1.0, 1.0, 0.8, 0.5);
 }
 
-class SetScissorRectTest : public ValidationTest {};
+class SetScissorTest : public ValidationTest {
+  protected:
+    void TestScissorCall(bool success,
+                          uint32_t x,
+                          uint32_t y,
+                          uint32_t width,
+                          uint32_t height) {
+        utils::BasicRenderPass rp = utils::CreateBasicRenderPass(device, kWidth, kHeight);
+
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&rp.renderPassInfo);
+        pass.SetScissorRect(x, y, width, height);
+        pass.EndPass();
+
+        if (success) {
+            encoder.Finish();
+        } else {
+            ASSERT_DEVICE_ERROR(encoder.Finish());
+        }
+    }
+
+    static constexpr uint32_t kWidth = 5;
+    static constexpr uint32_t kHeight = 3;
+};
 
 // Test to check basic use of SetScissor
-TEST_F(SetScissorRectTest, Success) {
+TEST_F(SetScissorTest, Success) {
-    DummyRenderPass renderPass(device);
+    TestScissorCall(true, 0, 0, kWidth, kHeight);
-
+    TestScissorCall(true, 0, 0, 1, 1);
-    wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-    {
-        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
-        pass.SetScissorRect(0, 0, 1, 1);
-        pass.EndPass();
-    }
-    encoder.Finish();
 }
 
-// Test to check that an empty scissor is not allowed
+// Test to check that an empty scissor is allowed
-TEST_F(SetScissorRectTest, EmptyScissor) {
+TEST_F(SetScissorTest, EmptyScissor) {
-    DummyRenderPass renderPass(device);
+    // Scissor width is 0
+    TestScissorCall(true, 0, 0, 0, kHeight);
 
-    // Width of scissor rect is zero.
+    // Scissor height is 0
-    {
+    TestScissorCall(true, 0, 0, kWidth, 0);
-        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
-        pass.SetScissorRect(0, 0, 0, 1);
-        pass.EndPass();
-        ASSERT_DEVICE_ERROR(encoder.Finish());
-    }
 
-    // Height of scissor rect is zero.
+    // Both scissor width and height are 0
-    {
+    TestScissorCall(true, 0, 0, 0, 0);
-        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
-        pass.SetScissorRect(0, 0, 1, 0);
-        pass.EndPass();
-        ASSERT_DEVICE_ERROR(encoder.Finish());
-    }
-
-    // Both width and height of scissor rect are zero.
-    {
-        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
-        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
-        pass.SetScissorRect(0, 0, 0, 0);
-        pass.EndPass();
-        ASSERT_DEVICE_ERROR(encoder.Finish());
-    }
 }
 
-// Test to check that a scissor larger than the framebuffer is allowed
+// Test to check that various scissors contained in the framebuffer is allowed
-TEST_F(SetScissorRectTest, ScissorLargerThanFramebuffer) {
+TEST_F(SetScissorTest, ScissorContainedInFramebuffer) {
-    DummyRenderPass renderPass(device);
+    // Width and height are set to the render target size.
+    TestScissorCall(true, 0, 0, kWidth, kHeight);
 
-    wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+    // Width/height at the limit with 0 x/y is valid.
-    {
+    TestScissorCall(true, kWidth, 0,  0, kHeight);
-        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+    TestScissorCall(true, 0, kHeight,  kWidth, 0);
-        pass.SetScissorRect(0, 0, renderPass.width + 1, renderPass.height + 1);
+}
-        pass.EndPass();
+
-    }
+// Test to check that a scissor larger than the framebuffer is disallowed
-    encoder.Finish();
+TEST_F(SetScissorTest, ScissorLargerThanFramebuffer) {
+    // Width/height is larger than the rendertarget's width/height.
+    TestScissorCall(false, 0, 0, kWidth + 1, kHeight);
+    TestScissorCall(false, 0, 0, kWidth, kHeight + 1);
+
+    // x + width is larger than the rendertarget's width.
+    TestScissorCall(false, 2, 0, kWidth - 1, kHeight);
+    TestScissorCall(false, kWidth, 0, 1, kHeight);
+    TestScissorCall(false, std::numeric_limits<uint32_t>::max(), 0, kWidth, kHeight);
+
+    // x + height is larger than the rendertarget's height.
+    TestScissorCall(false, 0, 2, kWidth , kHeight - 1);
+    TestScissorCall(false, 0, kHeight, kWidth, 1);
+    TestScissorCall(false, 0, std::numeric_limits<uint32_t>::max(), kWidth, kHeight);
 }
 
 class SetBlendColorTest : public ValidationTest {};