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

[Ander Conselvan de Oliveira]

On 06/20/2012 08:00 AM, Kristian Høgsberg wrote:
> 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?

Makes sense. The patch does fix the problem on my end.

Thanks,
Ander


> 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
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel