Vulkan: Fix deferred clears and noop clear and blit

src/libANGLE/Context.cpp

@@ -259,6 +259,17 @@ enum SubjectIndexes : angle::SubjectIndex
     kDrawFramebufferSubjectIndex,
     kProgramPipelineSubjectIndex
 };
+
+bool IsClearBufferEnabled(const FramebufferState &mState, GLenum buffer, GLint drawbuffer)
+{
+    return buffer != GL_COLOR || mState.getEnabledDrawBuffers()[drawbuffer];
+}
+
+bool IsColorMaskedOut(const BlendState &blend)
+{
+    return (!blend.colorMaskRed && !blend.colorMaskGreen && !blend.colorMaskBlue &&
+            !blend.colorMaskAlpha);
+}
 }  // anonymous namespace
 
 Context::Context(egl::Display *display,
@@ -3650,6 +3661,30 @@ void Context::blitFramebuffer(GLint srcX0,
     Framebuffer *drawFramebuffer = mState.getDrawFramebuffer();
     ASSERT(drawFramebuffer);
 
+    // Note that blitting is called against draw framebuffer.
+    // See the code in gl::Context::blitFramebuffer.
+    if ((mask & GL_COLOR_BUFFER_BIT) && !drawFramebuffer->hasEnabledDrawBuffer())
+    {
+        mask &= ~GL_COLOR_BUFFER_BIT;
+    }
+
+    if ((mask & GL_STENCIL_BUFFER_BIT) &&
+        drawFramebuffer->getState().getStencilAttachment() == nullptr)
+    {
+        mask &= ~GL_STENCIL_BUFFER_BIT;
+    }
+
+    if ((mask & GL_DEPTH_BUFFER_BIT) && drawFramebuffer->getState().getDepthAttachment() == nullptr)
+    {
+        mask &= ~GL_DEPTH_BUFFER_BIT;
+    }
+
+    // Early out if none of the specified attachments exist or are enabled.
+    if (mask == 0)
+    {
+        return;
+    }
+
     Rectangle srcArea(srcX0, srcY0, srcX1 - srcX0, srcY1 - srcY0);
     Rectangle dstArea(dstX0, dstY0, dstX1 - dstX0, dstY1 - dstY0);
 
@@ -3665,12 +3700,76 @@ void Context::blitFramebuffer(GLint srcX0,
 
 void Context::clear(GLbitfield mask)
 {
+    if (mState.isRasterizerDiscardEnabled())
+    {
+        return;
+    }
+
+    // Remove clear bits that are ineffective. An effective clear changes at least one fragment. If
+    // color/depth/stencil masks make the clear ineffective we skip it altogether.
+
+    // If all color channels in all draw buffers are masked, don't attempt to clear color.
+    if (mState.allActiveDrawBufferChannelsMasked())
+    {
+        mask &= ~GL_COLOR_BUFFER_BIT;
+    }
+
+    // If depth write is disabled, don't attempt to clear depth.
+    if (!mState.getDepthStencilState().depthMask)
+    {
+        mask &= ~GL_DEPTH_BUFFER_BIT;
+    }
+
+    // If all stencil bits are masked, don't attempt to clear stencil.
+    if (mState.getDepthStencilState().stencilWritemask == 0)
+    {
+        mask &= ~GL_STENCIL_BUFFER_BIT;
+    }
+
+    if (mask == 0)
+    {
+        return;
+    }
+
     ANGLE_CONTEXT_TRY(prepareForClear(mask));
     ANGLE_CONTEXT_TRY(mState.getDrawFramebuffer()->clear(this, mask));
 }
 
+bool Context::isClearBufferMaskedOut(GLenum buffer, GLint drawbuffer) const
+{
+    switch (buffer)
+    {
+        case GL_COLOR:
+            ASSERT(static_cast<size_t>(drawbuffer) < mState.getBlendStateArray().size());
+            return IsColorMaskedOut(mState.getBlendStateArray()[drawbuffer]);
+        case GL_DEPTH:
+            return mState.getDepthStencilState().isDepthMaskedOut();
+        case GL_STENCIL:
+            return mState.getDepthStencilState().isStencilMaskedOut();
+        case GL_DEPTH_STENCIL:
+            return mState.getDepthStencilState().isDepthMaskedOut() &&
+                   mState.getDepthStencilState().isStencilMaskedOut();
+        default:
+            UNREACHABLE();
+            return true;
+    }
+}
+
+bool Context::noopClearBuffer(GLenum buffer, GLint drawbuffer) const
+{
+    Framebuffer *framebufferObject = mState.getDrawFramebuffer();
+
+    return !IsClearBufferEnabled(framebufferObject->getState(), buffer, drawbuffer) ||
+           mState.isRasterizerDiscardEnabled() || isClearBufferMaskedOut(buffer, drawbuffer);
+}
+
 void Context::clearBufferfv(GLenum buffer, GLint drawbuffer, const GLfloat *values)
 {
+    if (noopClearBuffer(buffer, drawbuffer))
+    {
+        return;
+    }
+
     Framebuffer *framebufferObject          = mState.getDrawFramebuffer();
     const FramebufferAttachment *attachment = nullptr;
     if (buffer == GL_DEPTH)
@@ -3694,6 +3793,11 @@ void Context::clearBufferfv(GLenum buffer, GLint drawbuffer, const GLfloat *valu
 
 void Context::clearBufferuiv(GLenum buffer, GLint drawbuffer, const GLuint *values)
 {
+    if (noopClearBuffer(buffer, drawbuffer))
+    {
+        return;
+    }
+
     Framebuffer *framebufferObject          = mState.getDrawFramebuffer();
     const FramebufferAttachment *attachment = nullptr;
     if (buffer == GL_COLOR &&
@@ -3713,6 +3817,11 @@ void Context::clearBufferuiv(GLenum buffer, GLint drawbuffer, const GLuint *valu
 
 void Context::clearBufferiv(GLenum buffer, GLint drawbuffer, const GLint *values)
 {
+    if (noopClearBuffer(buffer, drawbuffer))
+    {
+        return;
+    }
+
     Framebuffer *framebufferObject          = mState.getDrawFramebuffer();
     const FramebufferAttachment *attachment = nullptr;
     if (buffer == GL_STENCIL)
@@ -3736,6 +3845,11 @@ void Context::clearBufferiv(GLenum buffer, GLint drawbuffer, const GLint *values
 
 void Context::clearBufferfi(GLenum buffer, GLint drawbuffer, GLfloat depth, GLint stencil)
 {
+    if (noopClearBuffer(buffer, drawbuffer))
+    {
+        return;
+    }
+
     Framebuffer *framebufferObject = mState.getDrawFramebuffer();
     ASSERT(framebufferObject);
 

src/libANGLE/Context.h

@@ -617,6 +617,9 @@ class Context final : public egl::LabeledObject, angle::NonCopyable, public angl
     bool noopDraw(PrimitiveMode mode, GLsizei count) const;
     bool noopDrawInstanced(PrimitiveMode mode, GLsizei count, GLsizei instanceCount) const;
 
+    bool isClearBufferMaskedOut(GLenum buffer, GLint drawbuffer) const;
+    bool noopClearBuffer(GLenum buffer, GLint drawbuffer) const;
+
   private:
     void initialize();
 

src/libANGLE/Framebuffer.cpp

@@ -262,49 +262,6 @@ angle::Result InitAttachment(const Context *context, FramebufferAttachment *atta
     }
     return angle::Result::Continue;
 }
-
-bool IsColorMaskedOut(const BlendState &blend)
-{
-    return (!blend.colorMaskRed && !blend.colorMaskGreen && !blend.colorMaskBlue &&
-            !blend.colorMaskAlpha);
-}
-
-bool IsDepthMaskedOut(const DepthStencilState &depthStencil)
-{
-    return !depthStencil.depthMask;
-}
-
-bool IsStencilMaskedOut(const DepthStencilState &depthStencil)
-{
-    return ((depthStencil.stencilMask & depthStencil.stencilWritemask) == 0);
-}
-
-bool IsClearBufferMaskedOut(const Context *context, GLenum buffer, GLint drawbuffer)
-{
-    switch (buffer)
-    {
-        case GL_COLOR:
-            ASSERT(static_cast<size_t>(drawbuffer) <
-                   context->getState().getBlendStateArray().size());
-            return IsColorMaskedOut(context->getState().getBlendStateArray()[drawbuffer]);
-        case GL_DEPTH:
-            return IsDepthMaskedOut(context->getState().getDepthStencilState());
-        case GL_STENCIL:
-            return IsStencilMaskedOut(context->getState().getDepthStencilState());
-        case GL_DEPTH_STENCIL:
-            return IsDepthMaskedOut(context->getState().getDepthStencilState()) &&
-                   IsStencilMaskedOut(context->getState().getDepthStencilState());
-        default:
-            UNREACHABLE();
-            return true;
-    }
-}
-
-bool IsClearBufferDisabled(const FramebufferState &mState, GLenum buffer, GLint drawbuffer)
-{
-    return buffer == GL_COLOR && !mState.getEnabledDrawBuffers()[drawbuffer];
-}
-
 }  // anonymous namespace
 
 // This constructor is only used for default framebuffers.
@@ -1501,39 +1458,9 @@ angle::Result Framebuffer::invalidateSub(const Context *context,
 
 angle::Result Framebuffer::clear(const Context *context, GLbitfield mask)
 {
-    const auto &glState = context->getState();
-    if (glState.isRasterizerDiscardEnabled())
-    {
-        return angle::Result::Continue;
-    }
-
-    // Remove clear bits that are ineffective. An effective clear changes at least one fragment. If
-    // color/depth/stencil masks make the clear ineffective we skip it altogether.
-
-    // If all color channels in all draw buffers are masked, don't attempt to clear color.
-    if (context->getState().allActiveDrawBufferChannelsMasked())
-    {
-        mask &= ~GL_COLOR_BUFFER_BIT;
-    }
-
-    // If depth write is disabled, don't attempt to clear depth.
-    if (!context->getState().getDepthStencilState().depthMask)
-    {
-        mask &= ~GL_DEPTH_BUFFER_BIT;
-    }
+    ASSERT(mask && !context->getState().isRasterizerDiscardEnabled());
 
-    // If all stencil bits are masked, don't attempt to clear stencil.
-    if (context->getState().getDepthStencilState().stencilWritemask == 0)
-    {
-        mask &= ~GL_STENCIL_BUFFER_BIT;
-    }
-
-    if (mask != 0)
-    {
-        ANGLE_TRY(mImpl->clear(context, mask));
-    }
-
-    return angle::Result::Continue;
+    return mImpl->clear(context, mask);
 }
 
 angle::Result Framebuffer::clearBufferfv(const Context *context,
@@ -1541,16 +1468,7 @@ angle::Result Framebuffer::clearBufferfv(const Context *context,
                                          GLint drawbuffer,
                                          const GLfloat *values)
 {
-    if (IsClearBufferDisabled(mState, buffer, drawbuffer) ||
-        context->getState().isRasterizerDiscardEnabled() ||
-        IsClearBufferMaskedOut(context, buffer, drawbuffer))
-    {
-        return angle::Result::Continue;
-    }
-
-    ANGLE_TRY(mImpl->clearBufferfv(context, buffer, drawbuffer, values));
-
-    return angle::Result::Continue;
+    return mImpl->clearBufferfv(context, buffer, drawbuffer, values);
 }
 
 angle::Result Framebuffer::clearBufferuiv(const Context *context,
@@ -1558,16 +1476,7 @@ angle::Result Framebuffer::clearBufferuiv(const Context *context,
                                           GLint drawbuffer,
                                           const GLuint *values)
 {
-    if (IsClearBufferDisabled(mState, buffer, drawbuffer) ||
-        context->getState().isRasterizerDiscardEnabled() ||
-        IsClearBufferMaskedOut(context, buffer, drawbuffer))
-    {
-        return angle::Result::Continue;
-    }
-
-    ANGLE_TRY(mImpl->clearBufferuiv(context, buffer, drawbuffer, values));
-
-    return angle::Result::Continue;
+    return mImpl->clearBufferuiv(context, buffer, drawbuffer, values);
 }
 
 angle::Result Framebuffer::clearBufferiv(const Context *context,
@@ -1575,16 +1484,7 @@ angle::Result Framebuffer::clearBufferiv(const Context *context,
                                          GLint drawbuffer,
                                          const GLint *values)
 {
-    if (IsClearBufferDisabled(mState, buffer, drawbuffer) ||
-        context->getState().isRasterizerDiscardEnabled() ||
-        IsClearBufferMaskedOut(context, buffer, drawbuffer))
-    {
-        return angle::Result::Continue;
-    }
-
-    ANGLE_TRY(mImpl->clearBufferiv(context, buffer, drawbuffer, values));
-
-    return angle::Result::Continue;
+    return mImpl->clearBufferiv(context, buffer, drawbuffer, values);
 }
 
 angle::Result Framebuffer::clearBufferfi(const Context *context,
@@ -1593,12 +1493,6 @@ angle::Result Framebuffer::clearBufferfi(const Context *context,
                                          GLfloat depth,
                                          GLint stencil)
 {
-    if (context->getState().isRasterizerDiscardEnabled() ||
-        IsClearBufferMaskedOut(context, buffer, drawbuffer))
-    {
-        return angle::Result::Continue;
-    }
-
     bool clearDepth   = context->getState().getDepthStencilState().depthMask;
     bool clearStencil = context->getState().getDepthStencilState().stencilWritemask != 0;
 
@@ -1655,31 +1549,9 @@ angle::Result Framebuffer::blit(const Context *context,
                                 GLbitfield mask,
                                 GLenum filter)
 {
-    GLbitfield blitMask = mask;
-
-    // Note that blitting is called against draw framebuffer.
-    // See the code in gl::Context::blitFramebuffer.
-    if ((mask & GL_COLOR_BUFFER_BIT) && !hasEnabledDrawBuffer())
-    {
-        blitMask &= ~GL_COLOR_BUFFER_BIT;
-    }
-
-    if ((mask & GL_STENCIL_BUFFER_BIT) && mState.getStencilAttachment() == nullptr)
-    {
-        blitMask &= ~GL_STENCIL_BUFFER_BIT;
-    }
-
-    if ((mask & GL_DEPTH_BUFFER_BIT) && mState.getDepthAttachment() == nullptr)
-    {
-        blitMask &= ~GL_DEPTH_BUFFER_BIT;
-    }
-
-    if (!blitMask)
-    {
-        return angle::Result::Continue;
-    }
+    ASSERT(mask != 0);
 
-    return mImpl->blit(context, sourceArea, destArea, blitMask, filter);
+    return mImpl->blit(context, sourceArea, destArea, mask, filter);
 }
 
 int Framebuffer::getSamples(const Context *context) const
@@ -2183,8 +2055,8 @@ angle::Result Framebuffer::ensureClearAttachmentsInitialized(const Context *cont
     const DepthStencilState &depthStencil = glState.getDepthStencilState();
 
     bool color = (mask & GL_COLOR_BUFFER_BIT) != 0 && !glState.allActiveDrawBufferChannelsMasked();
-    bool depth = (mask & GL_DEPTH_BUFFER_BIT) != 0 && !IsDepthMaskedOut(depthStencil);
-    bool stencil = (mask & GL_STENCIL_BUFFER_BIT) != 0 && !IsStencilMaskedOut(depthStencil);
+    bool depth = (mask & GL_DEPTH_BUFFER_BIT) != 0 && !depthStencil.isDepthMaskedOut();
+    bool stencil = (mask & GL_STENCIL_BUFFER_BIT) != 0 && !depthStencil.isStencilMaskedOut();
 
     if (!color && !depth && !stencil)
     {
@@ -2210,7 +2082,7 @@ angle::Result Framebuffer::ensureClearBufferAttachmentsInitialized(const Context
 {
     if (!context->isRobustResourceInitEnabled() ||
         context->getState().isRasterizerDiscardEnabled() ||
-        IsClearBufferMaskedOut(context, buffer, drawbuffer))
+        context->isClearBufferMaskedOut(buffer, drawbuffer))
     {
         return angle::Result::Continue;
     }

src/libANGLE/angletypes.cpp

@@ -104,6 +104,16 @@ DepthStencilState::DepthStencilState(const DepthStencilState &other)
     memcpy(this, &other, sizeof(DepthStencilState));
 }
 
+bool DepthStencilState::isDepthMaskedOut() const
+{
+    return !depthMask;
+}
+
+bool DepthStencilState::isStencilMaskedOut() const
+{
+    return (stencilMask & stencilWritemask) == 0;
+}
+
 bool operator==(const DepthStencilState &a, const DepthStencilState &b)
 {
     return memcmp(&a, &b, sizeof(DepthStencilState)) == 0;

src/libANGLE/angletypes.h

@@ -183,6 +183,9 @@ struct DepthStencilState final
     DepthStencilState();
     DepthStencilState(const DepthStencilState &other);
 
+    bool isDepthMaskedOut() const;
+    bool isStencilMaskedOut() const;
+
     bool depthTest;
     GLenum depthFunc;
     bool depthMask;

src/tests/gl_tests/StateChangeTest.cpp

@@ -2759,6 +2759,121 @@ TEST_P(SimpleStateChangeTest, DrawAndClearTextureRepeatedly)
     }
 }
 
+// Test that clear followed by rebind of framebuffer attachment works (with noop clear in between).
+TEST_P(SimpleStateChangeTestES3, ClearThenNoopClearThenRebindAttachment)
+{
+    // Create a texture with red
+    const GLColor kInitColor1 = GLColor::red;
+    GLTexture texture1;
+    glBindTexture(GL_TEXTURE_2D, texture1);
+    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, 1, 1, 0, GL_RGBA, GL_UNSIGNED_BYTE, &kInitColor1);
+
+    // Create a framebuffer to be cleared
+    GLTexture texture2;
+    glBindTexture(GL_TEXTURE_2D, texture2);
+    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, 1, 1, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr);
+
+    GLFramebuffer drawFBO;
+    glBindFramebuffer(GL_FRAMEBUFFER, drawFBO);
+    glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture2, 0);
+    ASSERT_GL_FRAMEBUFFER_COMPLETE(GL_FRAMEBUFFER);
+
+    EXPECT_GL_NO_ERROR();
+
+    // Clear the framebuffer to green
+    glClearColor(0.0f, 1.0f, 0.0f, 1.0f);
+    glClear(GL_COLOR_BUFFER_BIT);
+
+    // Clear again, but in a way that would be a no-op.  In the Vulkan backend, this will result in
+    // a framebuffer sync state, which extracts deferred clears.  However, as the clear is actually
+    // a noop, the deferred clears will remain unflushed.
+    glClear(0);
+
+    // Change framebuffer's attachment to the other texture.
+    glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture1, 0);
+    ASSERT_GL_FRAMEBUFFER_COMPLETE(GL_FRAMEBUFFER);
+
+    // A bogus draw to make sure the render pass is cleared in the Vulkan backend.
+    ANGLE_GL_PROGRAM(program, essl1_shaders::vs::Simple(), essl1_shaders::fs::Blue());
+    glUseProgram(program);
+
+    glEnable(GL_BLEND);
+    glBlendFunc(GL_ZERO, GL_ONE);
+    drawQuad(program, essl1_shaders::PositionAttrib(), 0.5);
+    EXPECT_GL_NO_ERROR();
+
+    // Expect red, which is the original contents of texture1.  If the clear is mistakenly applied
+    // to the new attachment, green will be read back.
+    EXPECT_PIXEL_COLOR_EQ(0, 0, kInitColor1);
+
+    // Attach back to texture2.  It should be cleared to green.
+    glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture2, 0);
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
+
+// Test that clear followed by rebind of framebuffer attachment works (with noop blit in between).
+TEST_P(SimpleStateChangeTestES3, ClearThenNoopBlitThenRebindAttachment)
+{
+    // Create a texture with red
+    const GLColor kInitColor1 = GLColor::red;
+    GLTexture texture1;
+    glBindTexture(GL_TEXTURE_2D, texture1);
+    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, 1, 1, 0, GL_RGBA, GL_UNSIGNED_BYTE, &kInitColor1);
+
+    // Create a framebuffer to be cleared
+    GLTexture texture2;
+    glBindTexture(GL_TEXTURE_2D, texture2);
+    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, 1, 1, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr);
+
+    GLFramebuffer drawFBO;
+    glBindFramebuffer(GL_FRAMEBUFFER, drawFBO);
+    glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture2, 0);
+    ASSERT_GL_FRAMEBUFFER_COMPLETE(GL_FRAMEBUFFER);
+
+    EXPECT_GL_NO_ERROR();
+
+    // Clear the framebuffer to green
+    glClearColor(0.0f, 1.0f, 0.0f, 1.0f);
+    glClear(GL_COLOR_BUFFER_BIT);
+
+    // Issue noop blit.  In the Vulkan backend, this will result in a framebuffer sync state, which
+    // extracts deferred clears.  However, as the blit is actually a noop, the deferred clears will
+    // remain unflushed.
+    GLTexture blitSrc;
+    glBindTexture(GL_TEXTURE_2D, blitSrc);
+    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, 1, 1, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr);
+
+    GLFramebuffer readFBO;
+    glBindFramebuffer(GL_READ_FRAMEBUFFER, readFBO);
+    glFramebufferTexture2D(GL_READ_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, blitSrc, 0);
+    ASSERT_GL_FRAMEBUFFER_COMPLETE(GL_READ_FRAMEBUFFER);
+
+    glBlitFramebuffer(0, 0, 1, 1, 0, 0, 1, 1, GL_DEPTH_BUFFER_BIT, GL_NEAREST);
+    EXPECT_GL_NO_ERROR();
+
+    // Change framebuffer's attachment to the other texture.
+    glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture1, 0);
+    ASSERT_GL_FRAMEBUFFER_COMPLETE(GL_FRAMEBUFFER);
+
+    // A bogus draw to make sure the render pass is cleared in the Vulkan backend.
+    ANGLE_GL_PROGRAM(program, essl1_shaders::vs::Simple(), essl1_shaders::fs::Blue());
+    glUseProgram(program);
+
+    glEnable(GL_BLEND);
+    glBlendFunc(GL_ZERO, GL_ONE);
+    drawQuad(program, essl1_shaders::PositionAttrib(), 0.5);
+    EXPECT_GL_NO_ERROR();
+
+    // Expect red, which is the original contents of texture1.  If the clear is mistakenly applied
+    // to the new attachment, green will be read back.
+    glBindFramebuffer(GL_READ_FRAMEBUFFER, drawFBO);
+    EXPECT_PIXEL_COLOR_EQ(0, 0, kInitColor1);
+
+    // Attach back to texture2.  It should be cleared to green.
+    glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture2, 0);
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
+
 // Validates disabling cull face really disables it.
 TEST_P(SimpleStateChangeTest, EnableAndDisableCullFace)
 {