From e73914d7dd223e83a996a97ce2fb934dd8803842 Mon Sep 17 00:00:00 2001 From: hujianwei Date: Sun, 6 Nov 2022 03:21:27 +0000 Subject: [PATCH] Correctly implement nested subsurface sync or desync * compositor.h (struct _RoleFuncs): Get rid of note_desync_child and co. * frame_clock.c (StartFrame): Fix typo. * subsurface.c (struct _Subsurface): New field `should_be_desync'. (NoteDesyncChild, NoteChildSynced): Delete functions. (IsParentSynchronous): New function. (SetSync1, NoteSubsurfaceDesynchronous, NoteSubsurfaceTeardown): New functions. (SetSync): Make all children synchronous when required. (SetDesync): Desynchronize all children if possible; otherwise, keep the surface synchronous but set the `should_be_desync' flag. (EarlyCommit): Merge pending state if required. (Setup): Call SetSync1 to make children synchronous. (Teardown): Call NoteSubsurfaceTeardown to make children desynchronous as required. (GetSubsurface): Stop setting note_child_synced/note_desync_child. * surface.c (InternalCommit1): New function. (InternalCommit): Move state merging logic to InternalCommit1. (XLSurfaceMergeCachedState): New function. * xdg_surface.c (struct _XdgRole): Remove `n_desync_children'. (UpdateFrameRefreshPrediction): Resort to counting the number of children on each frame instead. (Setup, NoteFrame, NoteChildSynced, NoteDesyncChild, HandleFreeze) (XLGetXdgSurface): Stop counting sync/desync children. * tests/subsurface_test.c (enum test_kind, test_names): Add SUBSURFACE_DESYNC_KIND. (LAST_TEST): Set to SUBSURFACE_DESYNC_KIND. (test_single_step): Implement SUBSURFACE_DESYNC_KIND. --- compositor.h | 3 +- frame_clock.c | 2 +- subsurface.c | 194 +++++++++++++++++++++++++++++++--------- surface.c | 19 +++- tests/subsurface_test.c | 147 ++++++++++++++++++++++++++++-- xdg_surface.c | 69 ++++++-------- 6 files changed, 339 insertions(+), 95 deletions(-) diff --git a/compositor.h b/compositor.h index 86eec72..fc433ae 100644 --- a/compositor.h +++ b/compositor.h @@ -1129,8 +1129,6 @@ struct _RoleFuncs void (*move_by) (Surface *, Role *, int, int); void (*rescale) (Surface *, Role *); void (*parent_rescale) (Surface *, Role *); - void (*note_desync_child) (Surface *, Role *); - void (*note_child_synced) (Surface *, Role *); void (*select_extra_events) (Surface *, Role *, unsigned long); void (*note_focus) (Surface *, Role *, FocusMode); void (*outputs_changed) (Surface *, Role *); @@ -1182,6 +1180,7 @@ extern Window XLWindowFromSurface (Surface *); extern void XLUpdateSurfaceOutputs (Surface *, int, int, int, int); extern void XLSurfaceSelectExtraEvents (Surface *, unsigned long); extern void XLSurfaceNoteFocus (Surface *, FocusMode); +extern void XLSurfaceMergeCachedState (Surface *); extern void SurfaceToWindow (Surface *, double, double, double *, double *); extern void ScaleToWindow (Surface *, double, double, double *, double *); diff --git a/frame_clock.c b/frame_clock.c index a693fe1..657dc22 100644 --- a/frame_clock.c +++ b/frame_clock.c @@ -442,7 +442,7 @@ StartFrame (FrameClock *clock, Bool urgent, Bool predict) /* Don't start the end frame timer if this frame is being drawn in response to configury. */ - predict = True; + predict = False; } clock->in_frame = True; diff --git a/subsurface.c b/subsurface.c index 38008f2..734410b 100644 --- a/subsurface.c +++ b/subsurface.c @@ -19,6 +19,7 @@ along with 12to11. If not, see . */ #include #include +#include #include "compositor.h" @@ -89,6 +90,9 @@ struct _Subsurface /* Commit callback attached to the parent. */ CommitCallback *commit_callback; + /* Whether or not this should be desynchronous. */ + Bool should_be_desync; + /* Whether or not this is synchronous. */ Bool synchronous; @@ -389,33 +393,28 @@ PlaceBelow (struct wl_client *client, struct wl_resource *resource, } static void -NoteDesyncChild (Surface *surface, Role *role) +SetSync1 (Subsurface *subsurface) { - Subsurface *subsurface; + Surface *child; + Subsurface *child_subsurface; + XLList *list; - subsurface = SubsurfaceFromRole (role); + /* Note that the given subsurface has become synchronous by setting + its synchronous flag to True. */ - if (!subsurface->parent || !subsurface->parent->role - || !subsurface->parent->role->funcs.note_desync_child) - return; + subsurface->synchronous = True; - subsurface->parent->role->funcs.note_desync_child (subsurface->parent, - subsurface->parent->role); -} + if (subsurface->role.surface) + { + list = subsurface->role.surface->subsurfaces; -static void -NoteChildSynced (Surface *surface, Role *role) -{ - Subsurface *subsurface; - - subsurface = SubsurfaceFromRole (role); - - if (!subsurface->parent || !subsurface->parent->role - || !subsurface->parent->role->funcs.note_child_synced) - return; - - subsurface->parent->role->funcs.note_child_synced (subsurface->parent, - subsurface->parent->role); + for (; list; list = list->next) + { + child = list->data; + child_subsurface = SubsurfaceFromRole (child->role); + SetSync1 (child_subsurface); + } + } } static void @@ -425,12 +424,90 @@ SetSync (struct wl_client *client, struct wl_resource *resource) subsurface = wl_resource_get_user_data (resource); - if (subsurface->role.surface - && !subsurface->synchronous) - NoteChildSynced (subsurface->role.surface, - &subsurface->role); + /* This subsurface should not actually be desynchronous. */ + subsurface->should_be_desync = False; - subsurface->synchronous = True; + /* Now, make each child synchronous recursively. */ + SetSync1 (subsurface); +} + +static Bool +IsParentSynchronous (Subsurface *subsurface) +{ + Surface *surface; + Subsurface *parent; + + surface = subsurface->parent; + + if (!surface || surface->role_type != SubsurfaceType) + return False; + + parent = SubsurfaceFromRole (surface->role); + + return parent->synchronous; +} + +static void +NoteSubsurfaceDesynchronous (Subsurface *subsurface, Bool apply_state) +{ + Surface *child; + Subsurface *child_subsurface; + XLList *list; + + /* Note that the given subsurface has become desynchronous, and + apply pending state. Make each of its children that should be + desynchronous desynchronous as well, but avoid applying their + pending state. */ + + subsurface->synchronous = False; + + if (subsurface->pending_commit && subsurface->role.surface + && apply_state) + { + XLCommitSurface (subsurface->role.surface, False); + + /* Set pending_commit to False only here, where it is certain + that the cached state has been applied. */ + subsurface->pending_commit = False; + } + + if (subsurface->role.surface) + { + list = subsurface->role.surface->subsurfaces; + + for (; list; list = list->next) + { + child = list->data; + child_subsurface = SubsurfaceFromRole (child->role); + + if (child_subsurface->should_be_desync) + NoteSubsurfaceDesynchronous (child_subsurface, + False); + } + } +} + +static void +NoteSubsurfaceTeardown (Subsurface *subsurface) +{ + Surface *child; + Subsurface *child_subsurface; + XLList *list; + + /* The same, but it avoids applying any pending state. Used during + teardown. */ + + list = subsurface->role.surface->subsurfaces; + subsurface->synchronous = False; + + for (; list; list = list->next) + { + child = list->data; + child_subsurface = SubsurfaceFromRole (child->role); + + if (child_subsurface->should_be_desync) + NoteSubsurfaceTeardown (child_subsurface); + } } static void @@ -440,18 +517,45 @@ SetDesync (struct wl_client *client, struct wl_resource *resource) subsurface = wl_resource_get_user_data (resource); - if (subsurface->role.surface - && subsurface->synchronous) - NoteDesyncChild (subsurface->role.surface, - &subsurface->role); + /* Set it so that this subsurface should be desynchronous. If the + parent is synchronous, then it does not actually become + desynchronous until the pending state is applied. */ + subsurface->should_be_desync = True; - subsurface->synchronous = False; + /* Return if the parent is synchronous, as Wayland specifies + children of synchronous subsurfaces are always synchronous. */ - if (subsurface->pending_commit - && subsurface->role.surface) - XLCommitSurface (subsurface->role.surface, False); + if (IsParentSynchronous (subsurface)) + return; - subsurface->pending_commit = False; + /* Make subsurface desynchronous and apply its pending state. If + any of its children are supposed to be desynchronous, make them + desynchronous as well, but do not apply the pending state. This + is how the documentation for the set_desync request is worded: + + If cached state exists when wl_surface.commit is called in + desynchronized mode, the pending state is added to the cached + state, and applied as a whole. This invalidates the cache. + + Note: even if a sub-surface is set to desynchronized, a parent + sub-surface may override it to behave as synchronized. For + details, see wl_subsurface. + + If a surface's parent surface behaves as desynchronized, then + the cached state is applied on set_desync. + + Notice how the last paragraph tries to stress that only surfaces + that are made desynchronous at the time of a set_desync request + made on them are supposed to have their cached state applied at + the time of that request. + + Normally, applying the cached state of the desynchronous + subsurface will cause the cached state of its children to be + applied. However, there could be no cached state at all on the + surface specified as the argument to the set_desync request, in + which case children should not have their pending state applied. + This behavior is subject to tests in subsurface_test.c. */ + NoteSubsurfaceDesynchronous (subsurface, True); } static const struct wl_subsurface_interface wl_subsurface_impl = @@ -487,6 +591,11 @@ EarlyCommit (Surface *surface, Role *role) subsurface->pending_commit = True; return False; } + else if (subsurface->pending_commit) + /* There is still pending state. Merge the state into the surface + first, before SubcompositorUpdate is called by + InternalCommit. */ + XLSurfaceMergeCachedState (surface); return True; } @@ -752,6 +861,10 @@ Setup (Surface *surface, Role *role) ViewSkip (surface->view); ViewSkip (surface->under); + /* Subsurfaces are synchronous by default. Make every child + synchronous. */ + SetSync1 (subsurface); + return True; } @@ -785,10 +898,9 @@ Teardown (Surface *surface, Role *role) subsurface = SubsurfaceFromRole (role); - /* If this subsurface is desynchronous, tell the toplevel parent - that it is now gone. */ - if (!subsurface->synchronous) - NoteDesyncChild (role->surface, role); + /* Make each of the surface's children that should be desynchronous + desynchronous. */ + NoteSubsurfaceTeardown (subsurface); role->surface = NULL; @@ -955,8 +1067,6 @@ GetSubsurface (struct wl_client *client, struct wl_resource *resource, subsurface->role.funcs.get_window = GetWindow; subsurface->role.funcs.rescale = Rescale; subsurface->role.funcs.parent_rescale = ParentRescale; - subsurface->role.funcs.note_child_synced = NoteChildSynced; - subsurface->role.funcs.note_desync_child = NoteDesyncChild; subsurface->parent = parent; subsurface->commit_callback diff --git a/surface.c b/surface.c index 766d9d6..1849271 100644 --- a/surface.c +++ b/surface.c @@ -1008,10 +1008,12 @@ TryEarlyRelease (Surface *surface) } static void -InternalCommit (Surface *surface, State *pending) +InternalCommit1 (Surface *surface, State *pending) { FrameCallback *start, *end; + /* Merge the state in pending into the surface's current state. */ + if (pending->pending & PendingBuffer) { /* The buffer may already released if its contents were @@ -1133,6 +1135,12 @@ InternalCommit (Surface *surface, State *pending) &surface->current_state.frame_callbacks); } } +} + +static void +InternalCommit (Surface *surface, State *pending) +{ + InternalCommit1 (surface, pending); /* Run commit callbacks. This tells synchronous subsurfaces to update, and tells explicit synchronization to wait for any sync @@ -1830,6 +1838,15 @@ XLSurfaceNoteFocus (Surface *surface, FocusMode focus) focus); } +/* Merge the cached state in surface into its current state in + preparation for commit. */ + +void +XLSurfaceMergeCachedState (Surface *surface) +{ + InternalCommit1 (surface, &surface->cached_state); +} + /* The following functions convert from window to surface diff --git a/tests/subsurface_test.c b/tests/subsurface_test.c index 12ad683..0e68d27 100644 --- a/tests/subsurface_test.c +++ b/tests/subsurface_test.c @@ -32,6 +32,7 @@ enum test_kind SUBSURFACE_TREE_KIND, SUBSURFACE_GROW_SHRINK_KIND, SUBSURFACE_STACKING_1_KIND, + SUBSURFACE_DESYNC_KIND, }; static const char *test_names[] = @@ -44,6 +45,7 @@ static const char *test_names[] = "subsurface_tree", "subsurface_grow_shrink", "subsurface_stacking_1", + "subsurface_desync_kind", }; struct test_subsurface @@ -55,7 +57,7 @@ struct test_subsurface struct wl_surface *surface; }; -#define LAST_TEST SUBSURFACE_STACKING_1_KIND +#define LAST_TEST SUBSURFACE_DESYNC_KIND /* The display. */ static struct test_display *display; @@ -77,9 +79,10 @@ static struct test_surface *test_surface; static struct wl_surface *wayland_surface; /* Various subsurfaces. */ -static struct test_subsurface *subsurfaces[4]; +static struct test_subsurface *subsurfaces[9]; /* Various buffers. */ +static struct wl_buffer *tiny_png; static struct wl_buffer *subsurface_base_png; static struct wl_buffer *subsurface_damage_png; static struct wl_buffer *subsurface_1_png; @@ -203,7 +206,6 @@ make_test_subsurface_with_parent (struct test_subsurface *parent) static void test_single_step (enum test_kind kind) { - struct wl_buffer *buffer; struct wl_region *region; test_log ("running test step: %s", test_names[kind]); @@ -211,16 +213,15 @@ test_single_step (enum test_kind kind) switch (kind) { case MAP_WINDOW_KIND: - buffer = load_png_image (display, "tiny.png"); + tiny_png = load_png_image (display, "tiny.png"); - if (!buffer) + if (!tiny_png) report_test_failure ("failed to load tiny.png"); - wl_surface_attach (wayland_surface, buffer, 0, 0); + wl_surface_attach (wayland_surface, tiny_png, 0, 0); wl_surface_damage (wayland_surface, 0, 0, INT_MAX, INT_MAX); submit_surface_opaque_region (wayland_surface, 0, 0, 4, 4); wl_surface_commit (wayland_surface); - wl_buffer_destroy (buffer); break; case SUBSURFACE_UNDER_KIND: @@ -552,6 +553,138 @@ test_single_step (enum test_kind kind) which lies the fourth subsurface (cow_transparent.png), and finally the third (small.png). */ sleep_or_verify (); + + test_single_step (SUBSURFACE_DESYNC_KIND); + break; + + case SUBSURFACE_DESYNC_KIND: + /* Hide every other subsurface other than subsurfaces[0]. */ + + wl_surface_attach (subsurfaces[1]->surface, NULL, 0, 0); + wl_surface_attach (subsurfaces[2]->surface, NULL, 0, 0); + wl_surface_attach (subsurfaces[3]->surface, NULL, 0, 0); + wl_surface_commit (subsurfaces[3]->surface); + wl_surface_commit (subsurfaces[2]->surface); + wl_surface_commit (subsurfaces[1]->surface); + wait_frame_callback (wayland_surface); + sleep_or_verify (); + + /* Create a tree of subsurfaces as follows: + + subsurfaces[4] (as a child of subsurfaces[0]) + subsurfaces[5] + subsurfaces[6] + subsurfaces[7] + subsurfaces[8] */ + + subsurfaces[4] = make_test_subsurface_with_parent (subsurfaces[0]); + + if (!subsurfaces[4]) + report_test_failure ("failed to create subsurfaces"); + + subsurfaces[5] + = make_test_subsurface_with_parent (subsurfaces[4]); + subsurfaces[6] + = make_test_subsurface_with_parent (subsurfaces[4]); + + if (!subsurfaces[5] || !subsurfaces[6]) + report_test_failure ("failed to create subsurfaces"); + + subsurfaces[7] + = make_test_subsurface_with_parent (subsurfaces[6]); + subsurfaces[8] + = make_test_subsurface_with_parent (subsurfaces[6]); + + if (!subsurfaces[7] || !subsurfaces[8]) + report_test_failure ("failed to create subsurfaces"); + + /* Confirm all the children. Attach tiny_png to prevent some + subsurfaces from being unmapped. */ + wl_surface_attach (subsurfaces[4]->surface, tiny_png, 0, 0); + wl_surface_attach (subsurfaces[6]->surface, tiny_png, 0, 0); + wl_surface_damage (subsurfaces[4]->surface, 0, 0, 4, 4); + wl_surface_damage (subsurfaces[6]->surface, 0, 0, 4, 4); + + wl_surface_commit (subsurfaces[6]->surface); + wl_surface_commit (subsurfaces[4]->surface); + wl_surface_commit (subsurfaces[0]->surface); + + /* Now, make subsurfaces[8] desynchronous. */ + wl_subsurface_set_desync (subsurfaces[8]->subsurface); + + /* Attach gradient.png to subsurfaces[8]. While subsurfaces[8] + is desync, 6 and 4 are both synchronous. */ + wl_surface_attach (subsurfaces[8]->surface, gradient_png, 0, 0); + wl_surface_damage (subsurfaces[8]->surface, 0, 0, 100, 300); + wl_surface_commit (subsurfaces[8]->surface); + + /* Thus, the state should not appear. */ + wait_frame_callback (wayland_surface); + sleep_or_verify (); + + /* Next, make subsurfaces[6] desynchronous. */ + wl_subsurface_set_desync (subsurfaces[6]->subsurface); + + /* gradient.png should still not appear. */ + wait_frame_callback (wayland_surface); + sleep_or_verify (); + + /* Finally, make subsurfaces[0] and subsurfaces[4] + desynchronous. As subsurfaces[0] has cached state (it was + committed earlier), its state and that of all its children + should be applied. As a result, gradient.png should appear + without having to commit subsurfaces[8] again. */ + wl_subsurface_set_desync (subsurfaces[4]->subsurface); + wl_subsurface_set_desync (subsurfaces[0]->subsurface); + + /* Commit wayland_surface to get a frame callback. */ + wait_frame_callback (wayland_surface); + + /* gradient.png should now appear onscreen. */ + sleep_or_verify (); + + /* Next, make subsurfaces[6] synchronous. Attach small.png to + subsurfaces[8] and gradient.png to subsurfaces[5]. */ + wl_subsurface_set_sync (subsurfaces[6]->subsurface); + + wl_surface_attach (subsurfaces[8]->surface, small_png, 0, 0); + wl_surface_damage (subsurfaces[8]->surface, 0, 0, 150, 150); + wl_surface_commit (subsurfaces[8]->surface); + + /* Commit subsurfaces[4] for a frame callback. Verify that the + contents did not change, as subsurfaces[6] is + synchronous. */ + wait_frame_callback (subsurfaces[4]->surface); + sleep_or_verify (); + + /* Commit subsurfaces[6], then commit subsurfaces[4] to apply + the pending state. The state in subsurfaces[8] should be + applied. */ + wl_surface_commit (subsurfaces[6]->surface); + wait_frame_callback (subsurfaces[4]->surface); + sleep_or_verify (); + + /* Now, attach big_png to subsurfaces[8]. */ + wl_surface_attach (subsurfaces[8]->surface, big_png, 0, 0); + wl_surface_damage (subsurfaces[8]->surface, 0, 0, 300, 300); + wl_surface_commit (subsurfaces[8]->surface); + + /* Make subsurfaces[6]->surface desynchronous. Nothing should + appear despite subsurfaces[8] having become desynchronous, as + subsurfaces[6] has no cached state. subsurfaces[4] is only + committed to get a frame callback. */ + wl_subsurface_set_desync (subsurfaces[6]->subsurface); + wait_frame_callback (subsurfaces[4]->surface); + sleep_or_verify (); + + /* Now, apply a buffer transform to subsurfaces[8]. Upon + commit, big.png should be displayed rotated 90 degrees + clockwise, by the buffer transform being merged with the + cached state of the surface prior to being committed. */ + wl_surface_set_buffer_transform (subsurfaces[8]->surface, + WL_OUTPUT_TRANSFORM_90); + wait_frame_callback (subsurfaces[8]->surface); + sleep_or_verify (); break; } diff --git a/xdg_surface.c b/xdg_surface.c index 0d61240..e7812c9 100644 --- a/xdg_surface.c +++ b/xdg_surface.c @@ -159,9 +159,6 @@ struct _XdgRole /* The input region of the attached subsurface. */ pixman_region32_t input_region; - /* The number of desynchronous children of this toplevel. */ - int n_desync_children; - /* The type of the attached role. */ XdgRoleImplementationType type; }; @@ -279,6 +276,29 @@ RunFrameCallbacksConditionally (XdgRole *role) role->state |= StatePendingFrameCallback; } +static void +UpdateFrameRefreshPrediction (XdgRole *role) +{ + int desync_children; + + /* Count the number of desynchronous children attached to this + surface, directly or indirectly. When this number is more than + 1, enable frame refresh prediction, which allows separate frames + from subsurfaces to be batched together. */ + + if (role->role.surface) + { + desync_children = 0; + XLUpdateDesynchronousChildren (role->role.surface, + &desync_children); + + if (desync_children) + XLFrameClockSetPredictRefresh (role->clock); + else + XLFrameClockDisablePredictRefresh (role->clock); + } +} + static void AllBuffersReleased (void *data) { @@ -680,18 +700,6 @@ Setup (Surface *surface, Role *role) SubcompositorInsert (xdg_role->subcompositor, surface->view); - /* Count the number of desynchronous children attached to this - surface, directly or indirectly. This number is then updated as - surfaces are attached, made desynchronous and/or removed in - NoteChildSynced and NoteDesyncChild. */ - XLUpdateDesynchronousChildren (surface, &xdg_role->n_desync_children); - - /* If there are desynchronous children, enable frame refresh - prediction in the frame clock, which batches subframes from - multiple subsurfaces together if they arrive in time. */ - if (xdg_role->n_desync_children) - XLFrameClockSetPredictRefresh (xdg_role->clock); - /* Retain the backing data. */ xdg_role->refcount++; @@ -1167,6 +1175,10 @@ NoteFrame (FrameMode mode, uint64_t id, void *data) if (!(role->state & StateFrameStarted)) { + /* Update whether or not frame refresh prediction is to be + used. */ + UpdateFrameRefreshPrediction (role); + if (XLFrameClockStartFrame (role->clock, False)) role->state |= StateFrameStarted; } @@ -1317,31 +1329,6 @@ Rescale (Surface *surface, Role *role) xdg_role->impl->funcs.handle_geometry_change (role, xdg_role->impl); } -static void -NoteChildSynced (Surface *surface, Role *role) -{ - XdgRole *xdg_role; - - xdg_role = XdgRoleFromRole (role); - - if (xdg_role->n_desync_children) - xdg_role->n_desync_children--; - - if (!xdg_role->n_desync_children) - XLFrameClockDisablePredictRefresh (xdg_role->clock); -} - -static void -NoteDesyncChild (Surface *surface, Role *role) -{ - XdgRole *xdg_role; - - xdg_role = XdgRoleFromRole (role); - - xdg_role->n_desync_children++; - XLFrameClockSetPredictRefresh (xdg_role->clock); -} - static void HandleFreeze (void *data) { @@ -1469,8 +1456,6 @@ XLGetXdgSurface (struct wl_client *client, struct wl_resource *resource, role->role.funcs.post_resize = PostResize; role->role.funcs.move_by = MoveBy; role->role.funcs.rescale = Rescale; - role->role.funcs.note_desync_child = NoteDesyncChild; - role->role.funcs.note_child_synced = NoteChildSynced; role->role.funcs.select_extra_events = SelectExtraEvents; role->role.funcs.note_focus = NoteFocus; role->role.funcs.outputs_changed = OutputsChanged;