GNOME Bugzilla – Bug 753622
splitmuxsink: do not destroy the multiqueue & muxer when going to NULL
Last modified: 2015-10-30 15:40:14 UTC
Created attachment 309257 [details] [review] splitmuxsink: do not destroy the multiqueue & muxer when going to NULL The release_pad() vfunc in splitmuxsink requires the multiqueue and muxer to be there, otherwise it returns. At the same time, when transitioning from READY to NULL, those elements are destroyed, so when an external user of the element tries to release a previously acquired pad *after* the state has changed to NULL (a totally reasonable thing to do), the pad is not really released and some resources are leaked. The attached patch avoids releasing the elements in READY->NULL and only releases them in NULL->READY in case they were previously created. The elements are not referenced by splitmuxsink code, they are only referenced by GstBin, so they are also removed in dispose() by GstBin.
Review of attachment 309257 [details] [review]: Looks OK, thanks!
Pushed, with one small change for video_ctx->reference_ctx introduced in other patches. Thanks! commit 5e4caca709c0f971dc942429a8578eb77133145a Author: George Kiagiadakis <george.kiagiadakis@collabora.com> Date: Fri Apr 17 14:25:43 2015 +0200 splitmuxsink: post messages when fragments are being opened and closed This can be useful for applications that need to track the created fragments (to log them in a recording database, for example) https://bugzilla.gnome.org/show_bug.cgi?id=750108
Actually, this one makes this break in the unit test. The multiqueue and muxer are created already in gst_splitmux_sink_request_new_pad(), so destroying them and recreating them in NULL->READY is no good. The unit test fails with this patch applied
Created attachment 314036 [details] [review] splitmuxsink: do not destroy the multiqueue & muxer when going to NULL
Created attachment 314037 [details] [review] tests/check/splitmux: test that the release_pad vfunc of splitmuxsink actually releases pads
Looking at this again, you are right that reset() cannot be done in NULL->READY. In fact it is not acceptable to reset as long as there are request pads alive. My new approach now will call reset() as soon as all request pads are released, or in READY->NULL if no pads were ever requested. I also extended the unit test to showcase the bug when the patch is not applied.
Thanks! :)