VSyncFrameAdvanceService: fix Scene3D rendering

When rendering with Scene3D it could be that doRender() is called a lot more
times than the speed at which the Qt3D simulation loop is running (this
happens when resizing the window)

This results in vsyncFrameAdvance->proceedToNextFrame() being called while the
simulation isn't in a waiting state (simulation loop not yet blocking on
vsyncFrameAdvance->waitForNextFrame()).

This means that when the simulation loop finally unlocks, it would be able to
make several loops for a given frame. Obviously this is really dangerous as
the simulation loop drives the synching of changes and this could result in
the data being prepared by the render jobs and the actual data in the managers
to be corrupt.

To fix that, vsyncFrameAdvance->waitForNextFrame() now acquires all available
resources + 1 to cope with proceedToNextFrame being called more than once.

Obviously this doesn't happen when Qt3D drives the rendering and the
VSyncFrameAdvanceService was adjusted to have different behaviors depending on
whether we are rendering with Scene3D or not.

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

5 files changed, 102 insertions, 7 deletions

diff --git a/src/render/backend/renderer.cpp b/src/render/backend/renderer.cpp
index 8e9f7b429..889888945 100644
--- a/src/render/backend/renderer.cpp
+++ b/src/render/backend/renderer.cpp
@@ -157,7 +157,7 @@ Renderer::Renderer(QRenderAspect::RenderType type)
     , m_graphicsContext(nullptr)
     , m_renderQueue(new RenderQueue())
     , m_renderThread(type == QRenderAspect::Threaded ? new RenderThread(this) : nullptr)
-    , m_vsyncFrameAdvanceService(new VSyncFrameAdvanceService())
+    , m_vsyncFrameAdvanceService(new VSyncFrameAdvanceService(m_renderThread != nullptr))
     , m_waitForInitializationToBeCompleted(0)
     , m_pickEventFilter(new PickEventFilter())
     , m_exposed(0)
diff --git a/src/render/frontend/qrenderaspect.cpp b/src/render/frontend/qrenderaspect.cpp
index a059efc0e..3c2b36d85 100644
--- a/src/render/frontend/qrenderaspect.cpp
+++ b/src/render/frontend/qrenderaspect.cpp
@@ -361,6 +361,7 @@ QRenderAspect::~QRenderAspect()
 {
 }
 
