GNOME Bugzilla – Bug 776334
Memory leak fixed in usage of gst_element_get_request_pad() API
Last modified: 2018-11-03 14:02:15 UTC
Created attachment 342296 [details] [review] Memory leak fixed in usage of gst_element_get_request() API Memory leak fixed in usage of gst_element_get_request() API. gst_element_get_request() returns requested GstPad which need to be released after its usage. Changes done in following file: ext/resindvd/resindvdbin.c gst/sdp/gstsdpdemux.c sys/dvb/dvbbasebin.c Added gst_object_unref() to release the memory. Find the attached patch.
Review of attachment 342296 [details] [review]: ::: ext/resindvd/resindvdbin.c @@ +719,3 @@ + if (gst_pad_link (pad, mq_sink) != GST_PAD_LINK_OK) { + gst_object_unref (mq_sink); This looks wrong. The pad is stored above in the list, and that's probably what should own the reference. And later when the list is emptied (and the pads are released), the unref should happen
Created attachment 342307 [details] [review] Memory leak fixed in usage of gst_element_get_request_pad() API Yes. I understood your point. I have attached the latest patch. Please review the patch.
Comment on attachment 342307 [details] [review] Memory leak fixed in usage of gst_element_get_request_pad() API commit 0fdd4e2539cc8c2c794419b13594ed7761d5fccd Author: Garima Gaur <garima.g@samsung.com> Date: Wed Dec 21 13:41:16 2016 +0530 gst: Fix memory leaks in usage of gst_element_get_request_pad() API The return value has to be unreffed at some point. https://bugzilla.gnome.org/show_bug.cgi?id=776334
What you found in resindvd is actually a leak, as said above, you would have to unref the pads when the list is emptied. Which currently does not happen.
Are you planning to provide a new patch for that?
Do you mean that following fix for resindvd was correct in earlier patch: --- a/ext/resindvd/resindvdbin.c +++ b/ext/resindvd/resindvdbin.c @@ -717,8 +717,10 @@ connect_thru_mq (RsnDvdBin * dvdbin, GstPad * pad) return FALSE; dvdbin->mq_req_pads = g_list_prepend (dvdbin->mq_req_pads, mq_sink); - if (gst_pad_link (pad, mq_sink) != GST_PAD_LINK_OK) + if (gst_pad_link (pad, mq_sink) != GST_PAD_LINK_OK) { + gst_object_unref (mq_sink); return FALSE; + } sinkname = gst_pad_get_name (mq_sink); tmp = sinkname + 5; @@ -727,6 +729,7 @@ connect_thru_mq (RsnDvdBin * dvdbin, GstPad * pad) mq_src = gst_element_get_static_pad (dvdbin->pieces[DVD_ELEM_MQUEUE], srcname); + gst_object_unref (mq_sink); g_free (sinkname); g_free (srcname);
No it wasn't. But the pads will have to be unreffed later after they are released from the multiqueue, and that part is missing.
I understood. But I not found the point where to release the memory for mq_sink. Please consider my patch for other 2 changes. Thanks.
That's done here: https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/resindvd/resindvdbin.c#n660 Just search for "gst_element_release_request_pad". As part of that loop, it should also unref the pads. (In reply to Garima from comment #8) > Please consider my patch for other 2 changes. Thanks. The other two are merged already.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/499.