diff --git a/ChangeLog b/ChangeLog index 54b3350983..4575a53d5b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,19 @@ +2006-07-13 Jan Schmidt + + * 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 * ext/vorbis/vorbisdec.c: (vorbis_dec_finalize), diff --git a/common b/common index dd173e2720..53ecdc0c97 160000 --- a/common +++ b/common @@ -1 +1 @@ -Subproject commit dd173e2720ac21e4a47c97705d7ff32271a0ee66 +Subproject commit 53ecdc0c97a2992e5abeddd41d514bc142401e5d diff --git a/gst/playback/gstdecodebin.c b/gst/playback/gstdecodebin.c index f67c4ec30f..71dd734a3d 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 4359d94b2f..160073686c 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 5fd862de63..40440293d7 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"); - XCloseDisplay (xvimagesink->xcontext->disp); + XvUngrabPort (xcontext->disp, xcontext->xv_port_id, 0); + + 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;