Add reference count for backend buffers

In the main rendering loop, first dirty GL resources are uploaded, then
render views are submitted, and then unused GL resources are released.
Consider the following piece of code:

  entity->setParent(nullptr);
  entity->setParent(root);

If this is executed inside a single frame, entity's children buffers
will be marked for release when they are removed from the scene, and
marked as dirty when they're added again. In the following frame, the
render thread will upload them at the beginning of the frame, and
incorrectly release them at the end of the frame.

This patch adds a reference count for buffers in BufferManager to
prevent this kind of race. The reference is incremented when a buffer is
added to the scene (in Buffer::initializeFromPeer) and decremented when
it's removed from the scene (in BufferFunctor::destroy).

Task-number: QTBUG-60726
Change-Id: I8eed7b4c2d8262ba846e73bf4299edae7bc16b3e
Reviewed-by: Paul Lemire <paul.lemire@kdab.com>

3 files changed, 27 insertions, 7 deletions

diff --git a/src/render/geometry/buffer.cpp b/src/render/geometry/buffer.cpp
index 55c86910f..ae2455184 100644
--- a/src/render/geometry/buffer.cpp
+++ b/src/render/geometry/buffer.cpp
@@ -133,6 +133,8 @@ void Buffer::initializeFromPeer(const Qt3DCore::QNodeCreatedChangeBasePtr &chang
     Q_ASSERT(m_manager);
     if (m_functor)
         m_manager->addDirtyBuffer(peerId());
+
+    m_manager->addBufferReference(peerId());
 }
 
 void Buffer::forceDataUpload()
@@ -210,7 +212,7 @@ Qt3DCore::QBackendNode *BufferFunctor::get(Qt3DCore::QNodeId id) const
 
 void BufferFunctor::destroy(Qt3DCore::QNodeId id) const
 {
-    m_manager->addBufferToRelease(id);
+    m_manager->removeBufferReference(id);
     return m_manager->releaseResource(id);
 }
 
diff --git a/src/render/geometry/buffermanager.cpp b/src/render/geometry/buffermanager.cpp
index 78c2c0082..5bd44f80f 100644
--- a/src/render/geometry/buffermanager.cpp
+++ b/src/render/geometry/buffermanager.cpp
@@ -66,18 +66,34 @@ QVector<Qt3DCore::QNodeId> BufferManager::dirtyBuffers()
 }
 
 // Called in QAspectThread::syncChanges
-void BufferManager::addBufferToRelease(Qt3DCore::QNodeId bufferId)
+void BufferManager::removeBufferReference(Qt3DCore::QNodeId bufferId)
 {
     QMutexLocker lock(&m_mutex);
-    m_buffersToRelease.push_back(bufferId);
+    Q_ASSERT(m_bufferReferences.contains(bufferId) && m_bufferReferences[bufferId] > 0);
+    m_bufferReferences[bufferId]--;
+}
+
+// Called in QAspectThread
+void BufferManager::addBufferReference(Qt3DCore::QNodeId bufferId)
+{
+    QMutexLocker lock(&m_mutex);
+    m_bufferReferences[bufferId]++;
 }
 
 // Called in Render thread
 QVector<Qt3DCore::QNodeId> BufferManager::takeBuffersToRelease()
 {
     QMutexLocker lock(&m_mutex);
-    // Clears the m_buffersToRelease vector
-    return std::move(m_buffersToRelease);
+    QVector<Qt3DCore::QNodeId> buffersToRelease;
+    QMutableHashIterator<Qt3DCore::QNodeId, int> it(m_bufferReferences);
+    while (it.hasNext()) {
+        it.next();
+        if (it.value() == 0) {
+            buffersToRelease.append(it.key());
+            it.remove();
+        }
+    }
+    return buffersToRelease;
 }
 
 } // namespace Render
diff --git a/src/render/geometry/buffermanager_p.h b/src/render/geometry/buffermanager_p.h
index 6862cd973..3eecb3664 100644
--- a/src/render/geometry/buffermanager_p.h
+++ b/src/render/geometry/buffermanager_p.h
@@ -76,13 +76,15 @@ public:
     QVector<Qt3DCore::QNodeId> dirtyBuffers();
 
     // Aspect Thread
-    void addBufferToRelease(Qt3DCore::QNodeId bufferId);
+    void addBufferReference(Qt3DCore::QNodeId bufferId);
+    void removeBufferReference(Qt3DCore::QNodeId bufferId);
+
     // Render Thread (no concurrent access)
     QVector<Qt3DCore::QNodeId> takeBuffersToRelease();
 
 private:
     QVector<Qt3DCore::QNodeId> m_dirtyBuffers;
-    QVector<Qt3DCore::QNodeId> m_buffersToRelease;
+    QHash<Qt3DCore::QNodeId, int> m_bufferReferences;
     QMutex m_mutex;
 };