gst/playback/gstdecodebin.c: Fix a caps leak when linking (#347304)

Original commit message from CVS:
* gst/playback/gstdecodebin.c: (find_compatibles):
Fix a caps leak when linking (#347304)
* sys/ximage/ximagesink.c: (gst_ximage_buffer_finalize),
(gst_ximagesink_ximage_destroy), (gst_ximagesink_xcontext_clear),
(gst_ximagesink_change_state):
* sys/xvimage/xvimagesink.c: (gst_xvimage_buffer_destroy),
(gst_xvimage_buffer_finalize), (gst_xvimagesink_check_xshm_calls),
(gst_xvimagesink_xvimage_new), (gst_xvimagesink_xvimage_put),
(gst_xvimagesink_xcontext_clear), (gst_xvimagesink_change_state):
Don't leak shared memory resources. Use the object lock to protect
against the xcontext disappearing while returning a buffer from the
pipeline. (#347304)

5 files changed, 152 insertions, 43 deletions

diff --git a/ChangeLog b/ChangeLog
index 54b335098..4575a53d5 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,19 @@
+2006-07-13  Jan Schmidt  <thaytan@mad.scientist.com>
+
+	* gst/playback/gstdecodebin.c: (find_compatibles):
+	Fix a caps leak when linking (#347304)
+
+	* sys/ximage/ximagesink.c: (gst_ximage_buffer_finalize),
+	(gst_ximagesink_ximage_destroy), (gst_ximagesink_xcontext_clear),
+	(gst_ximagesink_change_state):
+	* sys/xvimage/xvimagesink.c: (gst_xvimage_buffer_destroy),
+	(gst_xvimage_buffer_finalize), (gst_xvimagesink_check_xshm_calls),
+	(gst_xvimagesink_xvimage_new), (gst_xvimagesink_xvimage_put),
+	(gst_xvimagesink_xcontext_clear), (gst_xvimagesink_change_state):
+	Don't leak shared memory resources. Use the object lock to protect
+	against the xcontext disappearing while returning a buffer from the
+	pipeline. (#347304)
+
 2006-07-12  Edward Hervey  <edward@fluendo.com>
 
 	* ext/vorbis/vorbisdec.c: (vorbis_dec_finalize),
diff --git a/common b/common
-Subproject dd173e2720ac21e4a47c97705d7ff32271a0ee6
+Subproject 53ecdc0c97a2992e5abeddd41d514bc142401e5
diff --git a/gst/playback/gstdecodebin.c b/gst/playback/gstdecodebin.c
index f67c4ec30..71dd734a3 100644
--- a/gst/playback/gstdecodebin.c
+++ b/gst/playback/gstdecodebin.c
@@ -455,10 +455,14 @@ find_compatibles (GstDecodeBin * decode_bin, const GstCaps * caps)
       /* we only care about the sink templates */
       if (templ->direction == GST_PAD_SINK) {
         GstCaps *intersect;
+        GstCaps *tmpl_caps;
 
         /* try to intersect the caps with the caps of the template */
-        intersect = gst_caps_intersect (caps,
-            gst_static_caps_get (&templ->static_caps));
+        tmpl_caps = gst_static_caps_get (&templ->static_caps);
+
+        intersect = gst_caps_intersect (caps, tmpl_caps);
+        gst_caps_unref (tmpl_caps);
+
         /* check if the intersection is empty */
         if (!gst_caps_is_empty (intersect)) {
           /* non empty intersection, we can use this element */
diff --git a/sys/ximage/ximagesink.c b/sys/ximage/ximagesink.c
index 4359d94b2..160073686 100644
--- a/sys/ximage/ximagesink.c
+++ b/sys/ximage/ximagesink.c
@@ -196,6 +196,7 @@ gst_ximage_buffer_finalize (GstXImageBuffer * ximage)
 {
   GstXImageSink *ximagesink = NULL;
   gboolean recycled = FALSE;
+  gboolean running;
 
   g_return_if_fail (ximage != NULL);
 
@@ -205,9 +206,18 @@ gst_ximage_buffer_finalize (GstXImageBuffer * ximage)
     goto beach;
   }
 
-  /* If our geometry changed we can't reuse that image. */
-  if ((ximage->width != GST_VIDEO_SINK_WIDTH (ximagesink)) ||
+  GST_OBJECT_LOCK (ximagesink);
+  running = ximagesink->running;
+  GST_OBJECT_UNLOCK (ximagesink);
+
+  if (running == FALSE) {
+    /* If the sink is shutting down, need to clear the image */
+    GST_DEBUG_OBJECT (ximagesink,
+        "destroy image %p because the sink is shutting down", ximage);
+    gst_ximagesink_ximage_destroy (ximagesink, ximage);
+  } else if ((ximage->width != GST_VIDEO_SINK_WIDTH (ximagesink)) ||
       (ximage->height != GST_VIDEO_SINK_HEIGHT (ximagesink))) {
+    /* If our geometry changed we can't reuse that image. */
     GST_DEBUG_OBJECT (ximagesink,
         "destroy image %p as its size changed %dx%d vs current %dx%d",
         ximage, ximage->width, ximage->height,
@@ -523,8 +533,17 @@ gst_ximagesink_ximage_destroy (GstXImageSink * ximagesink,
     ximagesink->cur_image = NULL;
   }
 
+  /* Hold the object lock to ensure the XContext doesn't disappear */
+  GST_OBJECT_LOCK (ximagesink);
+
   /* We might have some buffers destroyed after changing state to NULL */
   if (!ximagesink->xcontext) {
+    GST_DEBUG_OBJECT (ximagesink, "Destroying XImage after XContext");
+#ifdef HAVE_XSHM
+    if (ximage->SHMInfo.shmaddr != ((void *) -1)) {
+      shmdt (ximage->SHMInfo.shmaddr);
+    }
+#endif
     goto beach;
   }
 
@@ -553,6 +572,8 @@ gst_ximagesink_ximage_destroy (GstXImageSink * ximagesink,
   g_mutex_unlock (ximagesink->x_lock);
 
 beach:
+  GST_OBJECT_UNLOCK (ximagesink);
+
   if (ximage->ximagesink) {
     /* Release the ref to our sink */
     ximage->ximagesink = NULL;
@@ -1144,8 +1165,23 @@ gst_ximagesink_xcontext_get (GstXImageSink * ximagesink)
 static void
 gst_ximagesink_xcontext_clear (GstXImageSink * ximagesink)
 {
+  GstXContext *xcontext;
+
   g_return_if_fail (GST_IS_XIMAGESINK (ximagesink));
-  g_return_if_fail (ximagesink->xcontext != NULL);
+
+  GST_OBJECT_LOCK (ximagesink);
+  if (ximagesink->xcontext == NULL) {
+    GST_OBJECT_UNLOCK (ximagesink);
+    return;
+  }
+
+  /* Take the xcontext reference and NULL it while we
+   * clean it up, so that any buffer-alloced buffers 
+   * arriving after this will be freed correctly */
+  xcontext = ximagesink->xcontext;
+  ximagesink->xcontext = NULL;
+
+  GST_OBJECT_UNLOCK (ximagesink);
 
   /* Wait for our event thread */
   if (ximagesink->event_thread) {
@@ -1153,19 +1189,18 @@ gst_ximagesink_xcontext_clear (GstXImageSink * ximagesink)
     ximagesink->event_thread = NULL;
   }
 
-  gst_caps_unref (ximagesink->xcontext->caps);
-  g_free (ximagesink->xcontext->par);
+  gst_caps_unref (xcontext->caps);
+  g_free (xcontext->par);
   g_free (ximagesink->par);
   ximagesink->par = NULL;
 
   g_mutex_lock (ximagesink->x_lock);
 
-  XCloseDisplay (ximagesink->xcontext->disp);
+  XCloseDisplay (xcontext->disp);
 
   g_mutex_unlock (ximagesink->x_lock);
 
   g_free (ximagesink->xcontext);
-  ximagesink->xcontext = NULL;
 }
 
 static void
@@ -1322,10 +1357,14 @@ gst_ximagesink_change_state (GstElement * element, GstStateChange transition)
 
   switch (transition) {
     case GST_STATE_CHANGE_NULL_TO_READY:
+      GST_OBJECT_LOCK (ximagesink);
       ximagesink->running = TRUE;
+
       /* Initializing the XContext */
       if (!ximagesink->xcontext)
         ximagesink->xcontext = gst_ximagesink_xcontext_get (ximagesink);
+      GST_OBJECT_UNLOCK (ximagesink);
+
       if (!ximagesink->xcontext) {
         ret = GST_STATE_CHANGE_FAILURE;
         goto beach;
@@ -1361,7 +1400,10 @@ gst_ximagesink_change_state (GstElement * element, GstStateChange transition)
       GST_VIDEO_SINK_HEIGHT (ximagesink) = 0;
       break;
     case GST_STATE_CHANGE_READY_TO_NULL:
+      GST_OBJECT_LOCK (ximagesink);
       ximagesink->running = FALSE;
+      GST_OBJECT_UNLOCK (ximagesink);
+
       if (ximagesink->ximage) {
         gst_buffer_unref (ximagesink->ximage);
         ximagesink->ximage = NULL;
@@ -1370,18 +1412,15 @@ gst_ximagesink_change_state (GstElement * element, GstStateChange transition)
         gst_buffer_unref (ximagesink->cur_image);
         ximagesink->cur_image = NULL;
       }
-      if (ximagesink->buffer_pool)
-        gst_ximagesink_bufferpool_clear (ximagesink);
+
+      gst_ximagesink_bufferpool_clear (ximagesink);
 
       if (ximagesink->xwindow) {
         gst_ximagesink_xwindow_destroy (ximagesink, ximagesink->xwindow);
         ximagesink->xwindow = NULL;
       }
 
-      if (ximagesink->xcontext) {
-        gst_ximagesink_xcontext_clear (ximagesink);
-        ximagesink->xcontext = NULL;
-      }
+      gst_ximagesink_xcontext_clear (ximagesink);
       break;
     default:
       break;
diff --git a/sys/xvimage/xvimagesink.c b/sys/xvimage/xvimagesink.c
index 5fd862de6..40440293d 100644
--- a/sys/xvimage/xvimagesink.c
+++ b/sys/xvimage/xvimagesink.c
@@ -215,6 +215,8 @@ gst_xvimage_buffer_destroy (GstXvImageBuffer * xvimage)
 {
   GstXvImageSink *xvimagesink;
 
+  GST_DEBUG_OBJECT (xvimage, "Destroying buffer");
+
   xvimagesink = xvimage->xvimagesink;
   if (xvimagesink == NULL)
     goto no_sink;
@@ -226,7 +228,16 @@ gst_xvimage_buffer_destroy (GstXvImageBuffer * xvimage)
     xvimagesink->cur_image = NULL;
 
   /* We might have some buffers destroyed after changing state to NULL */
-  if (xvimagesink->xcontext) {
+  GST_OBJECT_LOCK (xvimagesink);
+  if (xvimagesink->xcontext == NULL) {
+    GST_DEBUG_OBJECT (xvimagesink, "Destroying XvImage after Xcontext");
+#ifdef HAVE_XSHM
+    /* Need to free the shared memory segment even if the x context
+     * was already cleaned up */
+    if (xvimage->SHMInfo.shmaddr != ((void *) -1)) {
+      shmdt (xvimage->SHMInfo.shmaddr);
+    }
+#endif
     goto beach;
   }
 
@@ -235,8 +246,11 @@ gst_xvimage_buffer_destroy (GstXvImageBuffer * xvimage)
 #ifdef HAVE_XSHM
   if (xvimagesink->xcontext->use_xshm) {
     if (xvimage->SHMInfo.shmaddr != ((void *) -1)) {
+      GST_DEBUG_OBJECT (xvimagesink, "XServer ShmDetaching from 0x%x id 0x%x\n",
+          xvimage->SHMInfo.shmid, xvimage->SHMInfo.shmseg);
       XShmDetach (xvimagesink->xcontext->disp, &xvimage->SHMInfo);
       XSync (xvimagesink->xcontext->disp, FALSE);
+
       shmdt (xvimage->SHMInfo.shmaddr);
     }
     if (xvimage->xvimage)
@@ -257,6 +271,7 @@ gst_xvimage_buffer_destroy (GstXvImageBuffer * xvimage)
   g_mutex_unlock (xvimagesink->x_lock);
 
 beach:
+  GST_OBJECT_UNLOCK (xvimagesink);
   xvimage->xvimagesink = NULL;
   gst_object_unref (xvimagesink);
 
@@ -273,13 +288,21 @@ static void
 gst_xvimage_buffer_finalize (GstXvImageBuffer * xvimage)
 {
   GstXvImageSink *xvimagesink;
+  gboolean running;
 
   xvimagesink = xvimage->xvimagesink;
-  if (xvimagesink == NULL)
+  if (G_UNLIKELY (xvimagesink == NULL))
     goto no_sink;
 
+  GST_OBJECT_LOCK (xvimagesink);
+  running = xvimagesink->running;
+  GST_OBJECT_UNLOCK (xvimagesink);
+
   /* If our geometry changed we can't reuse that image. */
-  if ((xvimage->width != xvimagesink->video_width) ||
+  if (running == FALSE) {
+    GST_LOG_OBJECT (xvimage, "destroy image as sink is shutting down");
+    gst_xvimage_buffer_destroy (xvimage);
+  } else if ((xvimage->width != xvimagesink->video_width) ||
       (xvimage->height != xvimagesink->video_height)) {
     GST_LOG_OBJECT (xvimage,
         "destroy image as its size changed %dx%d vs current %dx%d",
@@ -440,6 +463,9 @@ gst_xvimagesink_check_xshm_calls (GstXContext * xcontext)
   /* Sync to ensure we see any errors we caused */
   XSync (xcontext->disp, FALSE);
 
+  GST_DEBUG ("XServer ShmAttached to 0x%x, id 0x%x\n", SHMInfo.shmid,
+      SHMInfo.shmseg);
+
   if (!error_caught) {
     did_attach = TRUE;
     /* store whether we succeeded in result */
@@ -454,6 +480,8 @@ beach:
   XSetErrorHandler (handler);
 
   if (did_attach) {
+    GST_DEBUG ("XServer ShmDetaching from 0x%x id 0x%x\n",
+        SHMInfo.shmid, SHMInfo.shmseg);
     XShmDetach (xcontext->disp, &SHMInfo);
     XSync (xcontext->disp, FALSE);
   }
@@ -476,6 +504,7 @@ gst_xvimagesink_xvimage_new (GstXvImageSink * xvimagesink, GstCaps * caps)
   g_return_val_if_fail (GST_IS_XVIMAGESINK (xvimagesink), NULL);
 
   xvimage = (GstXvImageBuffer *) gst_mini_object_new (GST_TYPE_XVIMAGE_BUFFER);
+  GST_DEBUG_OBJECT (xvimage, "Creating new XvImageBuffer");
 
   structure = gst_caps_get_structure (caps, 0);
 
@@ -556,6 +585,8 @@ gst_xvimagesink_xvimage_new (GstXvImageSink * xvimagesink, GstCaps * caps)
     }
 
     XSync (xvimagesink->xcontext->disp, FALSE);
+    GST_DEBUG_OBJECT (xvimagesink, "XServer ShmAttached to 0x%x, id 0x%x\n",
+        xvimage->SHMInfo.shmid, xvimage->SHMInfo.shmseg);
   } else
 #endif /* HAVE_XSHM */
   {
@@ -691,9 +722,11 @@ gst_xvimagesink_xvimage_put (GstXvImageSink * xvimagesink,
 #ifdef HAVE_XSHM
   if (xvimagesink->xcontext->use_xshm) {
     GST_LOG_OBJECT (xvimagesink,
-        "XvShmPutImage with image %dx%d and window %dx%d",
+        "XvShmPutImage with image %dx%d and window %dx%d, from xvimage %"
+        GST_PTR_FORMAT,
         xvimage->width, xvimage->height,
-        xvimagesink->xwindow->width, xvimagesink->xwindow->height);
+        xvimagesink->xwindow->width, xvimagesink->xwindow->height, xvimage);
+
     XvShmPutImage (xvimagesink->xcontext->disp,
         xvimagesink->xcontext->xv_port_id,
         xvimagesink->xwindow->win,
@@ -1533,9 +1566,21 @@ static void
 gst_xvimagesink_xcontext_clear (GstXvImageSink * xvimagesink)
 {
   GList *formats_list, *channels_list;
+  GstXContext *xcontext;
 
   g_return_if_fail (GST_IS_XVIMAGESINK (xvimagesink));
-  g_return_if_fail (xvimagesink->xcontext != NULL);
+
+  GST_OBJECT_LOCK (xvimagesink);
+  if (xvimagesink->xcontext == NULL) {
+    GST_OBJECT_UNLOCK (xvimagesink);
+    return;
+  }
+
+  /* Take the XContext from the sink and clean it up */
+  xcontext = xvimagesink->xcontext;
+  xvimagesink->xcontext = NULL;
+
+  GST_OBJECT_UNLOCK (xvimagesink);
 
   /* Wait for our event thread */
   if (xvimagesink->event_thread) {
@@ -1543,7 +1588,7 @@ gst_xvimagesink_xcontext_clear (GstXvImageSink * xvimagesink)
     xvimagesink->event_thread = NULL;
   }
 
-  formats_list = xvimagesink->xcontext->formats_list;
+  formats_list = xcontext->formats_list;
 
   while (formats_list) {
     GstXvImageFormat *format = formats_list->data;
@@ -1553,10 +1598,10 @@ gst_xvimagesink_xcontext_clear (GstXvImageSink * xvimagesink)
     formats_list = g_list_next (formats_list);
   }
 
-  if (xvimagesink->xcontext->formats_list)
-    g_list_free (xvimagesink->xcontext->formats_list);
+  if (xcontext->formats_list)
+    g_list_free (xcontext->formats_list);
 
-  channels_list = xvimagesink->xcontext->channels_list;
+  channels_list = xcontext->channels_list;
 
   while (channels_list) {
     GstColorBalanceChannel *channel = channels_list->data;
@@ -1565,26 +1610,26 @@ gst_xvimagesink_xcontext_clear (GstXvImageSink * xvimagesink)
     channels_list = g_list_next (channels_list);
   }
 
-  if (xvimagesink->xcontext->channels_list)
-    g_list_free (xvimagesink->xcontext->channels_list);
+  if (xcontext->channels_list)
+    g_list_free (xcontext->channels_list);
 
-  gst_caps_unref (xvimagesink->xcontext->caps);
-  if (xvimagesink->xcontext->last_caps)
-    gst_caps_replace (&xvimagesink->xcontext->last_caps, NULL);
+  gst_caps_unref (xcontext->caps);
+  if (xcontext->last_caps)
+    gst_caps_replace (&xcontext->last_caps, NULL);
 
-  g_free (xvimagesink->xcontext->par);
+  g_free (xcontext->par);
 
   g_mutex_lock (xvimagesink->x_lock);
 
-  XvUngrabPort (xvimagesink->xcontext->disp,
-      xvimagesink->xcontext->xv_port_id, 0);
+  GST_DEBUG_OBJECT (xvimagesink, "Closing display and freeing X Context");
+
+  XvUngrabPort (xcontext->disp, xcontext->xv_port_id, 0);
 
-  XCloseDisplay (xvimagesink->xcontext->disp);
+  XCloseDisplay (xcontext->disp);
 
   g_mutex_unlock (xvimagesink->x_lock);
 
-  g_free (xvimagesink->xcontext);
-  xvimagesink->xcontext = NULL;
+  g_free (xcontext);
 }
 
 static void
@@ -1812,11 +1857,17 @@ gst_xvimagesink_change_state (GstElement * element, GstStateChange transition)
 
   switch (transition) {
     case GST_STATE_CHANGE_NULL_TO_READY:
+      GST_OBJECT_LOCK (xvimagesink);
       xvimagesink->running = TRUE;
       /* Initializing the XContext */
       if (!xvimagesink->xcontext &&
-          !(xvimagesink->xcontext = gst_xvimagesink_xcontext_get (xvimagesink)))
+          !(xvimagesink->xcontext =
+              gst_xvimagesink_xcontext_get (xvimagesink))) {
+        GST_OBJECT_UNLOCK (xvimagesink);
         return GST_STATE_CHANGE_FAILURE;
+      }
+      GST_OBJECT_UNLOCK (xvimagesink);
+
       /* update object's par with calculated one if not set yet */
       if (!xvimagesink->par) {
         xvimagesink->par = g_new0 (GValue, 1);
@@ -1853,7 +1904,9 @@ gst_xvimagesink_change_state (GstElement * element, GstStateChange transition)
       GST_VIDEO_SINK_HEIGHT (xvimagesink) = 0;
       break;
     case GST_STATE_CHANGE_READY_TO_NULL:
+      GST_OBJECT_LOCK (xvimagesink);
       xvimagesink->running = FALSE;
+      GST_OBJECT_UNLOCK (xvimagesink);
       if (xvimagesink->cur_image) {
         gst_buffer_unref (xvimagesink->cur_image);
         xvimagesink->cur_image = NULL;
@@ -1862,18 +1915,15 @@ gst_xvimagesink_change_state (GstElement * element, GstStateChange transition)
         gst_buffer_unref (xvimagesink->xvimage);
         xvimagesink->xvimage = NULL;
       }
-      if (xvimagesink->image_pool)
-        gst_xvimagesink_imagepool_clear (xvimagesink);
+
+      gst_xvimagesink_imagepool_clear (xvimagesink);
 
       if (xvimagesink->xwindow) {
         gst_xvimagesink_xwindow_destroy (xvimagesink, xvimagesink->xwindow);
         xvimagesink->xwindow = NULL;
       }
 
-      if (xvimagesink->xcontext) {
-        gst_xvimagesink_xcontext_clear (xvimagesink);
-        xvimagesink->xcontext = NULL;
-      }
+      gst_xvimagesink_xcontext_clear (xvimagesink);
       break;
     default:
       break;