Convent SendBufferCaptureJob to direct sync

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

8 files changed, 79 insertions, 52 deletions

diff --git a/src/render/geometry/buffer.cpp b/src/render/geometry/buffer.cpp
index df270ba3e..0d634c911 100644
--- a/src/render/geometry/buffer.cpp
+++ b/src/render/geometry/buffer.cpp
@@ -96,12 +96,6 @@ void Buffer::updateDataFromGPUToCPU(QByteArray data)
     // Note: when this is called, data is what's currently in GPU memory
     // so m_data shouldn't be reuploaded
     m_data = data;
-    // Send data back to the frontend
-    auto e = Qt3DCore::QPropertyUpdatedChangePtr::create(peerId());
-    e->setDeliveryFlags(Qt3DCore::QSceneChange::DeliverToAll);
-    e->setPropertyName("downloadedData");
-    e->setValue(QVariant::fromValue(m_data));
-    notifyObservers(e);
 }
 
 void Buffer::forceDataUpload()
diff --git a/src/render/geometry/qbuffer.cpp b/src/render/geometry/qbuffer.cpp
index 1cccb7a1f..bbe08fdf7 100644
--- a/src/render/geometry/qbuffer.cpp
+++ b/src/render/geometry/qbuffer.cpp
@@ -310,23 +310,6 @@ QBuffer::~QBuffer()
 }
 
 /*!
- * \internal
- */
-void QBuffer::sceneChangeEvent(const Qt3DCore::QSceneChangePtr &change)
-{
-    if (change->type() == PropertyUpdated) {
-        QPropertyUpdatedChangePtr e = qSharedPointerCast<QPropertyUpdatedChange>(change);
-        const QByteArray propertyName = e->propertyName();
-        if (propertyName == QByteArrayLiteral("downloadedData")) {
-            const bool blocked = blockNotifications(true);
-            setData(e->value().toByteArray());
-            blockNotifications(blocked);
-            Q_EMIT dataAvailable();
-        }
-    }
-}
-
-/*!
  * Sets \a bytes as data.
  */
 void QBuffer::setData(const QByteArray &bytes)
diff --git a/src/render/geometry/qbuffer.h b/src/render/geometry/qbuffer.h
index 1bd1aa8fd..4d5f6c86e 100644
--- a/src/render/geometry/qbuffer.h
+++ b/src/render/geometry/qbuffer.h
@@ -118,9 +118,6 @@ public Q_SLOTS:
     void setSyncData(bool syncData);
     void setAccessType(AccessType access);
 
-protected:
-    void sceneChangeEvent(const Qt3DCore::QSceneChangePtr &change) override;
-
 Q_SIGNALS:
     void dataChanged(const QByteArray &bytes);
     void typeChanged(BufferType type);
diff --git a/src/render/jobs/sendbuffercapturejob.cpp b/src/render/jobs/sendbuffercapturejob.cpp
index 8683ea9f2..3fa185684 100644
--- a/src/render/jobs/sendbuffercapturejob.cpp
+++ b/src/render/jobs/sendbuffercapturejob.cpp
@@ -39,11 +39,12 @@
 
 #include "sendbuffercapturejob_p.h"
 
-
-#include "Qt3DRender/private/renderer_p.h"
-#include "Qt3DRender/private/nodemanagers_p.h"
+#include <Qt3DRender/private/nodemanagers_p.h>
 #include <Qt3DRender/private/job_common_p.h>
 #include <Qt3DRender/private/buffer_p.h>
+#include <Qt3DRender/private/buffermanager_p.h>
+#include <Qt3DCore/private/qaspectmanager_p.h>
+#include <Qt3DRender/private/qbuffer_p.h>
 
 QT_BEGIN_NAMESPACE
 
