From 4a328e3533f93ed65f9dc77f764b3cbe3c694266 Mon Sep 17 00:00:00 2001 From: Johan Klokkhammer Helsing Date: Wed, 17 Oct 2018 14:05:27 +0200 Subject: Compositor: Emit signals after applying pending surface state [ChangeLog][Compositor] Fixed a bug where some signals on QWaylandSurface were emitted before all double buffered state had been applied. Restructures QWaylandSurface::commit so no signals are emitted until all state mutations are completed. Adds a test to confirm that pending state is applied at once. Also fixes opaqueRegion, which is documented to be double buffered as well, but the old implementation set it immediately. Change-Id: I1c4dfea7c83dd9ee84dc8c03e6d92e2924bf2fad Reviewed-by: Paul Olav Tvete --- src/compositor/compositor_api/qwaylandsurface.cpp | 84 ++++++++++------------ src/compositor/compositor_api/qwaylandsurface_p.h | 6 +- tests/auto/compositor/compositor/mockclient.cpp | 2 +- .../auto/compositor/compositor/tst_compositor.cpp | 74 ++++++++++++++++++- 4 files changed, 113 insertions(+), 53 deletions(-) diff --git a/src/compositor/compositor_api/qwaylandsurface.cpp b/src/compositor/compositor_api/qwaylandsurface.cpp index 2521b7fa1..13ae28220 100644 --- a/src/compositor/compositor_api/qwaylandsurface.cpp +++ b/src/compositor/compositor_api/qwaylandsurface.cpp @@ -141,25 +141,6 @@ QWaylandSurfacePrivate::~QWaylandSurfacePrivate() c->destroy(); } -void QWaylandSurfacePrivate::setSize(const QSize &s) -{ - Q_Q(QWaylandSurface); - if (size != s) { - opaqueRegion = QRegion(); - size = s; - q->sizeChanged(); - } -} - -void QWaylandSurfacePrivate::setBufferScale(int scale) -{ - Q_Q(QWaylandSurface); - if (scale == bufferScale) - return; - bufferScale = scale; - emit q->bufferScaleChanged(); -} - void QWaylandSurfacePrivate::removeFrameCallback(QtWayland::FrameCallback *callback) { pendingFrameCallbacks.removeOne(callback); @@ -235,7 +216,7 @@ void QWaylandSurfacePrivate::surface_frame(Resource *resource, uint32_t callback void QWaylandSurfacePrivate::surface_set_opaque_region(Resource *, struct wl_resource *region) { - opaqueRegion = region ? QtWayland::Region::fromResource(region)->region() : QRegion(); + pending.opaqueRegion = region ? QtWayland::Region::fromResource(region)->region() : QRegion(); } void QWaylandSurfacePrivate::surface_set_input_region(Resource *, struct wl_resource *region) @@ -251,42 +232,53 @@ void QWaylandSurfacePrivate::surface_commit(Resource *) { Q_Q(QWaylandSurface); + // Needed in order to know whether we want to emit signals later + QSize oldBufferSize = bufferSize; + bool oldHasContent = hasContent; + int oldBufferScale = bufferScale; + + // Update all internal state if (pending.buffer.hasBuffer() || pending.newlyAttached) bufferRef = pending.buffer; + bufferSize = bufferRef.size(); + damage = pending.damage.intersected(QRect(QPoint(), bufferSize)); + hasContent = bufferRef.hasContent(); + bufferScale = pending.bufferScale; + frameCallbacks << pendingFrameCallbacks; + inputRegion = pending.inputRegion.intersected(QRect(QPoint(), bufferSize)); + opaqueRegion = pending.opaqueRegion.intersected(QRect(QPoint(), bufferSize)); + QPoint offsetForNextFrame = pending.offset; - auto buffer = bufferRef.buffer(); - if (buffer) - buffer->setCommitted(pending.damage); + // Clear per-commit state + pending.buffer = QWaylandBufferRef(); + pending.offset = QPoint(); + pending.newlyAttached = false; + pending.damage = QRegion(); + pendingFrameCallbacks.clear(); - setSize(bufferRef.size()); - damage = pending.damage.intersected(QRect(QPoint(), size)); + // Notify buffers and views + if (auto *buffer = bufferRef.buffer()) + buffer->setCommitted(damage); + for (auto *view : qAsConst(views)) + view->bufferCommitted(bufferRef, damage); - for (int i = 0; i < views.size(); i++) { - views.at(i)->bufferCommitted(bufferRef, damage); - } + // Now all double-buffered state has been applied so it's safe to emit general signals + // i.e. we won't have inconsistensies such as mismatched surface size and buffer scale in + // signal handlers. emit q->damaged(damage); - bool oldHasContent = hasContent; - hasContent = bufferRef.hasContent(); - if (oldHasContent != hasContent) - emit q->hasContentChanged(); - - if (!pending.offset.isNull()) - emit q->offsetForNextFrame(pending.offset); - - setBufferScale(pending.bufferScale); + if (oldBufferSize != bufferSize) + emit q->sizeChanged(); + if (oldBufferScale != bufferScale) + emit q->bufferScaleChanged(); - pending.buffer = QWaylandBufferRef(); - pending.offset = QPoint(); - pending.newlyAttached = false; - pending.damage = QRegion(); - - frameCallbacks << pendingFrameCallbacks; - pendingFrameCallbacks.clear(); + if (oldHasContent != hasContent) + emit q->hasContentChanged(); - inputRegion = pending.inputRegion.intersected(QRect(QPoint(), size)); + if (!offsetForNextFrame.isNull()) + emit q->offsetForNextFrame(offsetForNextFrame); emit q->redraw(); } @@ -469,7 +461,7 @@ bool QWaylandSurface::hasContent() const QSize QWaylandSurface::size() const { Q_D(const QWaylandSurface); - return d->size; + return d->bufferSize; } /*! diff --git a/src/compositor/compositor_api/qwaylandsurface_p.h b/src/compositor/compositor_api/qwaylandsurface_p.h index e0b624fce..df868de63 100644 --- a/src/compositor/compositor_api/qwaylandsurface_p.h +++ b/src/compositor/compositor_api/qwaylandsurface_p.h @@ -102,9 +102,6 @@ public: using QtWaylandServer::wl_surface::resource; - void setSize(const QSize &size); - void setBufferScale(int bufferScale); - void removeFrameCallback(QtWayland::FrameCallback *callback); void notifyViewsAboutDestruction(); @@ -155,6 +152,7 @@ public: //member variables bool newlyAttached; QRegion inputRegion; int bufferScale; + QRegion opaqueRegion; } pending; QPoint lastLocalMousePos; @@ -168,7 +166,7 @@ public: //member variables QRegion inputRegion; QRegion opaqueRegion; - QSize size; + QSize bufferSize; int bufferScale = 1; bool isCursorSurface = false; bool destroyed = false; diff --git a/tests/auto/compositor/compositor/mockclient.cpp b/tests/auto/compositor/compositor/mockclient.cpp index e6eac1553..f74314407 100644 --- a/tests/auto/compositor/compositor/mockclient.cpp +++ b/tests/auto/compositor/compositor/mockclient.cpp @@ -166,7 +166,7 @@ void MockClient::handleGlobalRemove(void *data, wl_registry *wl_registry, uint32 void MockClient::handleGlobal(uint32_t id, const QByteArray &interface) { if (interface == "wl_compositor") { - compositor = static_cast(wl_registry_bind(registry, id, &wl_compositor_interface, 1)); + compositor = static_cast(wl_registry_bind(registry, id, &wl_compositor_interface, 3)); } else if (interface == "wl_output") { auto output = static_cast(wl_registry_bind(registry, id, &wl_output_interface, 2)); m_outputs.insert(id, output); diff --git a/tests/auto/compositor/compositor/tst_compositor.cpp b/tests/auto/compositor/compositor/tst_compositor.cpp index c7661e8db..2c0e46b23 100644 --- a/tests/auto/compositor/compositor/tst_compositor.cpp +++ b/tests/auto/compositor/compositor/tst_compositor.cpp @@ -74,6 +74,7 @@ private slots: void modes(); void sizeFollowsWindow(); void mapSurface(); + void mapSurfaceHiDpi(); void frameCallback(); void removeOutput(); @@ -421,9 +422,78 @@ void tst_WaylandCompositor::mapSurface() wl_surface_damage(surface, 0, 0, size.width(), size.height()); wl_surface_commit(surface); - QTRY_COMPARE(waylandSurface->size(), size); - QTRY_COMPARE(waylandSurface->hasContent(), true); QTRY_COMPARE(hasContentSpy.count(), 1); + QCOMPARE(waylandSurface->hasContent(), true); + QCOMPARE(waylandSurface->size(), size); + + wl_surface_destroy(surface); +} + +void tst_WaylandCompositor::mapSurfaceHiDpi() +{ + TestCompositor compositor; + compositor.create(); + + MockClient client; + + wl_surface *surface = client.createSurface(); + QTRY_COMPARE(compositor.surfaces.size(), 1); + + QWaylandSurface *waylandSurface = compositor.surfaces.at(0); + + constexpr int bufferScale = 2; + const QSize surfaceSize(128, 128); + const QSize bufferSize = surfaceSize * bufferScale; + const QPoint attachOffset(1, 2); // in surface-local coordinates + + client.createShellSurface(surface); + ShmBuffer buffer(bufferSize, client.shm); + wl_surface_attach(surface, buffer.handle, attachOffset.x(), attachOffset.y()); + wl_surface_set_buffer_scale(surface, bufferScale); + // wl_surface_damage is given in surface coordinates + wl_surface_damage(surface, 0, 0, surfaceSize.width(), surfaceSize.height()); + + auto verifyComittedState = [=]() { + QCOMPARE(waylandSurface->size(), bufferSize); + QCOMPARE(waylandSurface->bufferScale(), bufferScale); + QCOMPARE(waylandSurface->hasContent(), true); + }; + + QObject::connect(waylandSurface, &QWaylandSurface::damaged, [=] (const QRegion &damage) { + // Currently, QWaylandSurface::size returns the size in pixels. + // Should be fixed or removed for Qt 6. + QCOMPARE(damage, QRect(QPoint(), surfaceSize)); + verifyComittedState(); + }); + QSignalSpy damagedSpy(waylandSurface, SIGNAL(damaged(const QRegion &))); + + QObject::connect(waylandSurface, &QWaylandSurface::hasContentChanged, verifyComittedState); + QSignalSpy hasContentSpy(waylandSurface, SIGNAL(hasContentChanged())); + + QObject::connect(waylandSurface, &QWaylandSurface::sizeChanged, verifyComittedState); + QSignalSpy sizeSpy(waylandSurface, SIGNAL(sizeChanged())); + + QObject::connect(waylandSurface, &QWaylandSurface::bufferScaleChanged, verifyComittedState); + QSignalSpy bufferScaleSpy(waylandSurface, SIGNAL(bufferScaleChanged())); + + QObject::connect(waylandSurface, &QWaylandSurface::offsetForNextFrame, [=](const QPoint &offset) { + QCOMPARE(offset, attachOffset); + verifyComittedState(); + }); + QSignalSpy offsetSpy(waylandSurface, SIGNAL(offsetForNextFrame(const QPoint &))); + + // No state should be applied before the commit + QCOMPARE(waylandSurface->size(), QSize()); + QCOMPARE(waylandSurface->hasContent(), false); + QCOMPARE(waylandSurface->bufferScale(), 1); + QCOMPARE(offsetSpy.count(), 0); + + wl_surface_commit(surface); + + QTRY_COMPARE(hasContentSpy.count(), 1); + QTRY_COMPARE(sizeSpy.count(), 1); + QTRY_COMPARE(bufferScaleSpy.count(), 1); + QTRY_COMPARE(offsetSpy.count(), 1); wl_surface_destroy(surface); } -- cgit v1.2.3