Vulkan: do not end render pass when invalidating

src/libANGLE/renderer/vulkan/ContextVk.cpp

@@ -2475,10 +2475,9 @@ void ContextVk::optimizeRenderPassForPresent(VkFramebuffer framebufferHandle)
     RenderTargetVk *depthStencilRenderTarget = mDrawFramebuffer->getDepthStencilRenderTarget();
     if (depthStencilRenderTarget)
     {
-        size_t depthStencilAttachmentIndexVk = mDrawFramebuffer->getDepthStencilAttachmentIndexVk();
         // Change depthstencil attachment storeOp to DONT_CARE
-        mRenderPassCommands->invalidateRenderPassStencilAttachment(depthStencilAttachmentIndexVk);
-        mRenderPassCommands->invalidateRenderPassDepthAttachment(depthStencilAttachmentIndexVk);
+        mRenderPassCommands->invalidateRenderPassStencilAttachment();
+        mRenderPassCommands->invalidateRenderPassDepthAttachment();
         // Mark content as invalid so that we will not load them in next renderpass
         depthStencilRenderTarget->invalidateEntireContent();
     }
@@ -2892,10 +2891,16 @@ angle::Result ContextVk::syncState(const gl::Context *context,
                 mGraphicsPipelineDesc->updateStencilTestEnabled(&mGraphicsPipelineTransition,
                                                                 glState.getDepthStencilState(),
                                                                 glState.getDrawFramebuffer());
-                if (mState.isStencilTestEnabled() && mRenderPassCommands->started())
+                if (mRenderPassCommands->started())
                 {
                     vk::ResourceAccess access = GetStencilAccess(mState.getDepthStencilState());
                     mRenderPassCommands->onStencilAccess(access);
+                    // Did this depth-state change undo a previous invalidation of the depth-stencil
+                    // attachment?
+                    if (mRenderPassCommands->shouldRestoreDepthStencilAttachment())
+                    {
+                        mDrawFramebuffer->restoreDepthStencilDefinedContents();
+                    }
                 }
                 break;
             case gl::State::DIRTY_BIT_STENCIL_FUNCS_FRONT:
@@ -4499,16 +4504,9 @@ angle::Result ContextVk::startRenderPass(gl::Rectangle renderArea,
             this, mRenderPassCommandBuffer);
     }
 
