Metal: Fix FramebufferMtl's read-after-delete

src/libANGLE/renderer/metal/ContextMtl.h

@@ -363,6 +363,12 @@ class ContextMtl : public ContextImpl, public mtl::Context
     VertexArrayMtl *mVertexArray     = nullptr;
     ProgramMtl *mProgram             = nullptr;
 
+    // Special flag to indicate current draw framebuffer is default framebuffer.
+    // We need this instead of calling mDrawFramebuffer->getState().isDefault() because
+    // mDrawFramebuffer might point to a deleted object, ContextMtl only knows about this very late,
+    // only during syncState() function call.
+    bool mDrawFramebufferIsDefault = true;
+
     using DirtyBits = angle::BitSet<DIRTY_BIT_MAX>;
 
     gl::AttributesMask mDirtyDefaultAttribsMask;

src/libANGLE/renderer/metal/ContextMtl.mm

@@ -663,7 +663,7 @@ ProgramImpl *ContextMtl::createProgram(const gl::ProgramState &state)
 // Framebuffer creation
 FramebufferImpl *ContextMtl::createFramebuffer(const gl::FramebufferState &state)
 {
-    return new FramebufferMtl(state, false, false);
+    return new FramebufferMtl(state, false);
 }
 
 // Texture creation
@@ -937,9 +937,16 @@ void ContextMtl::present(const gl::Context *context, id<CAMetalDrawable> present
 {
     ensureCommandBufferValid();
 
-    if (hasStartedRenderPass(mDrawFramebuffer))
+    // Always discard default FBO's depth stencil buffers at the end of the frame:
+    if (mDrawFramebufferIsDefault && hasStartedRenderPass(mDrawFramebuffer))
     {
-        mDrawFramebuffer->onFinishedDrawingToFrameBuffer(context, &mRenderEncoder);
+        constexpr GLenum dsAttachments[] = {GL_DEPTH, GL_STENCIL};
+        (void)mDrawFramebuffer->invalidate(context, 2, dsAttachments);
+
+        endEncoding(false);
+
+        // Reset discard flag by notify framebuffer that a new render pass has started.
+        mDrawFramebuffer->onStartedDrawingToFrameBuffer(context);
     }
 
     endEncoding(false);
@@ -1172,14 +1179,10 @@ void ContextMtl::updateDrawFrameBufferBinding(const gl::Context *context)
 {
     const gl::State &glState = getState();
 
-    auto oldFrameBuffer = mDrawFramebuffer;
-
-    mDrawFramebuffer = mtl::GetImpl(glState.getDrawFramebuffer());
+    mDrawFramebuffer          = mtl::GetImpl(glState.getDrawFramebuffer());
+    mDrawFramebufferIsDefault = mDrawFramebuffer->getState().isDefault();
 
-    if (oldFrameBuffer && hasStartedRenderPass(oldFrameBuffer->getRenderPassDesc(this)))
-    {
-        oldFrameBuffer->onFinishedDrawingToFrameBuffer(context, &mRenderEncoder);
-    }
+    mDrawFramebuffer->onStartedDrawingToFrameBuffer(context);
 
     onDrawFrameBufferChange(context, mDrawFramebuffer);
 }

src/libANGLE/renderer/metal/FrameBufferMtl.h

@@ -24,9 +24,7 @@ class SurfaceMtl;
 class FramebufferMtl : public FramebufferImpl
 {
   public:
-    explicit FramebufferMtl(const gl::FramebufferState &state,
-                            bool flipY,
-                            bool alwaysDiscardDepthStencil);
+    explicit FramebufferMtl(const gl::FramebufferState &state, bool flipY);
     ~FramebufferMtl() override;
     void destroy(const gl::Context *context) override;
 
@@ -91,9 +89,8 @@ class FramebufferMtl : public FramebufferImpl
 
     const mtl::RenderPassDesc &getRenderPassDesc(ContextMtl *context);
 
-    // Call this to notify FramebufferMtl whenever its render pass has ended.
-    void onFinishedDrawingToFrameBuffer(const gl::Context *context,
-                                        mtl::RenderCommandEncoder *encoder);
+    // Call this to notify FramebufferMtl whenever its render pass has started.
+    void onStartedDrawingToFrameBuffer(const gl::Context *context);
 
     // The actual area will be adjusted based on framebuffer flipping property.
     gl::Rectangle getReadPixelArea(const gl::Rectangle &glArea);
@@ -140,13 +137,9 @@ class FramebufferMtl : public FramebufferImpl
     // depth & stencil attachments as of now. Separate depth & stencil could be useful to
     // save spaces on iOS devices. See doc/PackedDepthStencilSupport.md.
     std::array<RenderTargetMtl *, mtl::kMaxRenderTargets> mColorRenderTargets;
-    std::array<bool, mtl::kMaxRenderTargets> mDiscardColors;
     RenderTargetMtl *mDepthRenderTarget   = nullptr;
-    bool mDiscardDepth                    = false;
     RenderTargetMtl *mStencilRenderTarget = nullptr;
-    bool mDiscardStencil                  = false;
     mtl::RenderPassDesc mRenderPassDesc;
-    const bool mAlwaysDiscardDepthStencil;
     const bool mFlipY = false;
 };
 }  // namespace rx

src/libANGLE/renderer/metal/FrameBufferMtl.mm

@@ -45,8 +45,8 @@ const gl::InternalFormat &GetReadAttachmentInfo(const gl::Context *context,
 }
 
 // FramebufferMtl implementation
-FramebufferMtl::FramebufferMtl(const gl::FramebufferState &state, bool flipY, bool alwaysDiscard)
-    : FramebufferImpl(state), mAlwaysDiscardDepthStencil(alwaysDiscard), mFlipY(flipY)
+FramebufferMtl::FramebufferMtl(const gl::FramebufferState &state, bool flipY)
+    : FramebufferImpl(state), mFlipY(flipY)
 {
     reset();
 }
@@ -59,12 +59,7 @@ void FramebufferMtl::reset()
     {
         rt = nullptr;
     }
-    for (auto &discardColor : mDiscardColors)
-    {
-        discardColor = false;
-    }
     mDepthRenderTarget = mStencilRenderTarget = nullptr;
-    mDiscardDepth = mDiscardStencil = false;
 }
 
 void FramebufferMtl::destroy(const gl::Context *context)
