From 4377a5d71cf400cdc6b405f744fcf31c007acd62 Mon Sep 17 00:00:00 2001 From: George Kiagiadakis Date: Mon, 23 Jun 2014 17:27:01 +0300 Subject: [PATCH] waylandsink: remove the ugly gst_wl_display_stop() now that this mechanism is not needed anymore Because we no longer have a custom buffer pool that holds a reference to the display, there is no way for a cyclic reference to happen like before, so we no longer need to explicitly call a function from the display to release the wl_buffers. However, the general mechanism of registering buffers to the display and forcibly releasing them when the display is destroyed is still needed to avoid potential memory leaks. The comment in wlbuffer.c is updated to reflect the current situation. --- ext/wayland/gstwaylandsink.c | 9 +-------- ext/wayland/wlbuffer.c | 44 +++++++++++++++++++------------------------- ext/wayland/wldisplay.c | 18 +++++++----------- ext/wayland/wldisplay.h | 1 - 4 files changed, 27 insertions(+), 45 deletions(-) diff --git a/ext/wayland/gstwaylandsink.c b/ext/wayland/gstwaylandsink.c index e081280..ef669d3 100644 --- a/ext/wayland/gstwaylandsink.c +++ b/ext/wayland/gstwaylandsink.c @@ -212,12 +212,8 @@ gst_wayland_sink_finalize (GObject * object) if (sink->last_buffer) gst_buffer_unref (sink->last_buffer); - if (sink->display) { - /* the display must be stopped before droping our reference to it - * - see the comment on wlbuffer.c for details */ - gst_wl_display_stop (sink->display); + if (sink->display) g_object_unref (sink->display); - } if (sink->window) g_object_unref (sink->window); if (sink->pool) @@ -358,9 +354,6 @@ gst_wayland_sink_change_state (GstElement * element, GstStateChange transition) * restarted (GstVideoOverlay behaves like that in other sinks) */ if (sink->display && !sink->window) { /* -> the window was toplevel */ - /* the display must be stopped before droping our reference to it - * - see the comment on wlbuffer.c for details */ - gst_wl_display_stop (sink->display); g_clear_object (&sink->display); } g_mutex_unlock (&sink->display_lock); diff --git a/ext/wayland/wlbuffer.c b/ext/wayland/wlbuffer.c index 1806b17..c2bf320 100644 --- a/ext/wayland/wlbuffer.c +++ b/ext/wayland/wlbuffer.c @@ -28,14 +28,14 @@ * references that can be dangerous. The reference cycles looks like: * * ---------------- - * | GstWlDisplay | ----------------------------------> - * ---------------- | - * ^ | - * | V - * ------------------------ ------------- --------------- - * | GstWaylandBufferPool | --> | GstBuffer | ==> | GstWlBuffer | - * | | <-- | | <-- | | - * ------------------------ ------------- --------------- + * | GstWlDisplay | ----------------------------> + * ---------------- | + * | + * V + * ----------------- ------------- --------------- + * | GstBufferPool | --> | GstBuffer | ==> | GstWlBuffer | + * | | <-- | | <-- | | + * ----------------- ------------- --------------- * * A GstBufferPool normally holds references to its GstBuffers and each buffer * holds a reference to a GstWlBuffer (saved in the GstMiniObject qdata). @@ -48,31 +48,25 @@ * to the GstBuffer, which prevents it from returning to the pool. When the * last GstWlBuffer receives a release event and unrefs the last GstBuffer, * the GstBufferPool will be able to stop and if no-one is holding a strong - * ref to it, it will be destroyed. This will destroy that last GstBuffer and - * also the GstWlBuffer. This will all happen in the same context of the + * ref to it, it will be destroyed. This will destroy the pool's GstBuffers and + * also the GstWlBuffers. This will all happen in the same context of the last * gst_buffer_unref, which will be called from the buffer_release() callback. * - * The big problem here lies in the fact that buffer_release() will be called - * from the event loop thread of GstWlDisplay and the second big problem is - * that the GstWaylandBufferPool holds a strong ref to the GstWlDisplay. - * Therefore, if the buffer_release() causes the pool to be destroyed, it may - * also cause the GstWlDisplay to be destroyed and that will happen in the - * context of the event loop thread that GstWlDisplay runs. Destroying the - * GstWlDisplay will need to join the thread (from inside the thread!) and boom. + * The problem here lies in the fact that buffer_release() will be called + * from the event loop thread of GstWlDisplay, so it's as if the display + * holds a reference to the GstWlBuffer, but without having an actual reference. + * When we kill the display, there is no way for the GstWlBuffer, the associated + * 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. * * However, we want to be absolutely certain, so a solution is introduced - * by explicitly releasing all the buffer references and destroying the - * GstWlBuffers as soon as we know that we are not going to use them again. - * All the GstWlBuffers are registered in a hash set inside GstWlDisplay - * and there is gst_wl_display_stop(), which stops the event loop thread - * and releases all the buffers explicitly. This gets called from GstWaylandSink - * right before dropping its own reference to the GstWlDisplay, leaving - * a possible last (but safe now!) reference to the pool, which may be - * referenced by an upstream element. + * 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. */ #include "wlbuffer.h" diff --git a/ext/wayland/wldisplay.c b/ext/wayland/wldisplay.c index 17bfa8c..bcffa2c 100644 --- a/ext/wayland/wldisplay.c +++ b/ext/wayland/wldisplay.c @@ -54,6 +54,13 @@ gst_wl_display_finalize (GObject * gobject) { GstWlDisplay *self = GST_WL_DISPLAY (gobject); + gst_poll_set_flushing (self->wl_fd_poll, TRUE); + g_thread_join (self->thread); + + g_hash_table_foreach (self->buffers, + (GHFunc) gst_wl_buffer_force_release_and_unref, NULL); + g_hash_table_remove_all (self->buffers); + g_array_unref (self->formats); gst_poll_free (self->wl_fd_poll); g_hash_table_unref (self->buffers); @@ -266,17 +273,6 @@ gst_wl_display_new_existing (struct wl_display * display, } void -gst_wl_display_stop (GstWlDisplay * self) -{ - gst_poll_set_flushing (self->wl_fd_poll, TRUE); - g_thread_join (self->thread); - - g_hash_table_foreach (self->buffers, - (GHFunc) gst_wl_buffer_force_release_and_unref, NULL); - g_hash_table_remove_all (self->buffers); -} - -void gst_wl_display_register_buffer (GstWlDisplay * self, gpointer buf) { g_hash_table_add (self->buffers, buf); diff --git a/ext/wayland/wldisplay.h b/ext/wayland/wldisplay.h index ac1df84..38f5452 100644 --- a/ext/wayland/wldisplay.h +++ b/ext/wayland/wldisplay.h @@ -74,7 +74,6 @@ GstWlDisplay *gst_wl_display_new_existing (struct wl_display * display, gboolean take_ownership, GError ** error); /* see wlbuffer.c for explanation */ -void gst_wl_display_stop (GstWlDisplay * self); void gst_wl_display_register_buffer (GstWlDisplay * self, gpointer buf); void gst_wl_display_unregister_buffer (GstWlDisplay * self, gpointer buf); -- 2.7.4