-    if (mState.isDepthTestEnabled())
-    {
-        vk::ResourceAccess access = GetDepthAccess(mState.getDepthStencilState());
-        mRenderPassCommands->onDepthAccess(access);
-    }
-    if (mState.isStencilTestEnabled())
-    {
-        vk::ResourceAccess access = GetStencilAccess(mState.getDepthStencilState());
-        mRenderPassCommands->onStencilAccess(access);
-    }
+    const gl::DepthStencilState &dsState = mState.getDepthStencilState();
+    mRenderPassCommands->onDepthAccess(GetDepthAccess(dsState));
+    mRenderPassCommands->onStencilAccess(GetStencilAccess(dsState));
 
     if (commandBufferOut)
     {
@@ -4860,7 +4858,7 @@ void ContextVk::setDefaultUniformBlocksMinSizeForTesting(size_t minSize)
 
 angle::Result ContextVk::updateRenderPassDepthAccess()
 {
-    if (mState.isDepthTestEnabled() && hasStartedRenderPass())
+    if (hasStartedRenderPass())
     {
         vk::ResourceAccess access = GetDepthAccess(mState.getDepthStencilState());
 
@@ -4874,6 +4872,12 @@ angle::Result ContextVk::updateRenderPassDepthAccess()
         else
         {
             mRenderPassCommands->onDepthAccess(access);
+            // Did this depth-state change undo a previous invalidation of the depth-stencil
+            // attachment?
+            if (mRenderPassCommands->shouldRestoreDepthStencilAttachment())
+            {
+                mDrawFramebuffer->restoreDepthStencilDefinedContents();
+            }
         }
     }
 

src/libANGLE/renderer/vulkan/FramebufferVk.cpp

@@ -340,6 +340,9 @@ angle::Result FramebufferVk::invalidateSub(const gl::Context *context,
 
     if (area.encloses(contextVk->getStartedRenderPassCommands().getRenderArea()))
     {
+        // Because the render pass's render area is within the invalidated area, it is fine for
+        // invalidateImpl() to use a storeOp of DONT_CARE (i.e. fine to not store the contents of
+        // the invalidated area).
         ANGLE_TRY(invalidateImpl(contextVk, count, attachments, true));
     }
 
@@ -1444,31 +1447,25 @@ angle::Result FramebufferVk::invalidateImpl(ContextVk *contextVk,
         {
             if (invalidateDepthBuffer)
             {
-                contextVk->getStartedRenderPassCommands().invalidateRenderPassDepthAttachment(
-                    attachmentIndexVk);
+                contextVk->getStartedRenderPassCommands().invalidateRenderPassDepthAttachment();
             }
 
             if (invalidateStencilBuffer)
             {
-                contextVk->getStartedRenderPassCommands().invalidateRenderPassStencilAttachment(
-                    attachmentIndexVk);
+                contextVk->getStartedRenderPassCommands().invalidateRenderPassStencilAttachment();
             }
         }
-
-        // NOTE: Possible future optimization is to delay setting the storeOp and only do so if the
-        // render pass is closed by itself before another draw call.  Otherwise, in a situation like
-        // this:
-        //
-        //     draw()
-        //     invalidate()
-        //     draw()
-        //
-        // We would be discarding the attachments only to load them for the next draw (which is less
-        // efficient than keeping the render pass open and not do the discard at all).  While dEQP
-        // tests this pattern, this optimization may not be necessary if no application does this.
-        // It is expected that an application would invalidate() when it's done with the
-        // framebuffer, so the render pass would have closed either way.
-        ANGLE_TRY(contextVk->flushCommandsAndEndRenderPass());
+        if (invalidateColorBuffers.any())
+        {
+            // Only end the render pass if invalidating at least one color buffer.  Do not end the
+            // render pass if only the depth and/or stencil buffer is invalidated.  At least one
+            // application invalidates those every frame, disables depth, and then continues to
+            // draw only to the color buffer.
+            //
+            // Since we are not aware of any application that invalidates a color buffer and
+            // continues to draw to it, we leave that unoptimized.
+            ANGLE_TRY(contextVk->flushCommandsAndEndRenderPass());
+        }
     }
 
     // If not a partial invalidate, mark the contents of the invalidated attachments as undefined,
@@ -2356,6 +2353,18 @@ angle::Result FramebufferVk::startNewRenderPass(ContextVk *contextVk,
     return angle::Result::Continue;
 }
 
+void FramebufferVk::restoreDepthStencilDefinedContents()
+{
+    // If the depthStencilRenderTarget does not have "defined content" (i.e. meaning that a future
+    // render pass should use a loadOp of DONT_CARE), we should restore it (i.e. so that a future
+    // render pass uses a loadOp of LOAD).
+    RenderTargetVk *depthStencilRenderTarget = mRenderTargetCache.getDepthStencil(true);
+    if (depthStencilRenderTarget)
+    {
+        depthStencilRenderTarget->restoreEntireContent();
+    }
+}
+
 void FramebufferVk::updateActiveColorMasks(size_t colorIndexGL, bool r, bool g, bool b, bool a)
 {
     mActiveColorComponentMasksForClear[0].set(colorIndexGL, r);

src/libANGLE/renderer/vulkan/FramebufferVk.h

@@ -117,6 +117,7 @@ class FramebufferVk : public FramebufferImpl
     angle::Result startNewRenderPass(ContextVk *contextVk,
                                      const gl::Rectangle &renderArea,
                                      vk::CommandBuffer **commandBufferOut);
+    void restoreDepthStencilDefinedContents();
 
     RenderTargetVk *getFirstRenderTarget() const;
     GLint getSamples() const;

src/libANGLE/renderer/vulkan/RenderTargetVk.h

@@ -101,6 +101,7 @@ class RenderTargetVk final : public FramebufferAttachmentRenderTarget
     // Mark content as undefined so that certain optimizations are possible such as using DONT_CARE
     // as loadOp of the render target in the next renderpass.
     void invalidateEntireContent() { mContentDefined = false; }
+    void restoreEntireContent() { mContentDefined = true; }
 
     // See the description of mIsImageTransient for details of how the following two can
     // interact.

src/libANGLE/renderer/vulkan/vk_helpers.cpp

@@ -589,6 +589,10 @@ CommandBufferHelper::CommandBufferHelper()
       mIsRenderPassCommandBuffer(false),
       mDepthStartAccess(ResourceAccess::Unused),
       mStencilStartAccess(ResourceAccess::Unused),
+      mDepthEnabled(false),
+      mDepthInvalidatedState(NeverInvalidated),
+      mStencilEnabled(false),
+      mStencilInvalidatedState(NeverInvalidated),
       mDepthStencilAttachmentIndex(kInvalidAttachmentIndex)
 {}
 
@@ -726,6 +730,72 @@ void CommandBufferHelper::imageWrite(ResourceUseList *resourceUseList,
     }
 }
 
+void CommandBufferHelper::onDepthAccess(ResourceAccess access)
+{
+    // TODO(ianelliott): Rework the handling of invalidated attachments in a follow-up CL, using
+    // the count of commands in the SecondaryCommandBuffer.
+    // See https://issuetracker.google.com/issues/163854287
+    InvalidatedState invalidatedState;
+
+    if (access == vk::ResourceAccess::Write)
+    {
+        // This handles various scenarios that an app/test can do with valid GLES usage.  For
+        // example, consider an app that invalidates, doesn't disable the functionality, and draws
+        // again.  In that case, the drawing that occurs after the invalidate means that there is
+        // once again valid content in the attachment (i.e. that should not be discarded).  Since
+        // we don't track draws, we must be conservative and assume that a draw may have occured
+        // since invalidation unless the functionality has also been disabled and the re-enabled.
+        invalidatedState = (!mDepthEnabled) ? NoLongerInvalidated : Invalidated;
+        // Keep track of whether depth functionality is enabled
+        mDepthEnabled = true;
+    }
+    else
+    {
+        invalidatedState = Invalidated;
+        // Keep track of whether depth functionality is enabled
+        mDepthEnabled = false;
+    }
+
+    // Update the access for optimizing this render pass's loadOp
+    UpdateAccess(&mDepthStartAccess, access);
+    ASSERT((mRenderPassDesc.getDepthStencilAccess() != ResourceAccess::ReadOnly) ||
+           mDepthStartAccess != ResourceAccess::Write);
+    // Update the invalidate state for optimizing this render pass's storeOp
+    UpdateInvalidatedState(&mDepthInvalidatedState, invalidatedState);
+}
+
+void CommandBufferHelper::onStencilAccess(ResourceAccess access)
+{
+    // TODO(ianelliott): Rework the handling of invalidated attachments in a follow-up CL, using
+    // the count of commands in the SecondaryCommandBuffer.
+    // See https://issuetracker.google.com/issues/163854287
+    InvalidatedState invalidatedState;
+
+    if (access == vk::ResourceAccess::Write)
+    {
+        // This handles various scenarios that an app/test can do with valid GLES usage.  For
+        // example, consider an app that invalidates, doesn't disable the functionality, and draws
+        // again.  In that case, the drawing that occurs after the invalidate means that there is
+        // once again valid content in the attachment (i.e. that should not be discarded).  Since
+        // we don't track draws, we must be conservative and assume that a draw may have occured
+        // since invalidation unless the functionality has also been disabled and the re-enabled.
+        invalidatedState = (!mStencilEnabled) ? NoLongerInvalidated : Invalidated;
+        // Keep track of whether stencil functionality is enabled
+        mStencilEnabled = true;
+    }
+    else
+    {
+        invalidatedState = Invalidated;
+        // Keep track of whether stencil functionality is enabled
+        mStencilEnabled = false;
+    }
+
+    // Update the access for optimizing this render pass's loadOp
+    UpdateAccess(&mStencilStartAccess, access);
+    // Update the invalidate state for optimizing this render pass's stencilStoreOp
+    UpdateInvalidatedState(&mStencilInvalidatedState, invalidatedState);
+}
+
 void CommandBufferHelper::executeBarriers(ContextVk *contextVk, PrimaryCommandBuffer *primary)
 {
     // make a local copy for faster access
@@ -832,6 +902,17 @@ void CommandBufferHelper::endRenderPass()
         return;
     }
 
+    // Address invalidated depth/stencil attachments
+    if (mDepthInvalidatedState == Invalidated && !mDepthEnabled)
+    {
+        mAttachmentOps[mDepthStencilAttachmentIndex].storeOp = VK_ATTACHMENT_STORE_OP_DONT_CARE;
+    }
+    if (mStencilInvalidatedState == Invalidated && !mStencilEnabled)
+    {
+        mAttachmentOps[mDepthStencilAttachmentIndex].stencilStoreOp =
+            VK_ATTACHMENT_STORE_OP_DONT_CARE;
+    }
+
     // Depth/Stencil buffer optimization: if we are loading or clearing the buffer, but the
     // buffer has not been used, and the data has also not been stored back into buffer, then
     // just skip the load/clear op.
@@ -1050,6 +1131,10 @@ void CommandBufferHelper::reset()
         mRebindTransformFeedbackBuffers    = false;
         mDepthStartAccess                  = ResourceAccess::Unused;
         mStencilStartAccess                = ResourceAccess::Unused;
+        mDepthInvalidatedState             = NeverInvalidated;
+        mDepthEnabled                      = false;
+        mStencilInvalidatedState           = NeverInvalidated;
+        mStencilEnabled                    = false;
         mDepthStencilAttachmentIndex       = kInvalidAttachmentIndex;
         mRenderPassUsedImages.clear();
     }

src/libANGLE/renderer/vulkan/vk_helpers.h

@@ -870,6 +870,27 @@ enum class AliasingMode
     Disallowed,
 };
 
+enum InvalidatedState
+{
+    // The attachment has been invalidated and is currently still invalid.
+    Invalidated,
+    // The attachment was previously invalidated, but has since been used while enabled for
+    // drawing, meaning that it has valid contents (and therefore this render pass should STORE it,
+    // and a future render pass should LOAD it).
+    NoLongerInvalidated,
+    // The attachment has never been invalidated.  Since this is the highest value, it should never
+    // be given to UpdateInvalidatedState().  Instead, it should only be set starting a render pass.
+    NeverInvalidated,
+};
+
+inline void UpdateInvalidatedState(InvalidatedState *oldState, InvalidatedState newState)
+{
+    if (newState > *oldState)
+    {
+        *oldState = newState;
+    }
+}
+
 // CommandBufferHelper (CBH) class wraps ANGLE's custom command buffer
 //  class, SecondaryCommandBuffer. This provides a way to temporarily
 //  store Vulkan commands that be can submitted in-line to a primary
@@ -957,17 +978,29 @@ class CommandBufferHelper : angle::NonCopyable
         SetBitField(mAttachmentOps[attachmentIndex].storeOp, VK_ATTACHMENT_STORE_OP_DONT_CARE);
     }
 