@@ -289,7 +284,7 @@ angle::Result FramebufferMtl::syncState(const gl::Context *context,
                 {
                     ASSERT(dirtyBit >= gl::Framebuffer::DIRTY_BIT_COLOR_BUFFER_CONTENTS_0 &&
                            dirtyBit < gl::Framebuffer::DIRTY_BIT_COLOR_BUFFER_CONTENTS_MAX);
-                    // NOTE(hqle): What are we supposed to do?
+                    // NOTE: might need to notify context.
                 }
                 break;
             }
@@ -340,48 +335,66 @@ const mtl::RenderPassDesc &FramebufferMtl::getRenderPassDesc(ContextMtl *context
     return mRenderPassDesc;
 }
 
-void FramebufferMtl::onFinishedDrawingToFrameBuffer(const gl::Context *context,
-                                                    mtl::RenderCommandEncoder *encoder)
+void FramebufferMtl::onStartedDrawingToFrameBuffer(const gl::Context *context)
 {
-    if (!encoder->valid())
+    // Compute loadOp based on previous storeOp and reset storeOp flags:
+    for (mtl::RenderPassColorAttachmentDesc &colorAttachment : mRenderPassDesc.colorAttachments)
     {
-        return;
-    }
-
-    ContextMtl *contextMtl = mtl::GetImpl(context);
-
-    ASSERT(encoder->renderPassDesc().equalIgnoreLoadStoreOptions(mRenderPassDesc));
-
-    for (uint32_t i = 0; i < mRenderPassDesc.numColorAttachments; ++i)
-    {
-        if (mDiscardColors[i])
+        if (colorAttachment.storeAction == MTLStoreActionDontCare)
+        {
+            // If we previously discarded attachment's content, then don't need to load it.
+            colorAttachment.loadAction = MTLLoadActionDontCare;
+        }
+        else
         {
-            encoder->setColorStoreAction(MTLStoreActionDontCare, i);
+            colorAttachment.loadAction = MTLLoadActionLoad;
         }
+        colorAttachment.storeAction = MTLStoreActionStore;  // Default action is store
     }
-    encoder->setDepthStencilStoreAction(
-        (mDiscardDepth || mAlwaysDiscardDepthStencil) ? MTLStoreActionDontCare
-                                                      : MTLStoreActionStore,
-        (mDiscardStencil || mAlwaysDiscardDepthStencil) ? MTLStoreActionDontCare
-                                                        : MTLStoreActionStore);
+    // Depth load/store
+    if (mRenderPassDesc.depthAttachment.storeAction == MTLStoreActionDontCare)
+    {
+        mRenderPassDesc.depthAttachment.loadAction = MTLLoadActionDontCare;
+    }
+    else
+    {
+        mRenderPassDesc.depthAttachment.loadAction = MTLLoadActionLoad;
+    }
+    mRenderPassDesc.depthAttachment.storeAction = MTLStoreActionStore;
 
-    contextMtl->endEncoding(encoder);
+    // Stencil load/store
+    if (mRenderPassDesc.stencilAttachment.storeAction == MTLStoreActionDontCare)
+    {
+        mRenderPassDesc.stencilAttachment.loadAction = MTLLoadActionDontCare;
+    }
+    else
+    {
+        mRenderPassDesc.stencilAttachment.loadAction = MTLLoadActionLoad;
+    }
+    mRenderPassDesc.stencilAttachment.storeAction = MTLStoreActionStore;
 }
 
 angle::Result FramebufferMtl::updateColorRenderTarget(const gl::Context *context,
                                                       size_t colorIndexGL)
 {
+    ASSERT(colorIndexGL < mtl::kMaxRenderTargets);
+    // Reset load store action
+    mRenderPassDesc.colorAttachments[colorIndexGL].reset();
     return updateCachedRenderTarget(context, mState.getColorAttachment(colorIndexGL),
                                     &mColorRenderTargets[colorIndexGL]);
 }
 
 angle::Result FramebufferMtl::updateDepthRenderTarget(const gl::Context *context)
 {
+    // Reset load store action
+    mRenderPassDesc.depthAttachment.reset();
     return updateCachedRenderTarget(context, mState.getDepthAttachment(), &mDepthRenderTarget);
 }
 
 angle::Result FramebufferMtl::updateStencilRenderTarget(const gl::Context *context)
 {
+    // Reset load store action
+    mRenderPassDesc.stencilAttachment.reset();
     return updateCachedRenderTarget(context, mState.getStencilAttachment(), &mStencilRenderTarget);
 }
 
