Destroy FBOs when RenderTarget node is destroyed

It appears we never destroyed FBOs which lead to bugs
when destroying and recreating a RenderTarget

Change-Id: I507b045d9b9e1088ff49f719c8846cc43c4fc8f2
Reviewed-by: Mike Krus <mike.krus@kdab.com>

8 files changed, 131 insertions, 1 deletions

diff --git a/src/plugins/renderers/opengl/graphicshelpers/submissioncontext.cpp b/src/plugins/renderers/opengl/graphicshelpers/submissioncontext.cpp
index e4b223185..c8d459b01 100644
--- a/src/plugins/renderers/opengl/graphicshelpers/submissioncontext.cpp
+++ b/src/plugins/renderers/opengl/graphicshelpers/submissioncontext.cpp
@@ -500,6 +500,14 @@ void SubmissionContext::activateRenderTarget(Qt3DCore::QNodeId renderTargetNodeI
     activateDrawBuffers(attachments);
 }
 
+void SubmissionContext::releaseRenderTarget(const Qt3DCore::QNodeId id)
+{
+    if (m_renderTargets.contains(id)) {
+        const GLuint fboId = m_renderTargets.take(id);
+        m_glHelper->releaseFrameBufferObject(fboId);
+    }
+}
+
 GLuint SubmissionContext::createRenderTarget(Qt3DCore::QNodeId renderTargetNodeId, const AttachmentPack &attachments)
 {
     const GLuint fboId = m_glHelper->createFrameBufferObject();
diff --git a/src/plugins/renderers/opengl/graphicshelpers/submissioncontext_p.h b/src/plugins/renderers/opengl/graphicshelpers/submissioncontext_p.h
index b663e47ec..5e3a0c04b 100644
--- a/src/plugins/renderers/opengl/graphicshelpers/submissioncontext_p.h
+++ b/src/plugins/renderers/opengl/graphicshelpers/submissioncontext_p.h
@@ -113,6 +113,7 @@ public:
     // FBO
     GLuint activeFBO() const { return m_activeFBO; }
     void activateRenderTarget(const Qt3DCore::QNodeId id, const AttachmentPack &attachments, GLuint defaultFboId);
+    void releaseRenderTarget(const Qt3DCore::QNodeId id);
     QSize renderTargetSize(const QSize &surfaceSize) const;
     QImage readFramebuffer(const QRect &rect);
     void blitFramebuffer(Qt3DCore::QNodeId outputRenderTargetId, Qt3DCore::QNodeId inputRenderTargetId,
diff --git a/src/plugins/renderers/opengl/renderer/renderer.cpp b/src/plugins/renderers/opengl/renderer/renderer.cpp
index af43a4fe5..79eca16fa 100644
--- a/src/plugins/renderers/opengl/renderer/renderer.cpp
+++ b/src/plugins/renderers/opengl/renderer/renderer.cpp
@@ -1434,6 +1434,13 @@ void Renderer::updateGLResources()
 
     // Record list of buffer that might need uploading
     lookForDownloadableBuffers();
+
+    // Remove destroyed FBOs
+    {
+        const QNodeIdVector destroyedRenderTargetIds = m_nodesManager->renderTargetManager()->takeRenderTargetIdsToCleanup();
+        for (const Qt3DCore::QNodeId &renderTargetId : destroyedRenderTargetIds)
+            m_submissionContext->releaseRenderTarget(renderTargetId);
+    }
 }
 
 // Render Thread
diff --git a/src/render/backend/managers_p.h b/src/render/backend/managers_p.h
index d7931c239..a3d42d24a 100644
--- a/src/render/backend/managers_p.h
+++ b/src/render/backend/managers_p.h
@@ -300,6 +300,35 @@ class Q_3DRENDERSHARED_PRIVATE_EXPORT RenderTargetManager : public Qt3DCore::QRe
 {
 public:
     RenderTargetManager() {}
+
+    // Called in AspectThread by RenderTarget node functor destroy
+    void addRenderTargetIdToCleanup(Qt3DCore::QNodeId id)
+    {
+        m_renderTargetIdsToCleanup.push_back(id);
+    }
+
+    // Called in AspectThread by RenderTarget node functor create
+    void removeRenderTargetToCleanup(Qt3DCore::QNodeId id)
+    {
+        m_renderTargetIdsToCleanup.removeAll(id);
+    }
+
+    // Called by RenderThread in updateGLResources (locked)
+    QVector<Qt3DCore::QNodeId> takeRenderTargetIdsToCleanup()
+    {
+        return std::move(m_renderTargetIdsToCleanup);
+    }
+
+#ifdef QT_BUILD_INTERNAL
+    // For unit testing purposes only
+    QVector<Qt3DCore::QNodeId> renderTargetIdsToCleanup() const
+    {
+        return m_renderTargetIdsToCleanup;
+    }
+#endif
+
+private:
+    QVector<Qt3DCore::QNodeId> m_renderTargetIdsToCleanup;
 };
 
 class Q_3DRENDERSHARED_PRIVATE_EXPORT RenderPassManager : public Qt3DCore::QResourceManager<
diff --git a/src/render/backend/rendertarget.cpp b/src/render/backend/rendertarget.cpp
index 088818c94..f78d149db 100644
--- a/src/render/backend/rendertarget.cpp
+++ b/src/render/backend/rendertarget.cpp
@@ -41,6 +41,7 @@
 #include <Qt3DRender/qrendertarget.h>
 #include <Qt3DRender/private/qrendertarget_p.h>
 #include <Qt3DRender/qrendertargetoutput.h>
+#include <Qt3DRender/private/managers_p.h>
 #include <QVariant>
 
 QT_BEGIN_NAMESPACE
@@ -94,6 +95,33 @@ QVector<Qt3DCore::QNodeId> RenderTarget::renderOutputs() const
     return m_renderOutputs;
 }
 
