[RESEND PATCH weston v3] pixman: Destroy pixman images when underlying buffer is destroyed
Pekka Paalanen
ppaalanen at gmail.com
Tue Dec 3 00:14:09 PST 2013
On Tue, 03 Dec 2013 08:59:16 +0100
Lubomir Rintel <lkundrak at v3.sk> wrote:
> On Mon, 2013-12-02 at 15:53 -0800, Kristian Høgsberg wrote:
> > On Sat, Nov 30, 2013 at 03:41:00PM +0100, Lubomir Rintel wrote:
> > > While the pixman image might be attached, the underlying buffer
> > > might be already gone under certain circumstances. This is easily
> > > reproduced by attempting to resize gnome-terminal on a fbdev
> > > backend.
> > >
> > > $ WAYLAND_DEBUG=1 strace -emunmap weston
> > > --backend=fbdev-backend.so ...
> > > [1524826.942] wl_shm at 7.create_pool(new id wl_shm_pool at 23, fd 40,
> > > 1563540) [1524827.315] wl_shm_pool at 23.create_buffer(new id
> > > wl_buffer at 24, 0, 759, 515, 3036, 0) ...
> > > [1524829.488] wl_surface at 14.attach(wl_buffer at 24, 0, 0)
> > > [1524829.766] wl_surface at 14.set_buffer_scale(1)
> > > [1524829.904] wl_surface at 14.damage(0, 0, 759, 515)
> > > [1524830.248] wl_surface at 14.frame(new id wl_callback at 25)
> > > [1524830.450] wl_surface at 14.commit()
> > > ...
> > > [1524846.706] wl_shm at 7.create_pool(new id wl_shm_pool at 26, fd 40,
> > > 1545000) [1524847.215] wl_shm_pool at 26.create_buffer(new id
> > > wl_buffer at 27, 0, 750, 515, 3000, 0) [1524847.735]
> > > wl_buffer at 24.destroy() [1524847.953] ->
> > > wl_display at 1.delete_id(24) [1524848.144] wl_shm_pool at 23.destroy()
> > > munmap(0xb5b2e000, 1563540) = 0
> > > [1524849.021] -> wl_display at 1.delete_id(23)
> > > [1524849.425] wl_surface at 14.attach(wl_buffer at 27, 0, 0)
> > > [1524849.730] wl_surface at 14.set_buffer_scale(1)
> > > [1524849.821] wl_surface at 14.damage(0, 0, 750, 515)
> > > <No commit yet, so drawing is attempted from older buffer that
> > > used to be attached to the surface, which happens to come from a
> > > destroyed pool, resulting it an invalid read from address
> > > 0xb5b2e000>
> > >
> > > Signed-off-by: Lubomir Rintel <lkundrak at v3.sk>
> > > ---
> > > Hi,
> > >
> > > This has been previously discussed in this thread:
> > > http://lists.freedesktop.org/archives/wayland-devel/2013-November/012122.html
> > >
> > > Pekka doesn't mind, Arnaud's suggestion does not seem doable.
> > > Please let me know if there's anything else that should be done
> > > before this is merged.
> > >
> > > Thank you!
> > > Lubo
> > >
> > > src/pixman-renderer.c | 27 ++++++++++++++++++++++++++-
> > > 1 file changed, 26 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/pixman-renderer.c b/src/pixman-renderer.c
> > > index 5961965..1a1e6ae 100644
> > > --- a/src/pixman-renderer.c
> > > +++ b/src/pixman-renderer.c
> > > @@ -42,6 +42,7 @@ struct pixman_surface_state {
> > > pixman_image_t *image;
> > > struct weston_buffer_reference buffer_ref;
> > >
> > > + struct wl_listener buffer_destroy_listener;
> > > struct wl_listener surface_destroy_listener;
> > > struct wl_listener renderer_destroy_listener;
> > > };
> > > @@ -450,6 +451,22 @@ pixman_renderer_flush_damage(struct
> > > weston_surface *surface) }
> > >
> > > static void
> > > +buffer_state_handle_buffer_destroy(struct wl_listener *listener,
> > > void *data) +{
> > > + struct pixman_surface_state *ps;
> > > +
> > > + ps = container_of(listener, struct pixman_surface_state,
> > > + buffer_destroy_listener);
> > > +
> > > + if (ps->image) {
> > > + pixman_image_unref(ps->image);
> > > + ps->image = NULL;
> > > + }
> > > +
> > > + ps->buffer_destroy_listener.notify = NULL;
> > > +}
> > > +
> > > +static void
> > > pixman_renderer_attach(struct weston_surface *es, struct
> > > weston_buffer *buffer) {
> > > struct pixman_surface_state *ps = get_surface_state(es);
> > > @@ -499,6 +516,13 @@ pixman_renderer_attach(struct weston_surface
> > > *es, struct weston_buffer *buffer) buffer->width, buffer->height,
> > > wl_shm_buffer_get_data(shm_buffer),
> > > wl_shm_buffer_get_stride(shm_buffer));
> > > +
> > > + if (ps->buffer_destroy_listener.notify)
> > > +
> > > wl_list_remove(&ps->buffer_destroy_listener.link);
> >
> > We need to remove the old listener early in the function, before
> >
> > if (!buffer)
> > return;
> >
> > so that we also remove the listener if a NULL buffer is attached.
>
> Why? There's no harm invoking the listener if a null buffer has been
> reattached to a surface. It checks for actual presence for pixmap data
> and still gets unhooked upon surface destroy. (Apart from that the
> code might look a bit more elegant and the ps->image NULL check in the
> listener might be unnecessary.)
I didn't think it through, but maybe this has to do with
wl_buffer.release semantics.
When a NULL wl_buffer is attached, the surface contents are removed.
Because of that, the surface also gets hidden. (Maybe that causes you
to never end up painting it, but that's slightly subtle, if you still
have listeners as if the content was there.)
When the old wl_buffer becomes unneeded and the last
weston_buffer_ref to it is dropped, the compositor sends
wl_buffer.release event. That event is a promise, that the server does
not use the wl_buffer contents anymore, until the client attach/commits
it again. So when the compositor sends the release event, it is as if
the buffer stops to exist, as long as repainting is concerned.
wl_buffer protocol objects are usually re-used rather than
continuously destroyed and re-created.
Therefore maybe removing the listener early is more consistent on how
wl_buffers are used?
Thanks,
pq
> > > + ps->buffer_destroy_listener.notify =
> > > + buffer_state_handle_buffer_destroy;
> > > + wl_signal_add(&buffer->destroy_signal,
> > > + &ps->buffer_destroy_listener);
> > > }
> > >
> > > static void
> > > @@ -506,7 +530,8 @@ pixman_renderer_surface_state_destroy(struct
> > > pixman_surface_state *ps) {
> > > wl_list_remove(&ps->surface_destroy_listener.link);
> > > wl_list_remove(&ps->renderer_destroy_listener.link);
> > > -
> > > + if (ps->buffer_destroy_listener.notify)
> > > +
> > > wl_list_remove(&ps->buffer_destroy_listener.link);
> > > ps->surface->renderer_state = NULL;
> > >
> > > --
> > > 1.8.4.2
> > >
> > > _______________________________________________
> > > wayland-devel mailing list
> > > wayland-devel at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
>
More information about the wayland-devel
mailing list