Do not delay cleanup of backend Texture instances

If we delay the cleanup of backend Texture instances, in the case of a texture
being reparented from an existing node with backend to a node with no
backend, the resulting node destroyed, node created changes delivered in the
same loop would result in the Texture still being marked for destruction and
eventually being destroyed even though it was recreated.

This patch now still marks textures that were destroyed (by storing their
ids), so that the GL Texture managers can update the resources
accordingly. However, we now cleanup Textures immediately. We also remove
textures ids marked for destruction in the GL texture managers if we detect a
Texture with the same id is readded.

Change-Id: I29092d7dff9f70bb9fb4b15f4e9419ee1791b6e2
Task-number: QTBUG-69379
Reviewed-by: Mike Krus <mike.krus@kdab.com>
Reviewed-by: Sean Harmer <sean.harmer@kdab.com>

7 files changed, 104 insertions, 12 deletions

diff --git a/src/render/backend/managers_p.h b/src/render/backend/managers_p.h
index c9bcb799e..6af34fa6d 100644
--- a/src/render/backend/managers_p.h
+++ b/src/render/backend/managers_p.h
@@ -230,12 +230,27 @@ public:
         m_textureIdsToCleanup.push_back(id);
     }
 
+    // Called in AspectThread by Texture node functor create
+    void removeTextureIdToCleanup(Qt3DCore::QNodeId id)
+    {
+        m_textureIdsToCleanup.removeAll(id);
+    }
+
     // Called by RenderThread in updateGLResources (locked)
     QVector<Qt3DCore::QNodeId> takeTexturesIdsToCleanup()
     {
         return std::move(m_textureIdsToCleanup);
     }
 
+#ifdef QT_BUILD_INTERNAL
+    // For unit testing purposes only
+    QVector<Qt3DCore::QNodeId> textureIdsToCleanup() const
+    {
+        return m_textureIdsToCleanup;
+    }
+
+#endif
+
 private:
     QVector<Qt3DCore::QNodeId> m_textureIdsToCleanup;
 };
diff --git a/src/render/renderers/opengl/renderer/renderer.cpp b/src/render/renderers/opengl/renderer/renderer.cpp
index f04b864a3..27d46cc81 100644
--- a/src/render/renderers/opengl/renderer/renderer.cpp
+++ b/src/render/renderers/opengl/renderer/renderer.cpp
@@ -1191,16 +1191,13 @@ void Renderer::updateGLResources()
                 glTexture->getOrCreateGLTexture();
         }
     }
-    // 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 that the previous updateTexture call
-    // has a chance to find a shared texture
+    // 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
+    // that the previous updateTexture call has a chance to find a shared
+    // texture and avoid possible destroying recreating a new texture
     const QVector<Qt3DCore::QNodeId> cleanedUpTextureIds = m_nodesManager->textureManager()->takeTexturesIdsToCleanup();
-    for (const Qt3DCore::QNodeId textureCleanedUpId: cleanedUpTextureIds) {
+    for (const Qt3DCore::QNodeId textureCleanedUpId: cleanedUpTextureIds)
         cleanupTexture(m_nodesManager->textureManager()->lookupResource(textureCleanedUpId));
-        // We can really release the texture at this point
-        m_nodesManager->textureManager()->releaseResource(textureCleanedUpId);
-    }
 }
 
 // Render Thread
diff --git a/src/render/texture/texture.cpp b/src/render/texture/texture.cpp
index 2585165e7..914a4d9d8 100644
--- a/src/render/texture/texture.cpp
+++ b/src/render/texture/texture.cpp
@@ -372,6 +372,10 @@ Qt3DCore::QBackendNode *TextureFunctor::create(const Qt3DCore::QNodeCreatedChang
     Texture *backend = m_textureNodeManager->getOrCreateResource(change->subjectId());
     backend->setTextureImageManager(m_textureImageManager);
     backend->setRenderer(m_renderer);
+    // Remove id from cleanupList if for some reason we were in the dirty list of texture
+    // (Can happen when a node destroyed is followed by a node created change
+    // in the same loop, when changing parent for instance)
+    m_textureNodeManager->removeTextureIdToCleanup(change->subjectId());
     return backend;
 }
 
