Compositor: Emit signals after applying pending surface statev5.12.0-beta3

[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 <paul.tvete@qt.io>

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_compositor *>(wl_registry_bind(registry, id, &wl_compositor_interface, 1));
+        compositor = static_cast<wl_compositor *>(wl_registry_bind(registry, id, &wl_compositor_interface, 3));
     } else if (interface == "wl_output") {
         auto output = static_cast<wl_output *>(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);
 }