[PATCH weston] desktop-shell: do not black out with startup "none"

[Pekka Paalanen]

On Tue, 26 May 2015 07:24:04 -0500
Derek Foreman <derekf at osg.samsung.com> wrote:

> On 26/05/15 03:54 AM, Pekka Paalanen wrote:
> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > 
> > Do not use a black blanket surface when the startup animation is
> > specified to be "none". This is the final fix needed to make the
> > screenshot test deterministic and independent of weston-desktop-shell.
> > 
> > Previously, the black surface would cover all outputs until
> > weston-desktop-shell signalled ready. Then, depending on the set
> > animation, either the black surface was immediately removed (none) or a
> > fade-in started (fade).
> > 
> > Now, when there is no black surface at all for "none", the compositor
> > will show garbage until weston-desktop-shell gets everything up. This
> > may be undesireable but works for tests. To have the old "none"
> > behaviour back, I would propose to add a new startup-animation value
> > "black" for it.
> 
> This seems somewhat counter-intuitive to me (none now has a special
> meaning that isn't "no animation"), which is why I didn't write this
> patch myself. ;)

On the contary, to me it is more meaningful: none is like NULL, "do not
do anything". Setting up a black surface while w-d-s starts is
"something", though you can argue that's not an animation either
because there is first only black and then the desktop.

Btw. the "garbage" seems to be just black as well, as far as I've seen.
So visually it's not any different from before, but of course is less
reliably black.

> Maybe this somewhat oddball behavior deserves a mention in the
> weston.ini man page?

Now that I actually read the whole entry for startup-animation, I see
it is completely wrong to begin with. It is says it is the window
opening animation.

Soo, fixing that would be a separate issue, and even if it wasn't, I
didn't think this patch would need a change there.

> It does solve a significant problem though, so:
> 
> Reviewed-By: Derek Foreman <derekf at osg.samsung.com>
> 
> (one more nit below)
> 
> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > 
> > ---
> > 
> > Bryce, this patch should be good to land once someone gives it a
> > tested-by or a reviewed-by. When this lands, I think RC2 is ready for
> > release.
> > 
> > Thanks,
> > pq
> > ---
> >  desktop-shell/shell.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> > index 452cd5f..515d05c 100644
> > --- a/desktop-shell/shell.c
> > +++ b/desktop-shell/shell.c
> > @@ -5336,9 +5336,12 @@ do_shell_fade_startup(void *data)
> >  {
> >  	struct desktop_shell *shell = data;
> >  
> > -	if (shell->startup_animation_type == ANIMATION_FADE)
> > +	if (shell->startup_animation_type == ANIMATION_FADE) {
> >  		shell_fade(shell, FADE_IN);
> > -	else if (shell->startup_animation_type == ANIMATION_NONE) {
> > +	} else {
> 
> since the default conversion is "ANIMATION_NONE" this is a bit less
> informative than it could be if the string were printed before it were
> parsed?

Actually this should probably be an assert. If the value is
ANIMATION_NONE, we should never even get here. So it's more like an
arbitrary value check.

I simply didn't want to remove the whole else-clause, because if we
ever get here, the black surface will exist and should go.

> > +		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;
> >  	}
> > @@ -5384,6 +5387,9 @@ shell_fade_init(struct desktop_shell *shell)
> >  		return;
> >  	}
> >  
> > +	if (shell->startup_animation_type == ANIMATION_NONE)
> > +		return;
> > +
> >  	shell->fade.view = shell_fade_create_surface(shell);
> >  	if (!shell->fade.view)
> >  		return;
> > 

Oh well. Pushed anyway, so we might get the RC2 out some day.
   039e9be..23ed5f2  master -> master


Thanks,
pq