@@ -383,9 +387,9 @@ Qt3DCore::QBackendNode *TextureFunctor::get(Qt3DCore::QNodeId id) const
 void TextureFunctor::destroy(Qt3DCore::QNodeId id) const
 {
     m_textureNodeManager->addTextureIdToCleanup(id);
-    // We only add ourselves to the dirty list
-    // The actual removal needs to be performed after we have
-    // destroyed the associated APITexture in the RenderThread
+    // We add ourselves to the dirty list to tell the shared texture managers
+    // in the renderer that this texture has been destroyed
+    m_textureNodeManager->releaseResource(id);
 }
 
 
diff --git a/src/render/texture/texture_p.h b/src/render/texture/texture_p.h
index 2c71eb10a..7e23f124c 100644
--- a/src/render/texture/texture_p.h
+++ b/src/render/texture/texture_p.h
@@ -175,7 +175,7 @@ private:
     QMutex m_flagsMutex;
 };
 
-class TextureFunctor : public Qt3DCore::QBackendNodeMapper
+class Q_AUTOTEST_EXPORT TextureFunctor : public Qt3DCore::QBackendNodeMapper
 {
 public:
     explicit TextureFunctor(AbstractRenderer *renderer,
diff --git a/tests/auto/core/common/qbackendnodetester.cpp b/tests/auto/core/common/qbackendnodetester.cpp
index 6f87e10d9..5d4a10b81 100644
--- a/tests/auto/core/common/qbackendnodetester.cpp
+++ b/tests/auto/core/common/qbackendnodetester.cpp
@@ -68,6 +68,11 @@ void QBackendNodeTester::sceneChangeEvent(QBackendNode *backend, const Qt3DCore:
     backend->sceneChangeEvent(e);
 }
 
+QNodeCreatedChangeBasePtr QBackendNodeTester::creationChange(QNode *frontend) const
+{
+    return frontend->createNodeCreationChange();
+}
+
 } // namespace Qt3DCore
 
 QT_END_NAMESPACE
diff --git a/tests/auto/core/common/qbackendnodetester.h b/tests/auto/core/common/qbackendnodetester.h
index 9f266e40d..a9738dff0 100644
--- a/tests/auto/core/common/qbackendnodetester.h
+++ b/tests/auto/core/common/qbackendnodetester.h
@@ -40,6 +40,7 @@
 #include <QObject>
 #include <Qt3DCore/qnodeid.h>
 #include <Qt3DCore/qscenechange.h>
+#include <Qt3DCore/qnodecreatedchange.h>
 
 QT_BEGIN_NAMESPACE
 
@@ -58,6 +59,7 @@ public:
     void setPeerId(QBackendNode *backend, QNodeId id);
     void simulateInitialization(QNode *frontend, QBackendNode *backend);
     void sceneChangeEvent(QBackendNode *backend, const Qt3DCore::QSceneChangePtr &e);
+    Qt3DCore::QNodeCreatedChangeBasePtr creationChange(QNode *frontend) const;
 };
 
 } // namespace Qt3DCore
diff --git a/tests/auto/render/textures/tst_textures.cpp b/tests/auto/render/textures/tst_textures.cpp
index f50131e12..364bfd7a9 100644
--- a/tests/auto/render/textures/tst_textures.cpp
+++ b/tests/auto/render/textures/tst_textures.cpp
@@ -583,6 +583,75 @@ private Q_SLOTS:
         QVERIFY(texImgDataMgr->getData(frontendGenerator).isNull());
         QVERIFY(texImgDataMgr->getData(backendGenerator).isNull());
     }
