subsurface: don't forget to repaint after the first sub-surface commit

For the following sequence, weston will not trigger a repaint:

1. create the main surface
2. create another surface and attach it as a sub-surface to the main surface
3. set the sub-surface to desync
4. attach a buffer to the main surface and commit it
5. attach a buffer to the sub-surface and commit it

Step 5 should cause the sub-surface to become mapped. However, Weston fails to
schedule a repaint in that case, so the sub-surface will not appear until
something else causes a repaint on that output, e.g. the main window.

And sub-surfaces are special when it comes to mapping because
weston_surface_is_mapped() will not return true until the parent surface is
mapped as well. So right now, weston_surface_map() may be called multiple times
and it will send the map_signal each time.

So to fix all this and make it clearer:

1. define a separate weston_surface_start_mapping() function to make it clearer
   that the (sub-)surface may not be fully mapped at the end
2. check surface->is_mapped explicitly to ensure that the sub-surface is only
   mapped once.
3. call weston_view_update_transform() for all views of the sub-surface when the
   parent surface is already mapped to ensure that a repaint for all relevant
   outputs is triggered.

The new test checks this by waiting for a frame event for the first subsurface
commit. Without these changes, the test will block until it is killed by the
timeout.

Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de>

libweston/compositor.c

@@ -2633,16 +2633,25 @@ weston_view_unmap(struct weston_view *view)
 	view->surface->compositor->view_list_needs_rebuild = true;
 }
 
+static void weston_surface_start_mapping(struct weston_surface *surface)
+{
+	assert(surface->is_mapped == false);
+
+	surface->is_mapping = true;
+	surface->is_mapped = true;
+	surface->compositor->view_list_needs_rebuild = true;
+	wl_signal_emit_mutable(&surface->map_signal, surface);
+}
+
 WL_EXPORT void
 weston_surface_map(struct weston_surface *surface)
 {
 	if (weston_surface_is_mapped(surface))
 		return;
 
-	surface->is_mapping = true;
-	surface->is_mapped = true;
-	surface->compositor->view_list_needs_rebuild = true;
-	wl_signal_emit_mutable(&surface->map_signal, surface);
+	assert(!weston_surface_to_subsurface(surface));
+
+	weston_surface_start_mapping(surface);
 }
 
 WL_EXPORT void
@@ -5354,25 +5363,27 @@ subsurface_committed(struct weston_surface *surface,
 		tmp = weston_coord_surface_add(tmp, new_origin);
 		weston_view_set_rel_position(view, tmp);
 	}
-	/* No need to check parent mappedness, because if parent is not
-	 * mapped, parent is not in a visible layer, so this sub-surface
-	 * will not be drawn either.
+	/* Explicitly check is_mapped here. weston_surface_is_mapped() is only
+	 * true if the parent is mapped as well and this should only be called
+	 * once, regardless of the parent state.
 	 */
-	if (!weston_surface_is_mapped(surface) &&
-	    weston_surface_has_content(surface)) {
-		weston_surface_map(surface);
-	}
+	if (!surface->is_mapped && weston_surface_has_content(surface)) {
+		struct weston_subsurface *sub = weston_surface_to_subsurface(surface);
+		struct weston_view *view;
 
-	/* Cannot call weston_view_update_transform() here, because that would
-	 * call it also for the parent surface, which might not be mapped yet.
-	 * That would lead to inconsistent state, where the window could never
-	 * be mapped.
-	 *
-	 * Instead just force the child surface to appear mapped, to make
-	 * weston_surface_is_mapped() return true, so that when the parent
-	 * surface does get mapped, this one will get included, too. See
-	 * view_list_add().
-	 */
+		/* Make sure the output and output_mask for the surface is
+		 * updated. If the parent is not yet mapped, all of this will
+		 * happen later with the parent. If the parent is already
+		 * mapped, call weston_view_update_transform() here.
+		 * Otherwise a desynced subsurface will not trigger a
+		 * repaint.
+		 */
+		if (sub->parent && weston_surface_is_mapped(sub->parent)) {
+			wl_list_for_each(view, &surface->views, surface_link)
+				weston_view_update_transform(view);
+		}
+		weston_surface_start_mapping(surface);
+	}
 }
 
 static struct weston_subsurface *

tests/subsurface-shot-test.c

@@ -503,3 +503,54 @@ TEST(subsurface_empty_mapping)
 	wl_subcompositor_destroy(subco);
 	client_destroy(client);
 }
+
+TEST(subsurface_desync_commit)
+{
+	struct client *client;
+	struct wl_subcompositor *subco;
+	struct buffer *bufs[2] = { 0 };
+	struct wl_surface *surf[2] = { 0 };
+	struct wl_subsurface *sub = { 0 };
+	pixman_color_t red;
+	pixman_color_t green;
+	unsigned i;
+	int frame;
+
+	color_rgb888(&red, 255, 0, 0);
+	color_rgb888(&green, 0, 255, 0);
+
+	client = create_client_and_test_surface(100, 50, 100, 100);
+	assert(client);
+	subco = get_subcompositor(client);
+
+	/* make the parent surface red */
+	surf[0] = client->surface->wl_surface;
+	client->surface->wl_surface = NULL; /* we stole it and destroy it */
+	/* make sure the surface was rendered before the subsurface is created */
+	frame_callback_set(surf[0], &frame);
+	bufs[0] = surface_commit_color(client, surf[0], &red, 100, 100);
+	frame_callback_wait(client, &frame);
+
+	/* create an empty subsurface on top */
+	surf[1] = wl_compositor_create_surface(client->wl_compositor);
+	sub = wl_subcompositor_get_subsurface(subco, surf[1], surf[0]);
+	wl_subsurface_set_desync (sub);
+
+	/* wait for the first buffer of the subsurface to be rendered */
+	frame_callback_set(surf[1], &frame);
+	bufs[1] = surface_commit_color(client, surf[1], &green, 100, 100);
+	frame_callback_wait(client, &frame);
+
+	wl_subsurface_destroy(sub);
+
+	for (i = 0; i < ARRAY_LENGTH(surf); i++)
+		if (surf[i])
+			wl_surface_destroy(surf[i]);
+
+	for (i = 0; i < ARRAY_LENGTH(bufs); i++)
+		if (bufs[i])
+			buffer_destroy(bufs[i]);
+
+	wl_subcompositor_destroy(subco);
+	client_destroy(client);
+}