GNOME Bugzilla – Bug 756611
playbin: Leak of playbin on errors from the source element
Last modified: 2015-10-20 07:18:22 UTC
+++ This bug was initially created as a clone of Bug #755867 +++ With the testcase from bug #756552, I still get > GStreamer-CRITICAL **: gst_poll_get_read_gpollfd: assertion 'set != NULL' failed after a while. So we still leak *something* :) At least we leak all playbin instances. This only happens if the example is set to use a non-existing URI.
Created attachment 313414 [details] [review] send async_start when subelements setup properly Not sure if this is the right way to fix. Here is my analysis and patch which does resolve the issue.. Narrowed it down to commit http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/gst/playback?id=73646bd04f4755ae3709ae4bd10b122e74358711 do_async_start (playbin); is being called when changing from ready_paused before subelements are being set to paused state. So in this case since the uri is not valid, uridecodebin is not able to set to paused state. And hence it fails. Because of this async_done is never getting posted. tried to fix this by calling async_start only when the subelements are able to setup properly. PS: uridecodebin and playsink also has similar issue i guess. When start msg is being posted before checking for failures. PPS: Along with this, one doubt i have is, in gstbin.c, the async msgs are being ignored when target state is <= READY. But i could see async_done msg being posted in PAUSED_TO_READY and READY_TO_NULL. Doesn't it make these calls void?(decodebin2, playsink, uridecodebin all have this) PPPS: :P... not sure if anything i said makes sense.. If it does not, then we should start from the commit i mentioned. On reverting those changes everything works :)...
Or if subelements fail, post async-done directly? Does that work? Otherwise, we should also post async-start if baseclass returns ASYNC or NO_PREROLL I think. Not sure about NO_PREROLL actually, in that case we immediately post async-done in the existing code IIRC. Will look closer later.
posting async-done when subelements fails is not working. I think the reason for this is, the element is still in ready state and not changed to paused state. So gstbin is just skipping the message being posted when the state is <= READY regarding posting async-done for NO_PREROLL, uridecodebin, playsink, decodebin2 already does that.. So probably even in playbin we should do that? So if i understand it correctly along with the change in the patch attached, should also check return value of base class change and do async_start for ASYNC and async_done for NO_PREROLL?? something like this.. ret = GST_ELEMENT_CLASS (parent_class)->change_state (element, transition); if (ret == GST_STATE_CHANGE_FAILURE) goto failure; else if (ret == GST_STATE_CHANGE_NO_PREROLL) do_async_done (decoder); else if (ret == GST_STATE_CHANGE_ASYNC) do_async_start (decoder);
on testing with the above change else if (ret == GST_STATE_CHANGE_ASYNC) do_async_start (playbin); The same issue happens. So maybe it is not correct :)
The only thing we need to ensure AFAIU, is that for each async-start we always post an async-done at some point. Do we?
Before the patch both are being posted for success cases, but only async-start is being posted for wrong uri.. After the patch both will be posted for success and failure cases.. But i am not sure about if just posting async-start and async-stop is enough.. Start and stop can be called only when the target state is either paused and playing.. am i right about this? That is the reason calling async-done on failures of wrong uri does not work..
Created attachment 313556 [details] [review] playbin: Post async-done if the async READY->PAUSED state change fails
Created attachment 313557 [details] [review] playbin: Post async-done in READY->PAUSED if we're in a live pipeline
Created attachment 313558 [details] [review] uridecodebin: Always post async-done immediately if we're a live pipeline Not only if the base class told us, but also if one of our own elements did.
The remaining problem is IMHO that GstBin is not removing messages here as needed. Patch coming
Created attachment 313559 [details] [review] bin: Make sure to free all cached messages when going to NULL An ASYNC READY->PAUSED might have failed without the bin code noticing during the state change, in which case we will never get PAUSED->READY and would leak messages.
Created attachment 313560 [details] [review] playsink: Immediately error out if state change fails Otherwise we chain up to the parent class' change_state function and might override the failure with SUCCESS.
Created attachment 313561 [details] [review] decodebin/playsink/subtitleoverlay: Post async-done on state change failures
I think with all this, everything is working as expected now.
I'm not sure if posting async-done is necessary if the READY->PAUSED state change (or any state change) fails. It doesn't hurt though, might just be additional noise.
Created attachment 313600 [details] [review] decodebin/playbin/playsink/subtitleoverlay: Post async-done on state change failures
Created attachment 313601 [details] [review] playbin/uridecodebin: Always post async-done immediately if we're a live pipeline Not only if the base class told us, but also if one of our own elements did.
Created attachment 313602 [details] [review] playsink: Immediately error out if state change fails Otherwise we chain up to the parent class' change_state function and might override the failure with SUCCESS.
Created attachment 313603 [details] [review] decodebin/playbin/playsink/subtitleoverlay: Post async-done on state change failures
bin: Make sure to free all cached messages when going to NULL This patch is enough to fix this particular issue.. other patches also make sense. Except that posting async-done for all state change failures is kind of noise. But it maintains consistency though..
Will go to 1.6.1 in a bit, thanks for testing Vineeth! :) commit 2955eeb0eda1c87fb323368c86ca6456c4da522d Author: Sebastian Dröge <sebastian@centricular.com> Date: Sat Oct 17 22:13:08 2015 +0300 bin: Make sure to free all cached messages when going to NULL An ASYNC READY->PAUSED might have failed without the bin code noticing during the state change, in which case we will never get PAUSED->READY and would leak messages. https://bugzilla.gnome.org/show_bug.cgi?id=756611 commit 4d6aa0f83121c7c19561124ccff0f59faa4dca97 Author: Sebastian Dröge <sebastian@centricular.com> Date: Sat Oct 17 22:25:22 2015 +0300 decodebin/playbin/playsink/subtitleoverlay: Post async-done on state change failures https://bugzilla.gnome.org/show_bug.cgi?id=756611 commit 87dbe54797d7e5e1d89f3549d9b888533482496d Author: Sebastian Dröge <sebastian@centricular.com> Date: Sat Oct 17 22:20:31 2015 +0300 playsink: Immediately error out if state change fails Otherwise we chain up to the parent class' change_state function and might override the failure with SUCCESS. https://bugzilla.gnome.org/show_bug.cgi?id=756611 commit 92061cb19e81e5846628dfb032e29defa917413e Author: Sebastian Dröge <sebastian@centricular.com> Date: Sat Oct 17 21:47:07 2015 +0300 playbin/uridecodebin: Always post async-done immediately if we're a live pipeline Not only if the base class told us, but also if one of our own elements did. https://bugzilla.gnome.org/show_bug.cgi?id=756611