@@ -420,19 +433,16 @@ angle::Result FramebufferMtl::prepareRenderPass(const gl::Context *context,
 
         mtl::RenderPassColorAttachmentDesc &colorAttachment =
             desc.colorAttachments[attachmentIdx++];
-        colorAttachment.reset();
         colorRenderTarget->toRenderPassAttachmentDesc(&colorAttachment);
     }
 
     if (mDepthRenderTarget)
     {
-        desc.depthAttachment.reset();
         mDepthRenderTarget->toRenderPassAttachmentDesc(&desc.depthAttachment);
     }
 
     if (mStencilRenderTarget)
     {
-        desc.stencilAttachment.reset();
         mStencilRenderTarget->toRenderPassAttachmentDesc(&desc.stencilAttachment);
     }
 
@@ -475,30 +485,34 @@ angle::Result FramebufferMtl::clearWithLoadOp(const gl::Context *context,
         {
             if (startedRenderPass)
             {
+                // Render pass already started, and we want to clear this buffer,
+                // then discard its content before clearing.
                 encoder->setColorStoreAction(MTLStoreActionDontCare, attachmentIdx);
             }
             colorAttachment.loadAction = MTLLoadActionClear;
             overrideClearColor(texture, clearOpts.clearColor.value(), &colorAttachment.clearColor);
         }
-        else
+        else if (startedRenderPass)
         {
-            if (startedRenderPass)
-            {
-                encoder->setColorStoreAction(MTLStoreActionStore, attachmentIdx);
-            }
+            // If render pass already started and we don't want to clear this buffer,
+            // then store it with current render encoder and load it before clearing step
+            encoder->setColorStoreAction(MTLStoreActionStore, attachmentIdx);
             colorAttachment.loadAction = MTLLoadActionLoad;
         }
     }
 