+
+    void checkTextureIsMarkedForDeletion()
+    {
+        QScopedPointer<Qt3DRender::Render::NodeManagers> mgrs(new Qt3DRender::Render::NodeManagers());
+        Qt3DRender::Render::Renderer renderer(Qt3DRender::QRenderAspect::Synchronous);
+        Qt3DRender::Render::TextureManager *texMgr = mgrs->textureManager();
+        Qt3DRender::Render::TextureImageManager *texImgMgr = mgrs->textureImageManager();
+        renderer.setNodeManagers(mgrs.data());
+
+        Qt3DRender::Render::TextureFunctor textureBackendNodeMapper(&renderer,
+                                                                    texMgr,
+                                                                    texImgMgr);
+
+        // GIVEN
+        Qt3DRender::QAbstractTexture* frontendTexture = createQTexture(1, {1}, true);
+
+        Qt3DRender::Render::Texture *backendTexture = static_cast<Qt3DRender::Render::Texture *>(textureBackendNodeMapper.create(creationChange(frontendTexture)));
+        simulateInitialization(frontendTexture, backendTexture);
+
+        // THEN
+        QVERIFY(backendTexture != nullptr);
+        QCOMPARE(texMgr->textureIdsToCleanup().size(), 0);
+
+        QCOMPARE(texMgr->lookupResource(frontendTexture->id()), backendTexture);
+
+        // WHEN
+        textureBackendNodeMapper.destroy(frontendTexture->id());
+
+        // THEN
+        QCOMPARE(texMgr->textureIdsToCleanup().size(), 1);
+        QCOMPARE(texMgr->textureIdsToCleanup().first(), frontendTexture->id());
+        QVERIFY(texMgr->lookupResource(frontendTexture->id()) == nullptr);
+    }
+
+    void checkTextureDestructionReconstructionWithinSameLoop()
+    {
+        QScopedPointer<Qt3DRender::Render::NodeManagers> mgrs(new Qt3DRender::Render::NodeManagers());
+        Qt3DRender::Render::Renderer renderer(Qt3DRender::QRenderAspect::Synchronous);
+        Qt3DRender::Render::TextureManager *texMgr = mgrs->textureManager();
+        Qt3DRender::Render::TextureImageManager *texImgMgr = mgrs->textureImageManager();
+        renderer.setNodeManagers(mgrs.data());
+
+        Qt3DRender::Render::TextureFunctor textureBackendNodeMapper(&renderer,
+                                                                    texMgr,
+                                                                    texImgMgr);
+
+        // GIVEN
+        Qt3DRender::QAbstractTexture* frontendTexture = createQTexture(1, {1}, true);
+
+        Qt3DRender::Render::Texture *backendTexture = static_cast<Qt3DRender::Render::Texture *>(textureBackendNodeMapper.create(creationChange(frontendTexture)));
+        simulateInitialization(frontendTexture, backendTexture);
+
+        // WHEN
+        textureBackendNodeMapper.destroy(frontendTexture->id());
+
+        // THEN
+        QCOMPARE(texMgr->textureIdsToCleanup().size(), 1);
+        QCOMPARE(texMgr->textureIdsToCleanup().first(), frontendTexture->id());
+        QVERIFY(texMgr->lookupResource(frontendTexture->id()) == nullptr);
+
+        // WHEN
+        backendTexture = static_cast<Qt3DRender::Render::Texture *>(textureBackendNodeMapper.create(creationChange(frontendTexture)));
+        simulateInitialization(frontendTexture, backendTexture);
+
+        // THEN
+        QVERIFY(backendTexture != nullptr);
+        QCOMPARE(texMgr->textureIdsToCleanup().size(), 0);
+        QCOMPARE(texMgr->lookupResource(frontendTexture->id()), backendTexture);
+    }
 };
 
 QTEST_MAIN(tst_RenderTextures)