diff --git a/ext/wayland/wlbuffer.c b/ext/wayland/wlbuffer.c index d93fedb7ff..4ac99ef4b0 100644 --- a/ext/wayland/wlbuffer.c +++ b/ext/wayland/wlbuffer.c @@ -59,14 +59,21 @@ * GstBuffer and the GstBufferPool to get destroyed, so we are going to leak a * fair ammount of memory. * - * Normally, this will never happen, even if we don't take special care for it, - * because the compositor releases buffers almost immediately and when - * waylandsink stops, they are already released. + * Normally, this rarely happens, because the compositor releases buffers + * almost immediately and when waylandsink stops, they are already released. * * However, we want to be absolutely certain, so a solution is introduced * by registering all the GstWlBuffers with the display and explicitly - * releasing all the buffer references and destroying the GstWlBuffers as soon - * as the display is destroyed. + * releasing all the buffer references as soon as the display is destroyed. + * + * When the GstWlDisplay is finalized, it takes a reference to all the + * registered GstWlBuffers and then calls gst_wl_buffer_force_release_and_unref, + * which releases the potential reference to the GstBuffer, destroys the + * underlying wl_buffer and removes the reference that GstWlDisplay is holding. + * At that point, either the GstBuffer is alive somewhere and still holds a ref + * to the GstWlBuffer, which it will release when it gets destroyed, or the + * GstBuffer was destroyed in the meantime and the GstWlBuffer gets destroyed + * as soon as we remove the reference that GstWlDisplay holds. */ #include "wlbuffer.h" @@ -78,14 +85,32 @@ G_DEFINE_TYPE (GstWlBuffer, gst_wl_buffer, G_TYPE_OBJECT); static G_DEFINE_QUARK (GstWlBufferQDataQuark, gst_wl_buffer_qdata); +static void +gst_wl_buffer_dispose (GObject * gobject) +{ + GstWlBuffer *self = GST_WL_BUFFER (gobject); + + GST_TRACE_OBJECT (self, "dispose"); + + /* if the display is shutting down and we are trying to dipose + * the GstWlBuffer from another thread, unregister_buffer() will + * block and in the end the display will increase the refcount + * of this GstWlBuffer, so it will not be finalized */ + if (self->display) + gst_wl_display_unregister_buffer (self->display, self); + + G_OBJECT_CLASS (gst_wl_buffer_parent_class)->dispose (gobject); +} + static void gst_wl_buffer_finalize (GObject * gobject) { GstWlBuffer *self = GST_WL_BUFFER (gobject); - if (self->display) - gst_wl_display_unregister_buffer (self->display, self); - wl_buffer_destroy (self->wlbuffer); + GST_TRACE_OBJECT (self, "finalize"); + + if (self->wlbuffer) + wl_buffer_destroy (self->wlbuffer); G_OBJECT_CLASS (gst_wl_buffer_parent_class)->finalize (gobject); } @@ -95,6 +120,7 @@ gst_wl_buffer_class_init (GstWlBufferClass * klass) { GObjectClass *object_class = (GObjectClass *) klass; + object_class->dispose = gst_wl_buffer_dispose; object_class->finalize = gst_wl_buffer_finalize; } @@ -120,6 +146,19 @@ static const struct wl_buffer_listener buffer_listener = { buffer_release }; +static void +gstbuffer_disposed (GstWlBuffer * self) +{ + g_assert (!self->used_by_compositor); + self->gstbuffer = NULL; + + GST_TRACE_OBJECT (self, "owning GstBuffer was finalized"); + + /* this will normally destroy the GstWlBuffer, unless the display is + * finalizing and it has taken an additional reference to it */ + g_object_unref (self); +} + GstWlBuffer * gst_buffer_add_wl_buffer (GstBuffer * gstbuffer, struct wl_buffer *wlbuffer, GstWlDisplay * display) @@ -136,7 +175,7 @@ gst_buffer_add_wl_buffer (GstBuffer * gstbuffer, struct wl_buffer *wlbuffer, wl_buffer_add_listener (self->wlbuffer, &buffer_listener, self); gst_mini_object_set_qdata ((GstMiniObject *) gstbuffer, - gst_wl_buffer_qdata_quark (), self, g_object_unref); + gst_wl_buffer_qdata_quark (), self, (GDestroyNotify) gstbuffer_disposed); return self; } @@ -151,25 +190,30 @@ gst_buffer_get_wl_buffer (GstBuffer * gstbuffer) void gst_wl_buffer_force_release_and_unref (GstWlBuffer * self) { - /* detach from the GstBuffer */ - (void) gst_mini_object_steal_qdata ((GstMiniObject *) self->gstbuffer, - gst_wl_buffer_qdata_quark ()); - - /* force a buffer release - * at this point, the GstWlDisplay has killed its event loop, + /* Force a buffer release. + * At this point, the GstWlDisplay has killed its event loop, * so we don't need to worry about buffer_release() being called * at the same time from the event loop thread */ if (self->used_by_compositor) { GST_DEBUG_OBJECT (self, "forcing wl_buffer::release (GstBuffer: %p)", self->gstbuffer); - gst_buffer_unref (self->gstbuffer); self->used_by_compositor = FALSE; + gst_buffer_unref (self->gstbuffer); } - /* avoid unregistering from the display in finalize() because this - * function is being called from a hash table foreach function, - * which would be modified in gst_wl_display_unregister_buffer() */ + /* Finalize this GstWlBuffer early. + * This method has been called as a result of the display shutting down, + * so we need to stop using any wayland resources and disconnect from + * the display. The GstWlBuffer stays alive, though, to avoid race + * conditions with the GstBuffer being destroyed from another thread. + * The last reference is either owned by the GstBuffer or by us and + * it will be released at the end of this function. */ + GST_TRACE_OBJECT (self, "finalizing early"); + wl_buffer_destroy (self->wlbuffer); + self->wlbuffer = NULL; self->display = NULL; + + /* remove the reference that the caller (GstWlDisplay) owns */ g_object_unref (self); } diff --git a/ext/wayland/wldisplay.c b/ext/wayland/wldisplay.c index bcffa2cdc1..8c5eeaf9ae 100644 --- a/ext/wayland/wldisplay.c +++ b/ext/wayland/wldisplay.c @@ -47,6 +47,7 @@ gst_wl_display_init (GstWlDisplay * self) self->formats = g_array_new (FALSE, FALSE, sizeof (uint32_t)); self->wl_fd_poll = gst_poll_new (TRUE); self->buffers = g_hash_table_new (g_direct_hash, g_direct_equal); + g_mutex_init (&self->buffers_mutex); } static void @@ -57,6 +58,13 @@ gst_wl_display_finalize (GObject * gobject) gst_poll_set_flushing (self->wl_fd_poll, TRUE); g_thread_join (self->thread); + /* to avoid buffers being unregistered from another thread + * at the same time, take their ownership */ + g_mutex_lock (&self->buffers_mutex); + self->shutting_down = TRUE; + g_hash_table_foreach (self->buffers, (GHFunc) g_object_ref, NULL); + g_mutex_unlock (&self->buffers_mutex); + g_hash_table_foreach (self->buffers, (GHFunc) gst_wl_buffer_force_release_and_unref, NULL); g_hash_table_remove_all (self->buffers); @@ -64,6 +72,7 @@ gst_wl_display_finalize (GObject * gobject) g_array_unref (self->formats); gst_poll_free (self->wl_fd_poll); g_hash_table_unref (self->buffers); + g_mutex_clear (&self->buffers_mutex); if (self->shm) wl_shm_destroy (self->shm); @@ -275,11 +284,22 @@ gst_wl_display_new_existing (struct wl_display * display, void gst_wl_display_register_buffer (GstWlDisplay * self, gpointer buf) { + g_assert (!self->shutting_down); + + GST_TRACE_OBJECT (self, "registering GstWlBuffer %p", buf); + + g_mutex_lock (&self->buffers_mutex); g_hash_table_add (self->buffers, buf); + g_mutex_unlock (&self->buffers_mutex); } void gst_wl_display_unregister_buffer (GstWlDisplay * self, gpointer buf) { - g_hash_table_remove (self->buffers, buf); + GST_TRACE_OBJECT (self, "unregistering GstWlBuffer %p", buf); + + g_mutex_lock (&self->buffers_mutex); + if (G_LIKELY (!self->shutting_down)) + g_hash_table_remove (self->buffers, buf); + g_mutex_unlock (&self->buffers_mutex); } diff --git a/ext/wayland/wldisplay.h b/ext/wayland/wldisplay.h index 38f545267a..5505d60f7a 100644 --- a/ext/wayland/wldisplay.h +++ b/ext/wayland/wldisplay.h @@ -59,7 +59,9 @@ struct _GstWlDisplay GThread *thread; GstPoll *wl_fd_poll; + GMutex buffers_mutex; GHashTable *buffers; + gboolean shutting_down; }; struct _GstWlDisplayClass