@@ -51,8 +52,22 @@ namespace Qt3DRender {
 
 namespace Render {
 
+class SendBufferCaptureJobPrivate : public Qt3DCore::QAspectJobPrivate
+{
+public:
+    SendBufferCaptureJobPrivate() {}
+    ~SendBufferCaptureJobPrivate() {}
+
+    void postFrame(Qt3DCore::QAspectManager *aspectManager) override;
+
+    mutable QMutex m_mutex;
+    QVector<QPair<Qt3DCore::QNodeId, QByteArray>> m_buffersToCapture;
+    QVector<QPair<Qt3DCore::QNodeId, QByteArray>> m_buffersToNotify;
+};
+
 SendBufferCaptureJob::SendBufferCaptureJob()
-    : Qt3DCore::QAspectJob()
+    : Qt3DCore::QAspectJob(*new SendBufferCaptureJobPrivate)
+    , m_nodeManagers(nullptr)
 {
     SET_JOB_RUN_STAT_TYPE(this, JobTypes::SendBufferCapture, 0);
 }
@@ -61,26 +76,51 @@ SendBufferCaptureJob::~SendBufferCaptureJob()
 {
 }
 
-void SendBufferCaptureJob::addRequest(QPair<Buffer *, QByteArray> request)
+// Called from SubmitRenderView while rendering
+void SendBufferCaptureJob::addRequest(QPair<Qt3DCore::QNodeId, QByteArray> request)
 {
-    QMutexLocker locker(&m_mutex);
-    m_pendingSendBufferCaptures.push_back(request);
+    Q_D(SendBufferCaptureJob);
+    QMutexLocker locker(&d->m_mutex);
+    d->m_buffersToCapture.push_back(request);
 }
 
-// Called by aspect thread jobs to execute (no concurrency at that point)
+// Called by aspect thread jobs to execute (we may still be rendering at this point)
 bool SendBufferCaptureJob::hasRequests() const
 {
-    return m_pendingSendBufferCaptures.size() > 0;
+    Q_D(const SendBufferCaptureJob);
+    QMutexLocker locker(&d->m_mutex);
+    return d->m_buffersToCapture.size() > 0;
 }
 
 void SendBufferCaptureJob::run()
 {
-    QMutexLocker locker(&m_mutex);
-    for (const QPair<Buffer*, QByteArray> &pendingCapture : qAsConst(m_pendingSendBufferCaptures)) {
-        pendingCapture.first->updateDataFromGPUToCPU(pendingCapture.second);
+    Q_ASSERT(m_nodeManagers);
+    Q_D(SendBufferCaptureJob);
+    QMutexLocker locker(&d->m_mutex);
+    for (const QPair<Qt3DCore::QNodeId, QByteArray> &pendingCapture : qAsConst(d->m_buffersToCapture)) {
+        Buffer *buffer = m_nodeManagers->bufferManager()->lookupResource(pendingCapture.first);
+        // Buffer might have been destroyed between the time addRequest is made and this job gets run
+        // If it exists however, it cannot be destroyed before this job is done running
+        if (buffer != nullptr)
+            buffer->updateDataFromGPUToCPU(pendingCapture.second);
     }
+    d->m_buffersToNotify = std::move(d->m_buffersToCapture);
+}
 
-    m_pendingSendBufferCaptures.clear();
+void SendBufferCaptureJobPrivate::postFrame(Qt3DCore::QAspectManager *aspectManager)
+{
+    QMutexLocker locker(&m_mutex);
+    const QVector<QPair<Qt3DCore::QNodeId, QByteArray>> pendingSendBufferCaptures = std::move(m_buffersToNotify);
+    for (const auto &bufferDataPair : pendingSendBufferCaptures) {
+        QBuffer *frontendBuffer = static_cast<decltype(frontendBuffer)>(aspectManager->lookupNode(bufferDataPair.first));
+        if (!frontendBuffer)
+            continue;
+        QBufferPrivate *dFrontend = static_cast<decltype(dFrontend)>(Qt3DCore::QNodePrivate::get(frontendBuffer));
+        // Calling frontendBuffer->setData would result in forcing a sync against the backend
+        // which isn't necessary
+        dFrontend->setData(bufferDataPair.second);
+        Q_EMIT frontendBuffer->dataAvailable();
+    }
 }
 
 } // Render
diff --git a/src/render/jobs/sendbuffercapturejob_p.h b/src/render/jobs/sendbuffercapturejob_p.h
index f47c556df..3b9f5d12b 100644
--- a/src/render/jobs/sendbuffercapturejob_p.h
+++ b/src/render/jobs/sendbuffercapturejob_p.h
@@ -52,6 +52,7 @@
 //
 
 #include <Qt3DCore/qaspectjob.h>
+#include <Qt3DCore/qnodeid.h>
 #include <Qt3DRender/qt3drender_global.h>
 #include <Qt3DRender/private/qt3drender_global_p.h>
 #include <QMutex>
@@ -67,6 +68,7 @@ class NodeManagers;
 class Entity;
 class Renderer;
 class Buffer;
+class SendBufferCaptureJobPrivate;
 
 class Q_3DRENDERSHARED_PRIVATE_EXPORT SendBufferCaptureJob : public Qt3DCore::QAspectJob
 {
@@ -74,15 +76,15 @@ public:
     explicit SendBufferCaptureJob();
     ~SendBufferCaptureJob();
 
-    void addRequest(QPair<Buffer*, QByteArray> request);
+    void setManagers(NodeManagers *nodeManagers) { m_nodeManagers = nodeManagers; }
+    void addRequest(QPair<Qt3DCore::QNodeId, QByteArray> request);
     bool hasRequests() const;
 
     void run() final;
 
 private:
-    QMutex m_mutex;
-
-    QVector<QPair<Buffer*, QByteArray> > m_pendingSendBufferCaptures;
+    Q_DECLARE_PRIVATE(SendBufferCaptureJob)
+    NodeManagers *m_nodeManagers;
 };
 
 typedef QSharedPointer<SendBufferCaptureJob> SendBufferCaptureJobPtr;
diff --git a/src/render/renderers/opengl/renderer/renderer.cpp b/src/render/renderers/opengl/renderer/renderer.cpp
index f17e66108..35c4d553f 100644
--- a/src/render/renderers/opengl/renderer/renderer.cpp
+++ b/src/render/renderers/opengl/renderer/renderer.cpp
@@ -306,6 +306,7 @@ void Renderer::setNodeManagers(NodeManagers *managers)
     m_filterCompatibleTechniqueJob->setManager(m_nodesManager->techniqueManager());
     m_updateEntityLayersJob->setManager(m_nodesManager);
     m_updateTreeEnabledJob->setManagers(m_nodesManager);
+    m_sendBufferCaptureJob->setManagers(m_nodesManager);
 }
 
 void Renderer::setServices(QServiceLocator *services)
@@ -1045,6 +1046,7 @@ void Renderer::lookForDirtyBuffers()
     }
 }
 
