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

@@ -212,12 +212,8 @@ gst_wayland_sink_finalize (GObject * object)
 
   if (sink->last_buffer)
     gst_buffer_unref (sink->last_buffer);
-  if (sink->display) {
+  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);
     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);

ext/wayland/wlbuffer.c

@@ -28,14 +28,14 @@
  * references that can be dangerous. The reference cycles looks like:
  *
  *   ----------------
- *   | GstWlDisplay | ---------------------------------->
+ *   | GstWlDisplay | ---------------------------->
- *   ----------------                                    |
+ *   ----------------                              |
- *         ^                                             |
+ *                                                 |
- *         |                                             V
+ *                                                 V
- *   ------------------------     -------------     ---------------
+ *   -----------------     -------------     ---------------
- *   | GstWaylandBufferPool | --> | GstBuffer | ==> | GstWlBuffer |
+ *   | 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
+ * ref to it, it will be destroyed. This will destroy the pool's GstBuffers and
- * also the GstWlBuffer. This will all happen in the same context of the
+ * 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
+ * The 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
+ * from the event loop thread of GstWlDisplay, so it's as if the display
- * that the GstWaylandBufferPool holds a strong ref to the GstWlDisplay.
+ * holds a reference to the GstWlBuffer, but without having an actual reference.
- * Therefore, if the buffer_release() causes the pool to be destroyed, it may
+ * When we kill the display, there is no way for the GstWlBuffer, the associated
- * also cause the GstWlDisplay to be destroyed and that will happen in the
+ * GstBuffer and the GstBufferPool to get destroyed, so we are going to leak a
- * context of the event loop thread that GstWlDisplay runs. Destroying the
+ * fair ammount of memory.
- * GstWlDisplay will need to join the thread (from inside the thread!) and boom.
  *
  * 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
+ * by registering all the GstWlBuffers with the display and explicitly
- * GstWlBuffers as soon as we know that we are not going to use them again.
+ * releasing all the buffer references and destroying the GstWlBuffers as soon
- * All the GstWlBuffers are registered in a hash set inside GstWlDisplay
+ * as the display is destroyed.
- * 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.
  */
 
 #include "wlbuffer.h"

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);
@@ -265,17 +272,6 @@ gst_wl_display_new_existing (struct wl_display * display,
   return self;
 }
 
-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)
 {

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);