+RenderTargetFunctor::RenderTargetFunctor(AbstractRenderer *renderer, RenderTargetManager *manager)
+    : m_renderer(renderer)
+    , m_renderTargetManager(manager)
+{
+}
+
+QBackendNode *RenderTargetFunctor::create(const QNodeCreatedChangeBasePtr &change) const
+{
+    RenderTarget *backend = m_renderTargetManager->getOrCreateResource(change->subjectId());
+    // Remove from the list of ids to destroy in case we were added to it
+    m_renderTargetManager->removeRenderTargetToCleanup(change->subjectId());
+    backend->setRenderer(m_renderer);
+    return backend;
+}
+
+QBackendNode *RenderTargetFunctor::get(QNodeId id) const
+{
+    return m_renderTargetManager->lookupResource(id);
+}
+
+void RenderTargetFunctor::destroy(QNodeId id) const
+{
+    // We add ourselves to the dirty list to tell the renderer that this rendertarget has been destroyed
+    m_renderTargetManager->addRenderTargetIdToCleanup(id);
+    m_renderTargetManager->releaseResource(id);
+}
+
 } // namespace Render
 } // namespace Qt3DRender
 
diff --git a/src/render/backend/rendertarget_p.h b/src/render/backend/rendertarget_p.h
index eeaf94940..50b6248b2 100644
--- a/src/render/backend/rendertarget_p.h
+++ b/src/render/backend/rendertarget_p.h
@@ -82,6 +82,21 @@ private:
     QVector<Qt3DCore::QNodeId> m_renderOutputs;
 };
 
+class Q_AUTOTEST_EXPORT RenderTargetFunctor : public Qt3DCore::QBackendNodeMapper
+{
+public:
+    explicit RenderTargetFunctor(AbstractRenderer *renderer,
+                                 RenderTargetManager *manager);
+    Qt3DCore::QBackendNode *create(const Qt3DCore::QNodeCreatedChangeBasePtr &change) const final;
+    Qt3DCore::QBackendNode *get(Qt3DCore::QNodeId id) const final;
+    void destroy(Qt3DCore::QNodeId id) const final;
+
+private:
+    AbstractRenderer *m_renderer;
+    RenderTargetManager *m_renderTargetManager;
+};
+
+
 } // namespace Render
 
 } // namespace Qt3DRender
