Fix and improve FBO handling

FBO need to be rebuild when one of the attachments directly or indirectly
changes. By direct change we mean one of the FBO attachment texture being
resized or the list of attachments changing. By indirect we mean when
texture resource is recreated internally by the engine.

Failure to handle this cases resulted in FBO referencing invalid
attachments.

Change-Id: I8dd4c08e464eed7fb0eeefd61a4158304ab4245f
Task-number: QTBUG-64757
Reviewed-by: Mike Krus <mike.krus@kdab.com>

5 files changed, 99 insertions, 31 deletions

diff --git a/src/render/backend/attachmentpack.cpp b/src/render/backend/attachmentpack.cpp
index a2ac8c30c..0cd5d8673 100644
--- a/src/render/backend/attachmentpack.cpp
+++ b/src/render/backend/attachmentpack.cpp
@@ -88,6 +88,32 @@ int AttachmentPack::getDrawBufferIndex(QRenderTargetOutput::AttachmentPoint atta
     return -1;
 }
 
+bool operator ==(const Attachment &a, const Attachment &b)
+{
+    return (a.m_name == b.m_name &&
+            a.m_mipLevel == b.m_mipLevel &&
+            a.m_layer == b.m_layer &&
+            a.m_textureUuid == b.m_textureUuid &&
+            a.m_point == b.m_point &&
+            a.m_face == b.m_face);
+}
+
+bool operator !=(const Attachment &a, const Attachment &b)
+{
+    return !(a == b);
+}
+
+bool operator ==(const AttachmentPack &packA, const AttachmentPack &packB)
+{
+    return (packA.attachments() == packB.attachments() &&
+            packA.getGlDrawBuffers() == packB.getGlDrawBuffers());
+}
+
+bool operator !=(const AttachmentPack &packA, const AttachmentPack &packB)
+{
+    return !(packA == packB);
+}
+
 } // namespace Render
 } // namespace Qt3DRender
 
diff --git a/src/render/backend/attachmentpack_p.h b/src/render/backend/attachmentpack_p.h
index a3a2586dd..d0b65589f 100644
--- a/src/render/backend/attachmentpack_p.h
+++ b/src/render/backend/attachmentpack_p.h
@@ -99,6 +99,12 @@ private:
     QVector<int> m_drawBuffers;
 };
 
+bool operator ==(const Attachment &a, const Attachment &b);
+bool operator !=(const Attachment &a, const Attachment &b);
+
+bool operator ==(const AttachmentPack &packA, const AttachmentPack &packB);
+bool operator !=(const AttachmentPack &packA, const AttachmentPack &packB);
+
 } // namespace Render
 } // namespace Qt3DRender
 