-    void invalidateRenderPassDepthAttachment(size_t attachmentIndex)
+    void invalidateRenderPassDepthAttachment()
     {
         ASSERT(mIsRenderPassCommandBuffer);
-        SetBitField(mAttachmentOps[attachmentIndex].storeOp, VK_ATTACHMENT_STORE_OP_DONT_CARE);
+        mDepthInvalidatedState = Invalidated;
     }
 
-    void invalidateRenderPassStencilAttachment(size_t attachmentIndex)
+    void invalidateRenderPassStencilAttachment()
     {
         ASSERT(mIsRenderPassCommandBuffer);
-        SetBitField(mAttachmentOps[attachmentIndex].stencilStoreOp,
-                    VK_ATTACHMENT_STORE_OP_DONT_CARE);
+        mStencilInvalidatedState = Invalidated;
+    }
+
+    bool shouldRestoreDepthStencilAttachment()
+    {
+        ASSERT(mIsRenderPassCommandBuffer);
+        // Return true when both depth and stencil attachments were previously-invalidated, and at
+        // least one of those attachments are no longer invalidated.  When invalidated,
+        // RenderTargetVk::mContentDefined is set to false, which will result in the loadOp and
+        // stencilLoadOp of a future render pass being set to DONT_CARE.  ContextVk::syncState()
+        // will call this method to determine if RenderTargetVk::mContentDefined should be set back
+        // to true (i.e. use LOAD).
+        return mDepthInvalidatedState == NoLongerInvalidated ||
+               mStencilInvalidatedState == NoLongerInvalidated;
     }
 
     void updateRenderPassAttachmentFinalLayout(size_t attachmentIndex, ImageLayout finalLayout)