+// Called by Scene3DRenderer only
 void QRenderAspectPrivate::renderInitialize(QOpenGLContext *context)
 {
     if (m_renderer->api() == Render::AbstractRenderer::OpenGL)
diff --git a/src/render/services/vsyncframeadvanceservice.cpp b/src/render/services/vsyncframeadvanceservice.cpp
index 533b0fba1..880985aea 100644
--- a/src/render/services/vsyncframeadvanceservice.cpp
+++ b/src/render/services/vsyncframeadvanceservice.cpp
@@ -52,20 +52,22 @@ namespace Render {
 class VSyncFrameAdvanceServicePrivate Q_DECL_FINAL : public Qt3DCore::QAbstractFrameAdvanceServicePrivate
 {
 public:
-    VSyncFrameAdvanceServicePrivate()
+    explicit VSyncFrameAdvanceServicePrivate(bool drivenByRenderThread)
         : QAbstractFrameAdvanceServicePrivate(QStringLiteral("Renderer Aspect Frame Advance Service - aligned with vsync"))
         , m_semaphore(0)
         , m_elapsedTimeSincePreviousFrame(0)
+        , m_drivenByRenderThread(drivenByRenderThread)
     {
     }
 
     QSemaphore m_semaphore;
     QElapsedTimer m_elapsed;
     quint64 m_elapsedTimeSincePreviousFrame;
+    bool m_drivenByRenderThread;
 };
 
-VSyncFrameAdvanceService::VSyncFrameAdvanceService()
-    : QAbstractFrameAdvanceService(*new VSyncFrameAdvanceServicePrivate())
+VSyncFrameAdvanceService::VSyncFrameAdvanceService(bool drivenByRenderThread)
+    : QAbstractFrameAdvanceService(*new VSyncFrameAdvanceServicePrivate(drivenByRenderThread))
 {
 }
 
@@ -77,7 +79,17 @@ VSyncFrameAdvanceService::~VSyncFrameAdvanceService()
 qint64 VSyncFrameAdvanceService::waitForNextFrame()
 {
     Q_D(VSyncFrameAdvanceService);
-    d->m_semaphore.acquire(1);
+
+    // When rendering with Scene3D, we always want to acquire the available
+    // amount + 1 to handle the cases where for some reason proceedToNextFrame
+    // is being called more than once between calls to waitForNextFrame This
+    // could be the case when resizing the window
+
+    // When Qt3D is driving rendering however, this shouldn't happen
+    if (d->m_drivenByRenderThread)
+        d->m_semaphore.acquire(1);
+    else
+        d->m_semaphore.acquire(d->m_semaphore.available() + 1);
 
     const quint64 currentTime = d->m_elapsed.nsecsElapsed();
     qCDebug(VSyncAdvanceService) << "Elapsed nsecs since last call " << currentTime - d->m_elapsedTimeSincePreviousFrame;
diff --git a/src/render/services/vsyncframeadvanceservice_p.h b/src/render/services/vsyncframeadvanceservice_p.h
index 98daebf81..f33d41b98 100644
--- a/src/render/services/vsyncframeadvanceservice_p.h
+++ b/src/render/services/vsyncframeadvanceservice_p.h
@@ -64,7 +64,7 @@ class VSyncFrameAdvanceServicePrivate;
 class Q_AUTOTEST_EXPORT VSyncFrameAdvanceService Q_DECL_FINAL : public Qt3DCore::QAbstractFrameAdvanceService
 {
 public:
-    VSyncFrameAdvanceService();
+    explicit VSyncFrameAdvanceService(bool drivenByRenderThread);
     ~VSyncFrameAdvanceService();
 
     qint64 waitForNextFrame() Q_DECL_FINAL;
diff --git a/tests/auto/render/vsyncframeadvanceservice/tst_vsyncframeadvanceservice.cpp b/tests/auto/render/vsyncframeadvanceservice/tst_vsyncframeadvanceservice.cpp
index 3d873258f..6f455ea56 100644
--- a/tests/auto/render/vsyncframeadvanceservice/tst_vsyncframeadvanceservice.cpp
+++ b/tests/auto/render/vsyncframeadvanceservice/tst_vsyncframeadvanceservice.cpp
@@ -50,6 +50,63 @@ private:
     Qt3DRender::Render::VSyncFrameAdvanceService *m_tickService;
 };
 
+class FakeAspectThread Q_DECL_FINAL : public QThread
+{
+public:
+    FakeAspectThread(Qt3DRender::Render::VSyncFrameAdvanceService *tickService)
+        : m_tickService(tickService)
+        , m_count(0)
+        , m_running(true)
+        , m_waitForStarted(0)
+    {
+    }
+
+    int count() const { return m_count; }
+
+    void stopRunning()
+    {
+        QMutexLocker lock(&m_mutex);
+        m_running = false;
+    }
+
+    void waitForStarted()
+    {
+        m_waitForStarted.acquire(1);
+    }
+
+protected:
+    // QThread interface
+    void run() Q_DECL_FINAL
+    {
+        m_waitForStarted.release(1);
+        while (true) {
+
+            bool running = true;
+            {
+                QMutexLocker lock(&m_mutex);
+                running = m_running;
+            }
+
+            if (!running) {
+                qDebug() << "exiting";
+                return;
+            }
+
+            m_tickService->waitForNextFrame();
+            ++m_count;
+
+            QThread::msleep(100);
+        }
+    }
+
+private:
+    Qt3DRender::Render::VSyncFrameAdvanceService *m_tickService;
+    int m_count;
+    bool m_running;
+    QMutex m_mutex;
+    QSemaphore m_waitForStarted;
+};
+
 class tst_VSyncFrameAdvanceService : public QObject
 {
     Q_OBJECT
@@ -59,7 +116,7 @@ private Q_SLOTS:
     void checkSynchronisation()
     {
         // GIVEN
-        Qt3DRender::Render::VSyncFrameAdvanceService tickService;
+        Qt3DRender::Render::VSyncFrameAdvanceService tickService(true);
         FakeRenderThread renderThread(&tickService);
         QElapsedTimer t;
 
@@ -74,6 +131,31 @@ private Q_SLOTS:
         QVERIFY(t.elapsed() >= 950);
     }
 
+    void checkWaitForNextFrame()
+    {
+        // GIVEN
+        Qt3DRender::Render::VSyncFrameAdvanceService tickService(false);
+        FakeAspectThread aspectThread(&tickService);
+
+        // WHEN
+        aspectThread.start();
+        aspectThread.waitForStarted();
+
+        QElapsedTimer t;
+        t.start();
+
+        while (t.elapsed() < 1000)
+            tickService.proceedToNextFrame();
+
+        aspectThread.stopRunning();
+
+        // To make sure the aspectThread can finish
+        tickService.proceedToNextFrame();
+        aspectThread.wait();
+
+        // THEN
+        QCOMPARE(aspectThread.count(), 10);
+    }
 };
 
 QTEST_MAIN(tst_VSyncFrameAdvanceService)