diff --git a/src/render/renderers/opengl/graphicshelpers/submissioncontext.cpp b/src/render/renderers/opengl/graphicshelpers/submissioncontext.cpp
index 345f7a6d0..5453dd98d 100644
--- a/src/render/renderers/opengl/graphicshelpers/submissioncontext.cpp
+++ b/src/render/renderers/opengl/graphicshelpers/submissioncontext.cpp
@@ -478,9 +478,8 @@ void SubmissionContext::activateRenderTarget(Qt3DCore::QNodeId renderTargetNodeI
         // New RenderTarget
         if (!m_renderTargets.contains(renderTargetNodeId)) {
             if (m_defaultFBO && fboId == m_defaultFBO) {
-                // this is the default fbo that some platforms create (iOS), we just register it
-                // Insert FBO into hash
-                m_renderTargets.insert(renderTargetNodeId, fboId);
+                // this is the default fbo that some platforms create (iOS), we never
+                // register it
             } else {
                 fboId = createRenderTarget(renderTargetNodeId, attachments);
             }
@@ -489,6 +488,7 @@ void SubmissionContext::activateRenderTarget(Qt3DCore::QNodeId renderTargetNodeI
         }
     }
     m_activeFBO = fboId;
+    m_activeFBONodeId = renderTargetNodeId;
     m_glHelper->bindFrameBufferObject(m_activeFBO, GraphicsHelperInterface::FBODraw);
     // Set active drawBuffers
     activateDrawBuffers(attachments);
@@ -497,7 +497,8 @@ void SubmissionContext::activateRenderTarget(Qt3DCore::QNodeId renderTargetNodeI
 void SubmissionContext::releaseRenderTarget(const Qt3DCore::QNodeId id)
 {
     if (m_renderTargets.contains(id)) {
-        const GLuint fboId = m_renderTargets.take(id);
+        const RenderTargetInfo targetInfo = m_renderTargets.take(id);
+        const GLuint fboId = targetInfo.fboId;
         m_glHelper->releaseFrameBufferObject(fboId);
     }
 }
@@ -507,11 +508,11 @@ GLuint SubmissionContext::createRenderTarget(Qt3DCore::QNodeId renderTargetNodeI
     const GLuint fboId = m_glHelper->createFrameBufferObject();
     if (fboId) {
         // The FBO is created and its attachments are set once
-        // Insert FBO into hash
-        m_renderTargets.insert(renderTargetNodeId, fboId);
         // Bind FBO
         m_glHelper->bindFrameBufferObject(fboId, GraphicsHelperInterface::FBODraw);
-        bindFrameBufferAttachmentHelper(fboId, attachments);
+        // Insert FBO into hash
+        const RenderTargetInfo info = bindFrameBufferAttachmentHelper(fboId, attachments);
+        m_renderTargets.insert(renderTargetNodeId, info);
     } else {
         qCritical("Failed to create FBO");
     }
@@ -520,31 +521,38 @@ GLuint SubmissionContext::createRenderTarget(Qt3DCore::QNodeId renderTargetNodeI
 
 GLuint SubmissionContext::updateRenderTarget(Qt3DCore::QNodeId renderTargetNodeId, const AttachmentPack &attachments, bool isActiveRenderTarget)
 {
-    const GLuint fboId = m_renderTargets.value(renderTargetNodeId);
+    const RenderTargetInfo fboInfo = m_renderTargets.value(renderTargetNodeId);
+    const GLuint fboId =fboInfo.fboId;
 
-    // We need to check if  one of the attachment was resized
-    bool needsResize = !m_renderTargetsSize.contains(fboId);    // not even initialized yet?
-    if (!needsResize) {
+    // We need to check if  one of the attachnent have changed QTBUG-64757
+    bool needsRebuild = attachments != fboInfo.attachments;
+
+    // Even if the attachment packs are the same, one of the inner texture might have
+    // been resized or recreated, we need to check for that
+    if (!needsRebuild) {
         // render target exists, has attachment been resized?
         GLTextureManager *glTextureManager = m_renderer->nodeManagers()->glTextureManager();
-        const QSize s = m_renderTargetsSize[fboId];
+        const QSize s = fboInfo.size;
+
         const auto attachments_ = attachments.attachments();
         for (const Attachment &attachment : attachments_) {
+            const bool textureWasUpdated = m_updateTextureIds.contains(attachment.m_textureUuid);
             GLTexture *rTex = glTextureManager->lookupResource(attachment.m_textureUuid);
-            // ### TODO QTBUG-64757 this check is insufficient since the
-            // texture may have changed to another one with the same size. That
-            // case is not handled atm.
-            needsResize |= (rTex != nullptr && rTex->size() != s);
-            if (isActiveRenderTarget) {
-                if (attachment.m_point == QRenderTargetOutput::Color0)
+            if (rTex) {
+                const bool sizeHasChanged = rTex->size() != s;
+                needsRebuild |= sizeHasChanged;
+                if (isActiveRenderTarget && attachment.m_point == QRenderTargetOutput::Color0)
                     m_renderTargetFormat = rTex->properties().format;
             }
+            needsRebuild |= textureWasUpdated;
         }
     }
 
-    if (needsResize) {
+    if (needsRebuild) {
         m_glHelper->bindFrameBufferObject(fboId, GraphicsHelperInterface::FBODraw);
-        bindFrameBufferAttachmentHelper(fboId, attachments);
+        const RenderTargetInfo updatedInfo = bindFrameBufferAttachmentHelper(fboId, attachments);
+        // Update our stored Render Target Info
+        m_renderTargets.insert(renderTargetNodeId, updatedInfo);
     }
 
     return fboId;
@@ -555,8 +563,8 @@ QSize SubmissionContext::renderTargetSize(const QSize &surfaceSize) const
     QSize renderTargetSize;
     if (m_activeFBO != m_defaultFBO) {
         // For external FBOs we may not have a m_renderTargets entry.
-        if (m_renderTargetsSize.contains(m_activeFBO)) {
-            renderTargetSize = m_renderTargetsSize[m_activeFBO];
+        if (m_renderTargets.contains(m_activeFBONodeId)) {
+            renderTargetSize = m_renderTargets[m_activeFBONodeId].size;
         } else if (surfaceSize.isValid()) {
             renderTargetSize = surfaceSize;
         } else {
@@ -801,7 +809,7 @@ bool SubmissionContext::activateShader(ProgramDNA shaderDNA)
     return true;
 }
 
-void SubmissionContext::bindFrameBufferAttachmentHelper(GLuint fboId, const AttachmentPack &attachments)
+SubmissionContext::RenderTargetInfo SubmissionContext::bindFrameBufferAttachmentHelper(GLuint fboId, const AttachmentPack &attachments)
 {
     // Set FBO attachments. These are normally textures, except that on Open GL
     // ES <= 3.1 we must use a renderbuffer if a combined depth+stencil is
@@ -836,7 +844,7 @@ void SubmissionContext::bindFrameBufferAttachmentHelper(GLuint fboId, const Atta
             }
         }
     }
-    m_renderTargetsSize.insert(fboId, fboSize);
+    return {fboId, fboSize, attachments};
 }
 
 void SubmissionContext::activateDrawBuffers(const AttachmentPack &attachments)
@@ -1096,6 +1104,11 @@ void SubmissionContext::clearStencilValue(int stencil)
     }
 }
 
+void SubmissionContext::setUpdatedTexture(const Qt3DCore::QNodeIdVector &updatedTextureIds)
+{
+    m_updateTextureIds = updatedTextureIds;
+}
+
 // It will be easier if the QGraphicContext applies the QUniformPack
 // than the other way around
 bool SubmissionContext::setParameters(ShaderParameterPack &parameterPack)
@@ -1454,13 +1467,13 @@ void SubmissionContext::blitFramebuffer(Qt3DCore::QNodeId inputRenderTargetId,
 
     // Up until this point the input and output rects are normal Qt rectangles.
     // Convert them to GL rectangles (Y at bottom).
-    const int inputFboHeight = inputFboId == defaultFboId ? m_surfaceSize.height() : m_renderTargetsSize[inputFboId].height();
+    const int inputFboHeight = inputFboId == defaultFboId ? m_surfaceSize.height() : m_renderTargets[inputRenderTargetId].size.height();
     const GLint srcX0 = inputRect.left();
     const GLint srcY0 = inputFboHeight - (inputRect.top() + inputRect.height());
     const GLint srcX1 = srcX0 + inputRect.width();
     const GLint srcY1 = srcY0 + inputRect.height();
 
-    const int outputFboHeight = outputFboId == defaultFboId ? m_surfaceSize.height() : m_renderTargetsSize[outputFboId].height();
+    const int outputFboHeight = outputFboId == defaultFboId ? m_surfaceSize.height() : m_renderTargets[outputRenderTargetId].size.height();
     const GLint dstX0 = outputRect.left();
     const GLint dstY0 = outputFboHeight - (outputRect.top() + outputRect.height());
     const GLint dstX1 = dstX0 + outputRect.width();
diff --git a/src/render/renderers/opengl/graphicshelpers/submissioncontext_p.h b/src/render/renderers/opengl/graphicshelpers/submissioncontext_p.h
index 74238b173..6f2888b93 100644
--- a/src/render/renderers/opengl/graphicshelpers/submissioncontext_p.h
+++ b/src/render/renderers/opengl/graphicshelpers/submissioncontext_p.h
@@ -60,6 +60,7 @@
 #include <Qt3DRender/qattribute.h>
 #include <Qt3DRender/private/handle_types_p.h>
 #include <Qt3DRender/private/shadercache_p.h>
+#include <Qt3DRender/private/attachmentpack_p.h>
 
 QT_BEGIN_NAMESPACE
 
@@ -148,7 +149,16 @@ public:
     void    clearDepthValue(float depth);
     void    clearStencilValue(int stencil);
 
+    // Textures
+    void setUpdatedTexture(const Qt3DCore::QNodeIdVector &updatedTextureIds);
+
 private:
+    struct RenderTargetInfo {
+        GLuint fboId;
+        QSize size;
+        AttachmentPack attachments;
+    };
+
     void initialize();
 
     // Material
@@ -156,7 +166,7 @@ private:
     void setActiveMaterial(Material* rmat);
 
     // FBO
-    void bindFrameBufferAttachmentHelper(GLuint fboId, const AttachmentPack &attachments);
+    RenderTargetInfo bindFrameBufferAttachmentHelper(GLuint fboId, const AttachmentPack &attachments);
     void activateDrawBuffers(const AttachmentPack &attachments);
     void resolveRenderTargetFormat();
     GLuint createRenderTarget(Qt3DCore::QNodeId renderTargetNodeId, const AttachmentPack &attachments);
@@ -177,8 +187,9 @@ private:
     ProgramDNA m_activeShaderDNA;
 
     QHash<Qt3DCore::QNodeId, HGLBuffer> m_renderBufferHash;
-    QHash<Qt3DCore::QNodeId, GLuint> m_renderTargets;
-    QHash<GLuint, QSize> m_renderTargetsSize;
+
+
+    QHash<Qt3DCore::QNodeId, RenderTargetInfo> m_renderTargets;
     QAbstractTexture::TextureFormat m_renderTargetFormat;
 
     // cache some current state, to make sure we don't issue unnecessary GL calls
@@ -189,6 +200,7 @@ private:
     Material* m_material;
     QRectF m_viewport;
     GLuint m_activeFBO;
+    Qt3DCore::QNodeId m_activeFBONodeId;
 
     GLBuffer *m_boundArrayBuffer;
     RenderStateSet* m_stateSet;
@@ -216,6 +228,8 @@ private:
     using VAOIndexAttribute = HGLBuffer;
     void enableAttribute(const VAOVertexAttribute &attr);
     void disableAttribute(const VAOVertexAttribute &attr);
+
+    Qt3DCore::QNodeIdVector m_updateTextureIds;
 };
 
 } // namespace Render
diff --git a/src/render/renderers/opengl/renderer/renderer.cpp b/src/render/renderers/opengl/renderer/renderer.cpp
index 6018aa398..47a7c5e01 100644
--- a/src/render/renderers/opengl/renderer/renderer.cpp
+++ b/src/render/renderers/opengl/renderer/renderer.cpp
@@ -1277,12 +1277,15 @@ void Renderer::updateGLResources()
             if (texture ==  nullptr)
                 continue;
 
-            // Create or Update GLTexture
+            // Create or Update GLTexture (the GLTexture instance is created
+            // (not the underlying GL instance) if required and all things that
+            // can take place without a GL context are done here)
             updateTexture(texture);
         }
         // We want to upload textures data at this point as the SubmissionThread and
         // AspectThread are locked ensuring no races between Texture/TextureImage and
         // GLTexture
+        QNodeIdVector updatedTexturesForFrame;
         if (m_submissionContext != nullptr) {
             GLTextureManager *glTextureManager = m_nodesManager->glTextureManager();
             const QVector<GLTexture *> glTextures = glTextureManager->activeResources();
@@ -1295,10 +1298,16 @@ void Renderer::updateGLResources()
                 // Gather these information and store them to be distributed by a change next frame
                 const QNodeIdVector referenceTextureIds = glTextureManager->referencedTextureIds(glTexture);
                 // Store properties and referenceTextureIds
-                if (info.wasUpdated)
+                if (info.wasUpdated) {
                     m_updatedTextureProperties.push_back({info.properties, referenceTextureIds});
+                    updatedTexturesForFrame += referenceTextureIds;
+                }
             }
         }
+
+        // If the underlying GL Texture was for whatever reason recreated, we need to make sure
+        // that if it is used as a color attachment, we rebuild the FBO next time it is used
+        m_submissionContext->setUpdatedTexture(std::move(updatedTexturesForFrame));
     }
     // When Textures are cleaned up, their id is saved so that they can be
     // cleaned up in the render thread Note: we perform this step in second so