-    MTLStoreAction preClearDethpStoreAction, preClearStencilStoreAction;
+    MTLStoreAction preClearDethpStoreAction   = MTLStoreActionStore,
+                   preClearStencilStoreAction = MTLStoreActionStore;
     if (clearOpts.clearDepth.valid())
     {
         preClearDethpStoreAction            = MTLStoreActionDontCare;
         tempDesc.depthAttachment.loadAction = MTLLoadActionClear;
         tempDesc.depthAttachment.clearDepth = clearOpts.clearDepth.value();
     }
-    else
+    else if (startedRenderPass)
     {
+        // If render pass already started and we don't want to clear this buffer,
+        // then store it with current render encoder and load it before clearing step
         preClearDethpStoreAction            = MTLStoreActionStore;
         tempDesc.depthAttachment.loadAction = MTLLoadActionLoad;
     }
@@ -509,13 +523,15 @@ angle::Result FramebufferMtl::clearWithLoadOp(const gl::Context *context,
         tempDesc.stencilAttachment.loadAction   = MTLLoadActionClear;
         tempDesc.stencilAttachment.clearStencil = clearOpts.clearStencil.value();
     }
-    else
+    else if (startedRenderPass)
     {
+        // If render pass already started and we don't want to clear this buffer,
+        // then store it with current render encoder and load it before clearing step
         preClearStencilStoreAction            = MTLStoreActionStore;
         tempDesc.stencilAttachment.loadAction = MTLLoadActionLoad;
     }
 
