[PATCH weston v4 3/3] desktop-shell: Enable per-output fade animations
Bryce Harrington
bryce at osg.samsung.com
Tue Aug 16 02:57:15 UTC 2016
On Mon, Aug 15, 2016 at 07:14:02PM -0500, Derek Foreman wrote:
> On 10/08/16 07:25 PM, Bryce Harrington wrote:
> > Instead of creating a single global fade surface across all outputs,
> > create a separate surface for each output. This lets us individually
> > fade each output (or block fading if the output has an inhibiting
> > surface).
> >
> > When there are client surfaces with valid idle inhibits present, fade
> > out only the outputs those surfaces are not visible on. Then fade those
> > out too, if the client terminates for some reason while the system is in
> > sleep/idle/offscreen mode.
>
> I'd love to see this as two patches - first the per output fade (which
> fixes what I consider to be a bug in the current implementation) and
> then a separate patch with the inhibit specific stuff in it...
>
> > This also fixes a potential issue if on multihead layout spanning a
> > desktop wider than 8096 (or higher than 8096), the fade animation may
> > not completely cover all surfaces.
> >
> > This assumes the output geometry doesn't change larger during the course
> > of the fade animation.
> >
> > Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> > ---
> > desktop-shell/shell.c | 167 ++++++++++++++++++++++++++++++--------------------
> > desktop-shell/shell.h | 14 ++---
> > 2 files changed, 109 insertions(+), 72 deletions(-)
> >
> > diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> > index 64979cd..0aae142 100644
> > --- a/desktop-shell/shell.c
> > +++ b/desktop-shell/shell.c
> > @@ -3606,10 +3606,23 @@ destroy_shell_surface(struct shell_surface *shsurf)
> > }
> >
> > static void
> > +shell_fade(struct desktop_shell *shell, enum fade_type type);
> > +
> > +static void
> > shell_destroy_shell_surface(struct wl_resource *resource)
> > {
> > struct shell_surface *shsurf = wl_resource_get_user_data(resource);
> >
> > + /* Re-queue the fade animation to be evaluated if client had been inhibiting */
> > + if (shsurf->surface->inhibit_idling &&
> > + (shsurf->shell->compositor->state == WESTON_COMPOSITOR_IDLE
> > + || shsurf->shell->compositor->state == WESTON_COMPOSITOR_OFFSCREEN
> > + || shsurf->shell->compositor->state == WESTON_COMPOSITOR_SLEEPING))
> > + {
> > + shsurf->surface->inhibit_idling = false;
> > + shell_fade(shsurf->shell, FADE_OUT);
>
> What happens if we're already in the middle of fading out when a shell
> surface with an inhibitor is destroyed? Did you test that case?
What should happen is that you'd see both monitors fading out, but the
one that wasn't inhibited will fade out faster.
Indeed, the purpose of this patch was to clean up misbehaviors relating
to the fade-out animation in relation to idle inhibit. I won't
guarantee that I caught every corner case - almost assuredly I didn't -
but I did test it manually quite a variety of different ways.
Unfortunately, due to the libweston-desktop changes that landed Friday,
a lot of the code here has been moved or removed, and this patch no
longer applies, and I guess would need to be majorly rewritten.
> > + }
> > +
> > if (!wl_list_empty(&shsurf->popup.grab_link))
> > remove_popup_grab(shsurf);
> > if (shsurf->resource)
> > @@ -4356,9 +4369,6 @@ xdg_shell_unversioned_dispatch(const void *implementation,
> > /***********************************/
> >
> > static void
> > -shell_fade(struct desktop_shell *shell, enum fade_type type);
> > -
> > -static void
> > configure_static_view(struct weston_view *ev, struct weston_layer *layer)
> > {
> > struct weston_view *v, *next;
> > @@ -5356,24 +5366,26 @@ static void
> > shell_fade_done(struct weston_view_animation *animation, void *data)
> > {
> > struct desktop_shell *shell = data;
> > + struct shell_output *shell_output;
> >
> > - shell->fade.animation = NULL;
> > -
> > - switch (shell->fade.type) {
> > - case FADE_IN:
> > - weston_surface_destroy(shell->fade.view->surface);
> > - shell->fade.view = NULL;
> > - break;
> > - case FADE_OUT:
> > - lock(shell);
> > - break;
> > - default:
> > - break;
> > + wl_list_for_each(shell_output, &shell->output_list, link) {
> > + shell_output->fade.animation = NULL;
> > + switch (shell_output->fade.type) {
> > + case FADE_IN:
> > + weston_surface_destroy(shell_output->fade.view->surface);
> > + shell_output->fade.view = NULL;
> > + break;
> > + case FADE_OUT:
> > + lock(shell);
> > + break;
> > + default:
> > + break;
> > + }
> > }
> > }
> >
> > static struct weston_view *
> > -shell_fade_create_surface(struct desktop_shell *shell)
> > +shell_fade_create_surface_for_output(struct desktop_shell *shell, struct shell_output *shell_output)
> > {
> > struct weston_compositor *compositor = shell->compositor;
> > struct weston_surface *surface;
> > @@ -5389,8 +5401,8 @@ shell_fade_create_surface(struct desktop_shell *shell)
> > return NULL;
> > }
> >
> > - weston_surface_set_size(surface, 8192, 8192);
> > - weston_view_set_position(view, 0, 0);
> > + weston_surface_set_size(surface, shell_output->output->width, shell_output->output->height);
> > + weston_view_set_position(view, shell_output->output->x, shell_output->output->y);
> > weston_surface_set_color(surface, 0.0, 0.0, 0.0, 1.0);
> > weston_layer_entry_insert(&compositor->fade_layer.view_list,
> > &view->layer_link);
> > @@ -5405,6 +5417,8 @@ static void
> > shell_fade(struct desktop_shell *shell, enum fade_type type)
> > {
> > float tint;
> > + struct shell_output *shell_output;
> > + uint32_t inhibit_mask = weston_compositor_inhibited_outputs(shell->compositor);
> >
> > switch (type) {
> > case FADE_IN:
> > @@ -5414,36 +5428,41 @@ shell_fade(struct desktop_shell *shell, enum fade_type type)
> > tint = 1.0;
> > break;
> > default:
> > - weston_log("shell: invalid fade type\n");
> > return;
> > }
> >
> > - shell->fade.type = type;
> > + /* Create a separate fade surface for each output */
> > + wl_list_for_each(shell_output, &shell->output_list, link) {
> > + if (inhibit_mask & (1 << shell_output->output->id))
> > + continue;
> >
> > - if (shell->fade.view == NULL) {
> > - shell->fade.view = shell_fade_create_surface(shell);
> > - if (!shell->fade.view)
> > - return;
> > + shell_output->fade.type = type;
> >
> > - shell->fade.view->alpha = 1.0 - tint;
> > - weston_view_update_transform(shell->fade.view);
> > - }
> > + if (shell_output->fade.view == NULL) {
> > + shell_output->fade.view = shell_fade_create_surface_for_output(shell, shell_output);
> > + if (!shell_output->fade.view)
> > + return;
> >
> > - if (shell->fade.view->output == NULL) {
> > - /* If the black view gets a NULL output, we lost the
> > - * last output and we'll just cancel the fade. This
> > - * happens when you close the last window under the
> > - * X11 or Wayland backends. */
> > - shell->locked = false;
> > - weston_surface_destroy(shell->fade.view->surface);
> > - shell->fade.view = NULL;
> > - } else if (shell->fade.animation) {
> > - weston_fade_update(shell->fade.animation, tint);
> > - } else {
> > - shell->fade.animation =
> > - weston_fade_run(shell->fade.view,
> > - 1.0 - tint, tint, 300.0,
> > - shell_fade_done, shell);
> > + shell_output->fade.view->alpha = 1.0 - tint;
> > + weston_view_update_transform(shell_output->fade.view);
> > + }
> > +
> > + if (shell_output->fade.view->output == NULL) {
> > + /* If the black view gets a NULL output, we lost the
> > + * last output and we'll just cancel the fade. This
> > + * happens when you close the last window under the
> > + * X11 or Wayland backends. */
> > + shell->locked = false;
> > + weston_surface_destroy(shell_output->fade.view->surface);
> > + shell_output->fade.view = NULL;
> > + } else if (shell_output->fade.animation) {
>
> Same as my previous question, I guess -does this clause make the fade
> animation restart if it's already running?
Remember that this patch changes things so you have a distinct fadeout
animation for each output. If there are any inhibits in effect then
that particular animation would not be started, thus if an inhibiting
client exits there isn't going to be an animation running on that
client's output anyway, and fadeouts running on other outputs are
independent so won't be molested.
Again though - caveat - the fadeout animation stuff adds an extra degree
of complexity, and there could well still be some corner cases. But the
code I guess will have to be redone entirely so all that would have to be
tested over again anyway. :-/
> > + weston_fade_update(shell_output->fade.animation, tint);
> > + } else {
> > + shell_output->fade.animation =
> > + weston_fade_run(shell_output->fade.view,
> > + 1.0 - tint, tint, 300.0,
> > + shell_fade_done, shell);
> > + }
> > }
> > }
> >
> > @@ -5451,6 +5470,7 @@ static void
> > do_shell_fade_startup(void *data)
> > {
> > struct desktop_shell *shell = data;
> > + struct shell_output *shell_output;
> >
> > if (shell->startup_animation_type == ANIMATION_FADE) {
> > shell_fade(shell, FADE_IN);
> > @@ -5458,8 +5478,10 @@ do_shell_fade_startup(void *data)
> > weston_log("desktop shell: "
> > "unexpected fade-in animation type %d\n",
> > shell->startup_animation_type);
> > - weston_surface_destroy(shell->fade.view->surface);
> > - shell->fade.view = NULL;
> > + wl_list_for_each(shell_output, &shell->output_list, link) {
> > + weston_surface_destroy(shell_output->fade.view->surface);
> > + shell_output->fade.view = NULL;
> > + }
> > }
> > }
> >
> > @@ -5467,15 +5489,22 @@ static void
> > shell_fade_startup(struct desktop_shell *shell)
> > {
> > struct wl_event_loop *loop;
> > + struct shell_output *shell_output;
> > + bool has_fade = false;
> >
> > - if (!shell->fade.startup_timer)
> > - return;
> > + wl_list_for_each(shell_output, &shell->output_list, link) {
> > + if (!shell_output->fade.startup_timer)
> > + continue;
> >
> > - wl_event_source_remove(shell->fade.startup_timer);
> > - shell->fade.startup_timer = NULL;
> > + wl_event_source_remove(shell_output->fade.startup_timer);
> > + shell_output->fade.startup_timer = NULL;
> > + has_fade = true;
> > + }
> >
> > - loop = wl_display_get_event_loop(shell->compositor->wl_display);
> > - wl_event_loop_add_idle(loop, do_shell_fade_startup, shell);
> > + if (has_fade) {
> > + loop = wl_display_get_event_loop(shell->compositor->wl_display);
> > + wl_event_loop_add_idle(loop, do_shell_fade_startup, shell);
> > + }
> > }
> >
> > static int
> > @@ -5496,27 +5525,35 @@ shell_fade_init(struct desktop_shell *shell)
> > */
> >
> > struct wl_event_loop *loop;
> > -
> > - if (shell->fade.view != NULL) {
> > - weston_log("%s: warning: fade surface already exists\n",
> > - __func__);
> > - return;
> > - }
> > + struct shell_output *shell_output;
> >
> > if (shell->startup_animation_type == ANIMATION_NONE)
> > return;
> >
> > - shell->fade.view = shell_fade_create_surface(shell);
> > - if (!shell->fade.view)
> > - return;
> > + wl_list_for_each(shell_output, &shell->output_list, link) {
> > + uint32_t inhibit_mask = weston_compositor_inhibited_outputs(shell->compositor);
>
> Considering when shell_fade_init() is run, is it even possible for there
> to be any inhibiting surfaces in play yet?
Not sure what you mean here?
> Thanks,
> Derek
>
> >
> > - weston_view_update_transform(shell->fade.view);
> > - weston_surface_damage(shell->fade.view->surface);
> > + if (shell_output->fade.view != NULL) {
> > + weston_log("%s: warning: fade surface already exists\n",
> > + __func__);
> > + continue;
> > + }
> >
> > - loop = wl_display_get_event_loop(shell->compositor->wl_display);
> > - shell->fade.startup_timer =
> > - wl_event_loop_add_timer(loop, fade_startup_timeout, shell);
> > - wl_event_source_timer_update(shell->fade.startup_timer, 15000);
> > + if (inhibit_mask & (1 << shell_output->output->id))
> > + continue;
> > +
> > + shell_output->fade.view = shell_fade_create_surface_for_output(shell, shell_output);
> > + if (!shell_output->fade.view)
> > + return;
> > +
> > + weston_view_update_transform(shell_output->fade.view);
> > + weston_surface_damage(shell_output->fade.view->surface);
> > +
> > + loop = wl_display_get_event_loop(shell->compositor->wl_display);
> > + shell_output->fade.startup_timer =
> > + wl_event_loop_add_timer(loop, fade_startup_timeout, shell);
> > + wl_event_source_timer_update(shell_output->fade.startup_timer, 15000);
> > + }
> > }
> >
> > static void
> > diff --git a/desktop-shell/shell.h b/desktop-shell/shell.h
> > index f73cae5..befe0cc 100644
> > --- a/desktop-shell/shell.h
> > +++ b/desktop-shell/shell.h
> > @@ -121,6 +121,13 @@ struct shell_output {
> >
> > struct weston_surface *background_surface;
> > struct wl_listener background_surface_listener;
> > +
> > + struct {
> > + struct weston_view *view;
> > + struct weston_view_animation *animation;
> > + enum fade_type type;
> > + struct wl_event_source *startup_timer;
> > + } fade;
> > };
> >
> > struct desktop_shell {
> > @@ -188,13 +195,6 @@ struct desktop_shell {
> > struct wl_list surfaces;
> > } input_panel;
> >
> > - struct {
> > - struct weston_view *view;
> > - struct weston_view_animation *animation;
> > - enum fade_type type;
> > - struct wl_event_source *startup_timer;
> > - } fade;
> > -
> > struct exposay exposay;
> >
> > bool allow_zap;
> >
More information about the wayland-devel
mailing list