diff --git a/src/render/frontend/qrenderaspect.cpp b/src/render/frontend/qrenderaspect.cpp
index fc366b891..a06032899 100644
--- a/src/render/frontend/qrenderaspect.cpp
+++ b/src/render/frontend/qrenderaspect.cpp
@@ -284,7 +284,7 @@ void QRenderAspectPrivate::registerBackendTypes()
     q->registerBackendType<QLevelOfDetail, true>(QSharedPointer<Render::NodeFunctor<Render::LevelOfDetail, Render::LevelOfDetailManager> >::create(m_renderer));
     q->registerBackendType<QLevelOfDetailSwitch, true>(QSharedPointer<Render::NodeFunctor<Render::LevelOfDetail, Render::LevelOfDetailManager> >::create(m_renderer));
     q->registerBackendType<QSceneLoader, true>(QSharedPointer<Render::RenderSceneFunctor>::create(m_renderer, m_nodeManagers->sceneManager()));
-    q->registerBackendType<QRenderTarget, true>(QSharedPointer<Render::NodeFunctor<Render::RenderTarget, Render::RenderTargetManager> >::create(m_renderer));
+    q->registerBackendType<QRenderTarget, true>(QSharedPointer<Render::RenderTargetFunctor>::create(m_renderer, m_nodeManagers->renderTargetManager()));
     q->registerBackendType<QRenderTargetOutput, true>(QSharedPointer<Render::NodeFunctor<Render::RenderTargetOutput, Render::AttachmentManager> >::create(m_renderer));
     q->registerBackendType<QRenderSettings, true>(QSharedPointer<Render::RenderSettingsFunctor>::create(m_renderer));
     q->registerBackendType<QRenderState, true>(QSharedPointer<Render::NodeFunctor<Render::RenderStateNode, Render::RenderStateManager> >::create(m_renderer));
diff --git a/tests/auto/render/rendertarget/tst_rendertarget.cpp b/tests/auto/render/rendertarget/tst_rendertarget.cpp
index 31eee6ec5..9b436aa5b 100644
--- a/tests/auto/render/rendertarget/tst_rendertarget.cpp
+++ b/tests/auto/render/rendertarget/tst_rendertarget.cpp
@@ -32,6 +32,7 @@
 #include <Qt3DRender/qrendertargetoutput.h>
 #include <Qt3DRender/private/qrendertarget_p.h>
 #include <Qt3DRender/private/rendertarget_p.h>
+#include <Qt3DRender/private/managers_p.h>
 #include "qbackendnodetester.h"
 #include "testrenderer.h"
 
@@ -205,6 +206,47 @@ private Q_SLOTS:
         }
     }
 
+    void checkRenderTargetManager()
+    {
+        // GIVEN
+        Qt3DRender::QRenderTarget renderTarget;
+        TestRenderer renderer;
+        Qt3DRender::Render::RenderTargetManager manager;
+        Qt3DRender::Render::RenderTargetFunctor creationFunctor(&renderer, &manager);
+
+        // THEN
+        QVERIFY(manager.renderTargetIdsToCleanup().isEmpty());
+
+        // WHEN
+        Qt3DCore::QNodeCreatedChangeBase changeObj(&renderTarget);
+        Qt3DCore::QNodeCreatedChangeBasePtr changePtr(&changeObj, [](Qt3DCore::QNodeCreatedChangeBase *) {});
+        auto backend = creationFunctor.create(changePtr);
+
+        // THEN
+        QVERIFY(backend != nullptr);
+        QVERIFY(manager.renderTargetIdsToCleanup().isEmpty());
+
+        {
+            // WHEN
+            auto sameBackend = creationFunctor.get(renderTarget.id());
+            // THEN
+            QCOMPARE(backend, sameBackend);
+        }
+
+        // WHEN
+        creationFunctor.destroy(renderTarget.id());
+
+        // THEN -> Should be in list of ids to remove and return null on get
+        QVERIFY(manager.renderTargetIdsToCleanup().contains(renderTarget.id()));
+        QVERIFY(creationFunctor.get(renderTarget.id()) == nullptr);
+
+        // WHEN -> Should be removed from list of ids to remove
+        creationFunctor.create(changePtr);
+
+        // THEN
+        QVERIFY(manager.renderTargetIdsToCleanup().isEmpty());
+        QVERIFY(creationFunctor.get(renderTarget.id()) != nullptr);
+    }
 };
 
 QTEST_MAIN(tst_RenderTarget)