From ec9057081f1094fbfeb11449bc533997731e4079 Mon Sep 17 00:00:00 2001
From: Johan Klokkhammer Helsing <johan.helsing@qt.io>
Date: Wed, 19 Jun 2019 14:05:22 +0200
Subject: [PATCH] Client: Fix stuttering when the GUI thread is busy

When we did invokeMethod for handling the frame callbacks, we had to wait for
the GUI thread to finish whatever it's doing before we would stop blocking.

Fix it by clearing the frame callback timer and stop blocking immediately,
while delaying the rest of the work until it can be run on the other thread.

Fixes: QTBUG-76397
Change-Id: I343e4feac4838926b4fa2ccac2948988bc6c3bb7
Reviewed-by: Paul Olav Tvete <paul.tvete@qt.io>
---
 src/client/qwaylandwindow.cpp | 59 +++++++++++++++++++++++--------------------
 src/client/qwaylandwindow_p.h |  2 +-
 2 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/src/client/qwaylandwindow.cpp b/src/client/qwaylandwindow.cpp
index cecdbda92..7c8ecadaa 100644
--- a/src/client/qwaylandwindow.cpp
+++ b/src/client/qwaylandwindow.cpp
@@ -610,29 +610,34 @@ const wl_callback_listener QWaylandWindow::callbackListener = {
         Q_UNUSED(callback);
         Q_UNUSED(time);
         auto *window = static_cast<QWaylandWindow*>(data);
-        if (window->thread() != QThread::currentThread())
-            QMetaObject::invokeMethod(window, [=] { window->handleFrameCallback(); }, Qt::QueuedConnection);
-        else
-            window->handleFrameCallback();
+        window->handleFrameCallback();
     }
 };
 
 void QWaylandWindow::handleFrameCallback()
 {
-    bool wasExposed = isExposed();
+    // Stop the timer and stop waiting immediately
+    int timerId = mFrameCallbackTimerId.fetchAndStoreOrdered(-1);
+    mWaitingForFrameCallback = false;
 
-    if (mFrameCallbackTimerId != -1) {
-        killTimer(mFrameCallbackTimerId);
-        mFrameCallbackTimerId = -1;
-    }
+    // The rest can wait until we can run it on the correct thread
+    auto doHandleExpose = [this, timerId]() {
+        if (timerId != -1)
+            killTimer(timerId);
 
-    mWaitingForFrameCallback = false;
-    mFrameCallbackTimedOut = false;
+        bool wasExposed = isExposed();
+        mFrameCallbackTimedOut = false;
+        if (!wasExposed && isExposed()) // Did setting mFrameCallbackTimedOut make the window exposed?
+            sendExposeEvent(QRect(QPoint(), geometry().size()));
+        if (wasExposed && hasPendingUpdateRequest())
+            deliverUpdateRequest();
+    };
 
-    if (!wasExposed && isExposed())
-        sendExposeEvent(QRect(QPoint(), geometry().size()));
-    if (wasExposed && hasPendingUpdateRequest())
-        deliverUpdateRequest();
+    if (thread() != QThread::currentThread()) {
+        QMetaObject::invokeMethod(this, doHandleExpose);
+    } else {
+        doHandleExpose();
+    }
 }
 
 QMutex QWaylandWindow::mFrameSyncMutex;
@@ -654,11 +659,11 @@ bool QWaylandWindow::waitForFrameSync(int timeout)
     }
 
     // Stop current frame timer if any, can't use killTimer directly, because we might be on a diffent thread
-    if (mFrameCallbackTimerId != -1) {
-        int id = mFrameCallbackTimerId;
-        mFrameCallbackTimerId = -1;
-        QMetaObject::invokeMethod(this, [=] { killTimer(id); }, Qt::QueuedConnection);
-    }
+    // Ordered semantics is needed to avoid stopping the timer twice and not miss it when it's
+    // started by other writes
+    int fcbId = mFrameCallbackTimerId.fetchAndStoreOrdered(-1);
+    if (fcbId != -1)
+        QMetaObject::invokeMethod(this, [=] { killTimer(fcbId); }, Qt::QueuedConnection);
 
     return !mWaitingForFrameCallback;
 }
@@ -1090,9 +1095,9 @@ void QWaylandWindow::timerEvent(QTimerEvent *event)
         }
     }
 
-    if (event->timerId() == mFrameCallbackTimerId) {
-        killTimer(mFrameCallbackTimerId);
-        mFrameCallbackTimerId = -1;
+
+    if (mFrameCallbackTimerId.testAndSetOrdered(event->timerId(), -1)) {
+        killTimer(event->timerId());
         qCDebug(lcWaylandBackingstore) << "Didn't receive frame callback in time, window should now be inexposed";
         mFrameCallbackTimedOut = true;
         mWaitingForUpdate = false;
@@ -1154,11 +1159,9 @@ void QWaylandWindow::handleUpdate()
     mWaitingForUpdate = false;
 
     // Stop current frame timer if any, can't use killTimer directly, see comment above.
-    if (mFrameCallbackTimerId != -1) {
-        int id = mFrameCallbackTimerId;
-        mFrameCallbackTimerId = -1;
-        QMetaObject::invokeMethod(this, [=] { killTimer(id); }, Qt::QueuedConnection);
-    }
+    int fcbId = mFrameCallbackTimerId.fetchAndStoreOrdered(-1);
+    if (fcbId != -1)
+        QMetaObject::invokeMethod(this, [=] { killTimer(fcbId); }, Qt::QueuedConnection);
 
     // Start a timer for handling the case when the compositor stops sending frame callbacks.
     QMetaObject::invokeMethod(this, [=] { // Again; can't do it directly
diff --git a/src/client/qwaylandwindow_p.h b/src/client/qwaylandwindow_p.h
index c47123dc9..e8c9d5684 100644
--- a/src/client/qwaylandwindow_p.h
+++ b/src/client/qwaylandwindow_p.h
@@ -216,7 +216,7 @@ protected:
     WId mWindowId;
     bool mWaitingForFrameCallback = false;
     bool mFrameCallbackTimedOut = false; // Whether the frame callback has timed out
-    int mFrameCallbackTimerId = -1; // Started on commit, reset on frame callback
+    QAtomicInt mFrameCallbackTimerId = -1; // Started on commit, reset on frame callback
     struct ::wl_callback *mFrameCallback = nullptr;
     struct ::wl_event_queue *mFrameQueue = nullptr;
     QWaitCondition mFrameSyncWait;
-- 
2.16.3