Renderer: fix OnDemand rendering

Note: this is a risky change

For some reason the renderer assumed that a RenderQueue would never have a
size of 0. This prevented the renderer from calling proceedToNextFrame() on
the vsync advance service as it somehow felt that it was a case of rendering
with a RenderQueue not yet ready. In the case of OnDemand rendering it is
perfectly valid to have a RenderQueue of 0 (nothing has changed that requires
rendering) but we still need to call proceedToNextFrame() otherwise
syncChanges() is never called and we end up never updating the aspects ever
again.

The renderer was updated to
1) check if a RenderQueue is complete
2) check if the RenderQueue is empty.
(a RenderQueue can be complete but empty (OnDemand case))
3) Proceed to next frame if RenderQueue is complete or RenderQueue is empty.

Change-Id: I27ae778831c9b136db1e1a69892f6fde291fd965
Task-number: QTBUG-59696
Task-number: QTBUG-54900
Reviewed-by: Sean Harmer <sean.harmer@kdab.com>

5 files changed, 89 insertions, 76 deletions

diff --git a/src/render/backend/renderer.cpp b/src/render/backend/renderer.cpp
index e40d5d68e..4080a72eb 100644
--- a/src/render/backend/renderer.cpp
+++ b/src/render/backend/renderer.cpp
@@ -536,17 +536,26 @@ void Renderer::render()
 
 void Renderer::doRender()
 {
-    bool submissionSucceeded = false;
-    bool hasCleanedQueueAndProceeded = false;
     Renderer::ViewSubmissionResultData submissionData;
+    bool hasCleanedQueueAndProceeded = false;
     bool preprocessingComplete = false;
-
-    if (isReadyToSubmit()) {
-
-        // Lock the mutex to protect access to m_surface and check if we are still set
-        // to the running state and that we have a valid surface on which to draw
-        // TO DO: Is that still needed given the surface changes
-        QMutexLocker locker(&m_mutex);
+    bool beganDrawing = false;
+    const bool canSubmit = isReadyToSubmit();
+
+    // Lock the mutex to protect access to the renderQueue while we look for its state
+    QMutexLocker locker(&m_renderQueueMutex);
+    const bool queueIsComplete = m_renderQueue->isFrameQueueComplete();
+    const bool queueIsEmpty = m_renderQueue->targetRenderViewCount() == 0;
+
+    // When using synchronous rendering (QtQuick)
+    // We are not sure that the frame queue is actually complete
+    // Since a call to render may not be synched with the completions
+    // of the RenderViewJobs
+    // In such a case we return early, waiting for a next call with
+    // the frame queue complete at this point
+
+    // RenderQueue is complete (but that means it may be of size 0)
+    if (canSubmit && (queueIsComplete && !queueIsEmpty)) {
         const QVector<Render::RenderView *> renderViews = m_renderQueue->nextFrameQueue();
 
 #ifdef QT3D_JOBS_RUN_STATS
@@ -561,8 +570,7 @@ void Renderer::doRender()
         submissionStatsPart2.jobId.typeAndInstance[1] = 0;
         submissionStatsPart2.threadId = reinterpret_cast<quint64>(QThread::currentThreadId());
 #endif
-
-        if (canRender() && (submissionSucceeded = renderViews.size() > 0) == true) {
+        if (canRender()) {
             // Clear all dirty flags but Compute so that
             // we still render every frame when a compute shader is used in a scene
             BackendNodeDirtySet changesToUnset = m_changeSet;
@@ -584,7 +592,8 @@ void Renderer::doRender()
                     // Reset state for each draw if we don't have complete control of the context
                     if (!m_ownedContext)
                         m_graphicsContext->setCurrentStateSet(nullptr);
-                    if (m_graphicsContext->beginDrawing(surface)) {
+                    beganDrawing = m_graphicsContext->beginDrawing(surface);
+                    if (beganDrawing) {
                         // 1) Execute commands for buffer uploads, texture updates, shader loading first
                         updateGLResources();
                         // 2) Update VAO and copy data into commands to allow concurrent submission
@@ -595,6 +604,7 @@ void Renderer::doRender()
             }
             // 2) Proceed to next frame and start preparing frame n + 1
             m_renderQueue->reset();
+            locker.unlock(); // Done protecting RenderQueue
             m_vsyncFrameAdvanceService->proceedToNextFrame();
             hasCleanedQueueAndProceeded = true;
 
@@ -638,61 +648,57 @@ void Renderer::doRender()
 #endif
     }
 
-    // Note: submissionSucceeded is false when
-    // * we cannot render because a shutdown has been scheduled
-    // * the renderqueue is incomplete (only when rendering with a Scene3D)
-    // Otherwise returns true even for cases like
-    // * No render view
-    // * No surface set
-    // * OpenGLContext failed to be set current
-    // This behavior is important as we need to
-    // call proceedToNextFrame despite rendering errors that aren't fatal
-
     // Only reset renderQueue and proceed to next frame if the submission
-    // succeeded or it we are using a render thread and that is wasn't performed
+    // succeeded or if we are using a render thread and that is wasn't performed
     // already
 
-    // If submissionSucceeded isn't true this implies that something went wrong
+    // If hasCleanedQueueAndProceeded isn't true this implies that something went wrong
     // with the rendering and/or the renderqueue is incomplete from some reason
     // (in the case of scene3d the render jobs may be taking too long ....)
-    if (m_renderThread || submissionSucceeded) {
-
-        if (!hasCleanedQueueAndProceeded) {
-            // Reset the m_renderQueue so that we won't try to render
-            // with a queue used by a previous frame with corrupted content
-            // if the current queue was correctly submitted
-            m_renderQueue->reset();
-
-            // We allow the RenderTickClock service to proceed to the next frame
-            // In turn this will allow the aspect manager to request a new set of jobs
-            // to be performed for each aspect
-            m_vsyncFrameAdvanceService->proceedToNextFrame();
-        }
+    // or alternatively it could be complete but empty (RenderQueue of size 0)
+    if (!hasCleanedQueueAndProceeded &&
+        (m_renderThread || queueIsComplete || queueIsEmpty)) {
+        // RenderQueue was full but something bad happened when
+        // trying to render it and therefore proceedToNextFrame was not called
+        // Note: in this case the renderQueue mutex is still locked
+
+        // Reset the m_renderQueue so that we won't try to render
+        // with a queue used by a previous frame with corrupted content
+        // if the current queue was correctly submitted
+        m_renderQueue->reset();
+
+        // We allow the RenderTickClock service to proceed to the next frame
+        // In turn this will allow the aspect manager to request a new set of jobs
+        // to be performed for each aspect
+        m_vsyncFrameAdvanceService->proceedToNextFrame();
+    }
 
-        // Perform the last swapBuffers calls after the proceedToNextFrame
-        // as this allows us to gain a bit of time for the preparation of the
-        // next frame
+    // Perform the last swapBuffers calls after the proceedToNextFrame
+    // as this allows us to gain a bit of time for the preparation of the
+    // next frame
+    // Finish up with last surface used in the list of RenderViews
+    if (beganDrawing) {
+        SurfaceLocker surfaceLock(submissionData.surface);
         // Finish up with last surface used in the list of RenderViews
-        if (submissionSucceeded) {
-            SurfaceLocker surfaceLock(submissionData.surface);
-            // Finish up with last surface used in the list of RenderViews
-            m_graphicsContext->endDrawing(submissionData.lastBoundFBOId == m_graphicsContext->defaultFBO() && surfaceLock.isSurfaceValid());
-        }
+        m_graphicsContext->endDrawing(submissionData.lastBoundFBOId == m_graphicsContext->defaultFBO() && surfaceLock.isSurfaceValid());
     }
 }
 
 // Called by RenderViewJobs
+// When the frameQueue is complete and we are using a renderThread
+// we allow the render thread to proceed
 void Renderer::enqueueRenderView(Render::RenderView *renderView, int submitOrder)
 {
-    QMutexLocker locker(&m_mutex); // Prevent out of order execution
+    QMutexLocker locker(&m_renderQueueMutex); // Prevent out of order execution
     // We cannot use a lock free primitive here because:
     // - QVector is not thread safe
     // - Even if the insert is made correctly, the isFrameComplete call
     //   could be invalid since depending on the order of execution
     //   the counter could be complete but the renderview not yet added to the
     //   buffer depending on whichever order the cpu decides to process this
-
-    if (m_renderQueue->queueRenderView(renderView, submitOrder)) {
+    const bool isQueueComplete = m_renderQueue->queueRenderView(renderView, submitOrder);
+    locker.unlock(); // We're done protecting the queue at this point
+    if (isQueueComplete) {
         if (m_renderThread && m_running.load())
             Q_ASSERT(m_submitRenderViewsSemaphore.available() == 0);
         m_submitRenderViewsSemaphore.release(1);
@@ -731,16 +737,6 @@ bool Renderer::isReadyToSubmit()
         // something to render
         // The case of shutdown should have been handled just before
         Q_ASSERT(m_renderQueue->isFrameQueueComplete());
-    } else {
-        // When using synchronous rendering (QtQuick)
-        // We are not sure that the frame queue is actually complete
-        // Since a call to render may not be synched with the completions
-        // of the RenderViewJobs
-        // In such a case we return early, waiting for a next call with
-        // the frame queue complete at this point
-        QMutexLocker locker(&m_mutex);
-        if (!m_renderQueue->isFrameQueueComplete())
-            return false;
     }
     return true;
 }
@@ -1431,22 +1427,25 @@ QVector<Qt3DCore::QAspectJobPtr> Renderer::renderBinJobs()
     renderBinJobs.push_back(m_textureGathererJob);
     renderBinJobs.push_back(m_shaderGathererJob);
 
-    // Traverse the current framegraph. For each leaf node create a
-    // RenderView and set its configuration then create a job to
-    // populate the RenderView with a set of RenderCommands that get
-    // their details from the RenderNodes that are visible to the
-    // Camera selected by the framegraph configuration
-    FrameGraphVisitor visitor(m_nodesManager->frameGraphManager());
-    const QVector<FrameGraphNode *> fgLeaves = visitor.traverse(frameGraphRoot());
-
-    const int fgBranchCount = fgLeaves.size();
-    for (int i = 0; i < fgBranchCount; ++i) {
-        RenderViewBuilder builder(fgLeaves.at(i), i, this);
-        renderBinJobs.append(builder.buildJobHierachy());
-    }
+    QMutexLocker lock(&m_renderQueueMutex);
+    if (m_renderQueue->wasReset()) { // Have we rendered yet? (Scene3D case)
+        // Traverse the current framegraph. For each leaf node create a
+        // RenderView and set its configuration then create a job to
+        // populate the RenderView with a set of RenderCommands that get
+        // their details from the RenderNodes that are visible to the
+        // Camera selected by the framegraph configuration
+        FrameGraphVisitor visitor(m_nodesManager->frameGraphManager());
+        const QVector<FrameGraphNode *> fgLeaves = visitor.traverse(frameGraphRoot());
+
+        const int fgBranchCount = fgLeaves.size();
+        for (int i = 0; i < fgBranchCount; ++i) {
+            RenderViewBuilder builder(fgLeaves.at(i), i, this);
+            renderBinJobs.append(builder.buildJobHierachy());
+        }
 
-    // Set target number of RenderViews
-    m_renderQueue->setTargetRenderViewCount(fgBranchCount);
+        // Set target number of RenderViews
+        m_renderQueue->setTargetRenderViewCount(fgBranchCount);
+    }
 
     return renderBinJobs;
 }
diff --git a/src/render/backend/renderer_p.h b/src/render/backend/renderer_p.h
index 9df24ef2c..af10108e9 100644
--- a/src/render/backend/renderer_p.h
+++ b/src/render/backend/renderer_p.h
@@ -259,7 +259,7 @@ public:
 
     ViewSubmissionResultData submitRenderViews(const QVector<Render::RenderView *> &renderViews);
 
-    QMutex* mutex() { return &m_mutex; }
+    QMutex* mutex() { return &m_renderQueueMutex; }
 
 
 #ifdef QT3D_RENDER_UNIT_TESTS
@@ -290,7 +290,7 @@ private:
     QScopedPointer<RenderThread> m_renderThread;
     QScopedPointer<VSyncFrameAdvanceService> m_vsyncFrameAdvanceService;
 
-    QMutex m_mutex;
+    QMutex m_renderQueueMutex;
     QSemaphore m_submitRenderViewsSemaphore;
     QSemaphore m_waitForInitializationToBeCompleted;
 
diff --git a/src/render/backend/renderqueue.cpp b/src/render/backend/renderqueue.cpp
index 6ec7da464..2fa1cb7d2 100644
--- a/src/render/backend/renderqueue.cpp
+++ b/src/render/backend/renderqueue.cpp
@@ -49,6 +49,7 @@ namespace Render {
 
 RenderQueue::RenderQueue()
     : m_noRender(false)
+    , m_wasReset(true)
     , m_targetRenderViewCount(0)
     , m_currentRenderViewCount(0)
     , m_currentWorkQueue(1)
@@ -70,6 +71,7 @@ void RenderQueue::reset()
     m_targetRenderViewCount = 0;
     m_currentWorkQueue.clear();
     m_noRender = false;
+    m_wasReset = true;
 }
 
 void RenderQueue::setNoRender()
@@ -88,6 +90,7 @@ bool RenderQueue::queueRenderView(RenderView *renderView, uint submissionOrderIn
     Q_ASSERT(!m_noRender);
     m_currentWorkQueue[submissionOrderIndex] = renderView;
     ++m_currentRenderViewCount;
+    Q_ASSERT(m_currentRenderViewCount <= m_targetRenderViewCount);
     return isFrameQueueComplete();
 }
 
@@ -109,6 +112,7 @@ void RenderQueue::setTargetRenderViewCount(int targetRenderViewCount)
     Q_ASSERT(!m_noRender);
     m_targetRenderViewCount = targetRenderViewCount;
     m_currentWorkQueue.resize(targetRenderViewCount);
+    m_wasReset = false;
 }
 
 /*!
@@ -119,7 +123,7 @@ void RenderQueue::setTargetRenderViewCount(int targetRenderViewCount)
 bool RenderQueue::isFrameQueueComplete() const
 {
     return (m_noRender
-            || (m_targetRenderViewCount && m_targetRenderViewCount == currentRenderViewCount()));
+            || (m_targetRenderViewCount > 0 && m_targetRenderViewCount == m_currentRenderViewCount));
 }
 
 } // namespace Render
diff --git a/src/render/backend/renderqueue_p.h b/src/render/backend/renderqueue_p.h
index 49316049b..611f5849a 100644
--- a/src/render/backend/renderqueue_p.h
+++ b/src/render/backend/renderqueue_p.h
@@ -77,9 +77,13 @@ public:
     void reset();
 
     void setNoRender();
+    inline bool isNoRender() const { return m_noRender; }
+
+    inline bool wasReset() const { return m_wasReset; }
 
 private:
     bool m_noRender;
+    bool m_wasReset;
     int m_targetRenderViewCount;
     int m_currentRenderViewCount;
     QVector<RenderView *> m_currentWorkQueue;
diff --git a/tests/auto/render/renderqueue/tst_renderqueue.cpp b/tests/auto/render/renderqueue/tst_renderqueue.cpp
index 2d25cbe57..163a699c1 100644
--- a/tests/auto/render/renderqueue/tst_renderqueue.cpp
+++ b/tests/auto/render/renderqueue/tst_renderqueue.cpp
@@ -55,10 +55,14 @@ void tst_RenderQueue::setRenderViewCount()
     // GIVEN
     Qt3DRender::Render::RenderQueue renderQueue;
 
+    // THEN
+    QCOMPARE(renderQueue.wasReset(), true);
+
     // WHEN
     renderQueue.setTargetRenderViewCount(7);
 
     // THEN
+    QCOMPARE(renderQueue.wasReset(), false);
     QVERIFY(renderQueue.targetRenderViewCount() == 7);
     QVERIFY(renderQueue.currentRenderViewCount()== 0);
 }
@@ -214,6 +218,7 @@ void tst_RenderQueue::resetQueue()
         // WHEN
         renderQueue.setTargetRenderViewCount(5);
         // THEN
+        QCOMPARE(renderQueue.wasReset(), false);
         QVERIFY(renderQueue.currentRenderViewCount() == 0);
 
         // WHEN
@@ -227,6 +232,7 @@ void tst_RenderQueue::resetQueue()
 
         // WHEN
         renderQueue.reset();
+        QCOMPARE(renderQueue.wasReset(), true);
         // THEN
         QVERIFY(renderQueue.currentRenderViewCount() == 0);
     }