From 88a0246a46c30e08e9730d16cf8739773447d058 Mon Sep 17 00:00:00 2001 From: Johan Klokkhammer Helsing Date: Mon, 24 Sep 2018 13:04:51 +0200 Subject: Client: Don't attach buffers to unexposed windows QBackingStore::flush is sometimes called with an unxeposed window, in that case, don't attach the buffer to the wl_surface immediately, as that causes protocol errors with xdg_shell. Flushed buffers are instead stored until we get the first configure event. [ChangeLog][QPA plugin][xdg-shell] Fixed a bug where buffers were sometimes attached and committed before the first configure event, causing protocol errors. Fixes: QTBUG-71345 Change-Id: If9409d97bd25f6b13940c56141920a664c349c8e Reviewed-by: Paul Olav Tvete Reviewed-by: David Edmundson --- src/client/qwaylandshmbackingstore.cpp | 3 +- src/client/qwaylandwindow.cpp | 22 ++++++++++++++ src/client/qwaylandwindow_p.h | 4 +++ .../xdg-shell-v6/qwaylandxdgshellv6.cpp | 19 ++++++++++-- .../xdg-shell-v6/qwaylandxdgshellv6_p.h | 2 +- .../xdg-shell/qwaylandxdgshell.cpp | 19 ++++++++++-- .../xdg-shell/qwaylandxdgshell_p.h | 2 +- tests/auto/client/shared/mocksurface.cpp | 12 +++++--- tests/auto/client/shared/mocksurface.h | 7 +++-- tests/auto/client/shared/mockxdgshellv6.cpp | 14 +++++++-- tests/auto/client/shared/mockxdgshellv6.h | 6 ++++ tests/auto/client/xdgshellv6/tst_xdgshellv6.cpp | 35 ++++++++++++++++++++++ 12 files changed, 126 insertions(+), 19 deletions(-) diff --git a/src/client/qwaylandshmbackingstore.cpp b/src/client/qwaylandshmbackingstore.cpp index ecb03c0d6..3fe2ce80c 100644 --- a/src/client/qwaylandshmbackingstore.cpp +++ b/src/client/qwaylandshmbackingstore.cpp @@ -234,8 +234,7 @@ void QWaylandShmBackingStore::flush(QWindow *window, const QRegion ®ion, cons mFrontBuffer = mBackBuffer; QMargins margins = windowDecorationMargins(); - - waylandWindow()->commit(mFrontBuffer, region.translated(margins.left(), margins.top())); + waylandWindow()->safeCommit(mFrontBuffer, region.translated(margins.left(), margins.top())); } void QWaylandShmBackingStore::resize(const QSize &size, const QRegion &) diff --git a/src/client/qwaylandwindow.cpp b/src/client/qwaylandwindow.cpp index 52bee6ae3..1801c2a4b 100644 --- a/src/client/qwaylandwindow.cpp +++ b/src/client/qwaylandwindow.cpp @@ -248,6 +248,7 @@ void QWaylandWindow::reset(bool sendDestroyEvent) } mMask = QRegion(); + mQueuedBuffer = nullptr; } QWaylandWindow *QWaylandWindow::fromWlSurface(::wl_surface *surface) @@ -566,8 +567,29 @@ void QWaylandWindow::damage(const QRect &rect) damage(rect.x(), rect.y(), rect.width(), rect.height()); } +void QWaylandWindow::safeCommit(QWaylandBuffer *buffer, const QRegion &damage) +{ + if (isExposed()) { + commit(buffer, damage); + } else { + mQueuedBuffer = buffer; + mQueuedBufferDamage = damage; + } +} + +void QWaylandWindow::handleExpose(const QRegion ®ion) +{ + QWindowSystemInterface::handleExposeEvent(window(), region); + if (mQueuedBuffer) { + commit(mQueuedBuffer, mQueuedBufferDamage); + mQueuedBuffer = nullptr; + mQueuedBufferDamage = QRegion(); + } +} + void QWaylandWindow::commit(QWaylandBuffer *buffer, const QRegion &damage) { + Q_ASSERT(isExposed()); if (buffer->committed()) { qCDebug(lcWaylandBackingstore) << "Buffer already committed, ignoring."; return; diff --git a/src/client/qwaylandwindow_p.h b/src/client/qwaylandwindow_p.h index d11ed871b..ad24b7358 100644 --- a/src/client/qwaylandwindow_p.h +++ b/src/client/qwaylandwindow_p.h @@ -116,6 +116,8 @@ public: using QtWayland::wl_surface::damage; void damage(const QRect &rect); + void safeCommit(QWaylandBuffer *buffer, const QRegion &damage); + void handleExpose(const QRegion ®ion); void commit(QWaylandBuffer *buffer, const QRegion &damage); void waitForFrameSync(); @@ -231,6 +233,8 @@ protected: Qt::WindowStates mLastReportedWindowStates = Qt::WindowNoState; QWaylandShmBackingStore *mBackingStore = nullptr; + QWaylandBuffer *mQueuedBuffer = nullptr; + QRegion mQueuedBufferDamage; private slots: void handleScreenRemoved(QScreen *qScreen); diff --git a/src/plugins/shellintegration/xdg-shell-v6/qwaylandxdgshellv6.cpp b/src/plugins/shellintegration/xdg-shell-v6/qwaylandxdgshellv6.cpp index 447e8fb6a..3d3af6929 100644 --- a/src/plugins/shellintegration/xdg-shell-v6/qwaylandxdgshellv6.cpp +++ b/src/plugins/shellintegration/xdg-shell-v6/qwaylandxdgshellv6.cpp @@ -258,9 +258,14 @@ void QWaylandXdgSurfaceV6::setAppId(const QString &appId) m_toplevel->set_app_id(appId); } +bool QWaylandXdgSurfaceV6::isExposed() const +{ + return m_configured || m_pendingConfigureSerial; +} + bool QWaylandXdgSurfaceV6::handleExpose(const QRegion ®ion) { - if (!m_configured && !region.isEmpty()) { + if (!isExposed() && !region.isEmpty()) { m_exposeRegion = region; return true; } @@ -333,10 +338,18 @@ void QWaylandXdgSurfaceV6::setPopup(QWaylandWindow *parent, QWaylandInputDevice void QWaylandXdgSurfaceV6::zxdg_surface_v6_configure(uint32_t serial) { - m_window->applyConfigureWhenPossible(); m_pendingConfigureSerial = serial; + if (!m_configured) { + // We have to do the initial applyConfigure() immediately, since that is the expose. + applyConfigure(); + } else { + // Later configures are probably resizes, so we have to queue them up for a time when we + // are not painting to the window. + m_window->applyConfigureWhenPossible(); + } + if (!m_exposeRegion.isEmpty()) { - QWindowSystemInterface::handleExposeEvent(m_window->window(), m_exposeRegion); + m_window->handleExpose(m_exposeRegion); m_exposeRegion = QRegion(); } } diff --git a/src/plugins/shellintegration/xdg-shell-v6/qwaylandxdgshellv6_p.h b/src/plugins/shellintegration/xdg-shell-v6/qwaylandxdgshellv6_p.h index 38b711f88..874dba014 100644 --- a/src/plugins/shellintegration/xdg-shell-v6/qwaylandxdgshellv6_p.h +++ b/src/plugins/shellintegration/xdg-shell-v6/qwaylandxdgshellv6_p.h @@ -85,7 +85,7 @@ public: void setTitle(const QString &title) override; void setAppId(const QString &appId) override; - bool isExposed() const override { return m_configured; } + bool isExposed() const override; bool handleExpose(const QRegion &) override; bool handlesActiveState() const { return m_toplevel; } void applyConfigure() override; diff --git a/src/plugins/shellintegration/xdg-shell/qwaylandxdgshell.cpp b/src/plugins/shellintegration/xdg-shell/qwaylandxdgshell.cpp index 0756e4d02..c723192c8 100644 --- a/src/plugins/shellintegration/xdg-shell/qwaylandxdgshell.cpp +++ b/src/plugins/shellintegration/xdg-shell/qwaylandxdgshell.cpp @@ -292,9 +292,14 @@ void QWaylandXdgSurface::setWindowFlags(Qt::WindowFlags flags) m_toplevel->requestWindowFlags(flags); } +bool QWaylandXdgSurface::isExposed() const +{ + return m_configured || m_pendingConfigureSerial; +} + bool QWaylandXdgSurface::handleExpose(const QRegion ®ion) { - if (!m_configured && !region.isEmpty()) { + if (!isExposed() && !region.isEmpty()) { m_exposeRegion = region; return true; } @@ -367,10 +372,18 @@ void QWaylandXdgSurface::setPopup(QWaylandWindow *parent, QWaylandInputDevice *d void QWaylandXdgSurface::xdg_surface_configure(uint32_t serial) { - m_window->applyConfigureWhenPossible(); m_pendingConfigureSerial = serial; + if (!m_configured) { + // We have to do the initial applyConfigure() immediately, since that is the expose. + applyConfigure(); + } else { + // Later configures are probably resizes, so we have to queue them up for a time when we + // are not painting to the window. + m_window->applyConfigureWhenPossible(); + } + if (!m_exposeRegion.isEmpty()) { - QWindowSystemInterface::handleExposeEvent(m_window->window(), m_exposeRegion); + m_window->handleExpose(m_exposeRegion); m_exposeRegion = QRegion(); } } diff --git a/src/plugins/shellintegration/xdg-shell/qwaylandxdgshell_p.h b/src/plugins/shellintegration/xdg-shell/qwaylandxdgshell_p.h index 45d7d4b0e..5e97a34b3 100644 --- a/src/plugins/shellintegration/xdg-shell/qwaylandxdgshell_p.h +++ b/src/plugins/shellintegration/xdg-shell/qwaylandxdgshell_p.h @@ -89,7 +89,7 @@ public: void setAppId(const QString &appId) override; void setWindowFlags(Qt::WindowFlags flags) override; - bool isExposed() const override { return m_configured; } + bool isExposed() const override; bool handleExpose(const QRegion &) override; bool handlesActiveState() const { return m_toplevel; } void applyConfigure() override; diff --git a/tests/auto/client/shared/mocksurface.cpp b/tests/auto/client/shared/mocksurface.cpp index c0d51618c..84dcda6b0 100644 --- a/tests/auto/client/shared/mocksurface.cpp +++ b/tests/auto/client/shared/mocksurface.cpp @@ -74,7 +74,7 @@ void Compositor::sendShellSurfaceConfigure(void *data, const QList &pa auto statesBytes = QByteArray::fromRawData(reinterpret_cast(states.data()), states.size() * static_cast(sizeof(uint))); toplevel->send_configure(size.width(), size.height(), statesBytes); - toplevel->xdgSurface()->send_configure(compositor->nextSerial()); + toplevel->xdgSurface()->sendConfigure(compositor->nextSerial()); } else if (auto wlShellSurface = surface->wlShellSurface()) { const uint edges = 0; wlShellSurface->send_configure(edges, size.width(), size.height()); @@ -123,13 +123,17 @@ void Surface::surface_destroy(Resource *resource) if (m_wlShellSurface) // on wl-shell the shell surface is automatically destroyed with the surface wl_resource_destroy(m_wlShellSurface->resource()->handle); Q_ASSERT(!m_wlShellSurface); - Q_ASSERT(!m_xdgToplevelV6); + Q_ASSERT(!m_xdgSurfaceV6); wl_resource_destroy(resource->handle); } -void Surface::surface_attach(Resource *resource, - struct wl_resource *buffer, int x, int y) +void Surface::surface_attach(Resource *resource, struct wl_resource *buffer, int x, int y) { + if (m_xdgSurfaceV6) { + // It's a protocol error to attach a buffer to an xdgSurface that's not configured + Q_ASSERT(xdgSurfaceV6()->configureSent()); + } + Q_UNUSED(resource); Q_UNUSED(x); Q_UNUSED(y); diff --git a/tests/auto/client/shared/mocksurface.h b/tests/auto/client/shared/mocksurface.h index 3b0f01fdb..949dc23dd 100644 --- a/tests/auto/client/shared/mocksurface.h +++ b/tests/auto/client/shared/mocksurface.h @@ -50,7 +50,8 @@ public: static Surface *fromResource(struct ::wl_resource *resource); void map(); bool isMapped() const; - XdgToplevelV6 *xdgToplevelV6() const { return m_xdgToplevelV6; } + XdgSurfaceV6 *xdgSurfaceV6() const { return m_xdgSurfaceV6; } + XdgToplevelV6 *xdgToplevelV6() const { return m_xdgSurfaceV6 ? m_xdgSurfaceV6->toplevel() : nullptr; } WlShellSurface *wlShellSurface() const { return m_wlShellSurface; } QSharedPointer mockSurface() const { return m_mockSurface; } @@ -69,7 +70,7 @@ protected: void surface_commit(Resource *resource) override; private: wl_resource *m_buffer = nullptr; - XdgToplevelV6 *m_xdgToplevelV6 = nullptr; + XdgSurfaceV6 *m_xdgSurfaceV6 = nullptr; WlShellSurface *m_wlShellSurface = nullptr; Compositor *m_compositor = nullptr; @@ -77,7 +78,7 @@ private: QList m_frameCallbackList; bool m_mapped = false; - friend class XdgToplevelV6; + friend class XdgSurfaceV6; friend class WlShellSurface; }; diff --git a/tests/auto/client/shared/mockxdgshellv6.cpp b/tests/auto/client/shared/mockxdgshellv6.cpp index 5dc8da599..05eff74ad 100644 --- a/tests/auto/client/shared/mockxdgshellv6.cpp +++ b/tests/auto/client/shared/mockxdgshellv6.cpp @@ -49,6 +49,18 @@ XdgSurfaceV6::XdgSurfaceV6(XdgShellV6 *shell, Surface *surface, wl_client *clien , m_surface(surface) , m_shell(shell) { + m_surface->m_xdgSurfaceV6 = this; +} + +XdgSurfaceV6::~XdgSurfaceV6() +{ + m_surface->m_xdgSurfaceV6 = nullptr; +} + +void XdgSurfaceV6::sendConfigure(uint32_t serial) +{ + send_configure(serial); + m_configureSent = true; } void XdgSurfaceV6::zxdg_surface_v6_get_toplevel(QtWaylandServer::zxdg_surface_v6::Resource *resource, uint32_t id) @@ -78,7 +90,6 @@ XdgToplevelV6::XdgToplevelV6(XdgSurfaceV6 *xdgSurface, wl_client *client, uint32 , m_mockToplevel(new MockXdgToplevelV6(this)) { auto *surface = m_xdgSurface->surface(); - surface->m_xdgToplevelV6 = this; m_xdgSurface->shell()->addToplevel(this); surface->map(); } @@ -86,7 +97,6 @@ XdgToplevelV6::XdgToplevelV6(XdgSurfaceV6 *xdgSurface, wl_client *client, uint32 XdgToplevelV6::~XdgToplevelV6() { m_xdgSurface->shell()->removeToplevel(this); - m_xdgSurface->surface()->m_xdgToplevelV6 = nullptr; m_mockToplevel->m_toplevel = nullptr; } diff --git a/tests/auto/client/shared/mockxdgshellv6.h b/tests/auto/client/shared/mockxdgshellv6.h index bd5e13063..a238fa562 100644 --- a/tests/auto/client/shared/mockxdgshellv6.h +++ b/tests/auto/client/shared/mockxdgshellv6.h @@ -46,8 +46,13 @@ class XdgSurfaceV6 : public QtWaylandServer::zxdg_surface_v6 { public: XdgSurfaceV6(XdgShellV6 *shell, Surface *surface, wl_client *client, uint32_t id); + ~XdgSurfaceV6() override; XdgShellV6 *shell() const { return m_shell; } Surface *surface() const { return m_surface; } + XdgToplevelV6 *toplevel() const { return m_toplevel; } + + void sendConfigure(uint32_t serial); + bool configureSent() const { return m_configureSent; } protected: void zxdg_surface_v6_destroy_resource(Resource *) override { delete this; } @@ -59,6 +64,7 @@ private: Surface *m_surface = nullptr; XdgToplevelV6 *m_toplevel = nullptr; XdgShellV6 *m_shell = nullptr; + bool m_configureSent = false; friend class XdgToplevelV6; }; diff --git a/tests/auto/client/xdgshellv6/tst_xdgshellv6.cpp b/tests/auto/client/xdgshellv6/tst_xdgshellv6.cpp index 91679cb57..3fd8153e6 100644 --- a/tests/auto/client/xdgshellv6/tst_xdgshellv6.cpp +++ b/tests/auto/client/xdgshellv6/tst_xdgshellv6.cpp @@ -101,6 +101,7 @@ private slots: void windowStateChangedEvents(); void windowGeometrySimple(); void windowGeometryFixed(); + void flushUnconfiguredXdgSurface(); private: MockCompositor *m_compositor = nullptr; @@ -360,6 +361,40 @@ void tst_WaylandClientXdgShellV6::windowGeometryFixed() QCOMPARE(geometrySpy.takeFirst().at(0).toRect().size(), initialWindowGeometry.size()); } +void tst_WaylandClientXdgShellV6::flushUnconfiguredXdgSurface() +{ + TestWindow window; + window.show(); + + QSharedPointer surface; + QTRY_VERIFY(surface = m_compositor->surface()); + + // Paint and flush some magenta + QBackingStore backingStore(&window); + QRect rect(QPoint(), window.size()); + backingStore.resize(rect.size()); + backingStore.beginPaint(rect); + QColor color = Qt::magenta; + QPainter p(backingStore.paintDevice()); + p.fillRect(rect, color); + p.end(); + backingStore.endPaint(); + backingStore.flush(rect); + + // We're not allowed to send buffer on this surface since it isn't yet configured. + // So, from the compositor's point of view there should be no buffer data yet. + m_compositor->processWaylandEvents(); + QVERIFY(surface->image.isNull()); + QVERIFY(!window.isExposed()); + + // Finally sending the configure should trigger an attach and commit with the + // right buffer. + m_compositor->sendShellSurfaceConfigure(surface); + QTRY_COMPARE(surface->image.size(), window.frameGeometry().size()); + QTRY_COMPARE(surface->image.pixel(window.frameMargins().left(), window.frameMargins().top()), color.rgba()); + QVERIFY(window.isExposed()); +} + int main(int argc, char **argv) { setenv("XDG_RUNTIME_DIR", ".", 1); -- cgit v1.2.3 From 16a98829a664403f2fb8962884701f5d4d31cacd Mon Sep 17 00:00:00 2001 From: Johan Klokkhammer Helsing Date: Fri, 12 Oct 2018 10:36:23 +0200 Subject: Client: Don't spam expose events Only send expose event when the geometry actually changed. Change-Id: Ic06986ce5d11e0ff7a842303f57093b8ff25b9f6 Reviewed-by: Paul Olav Tvete Reviewed-by: David Edmundson --- src/client/qwaylandwindow.cpp | 5 ++++- src/client/qwaylandwindow_p.h | 1 + tests/auto/client/xdgshellv6/tst_xdgshellv6.cpp | 23 +++++++++++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/client/qwaylandwindow.cpp b/src/client/qwaylandwindow.cpp index 1801c2a4b..4ac2ca51e 100644 --- a/src/client/qwaylandwindow.cpp +++ b/src/client/qwaylandwindow.cpp @@ -338,7 +338,9 @@ void QWaylandWindow::setGeometry(const QRect &rect) mSentInitialResize = true; } - sendExposeEvent(QRect(QPoint(), geometry().size())); + QRect exposeGeometry(QPoint(), geometry().size()); + if (exposeGeometry != mLastExposeGeometry) + sendExposeEvent(exposeGeometry); } void QWaylandWindow::resizeFromApplyConfigure(const QSize &sizeWithMargins, const QPoint &offset) @@ -356,6 +358,7 @@ void QWaylandWindow::sendExposeEvent(const QRect &rect) { if (!(mShellSurface && mShellSurface->handleExpose(rect))) QWindowSystemInterface::handleExposeEvent(window(), rect); + mLastExposeGeometry = rect; } diff --git a/src/client/qwaylandwindow_p.h b/src/client/qwaylandwindow_p.h index ad24b7358..56ebd3cc6 100644 --- a/src/client/qwaylandwindow_p.h +++ b/src/client/qwaylandwindow_p.h @@ -254,6 +254,7 @@ private: void handleScreenChanged(); bool mUpdateRequested = false; + QRect mLastExposeGeometry; static const wl_callback_listener callbackListener; static void frameCallback(void *data, struct wl_callback *wl_callback, uint32_t time); diff --git a/tests/auto/client/xdgshellv6/tst_xdgshellv6.cpp b/tests/auto/client/xdgshellv6/tst_xdgshellv6.cpp index 3fd8153e6..3c822325b 100644 --- a/tests/auto/client/xdgshellv6/tst_xdgshellv6.cpp +++ b/tests/auto/client/xdgshellv6/tst_xdgshellv6.cpp @@ -58,6 +58,14 @@ public: return QWindow::event(event); } + void exposeEvent(QExposeEvent *event) override + { + ++exposeEventCount; + QWindow::exposeEvent(event); + } + + int exposeEventCount = 0; + signals: void windowStateChangeEventReceived(uint oldState); }; @@ -102,6 +110,7 @@ private slots: void windowGeometrySimple(); void windowGeometryFixed(); void flushUnconfiguredXdgSurface(); + void dontSpamExposeEvents(); private: MockCompositor *m_compositor = nullptr; @@ -395,6 +404,20 @@ void tst_WaylandClientXdgShellV6::flushUnconfiguredXdgSurface() QVERIFY(window.isExposed()); } +void tst_WaylandClientXdgShellV6::dontSpamExposeEvents() +{ + TestWindow window; + window.show(); + + QSharedPointer surface; + QTRY_VERIFY(surface = m_compositor->surface()); + QTRY_VERIFY(window.exposeEventCount == 0); + + m_compositor->sendShellSurfaceConfigure(surface); + QTRY_VERIFY(window.isExposed()); + QTRY_VERIFY(window.exposeEventCount == 1); +} + int main(int argc, char **argv) { setenv("XDG_RUNTIME_DIR", ".", 1); -- cgit v1.2.3