Emit the committed() signal after the state is applied

Currently, the committed signal is emitted after the client has called
wl_surface.commit. However, this breaks with synchronized subsurfaces.

Notably, Firefox splits a web page in a bunch of smaller layers, which
can be backed by wl_subsurface objects.

All the subsurfaces are in the sync mode. If a layer needs to be
repainted, Firefox will commit the corresponding subsurface with a frame
callback.

Since the committed signal is emitted when the wl_surface.commit request
is invoked, kwin will schedule a new frame immediately. Meaning, that it
is quite likely that firefox will have old contents.

The right thing to do would be to schedule a frame when all the ancestors
of the layer subsurface have been committed.

This change re-jitters the commit logic so the committed signal is
emitted when a new state is applied to the surface. It also slightly
cleans up how SubSurfaceInterface::parentCommit() is called.

It will be nice to cleanup the commit logic further by calling the
surface role's commit hook unconditionally, i.e. not check whether it's
a subsurface. But doing so may result in infinite recursions. How to
clean up that is still TBD.

src/wayland/subcompositor_interface.cpp

@@ -174,37 +174,17 @@ void SubSurfaceInterfacePrivate::subsurface_set_desync(Resource *)
     }
     mode = SubSurfaceInterface::Mode::Desynchronized;
     if (!q->isSynchronized()) {
-        synchronizedCommit();
+        auto surfacePrivate = SurfaceInterfacePrivate::get(surface);
+        surfacePrivate->commitFromCache();
     }
     Q_EMIT q->modeChanged(SubSurfaceInterface::Mode::Desynchronized);
 }
 
 void SubSurfaceInterfacePrivate::commit()
 {
-    SurfaceInterfacePrivate *surfacePrivate = SurfaceInterfacePrivate::get(surface);
-
-    if (q->isSynchronized()) {
-        commitToCache();
-    } else {
-        if (hasCacheState) {
-            commitToCache();
-            commitFromCache();
-        } else {
-            surfacePrivate->swapStates(&surfacePrivate->pending, &surfacePrivate->current, true);
-        }
-
-        for (SubSurfaceInterface *subsurface : qAsConst(surfacePrivate->current.below)) {
-            SubSurfaceInterfacePrivate *subsurfacePrivate = SubSurfaceInterfacePrivate::get(subsurface);
-            subsurfacePrivate->parentCommit();
-        }
-        for (SubSurfaceInterface *subsurface : qAsConst(surfacePrivate->current.above)) {
-            SubSurfaceInterfacePrivate *subsurfacePrivate = SubSurfaceInterfacePrivate::get(subsurface);
-            subsurfacePrivate->parentCommit();
-        }
-    }
 }
 
-void SubSurfaceInterfacePrivate::parentCommit(bool synchronized)
+void SubSurfaceInterfacePrivate::parentCommit()
 {
     if (hasPendingPosition) {
         hasPendingPosition = false;
@@ -212,40 +192,12 @@ void SubSurfaceInterfacePrivate::parentCommit(bool synchronized)
         Q_EMIT q->positionChanged(position);
     }
 
-    if (synchronized || mode == SubSurfaceInterface::Mode::Synchronized) {
-        synchronizedCommit();
+    if (mode == SubSurfaceInterface::Mode::Synchronized) {
+        auto surfacePrivate = SurfaceInterfacePrivate::get(surface);
+        surfacePrivate->commitFromCache();
     }
 }
 
-void SubSurfaceInterfacePrivate::synchronizedCommit()
-{
-    const SurfaceInterfacePrivate *surfacePrivate = SurfaceInterfacePrivate::get(surface);
-    commitFromCache();
-
-    for (SubSurfaceInterface *subsurface : qAsConst(surfacePrivate->current.below)) {
-        SubSurfaceInterfacePrivate *subsurfacePrivate = SubSurfaceInterfacePrivate::get(subsurface);
-        subsurfacePrivate->parentCommit(true);
-    }
-    for (SubSurfaceInterface *subsurface : qAsConst(surfacePrivate->current.above)) {
-        SubSurfaceInterfacePrivate *subsurfacePrivate = SubSurfaceInterfacePrivate::get(subsurface);
-        subsurfacePrivate->parentCommit(true);
-    }
-}
-
-void SubSurfaceInterfacePrivate::commitToCache()
-{
-    SurfaceInterfacePrivate *surfacePrivate = SurfaceInterfacePrivate::get(surface);
-    surfacePrivate->swapStates(&surfacePrivate->pending, &surfacePrivate->cached, false);
-    hasCacheState = true;
-}
-
-void SubSurfaceInterfacePrivate::commitFromCache()
-{
-    SurfaceInterfacePrivate *surfacePrivate = SurfaceInterfacePrivate::get(surface);
-    surfacePrivate->swapStates(&surfacePrivate->cached, &surfacePrivate->current, true);
-    hasCacheState = false;
-}
-
 SubSurfaceInterface::SubSurfaceInterface(SurfaceInterface *surface, SurfaceInterface *parent,
                                          wl_resource *resource)
     : d(new SubSurfaceInterfacePrivate(this, surface, parent, resource))

src/wayland/subsurface_interface_p.h

@@ -41,12 +41,7 @@ public:
                                SurfaceInterface *parent, ::wl_resource *resource);
 
     void commit() override;
