diff options
author | Paul Olav Tvete <paul.tvete@qt.io> | 2020-04-25 13:46:09 +0200 |
---|---|---|
committer | Eskil Abrahamsen Blomfeldt <eskil.abrahamsen-blomfeldt@qt.io> | 2020-10-07 11:38:43 +0000 |
commit | 98aea22d839000a377d0fc646133f76e08b44f6e (patch) | |
tree | 2c9b60a5ee74c3550340588b4617916c715afed6 | |
parent | ccfebb67f427dbbc2dc887044f906591fc313b39 (diff) | |
download | qtwayland-98aea22d839000a377d0fc646133f76e08b44f6e.tar.gz |
Fix race condition on frame callback
If the frame listener callback is moved to a different queue while the
frame event is being processed, the event will not get delivered. (It
will not even show up in the WAYLAND_DEBUG output.) This will cause
waitForFrameSync() to hang until it times out.
To avoid this, perform the move just after the callback has been
created.
This exposed an issue with single-threaded rendering, where a new
update would be started from inside the callback, resetting
mWaitingForFrameCallback before waitForFrameSync could react to it.
This caused all rendering to freeze. To avoid that problem, do not
deliver update requests directly from the frame callback.
With the callback always on a separate queue, we then have to make
sure that queue is also dispatched during the main event loop,
otherwise the events may not be processed. To do this, we need a
mutex lock. But it turns out that we no longer need a global mutex
lock as long as the frame events are being dispatched on their own
queues, but can manage with per-window locks instead.
A final thing needed is to make sure the frame callback does not
request additional updates while we are already waiting for the main
thread to process the last one. This is to avoid flooding the main
event loop with events, when the main thread is processing them at
a slower pace than the frame rate.
Fixes: QTBUG-83263
Pick-to: 5.15
Done-with: Eskil Abrahamsen Blomfeldt <eskil.abrahamsen-blomfeldt@qt.io>
Change-Id: I0db9da64fc8ced147177391c2a7999c4cc7a0d58
Reviewed-by: Eskil Abrahamsen Blomfeldt <eskil.abrahamsen-blomfeldt@qt.io>
Reviewed-by: David Edmundson <davidedmundson@kde.org>
Reviewed-by: Paul Olav Tvete <paul.tvete@qt.io>
-rw-r--r-- | src/client/qwaylanddisplay.cpp | 32 | ||||
-rw-r--r-- | src/client/qwaylanddisplay_p.h | 12 | ||||
-rw-r--r-- | src/client/qwaylandwindow.cpp | 50 | ||||
-rw-r--r-- | src/client/qwaylandwindow_p.h | 5 | ||||
-rw-r--r-- | tests/auto/client/surface/tst_surface.cpp | 2 |
5 files changed, 73 insertions, 28 deletions
diff --git a/src/client/qwaylanddisplay.cpp b/src/client/qwaylanddisplay.cpp index e39f9128..e759c5d6 100644 --- a/src/client/qwaylanddisplay.cpp +++ b/src/client/qwaylanddisplay.cpp @@ -225,6 +225,17 @@ void QWaylandDisplay::flushRequests() if (wl_display_dispatch_pending(mDisplay) < 0) checkError(); + { + QReadLocker locker(&m_frameQueueLock); + for (const FrameQueue &q : mExternalQueues) { + QMutexLocker locker(q.mutex); + while (wl_display_prepare_read_queue(mDisplay, q.queue) != 0) + wl_display_dispatch_queue_pending(mDisplay, q.queue); + wl_display_read_events(mDisplay); + wl_display_dispatch_queue_pending(mDisplay, q.queue); + } + } + wl_display_flush(mDisplay); } @@ -234,6 +245,27 @@ void QWaylandDisplay::blockingReadEvents() checkError(); } +void QWaylandDisplay::destroyFrameQueue(const QWaylandDisplay::FrameQueue &q) +{ + QWriteLocker locker(&m_frameQueueLock); + auto it = std::find_if(mExternalQueues.begin(), + mExternalQueues.end(), + [&q] (const QWaylandDisplay::FrameQueue &other){ return other.queue == q.queue; }); + Q_ASSERT(it != mExternalQueues.end()); + mExternalQueues.erase(it); + if (q.queue != nullptr) + wl_event_queue_destroy(q.queue); + delete q.mutex; +} + +QWaylandDisplay::FrameQueue QWaylandDisplay::createFrameQueue() +{ + QWriteLocker locker(&m_frameQueueLock); + FrameQueue q{createEventQueue()}; + mExternalQueues.append(q); + return q; +} + wl_event_queue *QWaylandDisplay::createEventQueue() { return wl_display_create_queue(mDisplay); diff --git a/src/client/qwaylanddisplay_p.h b/src/client/qwaylanddisplay_p.h index 68d72812..86e3b729 100644 --- a/src/client/qwaylanddisplay_p.h +++ b/src/client/qwaylanddisplay_p.h @@ -55,6 +55,8 @@ #include <QtCore/QObject> #include <QtCore/QPointer> #include <QtCore/QRect> +#include <QtCore/QMutex> +#include <QtCore/QReadWriteLock> #include <QtCore/QWaitCondition> #include <QtCore/QLoggingCategory> @@ -119,6 +121,12 @@ class Q_WAYLAND_CLIENT_EXPORT QWaylandDisplay : public QObject, public QtWayland Q_OBJECT public: + struct FrameQueue { + FrameQueue(wl_event_queue *q = nullptr) : queue(q), mutex(new QMutex) {} + wl_event_queue *queue; + QMutex *mutex; + }; + QWaylandDisplay(QWaylandIntegration *waylandIntegration); ~QWaylandDisplay(void) override; @@ -207,6 +215,8 @@ public: void handleWindowDestroyed(QWaylandWindow *window); wl_event_queue *createEventQueue(); + FrameQueue createFrameQueue(); + void destroyFrameQueue(const FrameQueue &q); void dispatchQueueWhile(wl_event_queue *queue, std::function<bool()> condition, int timeout = -1); public slots: @@ -284,8 +294,10 @@ private: QPointer<QWaylandWindow> mLastInputWindow; QPointer<QWaylandWindow> mLastKeyboardFocus; QList<QWaylandWindow *> mActiveWindows; + QList<FrameQueue> mExternalQueues; struct wl_callback *mSyncCallback = nullptr; static const wl_callback_listener syncCallbackListener; + QReadWriteLock m_frameQueueLock; bool mClientSideInputContextRequested = !QPlatformInputContextFactory::requested().isNull(); bool mUsingInputContextFromCompositor = false; diff --git a/src/client/qwaylandwindow.cpp b/src/client/qwaylandwindow.cpp index 382c258f..76f9e9f7 100644 --- a/src/client/qwaylandwindow.cpp +++ b/src/client/qwaylandwindow.cpp @@ -76,7 +76,7 @@ QWaylandWindow *QWaylandWindow::mMouseGrab = nullptr; QWaylandWindow::QWaylandWindow(QWindow *window, QWaylandDisplay *display) : QPlatformWindow(window) , mDisplay(display) - , mFrameQueue(mDisplay->createEventQueue()) + , mFrameQueue(mDisplay->createFrameQueue()) , mResizeAfterSwap(qEnvironmentVariableIsSet("QT_WAYLAND_RESIZE_AFTER_SWAP")) { { @@ -95,6 +95,7 @@ QWaylandWindow::QWaylandWindow(QWindow *window, QWaylandDisplay *display) QWaylandWindow::~QWaylandWindow() { + mDisplay->destroyFrameQueue(mFrameQueue); mDisplay->handleWindowDestroyed(this); delete mWindowDecoration; @@ -624,33 +625,29 @@ void QWaylandWindow::handleFrameCallback() mFrameCallbackElapsedTimer.invalidate(); // The rest can wait until we can run it on the correct thread - auto doHandleExpose = [this]() { - 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(); + if (!mWaitingForUpdateDelivery) { + auto doHandleExpose = [this]() { + bool wasExposed = isExposed(); + mFrameCallbackTimedOut = false; + if (!wasExposed && isExposed()) // Did setting mFrameCallbackTimedOut make the window exposed? + sendExposeEvent(QRect(QPoint(), geometry().size())); + if (wasExposed && hasPendingUpdateRequest()) + deliverUpdateRequest(); + + mWaitingForUpdateDelivery = false; + }; + + // Queued connection, to make sure we don't call handleUpdate() from inside waitForFrameSync() + // in the single-threaded case. + mWaitingForUpdateDelivery = true; + QMetaObject::invokeMethod(this, doHandleExpose, Qt::QueuedConnection); } } -QMutex QWaylandWindow::mFrameSyncMutex; - bool QWaylandWindow::waitForFrameSync(int timeout) { - if (!mWaitingForFrameCallback) - return true; - - QMutexLocker locker(&mFrameSyncMutex); - - wl_proxy_set_queue(reinterpret_cast<wl_proxy *>(mFrameCallback), mFrameQueue); - mDisplay->dispatchQueueWhile(mFrameQueue, [&]() { return mWaitingForFrameCallback; }, timeout); + QMutexLocker locker(mFrameQueue.mutex); + mDisplay->dispatchQueueWhile(mFrameQueue.queue, [&]() { return mWaitingForFrameCallback; }, timeout); if (mWaitingForFrameCallback) { qCDebug(lcWaylandBackingstore) << "Didn't receive frame callback in time, window should now be inexposed"; @@ -1124,6 +1121,7 @@ void QWaylandWindow::timerEvent(QTimerEvent *event) } if (mFrameCallbackElapsedTimer.isValid() && callbackTimerExpired) { mFrameCallbackElapsedTimer.invalidate(); + qCDebug(lcWaylandBackingstore) << "Didn't receive frame callback in time, window should now be inexposed"; mFrameCallbackTimedOut = true; mWaitingForUpdate = false; @@ -1172,7 +1170,11 @@ void QWaylandWindow::handleUpdate() mFrameCallback = nullptr; } - mFrameCallback = mSurface->frame(); + QMutexLocker locker(mFrameQueue.mutex); + struct ::wl_surface *wrappedSurface = reinterpret_cast<struct ::wl_surface *>(wl_proxy_create_wrapper(mSurface->object())); + wl_proxy_set_queue(reinterpret_cast<wl_proxy *>(wrappedSurface), mFrameQueue.queue); + mFrameCallback = wl_surface_frame(wrappedSurface); + wl_proxy_wrapper_destroy(wrappedSurface); wl_callback_add_listener(mFrameCallback, &QWaylandWindow::callbackListener, this); mWaitingForFrameCallback = true; mWaitingForUpdate = false; diff --git a/src/client/qwaylandwindow_p.h b/src/client/qwaylandwindow_p.h index 9272d368..8b9a11e7 100644 --- a/src/client/qwaylandwindow_p.h +++ b/src/client/qwaylandwindow_p.h @@ -63,6 +63,7 @@ #include <qpa/qplatformwindow.h> #include <QtWaylandClient/private/qwayland-wayland.h> +#include <QtWaylandClient/private/qwaylanddisplay_p.h> #include <QtWaylandClient/qtwaylandclientglobal.h> struct wl_egl_window; @@ -226,10 +227,11 @@ protected: WId mWindowId; bool mWaitingForFrameCallback = false; bool mFrameCallbackTimedOut = false; // Whether the frame callback has timed out + bool mWaitingForUpdateDelivery = false; int mFrameCallbackCheckIntervalTimerId = -1; QElapsedTimer mFrameCallbackElapsedTimer; struct ::wl_callback *mFrameCallback = nullptr; - struct ::wl_event_queue *mFrameQueue = nullptr; + QWaylandDisplay::FrameQueue mFrameQueue; QWaitCondition mFrameSyncWait; // True when we have called deliverRequestUpdate, but the client has not yet attached a new buffer @@ -280,7 +282,6 @@ private: static const wl_callback_listener callbackListener; void handleFrameCallback(); - static QMutex mFrameSyncMutex; static QWaylandWindow *mMouseGrab; QReadWriteLock mSurfaceLock; diff --git a/tests/auto/client/surface/tst_surface.cpp b/tests/auto/client/surface/tst_surface.cpp index 289fbd62..c7b02e2c 100644 --- a/tests/auto/client/surface/tst_surface.cpp +++ b/tests/auto/client/surface/tst_surface.cpp @@ -64,7 +64,6 @@ void tst_surface::createDestroySurface() void tst_surface::waitForFrameCallbackRaster() { - QSKIP("TODO: This currently fails, needs a fix"); class TestWindow : public QRasterWindow { public: explicit TestWindow() { resize(40, 40); } @@ -100,7 +99,6 @@ void tst_surface::waitForFrameCallbackRaster() #if QT_CONFIG(opengl) void tst_surface::waitForFrameCallbackGl() { - QSKIP("TODO: This currently fails, needs a fix"); class TestWindow : public QOpenGLWindow { public: explicit TestWindow() |