@@ -1011,14 +1044,8 @@ class CommandBufferHelper : angle::NonCopyable
     // Dumping the command stream is disabled by default.
     static constexpr bool kEnableCommandStreamDiagnostics = false;
 
-    void onDepthAccess(ResourceAccess access)
-    {
-        UpdateAccess(&mDepthStartAccess, access);
-        ASSERT((mRenderPassDesc.getDepthStencilAccess() != ResourceAccess::ReadOnly) ||
-               mDepthStartAccess != ResourceAccess::Write);
-    }
-
-    void onStencilAccess(ResourceAccess access) { UpdateAccess(&mStencilStartAccess, access); }
+    void onDepthAccess(ResourceAccess access);
+    void onStencilAccess(ResourceAccess access);
 
     void updateRenderPassForResolve(vk::Framebuffer *newFramebuffer,
                                     const vk::RenderPassDesc &renderPassDesc);
@@ -1052,9 +1079,17 @@ class CommandBufferHelper : angle::NonCopyable
 
     bool mIsRenderPassCommandBuffer;
 
+    // State tracking for whether to optimize the loadOp to DONT_CARE
     ResourceAccess mDepthStartAccess;
     ResourceAccess mStencilStartAccess;
 
+    // State tracking for whether to optimize the storeOp to DONT_CARE
+    bool mDepthEnabled;
+    InvalidatedState mDepthInvalidatedState;
+    bool mStencilEnabled;
+    InvalidatedState mStencilInvalidatedState;
+
+    // Keep track of the depth/stencil attachment index
     uint32_t mDepthStencilAttachmentIndex;
 
     // Tracks resources used in the command buffer.

