Textures: properly abandon and release texture data

When a texture is destroyed it now properly destroys the GLTexture
if the GLTexture was not being shared.

Also, when a GLTexture is created but the texture image data for its content
already existed, make sure we still request the data to be uploaded.
This happens since functors can be shared, in which case the image data isn't
reloaded and therefore doesn't call texture->requestUpload() implicitly.

Task-number: QTBUG-57595
Change-Id: Id8f437ff64eea39be75ebb1a548516b29932e23e
Reviewed-by: Sean Harmer <sean.harmer@kdab.com>

8 files changed, 77 insertions, 13 deletions

diff --git a/src/render/backend/managers_p.h b/src/render/backend/managers_p.h
index a45f74d50..6b0aef4e8 100644
--- a/src/render/backend/managers_p.h
+++ b/src/render/backend/managers_p.h
@@ -230,6 +230,21 @@ class TextureManager : public Qt3DCore::QResourceManager<
 {
 public:
     TextureManager() {}
+
+    // Called in AspectThread by Texture node functor destroy
+    void addTextureIdToCleanup(Qt3DCore::QNodeId id)
+    {
+        m_textureIdsToCleanup.push_back(id);
+    }
+
+    // Called by RenderThread in updateGLResources (locked)
+    QVector<Qt3DCore::QNodeId> takeTexturesIdsToCleanup()
+    {
+        return std::move(m_textureIdsToCleanup);
+    }
+
+private:
+    QVector<Qt3DCore::QNodeId> m_textureIdsToCleanup;
 };
 
 class TransformManager : public Qt3DCore::QResourceManager<
diff --git a/src/render/backend/renderer.cpp b/src/render/backend/renderer.cpp
index 3e2070511..a0195a09c 100644
--- a/src/render/backend/renderer.cpp
+++ b/src/render/backend/renderer.cpp
@@ -489,11 +489,11 @@ void Renderer::doRender()
                     if (!m_ownedContext)
                         m_graphicsContext->setCurrentStateSet(nullptr);
                     if (m_graphicsContext->beginDrawing(surface)) {
-                    // 1) Execute commands for buffer uploads, texture updates, shader loading first
-                    updateGLResources();
-                    // 2) Update VAO and copy data into commands to allow concurrent submission
-                    prepareCommandsSubmission(renderViews);
-                    preprocessingComplete = true;
+                        // 1) Execute commands for buffer uploads, texture updates, shader loading first
+                        updateGLResources();
+                        // 2) Update VAO and copy data into commands to allow concurrent submission
+                        prepareCommandsSubmission(renderViews);
+                        preprocessingComplete = true;
                     }
                 }
             }
@@ -797,6 +797,7 @@ void Renderer::lookForDirtyShaders()
     }
 }
 
+// Render Thread
 void Renderer::updateGLResources()
 {
     const QVector<HBuffer> dirtyBufferHandles = std::move(m_dirtyBuffers);
@@ -824,8 +825,21 @@ void Renderer::updateGLResources()
         // Upload/Update texture
         updateTexture(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
+    const QVector<Qt3DCore::QNodeId> cleanedUpTextureIds = m_nodesManager->textureManager()->takeTexturesIdsToCleanup();
+    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
 void Renderer::updateTexture(Texture *texture)
 {
     // For implementing unique, non-shared, non-cached textures.
@@ -904,6 +918,16 @@ void Renderer::updateTexture(Texture *texture)
     texture->unsetDirty();
 }
 
+// Render Thread
+void Renderer::cleanupTexture(const Texture *texture)
+{
+    GLTextureManager *glTextureManager = m_nodesManager->glTextureManager();
+    GLTexture *glTexture = glTextureManager->lookupResource(texture->peerId());
+
+    if (glTexture != nullptr)
+        glTextureManager->abandon(glTexture, texture);
+}
+
 
 // Happens in RenderThread context when all RenderViewJobs are done
 // Returns the id of the last bound FBO
diff --git a/src/render/backend/renderer_p.h b/src/render/backend/renderer_p.h
index cc90abe00..965bccbcf 100644
--- a/src/render/backend/renderer_p.h
+++ b/src/render/backend/renderer_p.h
@@ -199,6 +199,7 @@ public:
 
     void updateGLResources();
     void updateTexture(Texture *texture);
+    void cleanupTexture(const Texture *texture);
 
     void prepareCommandsSubmission(const QVector<RenderView *> &renderViews);
     bool executeCommandsSubmission(const RenderView *rv);
diff --git a/src/render/texture/apitexturemanager_p.h b/src/render/texture/apitexturemanager_p.h
index 1be5a1af8..2fd340e7e 100644
--- a/src/render/texture/apitexturemanager_p.h
+++ b/src/render/texture/apitexturemanager_p.h
@@ -182,6 +182,7 @@ public:
             if (referencedTextureNodes.empty()) {
                 m_abandonedTextures.push_back(apiTexture);
                 m_sharedTextures.remove(apiTexture);
+                tex->destroyResources();
             }
         }
     }
