[PATCH weston 2/2] window: Don't free shm buffers while the compositor is using them

[Kristian Høgsberg]

On Tue, Jun 19, 2012 at 12:04:12PM -0400, Kristian Høgsberg wrote:
> On Tue, Jun 19, 2012 at 01:45:56PM +0300, Ander Conselvan de Oliveira wrote:
> > After commit 460a79bd, the compositor will only upload the contents of
> > an shm buffer to a texture during repaint, but at the point the buffer
> > might have been destroyed already. In that case, the surface simply
> > isn't drawn.
> 
> Yeah, I think we need to change the semantics of destroying a buffer
> that's attached to a surface.  Currently (or used to be before my
> commit broke it) you can attach a buffer and then destroy it
> immediately, but the surface keeps the contents and remains visible.
> The idea is that attaching has copy-semantics, or at least that the
> surface references the underlying content of the buffer (gem buffer or
> shm buffer for example), but not the buffer itself.  That's probably a
> little too subtle and under-defined.  It also leads to some
> awkwardness inside weston, where we can't just look at surface->buffer
> to see if a buffer is attached (surface->buffer may be NULL, but we
> may have a valid EGLImage, for example).
> 
> So let's just fix that and say that destroying a buffer that's
> attached to a surface is equivalent to attaching NULL, ie hiding the
> surface.

Ok, having thought about this some more, I don't we'll change it after
all.  The semantics we have now is not that unusual, the problem is
that we're batching more and more work in the repaint and in this case
it broke the shm texture updating.  I think we just need to flush the
batched up damage when the buffer is detroyed instead.  Does this work
for you?

Kristian

diff --git a/src/compositor.c b/src/compositor.c
index 00b8f3e..e98a1a7 100644
--- a/src/compositor.c
+++ b/src/compositor.c
@@ -180,12 +180,18 @@ weston_client_launch(struct weston_compositor *compositor,
 }
 
 static void
+update_shm_texture(struct weston_surface *surface);
+
+static void
 surface_handle_buffer_destroy(struct wl_listener *listener, void *data)
 {
 	struct weston_surface *es =
 		container_of(listener, struct weston_surface, 
 			     buffer_destroy_listener);
 
+	if (es->buffer && wl_buffer_is_shm(es->buffer))
+		update_shm_texture(es);
+
 	es->buffer = NULL;
 }
 


> > The compositor sends a release event when it's done using a buffer, so
> > this patch delays the destruction until then if necessary.
> > ---
> >  clients/window.c |   35 ++++++++++++++++++++++++++++++++++-
> >  1 files changed, 34 insertions(+), 1 deletions(-)
> > 
> > diff --git a/clients/window.c b/clients/window.c
> > index aaf2009..3ebbbeb 100644
> > --- a/clients/window.c
> > +++ b/clients/window.c
> > @@ -327,6 +327,7 @@ static const struct weston_option xkb_options[] = {
> >  static const cairo_user_data_key_t surface_data_key;
> >  struct surface_data {
> >  	struct wl_buffer *buffer;
> > +	int attach_count;
> >  };
> >  
> >  #define MULT(_d,c,a,t) \
> > @@ -402,6 +403,7 @@ display_get_buffer_for_surface(struct display *display,
> >  	struct surface_data *data;
> >  
> >  	data = cairo_surface_get_user_data (surface, &surface_data_key);
> > +	data->attach_count++;
> >  
> >  	return data->buffer;
> >  }
> > @@ -409,6 +411,7 @@ display_get_buffer_for_surface(struct display *display,
> >  struct shm_surface_data {
> >  	struct surface_data data;
> >  	struct shm_pool *pool;
> > +	int surface_gone;
> >  };
> >  
> >  static void
> > @@ -426,6 +429,32 @@ shm_surface_data_destroy(void *p)
> >  	free(data);
> >  }
> >  
> > +static void
> > +shm_buffer_destroy(void *p, struct wl_buffer *buffer)
> > +{
> > +	struct shm_surface_data *data = p;
> > +
> > +	data->data.attach_count = 0;
> > +
> > +	if (data->surface_gone)
> > +		shm_surface_data_destroy(data);
> > +}
> > +
> > +static const struct wl_buffer_listener shm_buffer_listener = {
> > +	shm_buffer_destroy
> > +};
> > +
> > +static void
> > +shm_surface_data_unref(void *p)
> > +{
> > +	struct shm_surface_data *data = p;
> > +
> > +	data->surface_gone = 1;
> > +
> > +	if (data->data.attach_count == 0)
> > +		shm_surface_data_destroy(p);
> > +}
> > +
> >  static struct wl_shm_pool *
> >  make_shm_pool(struct display *display, int size, void **data)
> >  {
> > @@ -543,8 +572,9 @@ display_create_shm_surface_from_pool(struct display *display,
> >  						       rectangle->height,
> >  						       stride);
> >  
> > +	data->surface_gone = 0;
> >  	cairo_surface_set_user_data (surface, &surface_data_key,
> > -				     data, shm_surface_data_destroy);
> > +				     data, shm_surface_data_unref);
> >  
> >  	if (flags & SURFACE_OPAQUE)
> >  		format = WL_SHM_FORMAT_XRGB8888;
> > @@ -556,6 +586,9 @@ display_create_shm_surface_from_pool(struct display *display,
> >  						      rectangle->height,
> >  						      stride, format);
> >  
> > +	data->data.attach_count = 0;
> > +	wl_buffer_add_listener(data->data.buffer, &shm_buffer_listener, data);
> > +
> >  	return surface;
> >  }
> >  
> > -- 
> > 1.7.4.1
> > 
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel