From a7e8bd4beaad4999e7b7ac7f830ccad0068a3966 Mon Sep 17 00:00:00 2001 From: Michael Olbrich Date: Mon, 25 Mar 2024 15:06:20 +0100 Subject: [PATCH] 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 --- libweston/compositor.c | 53 ++++++++++++++++++++++-------------- tests/subsurface-shot-test.c | 51 ++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 21 deletions(-) diff --git a/libweston/compositor.c b/libweston/compositor.c index 77ede354..139ea52f 100644 --- a/libweston/compositor.c +++ b/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 * diff --git a/tests/subsurface-shot-test.c b/tests/subsurface-shot-test.c index e31a3ae2..5acb613f 100644 --- a/tests/subsurface-shot-test.c +++ b/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); +}