-
-    void parentCommit(bool synchronized = false);
-    void synchronizedCommit();
-
-    void commitToCache();
-    void commitFromCache();
+    void parentCommit();
 
     SubSurfaceInterface *q;
     QPoint position = QPoint(0, 0);
@@ -54,7 +49,6 @@ public:
     SubSurfaceInterface::Mode mode = SubSurfaceInterface::Mode::Synchronized;
     QPointer<SurfaceInterface> surface;
     QPointer<SurfaceInterface> parent;
-    bool hasCacheState = false;
     bool hasPendingPosition = false;
 
 protected:

src/wayland/surface_interface.cpp

@@ -649,29 +649,54 @@ void SurfaceInterfacePrivate::swapStates(SurfaceState *source, SurfaceState *tar
     if (childrenChanged) {
         Q_EMIT q->childSubSurfacesChanged();
     }
+    // The position of a sub-surface is applied when its parent is committed.
+    for (SubSurfaceInterface *subsurface : qAsConst(current.below)) {
+        auto subsurfacePrivate = SubSurfaceInterfacePrivate::get(subsurface);
+        subsurfacePrivate->parentCommit();
+    }
+    for (SubSurfaceInterface *subsurface : qAsConst(current.above)) {
+        auto subsurfacePrivate = SubSurfaceInterfacePrivate::get(subsurface);
+        subsurfacePrivate->parentCommit();
+    }
+    if (role) {
+        role->commit();
+    }
+    Q_EMIT q->committed();
 }
 
 void SurfaceInterfacePrivate::commit()
 {
-    if (!subSurface) {
+    if (subSurface) {
+        commitSubSurface();
+    } else {
         swapStates(&pending, &current, true);
+    }
+}
 
-        // The position of a sub-surface is applied when its parent is committed.
-        for (SubSurfaceInterface *subsurface : qAsConst(current.below)) {
-            auto subsurfacePrivate = SubSurfaceInterfacePrivate::get(subsurface);
-            subsurfacePrivate->parentCommit();
-        }
-        for (SubSurfaceInterface *subsurface : qAsConst(current.above)) {
-            auto subsurfacePrivate = SubSurfaceInterfacePrivate::get(subsurface);
-            subsurfacePrivate->parentCommit();
+void SurfaceInterfacePrivate::commitSubSurface()
+{
+    if (subSurface->isSynchronized()) {
+        commitToCache();
+    } else {
+        if (hasCacheState) {
+            commitToCache();
+            commitFromCache();
+        } else {
+            swapStates(&pending, &current, true);
         }
     }
+}
 
-    if (role) {
-        role->commit();
-    }
+void SurfaceInterfacePrivate::commitToCache()
+{
+    swapStates(&pending, &cached, false);
+    hasCacheState = true;
+}
 
-    Q_EMIT q->committed();
+void SurfaceInterfacePrivate::commitFromCache()
+{
+    swapStates(&cached, &current, true);
+    hasCacheState = false;
 }
 
 QRegion SurfaceInterface::damage() const

src/wayland/surface_interface_p.h

@@ -95,7 +95,11 @@ public:
     void installPointerConstraint(ConfinedPointerV1Interface *confinement);
     void installIdleInhibitor(IdleInhibitorV1Interface *inhibitor);
 
+    void commitToCache();
+    void commitFromCache();
+
     void commit();
+    void commitSubSurface();
     QMatrix4x4 buildSurfaceToBufferMatrix(const SurfaceState *state);
     void swapStates(SurfaceState *source, SurfaceState *target, bool emitChanged);
 
@@ -110,6 +114,7 @@ public:
     QMatrix4x4 bufferToSurfaceMatrix;
     QSize bufferSize;
     QRegion inputRegion;
+    bool hasCacheState = false;
 
     // workaround for https://bugreports.qt.io/browse/QTBUG-52192
     // A subsurface needs to be considered mapped even if it doesn't have a buffer attached