+// Called in prepareSubmission
 void Renderer::lookForDownloadableBuffers()
 {
     m_downloadableBuffers.clear();
@@ -1052,7 +1054,7 @@ void Renderer::lookForDownloadableBuffers()
     for (const HBuffer &handle : activeBufferHandles) {
         Buffer *buffer = m_nodesManager->bufferManager()->data(handle);
         if (buffer->access() & QBuffer::Read)
-            m_downloadableBuffers.push_back(handle);
+            m_downloadableBuffers.push_back(buffer->peerId());
     }
 }
 
@@ -1358,6 +1360,9 @@ void Renderer::updateGLResources()
         // Record ids of texture to cleanup while we are still blocking the aspect thread
         m_textureIdsToCleanup += m_nodesManager->textureManager()->takeTexturesIdsToCleanup();
     }
+
+    // Record list of buffer that might need uploading
+    lookForDownloadableBuffers();
 }
 
 // Render Thread
@@ -1440,12 +1445,18 @@ void Renderer::cleanupTexture(Qt3DCore::QNodeId cleanedUpTextureId)
 // Called by SubmitRenderView
 void Renderer::downloadGLBuffers()
 {
-    lookForDownloadableBuffers();
-    const QVector<HBuffer> downloadableHandles = std::move(m_downloadableBuffers);
-    for (const HBuffer &handle : downloadableHandles) {
-        Buffer *buffer = m_nodesManager->bufferManager()->data(handle);
-        QByteArray content = m_submissionContext->downloadBufferContent(buffer);
-        m_sendBufferCaptureJob->addRequest(QPair<Buffer*, QByteArray>(buffer, content));
+    const QVector<Qt3DCore::QNodeId> downloadableHandles = std::move(m_downloadableBuffers);
+    for (const Qt3DCore::QNodeId &bufferId : downloadableHandles) {
+        BufferManager *bufferManager = m_nodesManager->bufferManager();
+        BufferManager::ReadLocker locker(const_cast<const BufferManager *>(bufferManager));
+        Buffer *buffer = bufferManager->lookupResource(bufferId);
+        // Buffer could have been destroyed at this point
+        if (!buffer)
+            continue;
+        // locker is protecting us from the buffer being destroy while we're looking
+        // up its content
+        const QByteArray content = m_submissionContext->downloadBufferContent(buffer);
+        m_sendBufferCaptureJob->addRequest(QPair<Qt3DCore::QNodeId, QByteArray>(bufferId, content));
     }
 }
 
diff --git a/src/render/renderers/opengl/renderer/renderer_p.h b/src/render/renderers/opengl/renderer/renderer_p.h
index db63f31d0..7b1349503 100644
--- a/src/render/renderers/opengl/renderer/renderer_p.h
+++ b/src/render/renderers/opengl/renderer/renderer_p.h
@@ -401,7 +401,7 @@ private:
     QVector<HVao> m_abandonedVaos;
 
     QVector<HBuffer> m_dirtyBuffers;
-    QVector<HBuffer> m_downloadableBuffers;
+    QVector<Qt3DCore::QNodeId> m_downloadableBuffers;
     QVector<HShader> m_dirtyShaders;
     QVector<HTexture> m_dirtyTextures;
     QVector<QPair<Texture::TextureUpdateInfo, Qt3DCore::QNodeIdVector>> m_updatedTextureProperties;
diff --git a/tests/auto/render/renderer/tst_renderer.cpp b/tests/auto/render/renderer/tst_renderer.cpp
index 65309707c..f8125bce4 100644
--- a/tests/auto/render/renderer/tst_renderer.cpp
+++ b/tests/auto/render/renderer/tst_renderer.cpp
@@ -88,7 +88,7 @@ private Q_SLOTS:
                  1); // SendRenderCaptureJob
 
         // WHEN
-        renderer.m_sendBufferCaptureJob->addRequest({nullptr, {}});
+        renderer.m_sendBufferCaptureJob->addRequest({Qt3DCore::QNodeId(), {}});
         jobs = renderer.preRenderingJobs();
 
         // THEN