diff --git a/src/render/texture/gltexture.cpp b/src/render/texture/gltexture.cpp
index a3b1f24a9..1a76617a7 100644
--- a/src/render/texture/gltexture.cpp
+++ b/src/render/texture/gltexture.cpp
@@ -72,17 +72,19 @@ GLTexture::GLTexture(TextureDataManager *texDataMgr,
     , m_dataFunctor(texGen)
 {
     // make sure texture generator is executed
+    // this is needed when Texture have the TargetAutomatic
+    // to ensure they are loaded before trying to instantiate the QOpenGLTexture
     if (!texGen.isNull())
         m_textureDataManager->requestData(texGen, this);
 }
 
 GLTexture::~GLTexture()
 {
-    if (m_gl) {
-        qWarning() << "[Qt3DRender::GLTexture] Destructor called without properly deleting texture";
-        delete m_gl;
-    }
+    destroyGLTexture();
+}
 
+void GLTexture::destroyResources()
+{
     // release texture data
     for (const Image &img : qAsConst(m_images))
         m_textureImageDataManager->releaseData(img.generator, this);
@@ -96,6 +98,8 @@ void GLTexture::destroyGLTexture()
     delete m_gl;
     m_gl = nullptr;
     m_dirty = 0;
+
+    destroyResources();
 }
 
 QOpenGLTexture* GLTexture::getOrCreateGLTexture()
@@ -255,8 +259,15 @@ void GLTexture::setImages(const QVector<Image> &images)
         // TextureDataManager.
 
         // make sure the generators are executed
+        bool newEntriesCreated = false;
         for (const Image& img : qAsConst(images)) {
-            m_textureImageDataManager->requestData(img.generator, this);
+            newEntriesCreated |= m_textureImageDataManager->requestData(img.generator, this);
+        }
+
+        if (!newEntriesCreated) {
+            // request a data upload (very important in case the image data already
+            // exists and wouldn't trigger an update)
+            requestUpload();
         }
     }
 }
diff --git a/src/render/texture/gltexture_p.h b/src/render/texture/gltexture_p.h
index ca73c1131..7c6bf9c90 100644
--- a/src/render/texture/gltexture_p.h
+++ b/src/render/texture/gltexture_p.h
@@ -174,6 +174,7 @@ private:
     QOpenGLTexture *buildGLTexture();
     void uploadGLTextureData();
     void updateGLTextureParameters();
+    void destroyResources();
 
     bool m_unique;
     DirtyFlags m_dirty;
diff --git a/src/render/texture/texture.cpp b/src/render/texture/texture.cpp
index 233dc364a..f9e0aa4ba 100644
--- a/src/render/texture/texture.cpp
+++ b/src/render/texture/texture.cpp
@@ -121,6 +121,8 @@ void Texture::removeTextureImage(Qt3DCore::QNodeId id)
     }
 }
 
+// This is called by Renderer::updateGLResources
+// when the texture has been marked for cleanup
 void Texture::cleanup()
 {
     // Whoever calls this must make sure to also check if this
@@ -147,6 +149,8 @@ void Texture::cleanup()
     m_parameters.maximumAnisotropy = 1.0f;
     m_parameters.comparisonFunction = QAbstractTexture::CompareLessEqual;
     m_parameters.comparisonMode = QAbstractTexture::CompareNone;
+
+    m_dirty = NotDirty;
 }
 
 // ChangeArbiter/Aspect Thread
@@ -292,7 +296,10 @@ Qt3DCore::QBackendNode *TextureFunctor::get(Qt3DCore::QNodeId id) const
 
 void TextureFunctor::destroy(Qt3DCore::QNodeId id) const
 {
-    m_textureNodeManager->releaseResource(id);
+    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
 }
 
 
diff --git a/src/render/texture/texturedatamanager_p.h b/src/render/texture/texturedatamanager_p.h
index d58676929..13ceca0ee 100644
--- a/src/render/texture/texturedatamanager_p.h
+++ b/src/render/texture/texturedatamanager_p.h
@@ -91,17 +91,21 @@ public:
      * If no data for the given generator exists, make sure that the
      * generators are executed the next frame. Reference generator by
      * given texture
+     *
+     * Returns true if the Entry for a given generator had to be created
      */
-    void requestData(const GeneratorPtr &generator, APITexture *tex)
+    bool requestData(const GeneratorPtr &generator, APITexture *tex)
     {
         QMutexLocker lock(&m_mutex);
 
         Entry *entry = findEntry(generator);
-        if (entry == nullptr)
+        const bool needsToBeCreated = (entry == nullptr);
+        if (needsToBeCreated)
             entry = createEntry(generator);
         Q_ASSERT(entry);
         if (!entry->referencingTextures.contains(tex))
             entry->referencingTextures.push_back(tex);
+        return needsToBeCreated;
     }
 
     /*!