-    // End current render pass.
+    // End current render encoder.
     if (startedRenderPass)
     {
         encoder->setDepthStencilStoreAction(preClearDethpStoreAction, preClearStencilStoreAction);
@@ -534,30 +550,9 @@ angle::Result FramebufferMtl::clearWithDraw(const gl::Context *context,
 {
     ContextMtl *contextMtl = mtl::GetImpl(context);
     DisplayMtl *display    = contextMtl->getDisplay();
-    mtl::RenderPassDesc rpDesc;
-    ANGLE_TRY(prepareRenderPass(context, mState.getEnabledDrawBuffers(), &rpDesc));
-
-    for (uint32_t i = 0, attachmentIdx = 0; i < rpDesc.numColorAttachments; ++i, ++attachmentIdx)
-    {
-        mtl::RenderPassColorAttachmentDesc &colorAttachment =
-            rpDesc.colorAttachments[attachmentIdx];
-
-        // Need to preserve previous content, since we only clear a portion of the framebuffer
-        colorAttachment.loadAction = MTLLoadActionLoad;
-    }
-
-    if (rpDesc.depthAttachment.texture)
-    {
-        rpDesc.depthAttachment.loadAction = MTLLoadActionLoad;
-    }
 
-    if (rpDesc.stencilAttachment.texture)
-    {
-        rpDesc.stencilAttachment.loadAction = MTLLoadActionLoad;
-    }
-
-    // Start new render encoder with loadOp=Load
-    mtl::RenderCommandEncoder *encoder = contextMtl->getRenderCommandEncoder(rpDesc);
+    // Start new render encoder if not already.
+    mtl::RenderCommandEncoder *encoder = contextMtl->getRenderCommandEncoder(mRenderPassDesc);
 
     display->getUtils().clearWithDraw(context, encoder, clearOpts);
 
@@ -646,24 +641,41 @@ angle::Result FramebufferMtl::invalidateImpl(ContextMtl *contextMtl,
     }
 
     // Set the appropriate storeOp for attachments.
-    size_t attachmentIdx = 0;
-    for (size_t colorIndexGL : mState.getEnabledDrawBuffers())
+    // If we already start the render pass, then need to set the store action now.
+    bool renderPassStarted = contextMtl->hasStartedRenderPass(mRenderPassDesc);
+    mtl::RenderCommandEncoder *encoder =
+        renderPassStarted ? contextMtl->getRenderCommandEncoder() : nullptr;
+
+    for (uint32_t i = 0; i < mRenderPassDesc.numColorAttachments; ++i)
     {
-        if (mColorRenderTargets[colorIndexGL] && invalidateColorBuffers.test(colorIndexGL))
+        if (invalidateColorBuffers.test(i))
         {
-            mDiscardColors[attachmentIdx] = true;
+            mtl::RenderPassColorAttachmentDesc &colorAttachment =
+                mRenderPassDesc.colorAttachments[i];
+            colorAttachment.storeAction = MTLStoreActionDontCare;
+            if (renderPassStarted)
+            {
+                encoder->setColorStoreAction(MTLStoreActionDontCare, i);
+            }
         }
-        ++attachmentIdx;
     }
 
     if (invalidateDepthBuffer && mDepthRenderTarget)
     {
-        mDiscardDepth = true;
+        mRenderPassDesc.depthAttachment.storeAction = MTLStoreActionDontCare;
+        if (renderPassStarted)
+        {
+            encoder->setDepthStoreAction(MTLStoreActionDontCare);
+        }
     }
 
     if (invalidateStencilBuffer && mStencilRenderTarget)
     {
-        mDiscardStencil = true;
+        mRenderPassDesc.stencilAttachment.storeAction = MTLStoreActionDontCare;
+        if (renderPassStarted)
+        {
+            encoder->setStencilStoreAction(MTLStoreActionDontCare);
+        }
     }
 
     return angle::Result::Continue;

src/libANGLE/renderer/metal/SurfaceMtl.mm

@@ -274,7 +274,7 @@ egl::Error SurfaceMtl::initialize(const egl::Display *display)
 FramebufferImpl *SurfaceMtl::createDefaultFramebuffer(const gl::Context *context,
                                                       const gl::FramebufferState &state)
 {
-    auto fbo = new FramebufferMtl(state, /* flipY */ true, /* alwaysDiscard */ true);
+    auto fbo = new FramebufferMtl(state, /* flipY */ true);
 
     return fbo;
 }

src/libANGLE/renderer/metal/mtl_command_buffer.h

@@ -236,6 +236,8 @@ class RenderCommandEncoder final : public CommandEncoder
 
     RenderCommandEncoder &setDepthStencilStoreAction(MTLStoreAction depthStoreAction,
                                                      MTLStoreAction stencilStoreAction);
+    RenderCommandEncoder &setDepthStoreAction(MTLStoreAction action);
+    RenderCommandEncoder &setStencilStoreAction(MTLStoreAction action);
 
     const RenderPassDesc &renderPassDesc() const { return mRenderPassDesc; }
 

src/libANGLE/renderer/metal/mtl_command_buffer.mm

@@ -753,6 +753,22 @@ RenderCommandEncoder &RenderCommandEncoder::setDepthStencilStoreAction(
     return *this;
 }
 
+RenderCommandEncoder &RenderCommandEncoder::setDepthStoreAction(MTLStoreAction action)
+{
+    // We only store the options, will defer the actual setting until the encoder finishes
+    mRenderPassDesc.depthAttachment.storeAction = action;
+
+    return *this;
+}
+
+RenderCommandEncoder &RenderCommandEncoder::setStencilStoreAction(MTLStoreAction action)
+{
+    // We only store the options, will defer the actual setting until the encoder finishes
+    mRenderPassDesc.stencilAttachment.storeAction = action;
+
+    return *this;
+}
+
 // BlitCommandEncoder
 BlitCommandEncoder::BlitCommandEncoder(CommandBuffer *cmdBuffer) : CommandEncoder(cmdBuffer, BLIT)
 {}

src/tests/gl_tests/DiscardFramebufferEXTTest.cpp

@@ -26,7 +26,10 @@ class DiscardFramebufferEXTTest : public ANGLETest
 
 TEST_P(DiscardFramebufferEXTTest, DefaultFramebuffer)
 {
-    ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("EXT_discard_framebuffer"));
+    ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_discard_framebuffer"));
+
+    // TODO: fix crash issue. http://anglebug.com/4141
+    ANGLE_SKIP_TEST_IF(IsD3D11());
 
     // These should succeed on the default framebuffer
     const GLenum discards1[] = {GL_COLOR_EXT};
@@ -61,7 +64,7 @@ TEST_P(DiscardFramebufferEXTTest, DefaultFramebuffer)
 
 TEST_P(DiscardFramebufferEXTTest, NonDefaultFramebuffer)
 {
-    ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("EXT_discard_framebuffer"));
+    ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_discard_framebuffer"));
 
     GLuint tex2D;
     GLuint framebuffer;