From ffa4f312c750ae0e7e36a656612148b1fa9c020c Mon Sep 17 00:00:00 2001 From: Vlad Zahorodnii Date: Fri, 4 Oct 2024 22:49:16 +0300 Subject: [PATCH 1/9] Resize X11 window immediately if it doesn't support _NET_WM_SYNC_REQUEST The fact that X11Window::handleSyncTimeout() is shared by code that runs when the client supports or doesn't support _NET_WM_SYNC_REQUEST makes the XSync code difficult to follow and more error prone. On the other hand, it appears that other window managers, e.g. Mutter and openbox, don't bother about throttling interactive resize when the client doesn't opt in into _NET_WM_SYNC_REQUEST, so let's do the same to simplify the code. As a backup plan, if it indeed becomes a problem later, we can always save current monotonic time in X11Window::doInteractiveResizeSync() and make X11Window::isWaitingForInteractiveResizeSync() check whether enough of time has passed since the last resize sync. --- autotests/integration/x11_window_test.cpp | 16 +++-------- src/x11window.cpp | 35 +++++++++-------------- 2 files changed, 17 insertions(+), 34 deletions(-) diff --git a/autotests/integration/x11_window_test.cpp b/autotests/integration/x11_window_test.cpp index 12f53c4220..a208fa3adb 100644 --- a/autotests/integration/x11_window_test.cpp +++ b/autotests/integration/x11_window_test.cpp @@ -1492,7 +1492,6 @@ void X11WindowTest::testNetWmKeyboardResize() QSignalSpy interactiveMoveResizeStartedSpy(window, &X11Window::interactiveMoveResizeStarted); QSignalSpy interactiveMoveResizeFinishedSpy(window, &X11Window::interactiveMoveResizeFinished); QSignalSpy interactiveMoveResizeSteppedSpy(window, &X11Window::interactiveMoveResizeStepped); - QSignalSpy frameGeometryChangedSpy(window, &X11Window::frameGeometryChanged); QVERIFY(interactiveMoveResizeStartedSpy.wait()); QCOMPARE(interactiveMoveResizeSteppedSpy.count(), 0); QVERIFY(window->isInteractiveResize()); @@ -1503,7 +1502,6 @@ void X11WindowTest::testNetWmKeyboardResize() Test::keyboardKeyPressed(KEY_RIGHT, timestamp++); Test::keyboardKeyReleased(KEY_RIGHT, timestamp++); QCOMPARE(interactiveMoveResizeSteppedSpy.count(), 1); - QVERIFY(frameGeometryChangedSpy.wait()); QCOMPARE(window->frameGeometry(), originalGeometry.adjusted(0, 0, 8, 0)); // Finish the interactive move. @@ -1531,7 +1529,6 @@ void X11WindowTest::testNetWmKeyboardResizeCancel() QSignalSpy interactiveMoveResizeStartedSpy(window, &X11Window::interactiveMoveResizeStarted); QSignalSpy interactiveMoveResizeFinishedSpy(window, &X11Window::interactiveMoveResizeFinished); QSignalSpy interactiveMoveResizeSteppedSpy(window, &X11Window::interactiveMoveResizeStepped); - QSignalSpy frameGeometryChangedSpy(window, &X11Window::frameGeometryChanged); QVERIFY(interactiveMoveResizeStartedSpy.wait()); QCOMPARE(interactiveMoveResizeSteppedSpy.count(), 0); QVERIFY(window->isInteractiveResize()); @@ -1542,7 +1539,6 @@ void X11WindowTest::testNetWmKeyboardResizeCancel() Test::keyboardKeyPressed(KEY_RIGHT, timestamp++); Test::keyboardKeyReleased(KEY_RIGHT, timestamp++); QCOMPARE(interactiveMoveResizeSteppedSpy.count(), 1); - QVERIFY(frameGeometryChangedSpy.wait()); QCOMPARE(window->frameGeometry(), originalGeometry.adjusted(0, 0, 8, 0)); // Cancel the interactive move. @@ -1741,7 +1737,6 @@ void X11WindowTest::testNetWmButtonSize() QSignalSpy interactiveMoveResizeStartedSpy(window, &X11Window::interactiveMoveResizeStarted); QSignalSpy interactiveMoveResizeFinishedSpy(window, &X11Window::interactiveMoveResizeFinished); QSignalSpy interactiveMoveResizeSteppedSpy(window, &X11Window::interactiveMoveResizeStepped); - QSignalSpy frameGeometryChangedSpy(window, &X11Window::frameGeometryChanged); QVERIFY(interactiveMoveResizeStartedSpy.wait()); QCOMPARE(interactiveMoveResizeSteppedSpy.count(), 0); QVERIFY(window->isInteractiveResize()); @@ -1750,7 +1745,6 @@ void X11WindowTest::testNetWmButtonSize() // Resize the window a tiny bit. Test::pointerMotionRelative(directionToVector(direction, QSizeF(8, 8)), timestamp++); QCOMPARE(interactiveMoveResizeSteppedSpy.count(), 1); - QVERIFY(frameGeometryChangedSpy.wait()); QCOMPARE(window->frameGeometry(), expandRect(originalGeometry, direction, QSizeF(8, 8))); // Finish the interactive move. @@ -1803,7 +1797,6 @@ void X11WindowTest::testNetWmButtonSizeCancel() QSignalSpy interactiveMoveResizeStartedSpy(window, &X11Window::interactiveMoveResizeStarted); QSignalSpy interactiveMoveResizeFinishedSpy(window, &X11Window::interactiveMoveResizeFinished); QSignalSpy interactiveMoveResizeSteppedSpy(window, &X11Window::interactiveMoveResizeStepped); - QSignalSpy frameGeometryChangedSpy(window, &X11Window::frameGeometryChanged); QVERIFY(interactiveMoveResizeStartedSpy.wait()); QCOMPARE(interactiveMoveResizeSteppedSpy.count(), 0); QVERIFY(window->isInteractiveResize()); @@ -1811,7 +1804,6 @@ void X11WindowTest::testNetWmButtonSizeCancel() // Resize the window a tiny bit. Test::pointerMotionRelative(QPointF(-8, -8), timestamp++); QCOMPARE(interactiveMoveResizeSteppedSpy.count(), 1); - QVERIFY(frameGeometryChangedSpy.wait()); QCOMPARE(window->frameGeometry(), originalGeometry.adjusted(-8, -8, 0, 0)); // Cancel the interactive resize. @@ -1888,7 +1880,7 @@ void X11WindowTest::testMinimumSize() window->updateInteractiveMoveResize(KWin::Cursors::self()->mouse()->pos(), Qt::KeyboardModifiers()); QCOMPARE(KWin::Cursors::self()->mouse()->pos(), cursorPos + QPoint(8, 0)); QCOMPARE(interactiveMoveResizeSteppedSpy.count(), 1); - QVERIFY(frameGeometryChangedSpy.wait()); + QVERIFY(!frameGeometryChangedSpy.wait(10)); // whilst X11 window size goes through scale, the increment is a logical value kwin side QCOMPARE(window->clientSize().width(), 100 / scale + 8); @@ -1910,7 +1902,7 @@ void X11WindowTest::testMinimumSize() window->updateInteractiveMoveResize(KWin::Cursors::self()->mouse()->pos(), Qt::KeyboardModifiers()); QCOMPARE(KWin::Cursors::self()->mouse()->pos(), cursorPos + QPoint(8, 8)); QCOMPARE(interactiveMoveResizeSteppedSpy.count(), 2); - QVERIFY(frameGeometryChangedSpy.wait()); + QVERIFY(!frameGeometryChangedSpy.wait(10)); QCOMPARE(window->clientSize().height(), 200 / scale + 8); // Finish the resize operation. @@ -1993,7 +1985,7 @@ void X11WindowTest::testMaximumSize() window->updateInteractiveMoveResize(KWin::Cursors::self()->mouse()->pos(), Qt::KeyboardModifiers()); QCOMPARE(KWin::Cursors::self()->mouse()->pos(), cursorPos + QPoint(-8, 0)); QCOMPARE(interactiveMoveResizeSteppedSpy.count(), 1); - QVERIFY(frameGeometryChangedSpy.wait()); + QVERIFY(!frameGeometryChangedSpy.wait(10)); QCOMPARE(window->clientSize().width(), 100 / scale - 8); window->keyPressEvent(Qt::Key_Down); @@ -2014,7 +2006,7 @@ void X11WindowTest::testMaximumSize() window->updateInteractiveMoveResize(KWin::Cursors::self()->mouse()->pos(), Qt::KeyboardModifiers()); QCOMPARE(KWin::Cursors::self()->mouse()->pos(), cursorPos + QPoint(-8, -8)); QCOMPARE(interactiveMoveResizeSteppedSpy.count(), 2); - QVERIFY(frameGeometryChangedSpy.wait()); + QVERIFY(!frameGeometryChangedSpy.wait(10)); QCOMPARE(window->clientSize().height(), 200 / scale - 8); // Finish the resize operation. diff --git a/src/x11window.cpp b/src/x11window.cpp index b71a9d3640..780ccfbc7e 100644 --- a/src/x11window.cpp +++ b/src/x11window.cpp @@ -4857,36 +4857,27 @@ void X11Window::doInteractiveResizeSync(const QRectF &rect) return; } - setMoveResizeGeometry(moveResizeFrameGeometry); - setAllowCommits(false); + if (m_syncRequest.counter == XCB_NONE) { + moveResize(rect); + } else { + if (!m_syncRequest.timeout) { + m_syncRequest.timeout = new QTimer(this); + connect(m_syncRequest.timeout, &QTimer::timeout, this, &X11Window::handleSyncTimeout); + m_syncRequest.timeout->setSingleShot(true); + } - if (!m_syncRequest.timeout) { - m_syncRequest.timeout = new QTimer(this); - connect(m_syncRequest.timeout, &QTimer::timeout, this, &X11Window::handleSyncTimeout); - m_syncRequest.timeout->setSingleShot(true); - } + setMoveResizeGeometry(moveResizeFrameGeometry); + setAllowCommits(false); - if (m_syncRequest.counter != XCB_NONE) { - m_syncRequest.timeout->start(250); sendSyncRequest(); - } else { - // For clients not supporting the XSYNC protocol, we limit the resizes to 30Hz - // to take pointless load from X11 and the client, the mouse is still moved at - // full speed and no human can control faster resizes anyway. - m_syncRequest.isPending = true; - m_syncRequest.interactiveResize = true; - m_syncRequest.timeout->start(33); - } + configure(nativeFrameGeometry, nativeWrapperGeometry, nativeClientGeometry); - configure(nativeFrameGeometry, nativeWrapperGeometry, nativeClientGeometry); + m_syncRequest.timeout->start(250); + } } void X11Window::handleSyncTimeout() { - if (m_syncRequest.counter == XCB_NONE) { // client w/o XSYNC support. allow the next resize event - m_syncRequest.isPending = false; // NEVER do this for clients with a valid counter - m_syncRequest.interactiveResize = false; // (leads to sync request races in some clients) - } performInteractiveResize(); } -- 2.46.1 From bc87fc3d80ad525786a9b15df5c2488a6110b53d Mon Sep 17 00:00:00 2001 From: Vlad Zahorodnii Date: Fri, 4 Oct 2024 22:55:44 +0300 Subject: [PATCH 2/9] Rename X11Window::handleSync,handleSyncTimeout --- src/syncalarmx11filter.cpp | 2 +- src/x11window.cpp | 6 +++--- src/x11window.h | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/syncalarmx11filter.cpp b/src/syncalarmx11filter.cpp index f4b4d49caa..133c3c6b99 100644 --- a/src/syncalarmx11filter.cpp +++ b/src/syncalarmx11filter.cpp @@ -28,7 +28,7 @@ bool SyncAlarmX11Filter::event(xcb_generic_event_t *event) return alarmEvent->alarm == syncRequest.alarm && alarmEvent->counter_value.hi == syncRequest.value.hi && alarmEvent->counter_value.lo == syncRequest.value.lo; }); if (client) { - client->handleSync(); + client->ackSync(); } return false; } diff --git a/src/x11window.cpp b/src/x11window.cpp index 780ccfbc7e..14d45abb27 100644 --- a/src/x11window.cpp +++ b/src/x11window.cpp @@ -3059,7 +3059,7 @@ void X11Window::checkApplicationMenuObjectPath() readApplicationMenuObjectPath(property); } -void X11Window::handleSync() +void X11Window::ackSync() { setReadyForPainting(); m_syncRequest.isPending = false; @@ -4862,7 +4862,7 @@ void X11Window::doInteractiveResizeSync(const QRectF &rect) } else { if (!m_syncRequest.timeout) { m_syncRequest.timeout = new QTimer(this); - connect(m_syncRequest.timeout, &QTimer::timeout, this, &X11Window::handleSyncTimeout); + connect(m_syncRequest.timeout, &QTimer::timeout, this, &X11Window::ackSyncTimeout); m_syncRequest.timeout->setSingleShot(true); } @@ -4876,7 +4876,7 @@ void X11Window::doInteractiveResizeSync(const QRectF &rect) } } -void X11Window::handleSyncTimeout() +void X11Window::ackSyncTimeout() { performInteractiveResize(); } diff --git a/src/x11window.h b/src/x11window.h index 4f3b43c8b0..b54db23345 100644 --- a/src/x11window.h +++ b/src/x11window.h @@ -293,8 +293,8 @@ public: return m_syncRequest; } bool wantsSyncCounter() const; - void handleSync(); - void handleSyncTimeout(); + void ackSync(); + void ackSyncTimeout(); bool allowWindowActivation(xcb_timestamp_t time = -1U, bool focus_in = false); -- 2.46.1 From b8016cbe28d847d052f1b0ee6c66bdaabbb88368 Mon Sep 17 00:00:00 2001 From: Vlad Zahorodnii Date: Fri, 4 Oct 2024 22:59:34 +0300 Subject: [PATCH 3/9] Rename X11Window::SyncRequest::isPending --- src/x11window.cpp | 12 ++++++------ src/x11window.h | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/x11window.cpp b/src/x11window.cpp index 14d45abb27..2ebb4a5957 100644 --- a/src/x11window.cpp +++ b/src/x11window.cpp @@ -309,7 +309,7 @@ X11Window::X11Window() m_syncRequest.counter = m_syncRequest.alarm = XCB_NONE; m_syncRequest.timeout = m_syncRequest.failsafeTimeout = nullptr; m_syncRequest.lastTimestamp = xTime(); - m_syncRequest.isPending = false; + m_syncRequest.pending = false; m_syncRequest.interactiveResize = false; // Set the initial mapping state @@ -2585,7 +2585,7 @@ void X11Window::getSyncCounter() */ void X11Window::sendSyncRequest() { - if (m_syncRequest.counter == XCB_NONE || m_syncRequest.isPending) { + if (m_syncRequest.counter == XCB_NONE || m_syncRequest.pending) { return; // do NOT, NEVER send a sync request when there's one on the stack. the clients will just stop respoding. FOREVER! ... } @@ -2599,7 +2599,7 @@ void X11Window::sendSyncRequest() return; } // failed during resize - m_syncRequest.isPending = false; + m_syncRequest.pending = false; m_syncRequest.interactiveResize = false; m_syncRequest.counter = XCB_NONE; m_syncRequest.alarm = XCB_NONE; @@ -2630,7 +2630,7 @@ void X11Window::sendSyncRequest() // Send the message to client sendClientMessage(window(), atoms->wm_protocols, atoms->net_wm_sync_request, m_syncRequest.value.lo, m_syncRequest.value.hi); - m_syncRequest.isPending = true; + m_syncRequest.pending = true; m_syncRequest.interactiveResize = isInteractiveResize(); m_syncRequest.lastTimestamp = xTime(); } @@ -3062,7 +3062,7 @@ void X11Window::checkApplicationMenuObjectPath() void X11Window::ackSync() { setReadyForPainting(); - m_syncRequest.isPending = false; + m_syncRequest.pending = false; if (m_syncRequest.failsafeTimeout) { m_syncRequest.failsafeTimeout->stop(); } @@ -4840,7 +4840,7 @@ void X11Window::leaveInteractiveMoveResize() bool X11Window::isWaitingForInteractiveResizeSync() const { - return m_syncRequest.isPending && m_syncRequest.interactiveResize; + return m_syncRequest.pending && m_syncRequest.interactiveResize; } void X11Window::doInteractiveResizeSync(const QRectF &rect) diff --git a/src/x11window.h b/src/x11window.h index b54db23345..10fa57eaf2 100644 --- a/src/x11window.h +++ b/src/x11window.h @@ -285,7 +285,7 @@ public: xcb_sync_alarm_t alarm; xcb_timestamp_t lastTimestamp; QTimer *timeout, *failsafeTimeout; - bool isPending; + bool pending; bool interactiveResize; }; const SyncRequest &syncRequest() const -- 2.46.1 From 61e7061998ce3244325af62aac198ecf058a1aeb Mon Sep 17 00:00:00 2001 From: Vlad Zahorodnii Date: Fri, 4 Oct 2024 23:12:44 +0300 Subject: [PATCH 4/9] Make XSync timeout more permissive There are two timeouts: a soft one and a hard one. If the soft timeout occurs, then the scheduled operation will be performed. For example, the window will be marked as ready for painting or resized. If the hard timeout occurs, _NET_WM_SYNC_REQUEST will be disabled. Such an arrangement is fine in its current present form, but in order to make Xwayland window resizing less glitchy, _NET_WM_SYNC_REQUEST code needs to interact with wl_surface event flow, which in its turn means that the timeout code also needs to be prepared for that. It is very challenging to make the timeout code not break, so this change seeks to re-arrange the timeout handling so it's easier to integrate with wl_surface commits. With the proposed changes, there is going to be only one timer: timeout. If it fires, the window will be resized immediately and XSync is going to be kept disabled until an acknowledgement arrives from the client. --- src/x11window.cpp | 101 +++++++++++++++++----------------------------- src/x11window.h | 5 ++- 2 files changed, 41 insertions(+), 65 deletions(-) diff --git a/src/x11window.cpp b/src/x11window.cpp index 2ebb4a5957..1d25ab82b1 100644 --- a/src/x11window.cpp +++ b/src/x11window.cpp @@ -307,8 +307,9 @@ X11Window::X11Window() // TODO: Do all as initialization m_syncRequest.counter = m_syncRequest.alarm = XCB_NONE; - m_syncRequest.timeout = m_syncRequest.failsafeTimeout = nullptr; + m_syncRequest.timeout = nullptr; m_syncRequest.lastTimestamp = xTime(); + m_syncRequest.enabled = false; m_syncRequest.pending = false; m_syncRequest.interactiveResize = false; @@ -460,9 +461,6 @@ void X11Window::releaseWindow(bool on_shutdown) ungrabXServer(); } - if (m_syncRequest.failsafeTimeout) { - m_syncRequest.failsafeTimeout->stop(); - } if (m_syncRequest.timeout) { m_syncRequest.timeout->stop(); } @@ -516,9 +514,6 @@ void X11Window::destroyWindow() m_frame.reset(); } - if (m_syncRequest.failsafeTimeout) { - m_syncRequest.failsafeTimeout->stop(); - } if (m_syncRequest.timeout) { m_syncRequest.timeout->stop(); } @@ -2550,6 +2545,7 @@ void X11Window::getSyncCounter() Xcb::Property syncProp(false, window(), atoms->net_wm_sync_request_counter, XCB_ATOM_CARDINAL, 0, 1); const xcb_sync_counter_t counter = syncProp.value(XCB_NONE); if (counter != XCB_NONE) { + m_syncRequest.enabled = true; m_syncRequest.counter = counter; m_syncRequest.value.hi = 0; m_syncRequest.value.lo = 0; @@ -2585,35 +2581,16 @@ void X11Window::getSyncCounter() */ void X11Window::sendSyncRequest() { - if (m_syncRequest.counter == XCB_NONE || m_syncRequest.pending) { + if (!m_syncRequest.enabled || m_syncRequest.pending) { return; // do NOT, NEVER send a sync request when there's one on the stack. the clients will just stop respoding. FOREVER! ... } - if (!m_syncRequest.failsafeTimeout) { - m_syncRequest.failsafeTimeout = new QTimer(this); - connect(m_syncRequest.failsafeTimeout, &QTimer::timeout, this, [this]() { - // client does not respond to XSYNC requests in reasonable time, remove support - if (!ready_for_painting) { - // failed on initial pre-show request - setReadyForPainting(); - return; - } - // failed during resize - m_syncRequest.pending = false; - m_syncRequest.interactiveResize = false; - m_syncRequest.counter = XCB_NONE; - m_syncRequest.alarm = XCB_NONE; - delete m_syncRequest.timeout; - delete m_syncRequest.failsafeTimeout; - m_syncRequest.timeout = nullptr; - m_syncRequest.failsafeTimeout = nullptr; - m_syncRequest.lastTimestamp = XCB_CURRENT_TIME; - }); - m_syncRequest.failsafeTimeout->setSingleShot(true); + if (!m_syncRequest.timeout) { + m_syncRequest.timeout = new QTimer(this); + m_syncRequest.timeout->setSingleShot(true); + connect(m_syncRequest.timeout, &QTimer::timeout, this, &X11Window::ackSyncTimeout); } - // if there's no response within 10 seconds, sth. went wrong and we remove XSYNC support from this client. - // see events.cpp X11Window::syncEvent() - m_syncRequest.failsafeTimeout->start(ready_for_painting ? 10000 : 1000); + m_syncRequest.timeout->start(ready_for_painting ? 10000 : 1000); // We increment before the notify so that after the notify // syncCounterSerial will equal the value we are expecting @@ -3061,29 +3038,40 @@ void X11Window::checkApplicationMenuObjectPath() void X11Window::ackSync() { - setReadyForPainting(); + // Note that a sync request can be ack'ed after the timeout. If that happens, just re-enable + // XSync back and do nothing more. m_syncRequest.pending = false; - if (m_syncRequest.failsafeTimeout) { - m_syncRequest.failsafeTimeout->stop(); + if (!m_syncRequest.enabled) { + m_syncRequest.enabled = true; + return; } - // Sync request can be acknowledged shortly after finishing resize. + m_syncRequest.timeout->stop(); + + finishSync(); +} + +void X11Window::ackSyncTimeout() +{ + // If a sync request times out, disable XSync temporarily until the client comes back to its senses. + m_syncRequest.enabled = false; + + finishSync(); +} + +void X11Window::finishSync() +{ + setReadyForPainting(); + if (m_syncRequest.interactiveResize) { m_syncRequest.interactiveResize = false; - if (m_syncRequest.timeout) { - m_syncRequest.timeout->stop(); - } - performInteractiveResize(); + + moveResize(moveResizeGeometry()); updateWindowPixmap(); + setAllowCommits(true); } } -void X11Window::performInteractiveResize() -{ - resize(moveResizeGeometry().size()); - setAllowCommits(true); -} - bool X11Window::belongToSameApplication(const X11Window *c1, const X11Window *c2, SameApplicationChecks checks) { bool same_app = false; @@ -4840,7 +4828,7 @@ void X11Window::leaveInteractiveMoveResize() bool X11Window::isWaitingForInteractiveResizeSync() const { - return m_syncRequest.pending && m_syncRequest.interactiveResize; + return m_syncRequest.enabled && m_syncRequest.pending && m_syncRequest.interactiveResize; } void X11Window::doInteractiveResizeSync(const QRectF &rect) @@ -4857,30 +4845,17 @@ void X11Window::doInteractiveResizeSync(const QRectF &rect) return; } - if (m_syncRequest.counter == XCB_NONE) { + if (!m_syncRequest.enabled) { moveResize(rect); } else { - if (!m_syncRequest.timeout) { - m_syncRequest.timeout = new QTimer(this); - connect(m_syncRequest.timeout, &QTimer::timeout, this, &X11Window::ackSyncTimeout); - m_syncRequest.timeout->setSingleShot(true); - } - setMoveResizeGeometry(moveResizeFrameGeometry); setAllowCommits(false); sendSyncRequest(); configure(nativeFrameGeometry, nativeWrapperGeometry, nativeClientGeometry); - - m_syncRequest.timeout->start(250); } } -void X11Window::ackSyncTimeout() -{ - performInteractiveResize(); -} - NETExtendedStrut X11Window::strut() const { NETExtendedStrut ext = info->extendedStrut(); @@ -4991,7 +4966,7 @@ void X11Window::damageNotifyEvent() Q_ASSERT(kwinApp()->operationMode() == Application::OperationModeX11); if (!readyForPainting()) { // avoid "setReadyForPainting()" function calling overhead - if (m_syncRequest.counter == XCB_NONE) { // cannot detect complete redraw, consider done now + if (!m_syncRequest.enabled) { // cannot detect complete redraw, consider done now setReadyForPainting(); } } @@ -5025,7 +5000,7 @@ void X11Window::updateWindowPixmap() void X11Window::associate() { auto handleMapped = [this]() { - if (syncRequest().counter == XCB_NONE) { // cannot detect complete redraw, consider done now + if (!m_syncRequest.enabled) { // cannot detect complete redraw, consider done now setReadyForPainting(); } }; diff --git a/src/x11window.h b/src/x11window.h index 10fa57eaf2..6b430b861a 100644 --- a/src/x11window.h +++ b/src/x11window.h @@ -284,7 +284,8 @@ public: xcb_sync_int64_t value; xcb_sync_alarm_t alarm; xcb_timestamp_t lastTimestamp; - QTimer *timeout, *failsafeTimeout; + QTimer *timeout; + bool enabled; bool pending; bool interactiveResize; }; @@ -295,6 +296,7 @@ public: bool wantsSyncCounter() const; void ackSync(); void ackSyncTimeout(); + void finishSync(); bool allowWindowActivation(xcb_timestamp_t time = -1U, bool focus_in = false); @@ -385,7 +387,6 @@ private: void getSyncCounter(); void sendSyncRequest(); void leaveInteractiveMoveResize() override; - void performInteractiveResize(); void establishCommandWindowGrab(uint8_t button); void establishCommandAllGrab(uint8_t button); -- 2.46.1 From dd10d1b329b06ab3377f669a610fec9726f28a70 Mon Sep 17 00:00:00 2001 From: Vlad Zahorodnii Date: Fri, 4 Oct 2024 23:20:03 +0300 Subject: [PATCH 5/9] Disable Xwayland surface commits for all sync requests --- src/x11window.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/x11window.cpp b/src/x11window.cpp index 1d25ab82b1..5688ea6da8 100644 --- a/src/x11window.cpp +++ b/src/x11window.cpp @@ -2604,7 +2604,7 @@ void X11Window::sendSyncRequest() kwinApp()->updateXTime(); } - // Send the message to client + setAllowCommits(false); sendClientMessage(window(), atoms->wm_protocols, atoms->net_wm_sync_request, m_syncRequest.value.lo, m_syncRequest.value.hi); m_syncRequest.pending = true; @@ -3049,6 +3049,7 @@ void X11Window::ackSync() m_syncRequest.timeout->stop(); finishSync(); + setAllowCommits(true); } void X11Window::ackSyncTimeout() @@ -3057,6 +3058,7 @@ void X11Window::ackSyncTimeout() m_syncRequest.enabled = false; finishSync(); + setAllowCommits(true); } void X11Window::finishSync() @@ -3068,7 +3070,6 @@ void X11Window::finishSync() moveResize(moveResizeGeometry()); updateWindowPixmap(); - setAllowCommits(true); } } @@ -4849,8 +4850,6 @@ void X11Window::doInteractiveResizeSync(const QRectF &rect) moveResize(rect); } else { setMoveResizeGeometry(moveResizeFrameGeometry); - setAllowCommits(false); - sendSyncRequest(); configure(nativeFrameGeometry, nativeWrapperGeometry, nativeClientGeometry); } -- 2.46.1 From 60ef3223e42a06110693fef3ec493662c799af38 Mon Sep 17 00:00:00 2001 From: Vlad Zahorodnii Date: Fri, 4 Oct 2024 23:40:41 +0300 Subject: [PATCH 6/9] Make Xwayland resizing less glitchy XSYNC requests and wl_surface commits are unsynchronized. This results in the window bouncing when it's being resized. Let's consider a concrete example. Assume that there is a window with 0,0 100x100 geometry and it is being resized by dragging the left edge. If the left edge is dragged by 10px, the following will occur: - wl_surface commits will be blocked (it is needed to ensure the consistent order of XSync acknowledgements and wl_surface commits) - an XSync request will be sent - a ConfigureNotify event will be sent - a client processes the ConfigureNotify by repainting the window and acknowledges the XSync request - kwin notices that the XSync request has been acked and updates the window position to 10,0 and unblocks surface commits The problem is that it can take a while for Xwayland to attach a new buffer to the surface with a size of 90x100. If kwin composes a frame in meanwhile, the effective geometry of the window will be 10,0 100x100, i.e. the right window edge will stick by 10px. Some time later, Xwayland would attach a buffer with the right size (90x100), and the right window edge will be put back in the right place. In order to address the bouncing, this change reworks how the synchronization is performed: - when a window is asked to resize, kwin will freeze wl_surface commits, send a sync request, and configure the window - after the client repaints the window, it acks the sync request - when kwin sees that the sync request has been acked, it will unfreeze the wl_surface commits. And here's the most important part: it will NOT update the window position until Xwayland commits something - when Xwayland commits the wl_surface, kwin will update the window geometry It's important to wait until Xwayland commits the wl_surface because, as it was said previously, it can take a while until Xwayland commits the wl_surface and kwin could compose a frame with the new position but old surface size in meanwhile. BUG: 486464 --- src/x11window.cpp | 40 +++++++++++++++++++++++++++++----------- src/x11window.h | 2 ++ 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/src/x11window.cpp b/src/x11window.cpp index 5688ea6da8..b22de557d8 100644 --- a/src/x11window.cpp +++ b/src/x11window.cpp @@ -311,6 +311,7 @@ X11Window::X11Window() m_syncRequest.lastTimestamp = xTime(); m_syncRequest.enabled = false; m_syncRequest.pending = false; + m_syncRequest.acked = false; m_syncRequest.interactiveResize = false; // Set the initial mapping state @@ -3046,9 +3047,13 @@ void X11Window::ackSync() return; } + m_syncRequest.acked = true; m_syncRequest.timeout->stop(); - finishSync(); + // With Xwayland, the sync request will be completed after the wl_surface is committed. + if (!waylandServer()) { + finishSync(); + } setAllowCommits(true); } @@ -3071,6 +3076,8 @@ void X11Window::finishSync() moveResize(moveResizeGeometry()); updateWindowPixmap(); } + + m_syncRequest.acked = false; } bool X11Window::belongToSameApplication(const X11Window *c1, const X11Window *c2, SameApplicationChecks checks) @@ -3945,6 +3952,19 @@ void X11Window::handleXwaylandScaleChanged() resize(moveResizeGeometry().size()); } +void X11Window::handleCommitted() +{ + if (surface()->isMapped()) { + if (m_syncRequest.acked) { + finishSync(); + } + + if (!m_syncRequest.enabled) { + setReadyForPainting(); + } + } +} + void X11Window::setAllowCommits(bool allow) { if (!waylandServer()) { @@ -4829,7 +4849,7 @@ void X11Window::leaveInteractiveMoveResize() bool X11Window::isWaitingForInteractiveResizeSync() const { - return m_syncRequest.enabled && m_syncRequest.pending && m_syncRequest.interactiveResize; + return m_syncRequest.enabled && m_syncRequest.interactiveResize && (m_syncRequest.pending || m_syncRequest.acked); } void X11Window::doInteractiveResizeSync(const QRectF &rect) @@ -4998,19 +5018,17 @@ void X11Window::updateWindowPixmap() void X11Window::associate() { - auto handleMapped = [this]() { - if (!m_syncRequest.enabled) { // cannot detect complete redraw, consider done now - setReadyForPainting(); + if (surface()->isMapped()) { + if (m_syncRequest.acked) { + finishSync(); } - }; - if (surface()->isMapped()) { - handleMapped(); - } else { - connect(surface(), &SurfaceInterface::mapped, this, handleMapped); + if (!m_syncRequest.enabled) { + setReadyForPainting(); + } } - m_pendingSurfaceId = 0; + connect(surface(), &SurfaceInterface::committed, this, &X11Window::handleCommitted); } QWindow *X11Window::findInternalWindow() const diff --git a/src/x11window.h b/src/x11window.h index 6b430b861a..73d4c23a4c 100644 --- a/src/x11window.h +++ b/src/x11window.h @@ -287,6 +287,7 @@ public: QTimer *timeout; bool enabled; bool pending; + bool acked; bool interactiveResize; }; const SyncRequest &syncRequest() const @@ -438,6 +439,7 @@ private: void checkOutput(); void associate(); void handleXwaylandScaleChanged(); + void handleCommitted(); void setAllowCommits(bool allow); -- 2.46.1 From 59f45806953e05bbf289480b9e5882e4d87ee434 Mon Sep 17 00:00:00 2001 From: Vlad Zahorodnii Date: Thu, 3 Oct 2024 00:43:57 +0300 Subject: [PATCH 7/9] scene: Use standard wl_surface item for Xwayland surfaces Besides unifying the code, it fixes some visual glitches caused by the opaque region getting out of sync. Xwayland MR: https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1698 --- src/scene/surfaceitem_wayland.cpp | 21 --------------------- src/scene/surfaceitem_wayland.h | 19 ------------------- src/scene/windowitem.cpp | 2 +- 3 files changed, 1 insertion(+), 41 deletions(-) diff --git a/src/scene/surfaceitem_wayland.cpp b/src/scene/surfaceitem_wayland.cpp index a4d012f774..04961952b9 100644 --- a/src/scene/surfaceitem_wayland.cpp +++ b/src/scene/surfaceitem_wayland.cpp @@ -12,11 +12,6 @@ #include "wayland/linuxdmabufv1clientbuffer.h" #include "wayland/subcompositor.h" #include "wayland/surface.h" -#include "window.h" - -#if KWIN_BUILD_X11 -#include "x11window.h" -#endif namespace KWin { @@ -247,22 +242,6 @@ bool SurfacePixmapWayland::isValid() const return m_bufferRef; } -#if KWIN_BUILD_X11 -SurfaceItemXwayland::SurfaceItemXwayland(X11Window *window, Item *parent) - : SurfaceItemWayland(window->surface(), parent) - , m_window(window) -{ -} - -QRegion SurfaceItemXwayland::opaque() const -{ - if (!m_window->hasAlpha()) { - return rect().toRect(); - } else { - return m_window->opaqueRegion() & rect().toRect(); - } -} -#endif } // namespace KWin #include "moc_surfaceitem_wayland.cpp" diff --git a/src/scene/surfaceitem_wayland.h b/src/scene/surfaceitem_wayland.h index 991cd168c3..fbc7498446 100644 --- a/src/scene/surfaceitem_wayland.h +++ b/src/scene/surfaceitem_wayland.h @@ -16,7 +16,6 @@ namespace KWin class GraphicsBuffer; class SubSurfaceInterface; class SurfaceInterface; -class X11Window; /** * The SurfaceItemWayland class represents a Wayland surface in the scene. @@ -83,22 +82,4 @@ private: SurfaceItemWayland *m_item; }; -#if KWIN_BUILD_X11 -/** - * The SurfaceItemXwayland class represents an Xwayland surface in the scene. - */ -class KWIN_EXPORT SurfaceItemXwayland : public SurfaceItemWayland -{ - Q_OBJECT - -public: - explicit SurfaceItemXwayland(X11Window *window, Item *parent = nullptr); - - QRegion opaque() const override; - -private: - X11Window *m_window; -}; -#endif - } // namespace KWin diff --git a/src/scene/windowitem.cpp b/src/scene/windowitem.cpp index 4283051e7e..913d2708f5 100644 --- a/src/scene/windowitem.cpp +++ b/src/scene/windowitem.cpp @@ -323,7 +323,7 @@ void WindowItemX11::initialize() if (!window()->surface()) { updateSurfaceItem(nullptr); } else { - updateSurfaceItem(std::make_unique(static_cast(window()), this)); + updateSurfaceItem(std::make_unique(window()->surface(), this)); } break; case Application::OperationModeWaylandOnly: -- 2.46.1 From 84f35749d9084692e0ea8ae4ea187178005457cc Mon Sep 17 00:00:00 2001 From: Vlad Zahorodnii Date: Mon, 14 Oct 2024 00:34:28 +0300 Subject: [PATCH 8/9] Send the initial sync request before mapping the frame window When the wl_surface commits are blocked by the initial sync request, it is really important that Xwayland doesn't commit the surface until the sync request is acked. However, with the current arrangement of the code, Xwayland may render something before the sync request is acked. It can happen because the frame window is mapped before the initial sync request is sent. There are many places where it can happen, e.g. in the setupCompositing() function, or the setMinimized() function, etc. In order to ensure that Xwayland won't render the wl_surface while we are waiting for the sync request to get acked, this change moves the initial sync request all the way to the top of the manage() function so the surface commits are blocked before the frame window is mapped. --- src/x11window.cpp | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/x11window.cpp b/src/x11window.cpp index b22de557d8..be35fe07fd 100644 --- a/src/x11window.cpp +++ b/src/x11window.cpp @@ -655,6 +655,18 @@ bool X11Window::manage(xcb_window_t w, bool isMapped) getSyncCounter(); setCaption(readName()); + if (Compositor::compositing()) { + // Sending ConfigureNotify is done when setting mapping state below, getting the + // first sync response means window is ready for compositing. + // + // The sync request will block wl_surface commits, and with Xwayland, it is really + // important that wl_surfaces commits are blocked before the frame window is mapped. + // Otherwise Xwayland can attach a buffer before the sync request is acked. + sendSyncRequest(); + } else { + ready_for_painting = true; // set to true in case compositing is turned on later + } + setupWindowRules(); connect(this, &X11Window::windowClassChanged, this, &X11Window::evaluateWindowRules); @@ -1093,14 +1105,6 @@ bool X11Window::manage(xcb_window_t w, bool isMapped) workspace()->restoreSessionStackingOrder(this); } - if (Compositor::compositing()) { - // Sending ConfigureNotify is done when setting mapping state below, - // Getting the first sync response means window is ready for compositing - sendSyncRequest(); - } else { - ready_for_painting = true; // set to true in case compositing is turned on later. bug #160393 - } - if (isShown()) { bool allow; if (session) { -- 2.46.1 From 3ecac02f2f7573d7b2714c1fb23bebaec8afa0e0 Mon Sep 17 00:00:00 2001 From: Vlad Zahorodnii Date: Thu, 17 Oct 2024 19:21:32 +0300 Subject: [PATCH 9/9] Block interactive resizing if there is a pending XSync request There cannot be two XSync requests in flight, the type of the sync requests doesn't matter either, e.g. one for interactive resize or one to determine whether the client has painted the initial frame. --- src/x11window.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/x11window.cpp b/src/x11window.cpp index be35fe07fd..3e989f29bd 100644 --- a/src/x11window.cpp +++ b/src/x11window.cpp @@ -4853,7 +4853,7 @@ void X11Window::leaveInteractiveMoveResize() bool X11Window::isWaitingForInteractiveResizeSync() const { - return m_syncRequest.enabled && m_syncRequest.interactiveResize && (m_syncRequest.pending || m_syncRequest.acked); + return m_syncRequest.enabled && (m_syncRequest.pending || m_syncRequest.acked); } void X11Window::doInteractiveResizeSync(const QRectF &rect) -- 2.46.1