From 294894610b02a2cd4682fafe139a60b2b96f3289 Mon Sep 17 00:00:00 2001 From: Paul Lemire Date: Thu, 13 Feb 2020 11:51:38 +0100 Subject: 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: I99b3df95b821670aa3bbd63209ff9bcc21afbf79 Reviewed-by: Mike Krus --- src/render/backend/managers_p.h | 29 +++++++++++++++ src/render/backend/rendertarget.cpp | 28 +++++++++++++++ src/render/backend/rendertarget_p.h | 15 ++++++++ src/render/frontend/qrenderaspect.cpp | 2 +- .../opengl/graphicshelpers/submissioncontext.cpp | 8 +++++ .../opengl/graphicshelpers/submissioncontext_p.h | 1 + src/render/renderers/opengl/renderer/renderer.cpp | 7 ++++ .../auto/render/rendertarget/tst_rendertarget.cpp | 42 ++++++++++++++++++++++ 8 files changed, 131 insertions(+), 1 deletion(-) diff --git a/src/render/backend/managers_p.h b/src/render/backend/managers_p.h index 759c16f64..b2dec9cb5 100644 --- a/src/render/backend/managers_p.h +++ b/src/render/backend/managers_p.h @@ -282,6 +282,35 @@ class RenderTargetManager : public Qt3DCore::QResourceManager< { 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 takeRenderTargetIdsToCleanup() + { + return std::move(m_renderTargetIdsToCleanup); + } + +#ifdef QT_BUILD_INTERNAL + // For unit testing purposes only + QVector renderTargetIdsToCleanup() const + { + return m_renderTargetIdsToCleanup; + } +#endif + +private: + QVector m_renderTargetIdsToCleanup; }; class 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 #include #include +#include #include QT_BEGIN_NAMESPACE @@ -94,6 +95,33 @@ QVector 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 30769dcfc..e156754b7 100644 --- a/src/render/backend/rendertarget_p.h +++ b/src/render/backend/rendertarget_p.h @@ -82,6 +82,21 @@ private: QVector 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 5784b35f4..a42da4908 100644 --- a/src/render/frontend/qrenderaspect.cpp +++ b/src/render/frontend/qrenderaspect.cpp @@ -269,7 +269,7 @@ void QRenderAspectPrivate::registerBackendTypes() registerBackendType(QSharedPointer >::create(m_renderer)); registerBackendType(QSharedPointer >::create(m_renderer)); registerBackendType(QSharedPointer::create(m_renderer, m_nodeManagers->sceneManager())); - registerBackendType(QSharedPointer >::create(m_renderer)); + registerBackendType(QSharedPointer::create(m_renderer, m_nodeManagers->renderTargetManager())); registerBackendType(QSharedPointer >::create(m_renderer)); registerBackendType(QSharedPointer::create(m_renderer)); registerBackendType(QSharedPointer >::create(m_renderer)); diff --git a/src/render/renderers/opengl/graphicshelpers/submissioncontext.cpp b/src/render/renderers/opengl/graphicshelpers/submissioncontext.cpp index 47779dded..b8229dd9e 100644 --- a/src/render/renderers/opengl/graphicshelpers/submissioncontext.cpp +++ b/src/render/renderers/opengl/graphicshelpers/submissioncontext.cpp @@ -506,6 +506,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/render/renderers/opengl/graphicshelpers/submissioncontext_p.h b/src/render/renderers/opengl/graphicshelpers/submissioncontext_p.h index a8700dd3a..0f769fa1c 100644 --- a/src/render/renderers/opengl/graphicshelpers/submissioncontext_p.h +++ b/src/render/renderers/opengl/graphicshelpers/submissioncontext_p.h @@ -111,6 +111,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/render/renderers/opengl/renderer/renderer.cpp b/src/render/renderers/opengl/renderer/renderer.cpp index d61d1d8ac..3e5bef3cd 100644 --- a/src/render/renderers/opengl/renderer/renderer.cpp +++ b/src/render/renderers/opengl/renderer/renderer.cpp @@ -1397,6 +1397,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/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 #include #include +#include #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) -- cgit v1.2.3