src/tests/gl_tests/VulkanPerformanceCounterTest.cpp

@@ -119,12 +119,12 @@ TEST_P(VulkanPerformanceCounterTest, SampleFromRGBTextureDoesNotBreakRenderPass)
     // First draw with textureRGBA which should start the renderpass
     glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
     glUniform1i(textureLoc, 0);
-    drawQuad(program, std::string(essl1_shaders::PositionAttrib()), 0.5f);
+    drawQuad(program, essl1_shaders::PositionAttrib(), 0.5f);
     ASSERT_GL_NO_ERROR();
 
     // Next draw with textureRGB which should not end the renderpass
     glUniform1i(textureLoc, 1);
-    drawQuad(program, std::string(essl1_shaders::PositionAttrib()), 0.5f);
+    drawQuad(program, essl1_shaders::PositionAttrib(), 0.5f);
     ASSERT_GL_NO_ERROR();
 
     uint32_t actualRenderPassCount = counters.renderPasses;
@@ -405,6 +405,59 @@ TEST_P(VulkanPerformanceCounterTest, ReadOnlyDepthStencilFeedbackLoopUsesSingleR
     ASSERT_GL_NO_ERROR();
 }
 
+// Tests that RGB texture should not break renderpass (similar to PUBG MOBILE).
+TEST_P(VulkanPerformanceCounterTest, InvalidatingAndUsingDepthDoesNotBreakRenderPass)
+{
+    const rx::vk::PerfCounters &counters = hackANGLE();
+
+    ANGLE_GL_PROGRAM(program, essl1_shaders::vs::Simple(), essl1_shaders::fs::Red());
+    glUseProgram(program);
+
+    // Setup to draw to color and depth
+    GLFramebuffer framebuffer;
+    GLTexture texture;
+    GLRenderbuffer renderbuffer;
+    glBindFramebuffer(GL_FRAMEBUFFER, framebuffer);
+    glBindTexture(GL_TEXTURE_2D, texture);
+    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, 16, 16, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr);
+    glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture, 0);
+    glBindRenderbuffer(GL_RENDERBUFFER, renderbuffer);
+    glRenderbufferStorage(GL_RENDERBUFFER, GL_DEPTH24_STENCIL8, 16, 16);
+    glFramebufferRenderbuffer(GL_FRAMEBUFFER, GL_DEPTH_STENCIL_ATTACHMENT, GL_RENDERBUFFER,
+                              renderbuffer);
+    ASSERT_GL_FRAMEBUFFER_COMPLETE(GL_FRAMEBUFFER);
+
+    uint32_t expectedRenderPassCount = counters.renderPasses + 1;
+
+    // First, clear and draw with depth buffer enabled
+    glEnable(GL_DEPTH_TEST);
+    glDepthMask(GL_TRUE);
+    glDepthFunc(GL_GEQUAL);
+    glClearDepthf(0.99f);
+    glEnable(GL_STENCIL_TEST);
+    glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
+    drawQuad(program, essl1_shaders::PositionAttrib(), 0.5f);
+    ASSERT_GL_NO_ERROR();
+
+    // Second, invalidate the depth buffer and draw with depth buffer disabled
+    const GLenum discards[] = {GL_DEPTH_ATTACHMENT, GL_STENCIL_ATTACHMENT};
+    // Note: PUBG uses glDiscardFramebufferEXT() instead of glInvalidateFramebuffer()
+    glDiscardFramebufferEXT(GL_FRAMEBUFFER, 2, discards);
+    ASSERT_GL_NO_ERROR();
+    glDisable(GL_DEPTH_TEST);
+    drawQuad(program, essl1_shaders::PositionAttrib(), 0.5f);
+    ASSERT_GL_NO_ERROR();
+
+    // Third, re-enable the depth buffer and draw again
+    ASSERT_GL_NO_ERROR();
+    glEnable(GL_DEPTH_TEST);
+    drawQuad(program, essl1_shaders::PositionAttrib(), 0.5f);
+    ASSERT_GL_NO_ERROR();
+
+    uint32_t actualRenderPassCount = counters.renderPasses;
+    EXPECT_EQ(expectedRenderPassCount, actualRenderPassCount);
+}
+
 ANGLE_INSTANTIATE_TEST(VulkanPerformanceCounterTest, ES3_VULKAN());
 ANGLE_INSTANTIATE_TEST(VulkanPerformanceCounterTest_ES31, ES31_VULKAN());