[PATCH weston 3/3] compositor: don't map surfaces without a buffer

[Pekka Paalanen]

On Fri,  3 Feb 2017 16:10:39 +0100
Emilio Pozuelo Monfort <pochu27 at gmail.com> wrote:

> We were calling weston_surface::committed on surfaces with
> no buffer attached. Stop doing that, since surface::committed
> will map the surfaces and put them in a visible layer. That may
> not be a problem for a single surface as it wouldn't be visible
> anyway because it's got no contents, but it is a problem if the
> surface has subsurfaces.
> 
> This fixes the subsurface_mapped test, so mark it as expected
> to succeed.
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=94735
> 
> Signed-off-by: Emilio Pozuelo Monfort <emilio.pozuelo at collabora.co.uk>
> ---
>  libweston/compositor.c       | 10 +++++++++-
>  tests/subsurface-shot-test.c |  2 +-
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/libweston/compositor.c b/libweston/compositor.c
> index 81392063..8a018897 100644
> --- a/libweston/compositor.c
> +++ b/libweston/compositor.c
> @@ -1589,6 +1589,12 @@ weston_surface_is_mapped(struct weston_surface *surface)
>  	return surface->is_mapped;
>  }
>  
> +static bool
> +weston_surface_has_content(struct weston_surface *surface)
> +{
> +	return surface->width > 0 && surface->height > 0;
> +}
> +
>  static void
>  surface_set_size(struct weston_surface *surface, int32_t width, int32_t height)
>  {
> @@ -2928,7 +2934,9 @@ weston_surface_commit_state(struct weston_surface *surface,
>  
>  	if (state->newly_attached || state->buffer_viewport.changed) {
>  		weston_surface_update_size(surface);
> -		if (surface->committed)
> +		if (surface->committed &&
> +		    (state->newly_attached &&
> +		     weston_surface_has_content(surface)))
>  			surface->committed(surface, state->sx, state->sy);

Hi,

I think this isn't right. The shells need to know when a surface
unmaps or changes dimensions, and they get that via the ->committed
hook (currently less well named, as ->configure was more descriptive,
but I think in the long term we'd like to eliminate the two hooks and
have just one called on every commit regardless of state - maybe have
the hook call weston_surface_apply_state so it can inspect
weston_surface state before and after).

If the viewport changed, it must lead to a call to ->committed, with or
without an attach.

Another thing is that weston_surface_update_size() updates the width,
height you use in weston_surface_has_content(). This means that a
surface first having content, receiving attach NULL + commit, will not
call ->committed.

When I was last thinking about the committed() hook, I came up with:
https://docs.google.com/spreadsheets/d/1b5azX4_gq9ZlYe4Y6UyHnHF8Wsiqnf1uHt9hR4whni4/edit#gid=0
but that just proves how complicated that design is, while not really
saving much in the committed() implementations.

Given my comments to the previous patch (the test), I think the fix
might be completely elsewhere.


Thanks,
pq

>  	}
>  
> diff --git a/tests/subsurface-shot-test.c b/tests/subsurface-shot-test.c
> index e7da1e0e..275d4726 100644
> --- a/tests/subsurface-shot-test.c
> +++ b/tests/subsurface-shot-test.c
> @@ -261,7 +261,7 @@ TEST(subsurface_z_order)
>  			buffer_destroy(bufs[i]);
>  }
>  
> -FAIL_TEST(subsurface_mapped)
> +TEST(subsurface_mapped)
>  {
>  	const char *test_name = get_test_name();
>  	struct client *client;

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170221/0f8c9e09/attachment.sig>