From c6d679eabc7bdb2c957f501d580f72c3023096a1 Mon Sep 17 00:00:00 2001 From: Johan Klokkhammer Helsing Date: Wed, 5 Jun 2019 13:10:38 +0200 Subject: [PATCH 1/4] Client: Don't add all windows to activePopups Neither Qt::ToolTip nor Qt::Popup are single bits in Qt::WindowFlags, and do in fact include Qt::Window. This meant that when we or'ed them and did a bitwise and with QWindow::type(), we would match more types than just Qt::Popup and Qt::ToolTip. We would for instance get any Qt::Window as well, which meant the main window would be added to activePopups, leading to strange things happening, such as crashes and the main window closing unexpectedly. [ChangeLog][QPA plugin] Fixed a crash when closing multiple popups at once. Fixes: QTBUG-76124 Change-Id: I1a6a59e161a436604a7ac8ab824396481dc99a20 Reviewed-by: Paul Olav Tvete (cherry picked from commit 4a9a5e2f62935488b1eeb06cd5d69a168a5a9308) --- src/plugins/platforms/wayland/qwaylandwindow.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/platforms/wayland/qwaylandwindow.cpp b/src/plugins/platforms/wayland/qwaylandwindow.cpp index 58e0fc5857e..cecdbda9214 100644 --- a/src/plugins/platforms/wayland/qwaylandwindow.cpp +++ b/src/plugins/platforms/wayland/qwaylandwindow.cpp @@ -393,7 +393,7 @@ QWaylandScreen *QWaylandWindow::calculateScreenFromSurfaceEvents() const void QWaylandWindow::setVisible(bool visible) { if (visible) { - if (window()->type() & (Qt::Popup | Qt::ToolTip)) + if (window()->type() == Qt::Popup || window()->type() == Qt::ToolTip) activePopups << this; initWindow(); mDisplay->flushRequests(); From 26c5d5e62b1c3642565511d1761bbc4bdda8ca1e Mon Sep 17 00:00:00 2001 From: Johan Klokkhammer Helsing Date: Wed, 19 Jun 2019 14:05:22 +0200 Subject: [PATCH 2/4] 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 --- .../platforms/wayland/qwaylandwindow.cpp | 61 ++++++++++--------- .../platforms/wayland/qwaylandwindow_p.h | 2 +- 2 files changed, 33 insertions(+), 30 deletions(-) diff --git a/src/plugins/platforms/wayland/qwaylandwindow.cpp b/src/plugins/platforms/wayland/qwaylandwindow.cpp index cecdbda9214..7c8ecadaa72 100644 --- a/src/plugins/platforms/wayland/qwaylandwindow.cpp +++ b/src/plugins/platforms/wayland/qwaylandwindow.cpp @@ -610,29 +610,34 @@ const wl_callback_listener QWaylandWindow::callbackListener = { Q_UNUSED(callback); Q_UNUSED(time); auto *window = static_cast(data); - if (window->thread() != QThread::currentThread()) - QMetaObject::invokeMethod(window, [=] { window->handleFrameCallback(); }, Qt::QueuedConnection); - else - window->handleFrameCallback(); + window->handleFrameCallback(); } }; void QWaylandWindow::handleFrameCallback() { - bool wasExposed = isExposed(); - - if (mFrameCallbackTimerId != -1) { - killTimer(mFrameCallbackTimerId); - mFrameCallbackTimerId = -1; - } - + // Stop the timer and stop waiting immediately + int timerId = mFrameCallbackTimerId.fetchAndStoreOrdered(-1); mWaitingForFrameCallback = false; - mFrameCallbackTimedOut = false; - if (!wasExposed && isExposed()) - sendExposeEvent(QRect(QPoint(), geometry().size())); - if (wasExposed && hasPendingUpdateRequest()) - deliverUpdateRequest(); + // The rest can wait until we can run it on the correct thread + auto doHandleExpose = [this, timerId]() { + if (timerId != -1) + killTimer(timerId); + + 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 (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/plugins/platforms/wayland/qwaylandwindow_p.h b/src/plugins/platforms/wayland/qwaylandwindow_p.h index c47123dc9c8..e8c9d5684cf 100644 --- a/src/plugins/platforms/wayland/qwaylandwindow_p.h +++ b/src/plugins/platforms/wayland/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; From 32b338a8dced136a4981fe136e1bd08627277bc2 Mon Sep 17 00:00:00 2001 From: David Edmundson Date: Sun, 23 Jun 2019 13:25:16 +0200 Subject: [PATCH 3/4] Client: Reset frame callback timer when hiding a window If we hide a window whilst a compositor has a pending frame to show, it's possible the compositor will not render the frame and not return the callback. If this happens on the next window expose we can be left with mFrameCallbackTimedOut still true causing isExposed() to remain false and us to not send the next buffer when we later show the window again. Change-Id: I507410415d1a930fd5fa736412055e68fdf6c1d3 Reviewed-by: Johan Helsing --- src/plugins/platforms/wayland/qwaylandwindow.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/plugins/platforms/wayland/qwaylandwindow.cpp b/src/plugins/platforms/wayland/qwaylandwindow.cpp index 7c8ecadaa72..2b243bc4409 100644 --- a/src/plugins/platforms/wayland/qwaylandwindow.cpp +++ b/src/plugins/platforms/wayland/qwaylandwindow.cpp @@ -254,6 +254,13 @@ void QWaylandWindow::reset(bool sendDestroyEvent) mFrameCallback = nullptr; } + int timerId = mFrameCallbackTimerId.fetchAndStoreOrdered(-1); + if (timerId != -1) { + killTimer(timerId); + } + mWaitingForFrameCallback = false; + mFrameCallbackTimedOut = false; + mMask = QRegion(); mQueuedBuffer = nullptr; } From 4d2c78dae4d9cd15312ee1263b57f9185c49dcfd Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Sat, 6 Jul 2019 21:05:32 +0200 Subject: [PATCH 4/4] Fix compilation with C++20 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implicit capture of 'this' in [=] is deprecated in C++20. Fix by using explicit captures. Change-Id: Ie3a94ec60d7c56b2856d201fa3d68d0670bdd7b9 Reviewed-by: Mårten Nordheim --- src/plugins/platforms/wayland/qwaylandwindow.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/plugins/platforms/wayland/qwaylandwindow.cpp b/src/plugins/platforms/wayland/qwaylandwindow.cpp index 2b243bc4409..5ea0dce1e6c 100644 --- a/src/plugins/platforms/wayland/qwaylandwindow.cpp +++ b/src/plugins/platforms/wayland/qwaylandwindow.cpp @@ -670,7 +670,7 @@ bool QWaylandWindow::waitForFrameSync(int timeout) // started by other writes int fcbId = mFrameCallbackTimerId.fetchAndStoreOrdered(-1); if (fcbId != -1) - QMetaObject::invokeMethod(this, [=] { killTimer(fcbId); }, Qt::QueuedConnection); + QMetaObject::invokeMethod(this, [this, fcbId] { killTimer(fcbId); }, Qt::QueuedConnection); return !mWaitingForFrameCallback; } @@ -1157,7 +1157,7 @@ void QWaylandWindow::handleUpdate() // ignore it if it times out before it's cleaned up by the invokeMethod call. int id = mFallbackUpdateTimerId; mFallbackUpdateTimerId = -1; - QMetaObject::invokeMethod(this, [=] { killTimer(id); }, Qt::QueuedConnection); + QMetaObject::invokeMethod(this, [this, id] { killTimer(id); }, Qt::QueuedConnection); } mFrameCallback = frame(); @@ -1168,10 +1168,10 @@ void QWaylandWindow::handleUpdate() // Stop current frame timer if any, can't use killTimer directly, see comment above. int fcbId = mFrameCallbackTimerId.fetchAndStoreOrdered(-1); if (fcbId != -1) - QMetaObject::invokeMethod(this, [=] { killTimer(fcbId); }, Qt::QueuedConnection); + QMetaObject::invokeMethod(this, [this, fcbId] { 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 + QMetaObject::invokeMethod(this, [this] { // Again; can't do it directly if (mWaitingForFrameCallback) mFrameCallbackTimerId = startTimer(100); }, Qt::QueuedConnection);