From b514b2e9960a853c8fbe8aaeab5896c42ac5fb5a Mon Sep 17 00:00:00 2001
From: Pixelflinger <pixelflinger@gmail.com>
Date: Thu, 30 Apr 2020 14:03:39 -0700
Subject: [PATCH] GL backend: Fix an issue with discardEnd flags

In OpenGL with use glInvalidateFramebuffer() for hinting controlling
the load/store of tiles. However, glInvalidateFramebuffer() conceptually
destroys the content of the framebuffer attachment.

This causes a problem when we want for instance to use a buffer for
reading only (e.g. a depth buffer) in multiple passes. In such
scenario, the attachment will be marked as "discard" (which is a
misnomer for storeOp==DONT_CARE in vulkan parlance), without the
intention of making the buffer invalid.

We fix this by ignoring the discardEnd flags entirely if a buffer
has not been written. In this case, we relying on the driver to not
write the tiles out -- but we don't have any other way to express
this in GL.

This issue cannot be encountered currently because the framegraph
is not aggressive enough setting the discardEnd flags.
---
 filament/backend/src/opengl/OpenGLDriver.cpp | 13 ++++++++++++-
 filament/backend/src/opengl/OpenGLDriver.h   |  4 ++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/filament/backend/src/opengl/OpenGLDriver.cpp b/filament/backend/src/opengl/OpenGLDriver.cpp
index f6d7493f46..28a82404f0 100644
--- a/filament/backend/src/opengl/OpenGLDriver.cpp
+++ b/filament/backend/src/opengl/OpenGLDriver.cpp
@@ -2092,6 +2092,10 @@ void OpenGLDriver::beginRenderPass(Handle<HwRenderTarget> rth,
                 params.clearColor, params.clearDepth, params.clearStencil);
     }
 
+    // we need to reset those after we call clearWithRasterPipe()
+    mRenderPassColorWrite = any(clearFlags & TargetBufferFlags::COLOR_ALL);
+    mRenderPassDepthWrite = any(clearFlags & TargetBufferFlags::DEPTH);
+
     gl.viewport(params.viewport.left, params.viewport.bottom,
             params.viewport.width, params.viewport.height);
 
@@ -2112,11 +2116,18 @@ void OpenGLDriver::endRenderPass(int) {
 
     GLRenderTarget const* const rt = handle_cast<GLRenderTarget*>(mRenderPassTarget);
 
-    const TargetBufferFlags discardFlags = mRenderPassParams.flags.discardEnd & rt->targets;
+    TargetBufferFlags discardFlags = mRenderPassParams.flags.discardEnd & rt->targets;
     if (rt->gl.fbo_read) {
         resolvePass(ResolveAction::STORE, rt, discardFlags);
     }
 
+    if (!mRenderPassColorWrite) {
+        discardFlags &= ~TargetBufferFlags::COLOR_ALL;
+    }
+    if (!mRenderPassDepthWrite) {
+        discardFlags &= ~TargetBufferFlags::DEPTH;
+    }
+
     // glInvalidateFramebuffer appeared on GLES 3.0 and GL4.3, for simplicity we just
     // ignore it on GL (rather than having to do a runtime check).
     if (GLES30_HEADERS) {
diff --git a/filament/backend/src/opengl/OpenGLDriver.h b/filament/backend/src/opengl/OpenGLDriver.h
index 34e950950f..cae46bddc8 100644
--- a/filament/backend/src/opengl/OpenGLDriver.h
+++ b/filament/backend/src/opengl/OpenGLDriver.h
@@ -316,6 +316,8 @@ private:
 
     void setRasterStateSlow(backend::RasterState rs) noexcept;
     void setRasterState(backend::RasterState rs) noexcept {
+        mRenderPassColorWrite |= rs.colorWrite;
+        mRenderPassDepthWrite |= rs.depthWrite;
         if (UTILS_UNLIKELY(rs != mRasterState)) {
             setRasterStateSlow(rs);
         }
@@ -374,6 +376,8 @@ private:
     // state required to represent the current render pass
     backend::Handle<backend::HwRenderTarget> mRenderPassTarget;
     backend::RenderPassParams mRenderPassParams;
+    GLboolean mRenderPassColorWrite{};
+    GLboolean mRenderPassDepthWrite{};
 
     void clearWithRasterPipe(backend::TargetBufferFlags clearFlags,
             math::float4 const& linearColor, GLfloat depth, GLint stencil) noexcept;