Vulkan: Defer clears even if following command is scissored

src/libANGLE/renderer/vulkan/ContextVk.cpp

@@ -4231,12 +4231,7 @@ angle::Result ContextVk::updateActiveTextures(const gl::Context *context)
             ASSERT(!mState.isDepthWriteEnabled());
 
             // Special handling for deferred clears.
-            if (mDrawFramebuffer->hasDeferredClears())
-            {
-                gl::Rectangle scissoredRenderArea =
-                    mDrawFramebuffer->getRotatedScissoredRenderArea(this);
-                ANGLE_TRY(mDrawFramebuffer->flushDeferredClears(this, scissoredRenderArea));
-            }
+            ANGLE_TRY(mDrawFramebuffer->flushDeferredClears(this));
 
             if (hasStartedRenderPass())
             {

src/libANGLE/renderer/vulkan/FramebufferVk.cpp

@@ -380,7 +380,7 @@ angle::Result FramebufferVk::invalidateSub(const gl::Context *context,
     // but if the framebuffer's attachments are used after this call not through the framebuffer,
     // those clears wouldn't get flushed otherwise (for example as the destination of
     // glCopyTex[Sub]Image, shader storage image, etc).
-    ANGLE_TRY(flushDeferredClears(contextVk, completeRenderArea));
+    ANGLE_TRY(flushDeferredClears(contextVk));
 
     if (contextVk->hasStartedRenderPass() &&
         rotatedInvalidateArea.encloses(contextVk->getStartedRenderPassCommands().getRenderArea()))
@@ -492,11 +492,8 @@ angle::Result FramebufferVk::clearImpl(const gl::Context *context,
     if (preferDrawOverClearAttachments && isMidRenderPassClear)
     {
         // On buggy hardware, prefer to clear with a draw call instead of vkCmdClearAttachments.
-        // In this case, flush deferred clears and don't merge current clears (so they take the draw
-        // path).  Note that this path could be optimized by a clearWithDraw-like call that operates
-        // on mDeferredClears.  However, having deferred clears mid-render-pass is rare (if at all
-        // possible).
-        ANGLE_TRY(flushDeferredClears(contextVk, scissoredRenderArea));
+        // Note that it's impossible to have deferred clears in the middle of the render pass.
+        ASSERT(!mDeferredClears.any());
 
         clearColorWithDraw   = clearColor;
         clearDepthWithDraw   = clearDepth;
@@ -547,7 +544,7 @@ angle::Result FramebufferVk::clearImpl(const gl::Context *context,
             // The subsequent draw call will then operate on the cleared attachments.
             if (clearAnyWithDraw)
             {
-                ANGLE_TRY(flushDeferredClears(contextVk, scissoredRenderArea));
+                ANGLE_TRY(flushDeferredClears(contextVk));
             }
             else
             {
@@ -696,14 +693,7 @@ angle::Result FramebufferVk::readPixels(const gl::Context *context,
     }
 
     // Flush any deferred clears.
-    gl::Rectangle rotatedFbRect = fbRect;
-    if (contextVk->isRotatedAspectRatioForReadFBO())
-    {
-        // The surface is rotated 90/270 degrees.  This changes the aspect ratio of the surface.
-        // Since fbRect.{x|y} are both 0, there's no need to swap them.
-        std::swap(rotatedFbRect.width, rotatedFbRect.height);
-    }
-    ANGLE_TRY(flushDeferredClears(contextVk, rotatedFbRect));
+    ANGLE_TRY(flushDeferredClears(contextVk));
 
     GLuint outputSkipBytes = 0;
     PackPixelsParams params;
@@ -872,7 +862,7 @@ angle::Result FramebufferVk::blit(const gl::Context *context,
 
     // We can sometimes end up in a blit with some clear commands saved. Ensure all clear commands
     // are issued before we issue the blit command.
-    ANGLE_TRY(flushDeferredClears(contextVk, getRotatedCompleteRenderArea(contextVk)));
+    ANGLE_TRY(flushDeferredClears(contextVk));
 
     const gl::State &glState              = contextVk->getState();
     const gl::Framebuffer *srcFramebuffer = glState.getReadFramebuffer();
@@ -1462,7 +1452,7 @@ angle::Result FramebufferVk::invalidateImpl(ContextVk *contextVk,
     }
 
     // If there are still deferred clears, flush them.  See relevant comment in invalidateSub.
-    ANGLE_TRY(flushDeferredClears(contextVk, getRotatedCompleteRenderArea(contextVk)));
+    ANGLE_TRY(flushDeferredClears(contextVk));
 
     const auto &colorRenderTargets           = mRenderTargetCache.getColors();
     RenderTargetVk *depthStencilRenderTarget = mRenderTargetCache.getDepthStencil();
@@ -1671,14 +1661,11 @@ angle::Result FramebufferVk::syncState(const gl::Context *context,
 
     vk::FramebufferDesc priorFramebufferDesc = mCurrentFramebufferDesc;
 
-    // Only defer clears for whole draw framebuffer ops. If the scissor test is on and the scissor
-    // rect doesn't match the draw rect, forget it.
-    //
-    // NOTE: Neither renderArea nor scissoredRenderArea are rotated, since scissoredRenderArea did
-    // not come from FramebufferVk::getRotatedScissoredRenderArea().
+    // Only defer clears for draw framebuffer ops.  Note that this will result in a render area that
+    // completely covers the framebuffer, even if the operation that follows is scissored.
     gl::Rectangle renderArea          = getNonRotatedCompleteRenderArea();
     gl::Rectangle scissoredRenderArea = ClipRectToScissor(context->getState(), renderArea, false);
-    bool deferClears = binding == GL_DRAW_FRAMEBUFFER && renderArea == scissoredRenderArea;
+    bool deferClears                  = binding == GL_DRAW_FRAMEBUFFER;
 
     // If we are notified that any attachment is dirty, but we have deferred clears for them, a
     // flushDeferredClears() call is missing somewhere.  ASSERT this to catch these bugs.
@@ -1758,7 +1745,7 @@ angle::Result FramebufferVk::syncState(const gl::Context *context,
         RotateRectangle(contextVk->getRotationReadFramebuffer(), invertViewport, renderArea.width,
                         renderArea.height, scissoredRenderArea, &rotatedScissoredRenderArea);
 
-        ANGLE_TRY(flushDeferredClears(contextVk, rotatedScissoredRenderArea));
+        ANGLE_TRY(flushDeferredClears(contextVk));
     }
 
     // No-op redundant changes to prevent closing the RenderPass.
@@ -2222,7 +2209,7 @@ angle::Result FramebufferVk::getSamplePosition(const gl::Context *context,
 }
 
 angle::Result FramebufferVk::startNewRenderPass(ContextVk *contextVk,
-                                                const gl::Rectangle &renderArea,
+                                                const gl::Rectangle &scissoredRenderArea,
                                                 vk::CommandBuffer **commandBufferOut)
 {
     ANGLE_TRY(contextVk->flushCommandsAndEndRenderPass());
@@ -2232,8 +2219,9 @@ angle::Result FramebufferVk::startNewRenderPass(ContextVk *contextVk,
     vk::PackedClearValuesArray packedClearValues;
     gl::DrawBufferMask previousUnresolveColorMask =
         mRenderPassDesc.getColorUnresolveAttachmentMask();
-    bool previousUnresolveDepth   = mRenderPassDesc.hasDepthUnresolveAttachment();
-    bool previousUnresolveStencil = mRenderPassDesc.hasStencilUnresolveAttachment();
+    const bool hasDeferredClears        = mDeferredClears.any();
+    const bool previousUnresolveDepth   = mRenderPassDesc.hasDepthUnresolveAttachment();
+    const bool previousUnresolveStencil = mRenderPassDesc.hasStencilUnresolveAttachment();
 
     // Make sure render pass and framebuffer are in agreement w.r.t unresolve attachments.
     ASSERT(mCurrentFramebufferDesc.getUnresolveAttachmentMask() ==
@@ -2456,6 +2444,14 @@ angle::Result FramebufferVk::startNewRenderPass(ContextVk *contextVk,
     vk::Framebuffer *framebuffer = nullptr;
     ANGLE_TRY(getFramebuffer(contextVk, &framebuffer, nullptr));
 
+    // If deferred clears were used in the render pass, expand the render area to the whole
+    // framebuffer.
+    gl::Rectangle renderArea = scissoredRenderArea;
+    if (hasDeferredClears)
+    {
+        renderArea = getRotatedCompleteRenderArea(contextVk);
+    }
+
     ANGLE_TRY(contextVk->beginNewRenderPass(*framebuffer, renderArea, mRenderPassDesc,
                                             renderPassAttachmentOps, depthStencilAttachmentIndex,
                                             packedClearValues, commandBufferOut));
@@ -2591,13 +2587,14 @@ GLint FramebufferVk::getSamples() const
     return firstRT ? firstRT->getImageForRenderPass().getSamples() : 1;
 }
 
-angle::Result FramebufferVk::flushDeferredClears(ContextVk *contextVk,
-                                                 const gl::Rectangle &renderArea)
+angle::Result FramebufferVk::flushDeferredClears(ContextVk *contextVk)
 {
     if (mDeferredClears.empty())
+    {
         return angle::Result::Continue;
+    }
 
-    return contextVk->startRenderPass(renderArea, nullptr);
+    return contextVk->startRenderPass(getRotatedCompleteRenderArea(contextVk), nullptr);
 }
 
 void FramebufferVk::updateRenderPassReadOnlyDepthMode(ContextVk *contextVk,

src/libANGLE/renderer/vulkan/FramebufferVk.h

@@ -116,7 +116,7 @@ class FramebufferVk : public FramebufferImpl
     RenderTargetVk *getColorReadRenderTarget() const;
 
     angle::Result startNewRenderPass(ContextVk *contextVk,
-                                     const gl::Rectangle &renderArea,
+                                     const gl::Rectangle &scissoredRenderArea,
                                      vk::CommandBuffer **commandBufferOut);
 
     RenderTargetVk *getFirstRenderTarget() const;
@@ -129,7 +129,7 @@ class FramebufferVk : public FramebufferImpl
                                  const vk::ImageView *resolveImageViewIn);
 
     bool hasDeferredClears() const { return !mDeferredClears.empty(); }
-    angle::Result flushDeferredClears(ContextVk *contextVk, const gl::Rectangle &renderArea);
+    angle::Result flushDeferredClears(ContextVk *contextVk);
     void setReadOnlyDepthFeedbackLoopMode(bool readOnlyDepthFeedbackModeEnabled)
     {
         mReadOnlyDepthFeedbackLoopMode = readOnlyDepthFeedbackModeEnabled;

src/tests/gl_tests/ClearTest.cpp

@@ -2393,6 +2393,38 @@ TEST_P(ClearTest, ScissoredClearThenNonScissoredDraw)
     EXPECT_PIXEL_COLOR_EQ(kSize - 1, kSize - 1, GLColor::cyan);
 }
 
+// Test that clear followed by a scissored masked clear works.
+TEST_P(ClearTest, ClearThenScissoredMaskedClear)
+{
+    constexpr GLsizei kSize = 16;
+
+    // Setup framebuffer
+    GLTexture color;
+    glBindTexture(GL_TEXTURE_2D, color);
+    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, kSize, kSize, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr);
+
+    GLFramebuffer fb;
+    glBindFramebuffer(GL_FRAMEBUFFER, fb);
+    glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, color, 0);
+    EXPECT_GL_NO_ERROR();
+    ASSERT_GL_FRAMEBUFFER_COMPLETE(GL_FRAMEBUFFER);
+
+    // Clear to red.
+    glClearColor(1.0f, 0.0f, 0.0f, 1.0f);
+    glClear(GL_COLOR_BUFFER_BIT);
+
+    // Mask red and clear to green with a scissor
+    glColorMask(GL_FALSE, GL_TRUE, GL_TRUE, GL_TRUE);
+    glScissor(0, 0, kSize / 2, kSize);
+    glEnable(GL_SCISSOR_TEST);
+    glClearColor(0.0f, 1.0f, 0.0f, 1.0f);
+    glClear(GL_COLOR_BUFFER_BIT);
+
+    // Verify that the left half is yellow, and the right half is red.
+    EXPECT_PIXEL_RECT_EQ(0, 0, kSize / 2, kSize, GLColor::yellow);
+    EXPECT_PIXEL_RECT_EQ(kSize / 2, 0, kSize / 2, kSize, GLColor::red);
+}
+
 #ifdef Bool
 // X11 craziness.
 #    undef Bool

src/tests/gl_tests/FramebufferTest.cpp

@@ -2372,6 +2372,86 @@ void main()
     ASSERT_GL_NO_ERROR();
 }
 
+// Tests that if the framebuffer is cleared, a feedback loop between a depth textures and the depth
+// buffer is established, and a scissored clear is issued, that the clear is not mistakenly
+// scissored.
+TEST_P(FramebufferTest_ES3, ReadOnlyDepthFeedbackLoopWithClearAndScissoredDraw)
+{
+    // Feedback loops are only supported on Vulkan.
+    // TODO(jmadill): Make GL extension. http://anglebug.com/4969
+    ANGLE_SKIP_TEST_IF(!IsVulkan());
+
+    constexpr GLuint kSize = 16;
+    glViewport(0, 0, kSize, kSize);
+
+    constexpr char kFS[] = R"(precision mediump float;
+varying vec2 v_texCoord;
+uniform sampler2D depth;
+void main()
+{
+    if (abs(texture2D(depth, v_texCoord).x - 0.5) < 0.1)
+    {
+        gl_FragColor = vec4(0, 1, 0, 1);
+    }
+    else
+    {
+        gl_FragColor = vec4(1, 0, 0, 1);
+    }
+})";
+
+    ANGLE_GL_PROGRAM(program, essl1_shaders::vs::Texture2D(), kFS);
+
+    GLFramebuffer framebuffer;
+    glBindFramebuffer(GL_FRAMEBUFFER, framebuffer);
+
+    GLTexture colorTexture;
+    glBindTexture(GL_TEXTURE_2D, colorTexture);
+    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA8, kSize, kSize, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr);
+
+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
+    glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, colorTexture, 0);
+
+    GLTexture depthTexture;
+    glBindTexture(GL_TEXTURE_2D, depthTexture);
+    glTexImage2D(GL_TEXTURE_2D, 0, GL_DEPTH_COMPONENT24, kSize, kSize, 0, GL_DEPTH_COMPONENT,
+                 GL_UNSIGNED_INT, nullptr);
+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
+    glFramebufferTexture2D(GL_FRAMEBUFFER, GL_DEPTH_ATTACHMENT, GL_TEXTURE_2D, depthTexture, 0);
+
+    ASSERT_GL_NO_ERROR();
+    ASSERT_GL_FRAMEBUFFER_COMPLETE(GL_FRAMEBUFFER);
+
+    // Clear color to blue and depth to 0.5.
+    glClearColor(0.0f, 0.0f, 1.0f, 1.0f);
+    glClearDepthf(0.5f);
+    glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
+
+    // Disable depth. Although this does not remove the feedback loop as defined by the
+    // spec it mimics what gfxbench does in its rendering tests.
+    glDepthMask(false);
+    glDisable(GL_DEPTH_TEST);
+
+    // Verify we can sample the depth texture and get 0.5.  Use a scissor.
+    glScissor(0, 0, kSize / 2, kSize);
+    glEnable(GL_SCISSOR_TEST);
+    drawQuad(program, essl1_shaders::PositionAttrib(), 0.5);
+    ASSERT_GL_NO_ERROR();
+
+    // Make sure the scissored region passes the depth test and is changed to green.
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+    EXPECT_PIXEL_COLOR_EQ(0, kSize - 1, GLColor::green);
+    EXPECT_PIXEL_COLOR_EQ(kSize / 2 - 1, 0, GLColor::green);
+    EXPECT_PIXEL_COLOR_EQ(kSize / 2 - 1, kSize - 1, GLColor::green);
+
+    // Make sure the region outside the scissor is cleared to blue.
+    EXPECT_PIXEL_COLOR_EQ(kSize / 2, 0, GLColor::blue);
+    EXPECT_PIXEL_COLOR_EQ(kSize / 2, kSize - 1, GLColor::blue);
+    EXPECT_PIXEL_COLOR_EQ(kSize - 1, 0, GLColor::blue);
+    EXPECT_PIXEL_COLOR_EQ(kSize - 1, kSize - 1, GLColor::blue);
+}
+
 // Covers a bug in ANGLE's Vulkan back-end. Our VkFramebuffer cache would in some cases forget to
 // check the draw states when computing a cache key.
 TEST_P(FramebufferTest_ES3, DisabledAttachmentRedefinition)

src/tests/gl_tests/VulkanPerformanceCounterTest.cpp

@@ -1591,6 +1591,79 @@ TEST_P(VulkanPerformanceCounterTest, MaskedClearDoesNotBreakRenderPass)
     EXPECT_PIXEL_COLOR_EQ(kSize / 2, kSize / 2, GLColor::blue);
 }
 
+// Tests that clear followed by scissored draw uses loadOp to clear.
+TEST_P(VulkanPerformanceCounterTest, ClearThenScissoredDraw)
+{
+    const rx::vk::PerfCounters &counters = hackANGLE();
+
+    uint32_t expectedRenderPassCount = counters.renderPasses + 1;
+    uint32_t expectedDepthClears     = counters.depthClears + 1;
+    uint32_t expectedStencilClears   = counters.stencilClears + 1;
+
+    constexpr GLsizei kSize = 64;
+
+    GLTexture texture;
+    glBindTexture(GL_TEXTURE_2D, texture);
+    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, kSize, kSize, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr);
+
+    GLRenderbuffer renderbuffer;
+    glBindRenderbuffer(GL_RENDERBUFFER, renderbuffer);
+    glRenderbufferStorage(GL_RENDERBUFFER, GL_DEPTH24_STENCIL8, kSize, kSize);
+
+    GLFramebuffer framebuffer;
+    glBindFramebuffer(GL_FRAMEBUFFER, framebuffer);
+    glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture, 0);
+    glFramebufferRenderbuffer(GL_FRAMEBUFFER, GL_DEPTH_STENCIL_ATTACHMENT, GL_RENDERBUFFER,
+                              renderbuffer);
+    ASSERT_GL_FRAMEBUFFER_COMPLETE(GL_FRAMEBUFFER);
+    ASSERT_GL_NO_ERROR();
+
+    // Clear depth/stencil
+    glClearColor(1.0f, 0.0f, 0.0f, 1.0f);
+    glClearDepthf(1.0f);
+    glClearStencil(0x55);
+    glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT | GL_STENCIL_BUFFER_BIT);
+
+    // Issue a scissored draw call, expecting depth/stencil to be 1.0 and 0x55.
+    glViewport(0, 0, kSize, kSize);
+    glScissor(0, 0, kSize / 2, kSize);
+    glEnable(GL_SCISSOR_TEST);
+
+    glEnable(GL_DEPTH_TEST);
+    glDepthFunc(GL_LESS);
+
+    glEnable(GL_STENCIL_TEST);
+    glStencilFunc(GL_EQUAL, 0x55, 0xFF);
+    glStencilOp(GL_KEEP, GL_KEEP, GL_KEEP);
+    glStencilMask(0xFF);
+
+    ANGLE_GL_PROGRAM(drawGreen, essl1_shaders::vs::Passthrough(), essl1_shaders::fs::Green());
+    drawQuad(drawGreen, essl1_shaders::PositionAttrib(), 0.95f);
+    ASSERT_GL_NO_ERROR();
+
+    // Break the render pass.
+    GLTexture copyTex;
+    glBindTexture(GL_TEXTURE_2D, copyTex);
+    glCopyTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA8, 0, 0, kSize, kSize, 0);
+    ASSERT_GL_NO_ERROR();
+
+    // Make sure a single render pass was used and depth/stencil clear used loadOp=CLEAR.
+    EXPECT_EQ(expectedRenderPassCount, counters.renderPasses);
+    EXPECT_EQ(expectedDepthClears, counters.depthClears);
+    EXPECT_EQ(expectedStencilClears, counters.stencilClears);
+
+    // Verify correctness.
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+    EXPECT_PIXEL_COLOR_EQ(kSize / 2 - 1, 0, GLColor::green);
+    EXPECT_PIXEL_COLOR_EQ(0, kSize - 1, GLColor::green);
+    EXPECT_PIXEL_COLOR_EQ(kSize / 2 - 1, kSize - 1, GLColor::green);
+
+    EXPECT_PIXEL_COLOR_EQ(kSize / 2, 0, GLColor::red);
+    EXPECT_PIXEL_COLOR_EQ(kSize - 1, 0, GLColor::red);
+    EXPECT_PIXEL_COLOR_EQ(kSize / 2, kSize - 1, GLColor::red);
+    EXPECT_PIXEL_COLOR_EQ(kSize - 1, kSize - 1, GLColor::red);
+}
+
 // Tests that scissored clears don't break the RP.
 TEST_P(VulkanPerformanceCounterTest, ScissoredClearDoesNotBreakRenderPass)
 {