Created attachment 295608 [details] [review]
wayland: Commit cached surface state when subsurface goes desynchronized

Review of attachment 295608 [details] [review]:

What's the rationale for this? What bug does it fix?

It made the weston subsurface work for me.

It's more complicated than that. I'm of the opinion that the subsurfaces test is broken and violates the spec, but talk to pq about that.

Thanks for the bug report. This particular bug has already been reported into our bug tracking system, but please feel free to report any further bugs you find.

*** This bug has been marked as a duplicate of bug 731494 ***

That is another issue, this had nothing to do with positioning. The issue was that because set_desync didn't cause the content to be drawn, it didn't receive a frame callback, and thus didn't continue drawing.

Unresolving as the bug it was marked as a duplicate of is related to when set_position takes effect; this is about that we didn't comply with the paragraph

"If a surface's parent surface behaves as desynchronized, then
the cached state is applied on set_desync"

in wl_subsurface.set_desync.

What's wrong with these two lines?

   if (surface->sub.synchronous)
     subsurface_parent_surface_committed (surface);

Shouldn't that make it implement the spec'd behavior?

subsurface_parent_surface_committed would call:

    commit_pending_state (surface, &surface->sub.pending);

which would then enter the block

  if (surface->sub.synchronous)
    {
      move_pending_state (pending, &surface->sub.pending);
      return;
    }

Here we'd move the subsurface->sub.pending to pending, which is subsurface->sub.pending, and obviously that makes no sense.

We'd also apply the pending position state even though, but as the spec dictates; from wl_subsurface.set_position:

	The next wl_surface.commit on the parent surface will reset
	the sub-surface's position to the scheduled coordinates.

meaning set_desync should not apply the positions.

This change would however mean that parent_surface_committed would be called for all the subsurfaces lower in the tree, which is a side effect of this change which probably is not what is supposed to happen.

Created attachment 295726 [details] [review]
wayland: Apply cached surface state when subsurface goes desynchronized

When a surface goes desynchronized after having been in synchronized
mode, it should apply the cached state as if it was committed as long as
it is not still effectively synchronized. A surface may still be
effectively synchronized even when its subsurface is set to be
desynchronized if its parent surface is a subsurface that is effectivey
synchronized. See is_surface_effctively_synchronized().

The above attached patch keeps the old semantics regarding positioning, but fixes the sync -> desync path.

Created attachment 298594 [details] [review]
wayland: Rework synchronized state application semantics

When a parent of a subsurface gets it state applied (either by a
wl_surface.commit, wl_subsurface.set_desync or a recursive
wl_surface.commit on a parent surface), the pending position state
of the subsurface should be applied. If the subsurface is in effective
synchronized mode (i.e. if its in explicit synchronized mode or any of
its parent surfaces is a subsurface in explicit synchronized mode), the
cached state should also be applied at this point, including its
subsurface children, recursively.

The latest patch updates mutter to be compliant according to the clarifications done as part of https://bugs.freedesktop.org/show_bug.cgi?id=88857 . It also fixes bug 731494.

Review of attachment 298594 [details] [review]:

Looks to properly implement the current spec. Few minor notes, as far as I'm concerned can be committed once these are fixed.

::: src/wayland/meta-wayland-surface.c
@@ +361,3 @@
+ * synchronized or itself is in synchronized mode. */
+static gboolean
+is_surface_effctively_synchronized (MetaWaylandSurface *surface)

should be "effectively"

@@ +507,3 @@
+   * cache the pending surface state until either one of the following two
+   * scenarios happens:
+   *  1) Its parent surface gets it state applied.

gets _its_ state applied

@@ +515,3 @@
+      move_pending_state (&surface->pending, &surface->sub.pending);
+    }
+  else

Style is no braces when all branches are single line

Pushed with the raised issues addressed. Thanks for the review.

Attachment 298594 [details] pushed as 868e142 - wayland: Rework synchronized state application semantics