GNOME Bugzilla – Bug 773341
urisourcebin: Cleanup unused output slot
Last modified: 2016-12-08 17:14:38 UTC
urisourcebin: Cleanup unused output slot With adaptivedemux, any output slots can be dynamically exposed or removed in running time. Because we cannot cleanup output slots in the streaming thread, new task will be introduced to cleanup output slots.
Created attachment 338244 [details] [review] urisourcebin-Cleanup-unused-output-slot.patch
Created attachment 338245 [details] [review] urisourcebin-Remove-trailing-whitespace.patch
Created attachment 338246 [details] [Dot graph] 1st period
Created attachment 338247 [details] [Dot graph] 2nd period
Current urisourcebin is disabling the call of free_output_slot (), because streaming thread cannot cleanup its output pad itself. This must be leak.... It can be reproduced with playbin3 and following MPD. (one parsebin does not be destroyed in the 2nd period) http://dash.edgesuite.net/dash264/TestCases/5b/nomor/3.mpd The details of the MPD is that, [1st Period] 2nd Period][3rd Period] |Video| Available | Available | Available | |Audio| Available | N/A | Available | When period was progressed from the 1st to the 2nd, there are 2 bugs. [bug 1] urisourcebin did not clean up output slot. For this, I introduced new GstTask and BUS to call free_output_slot () at another thread (not in the streaming thread). [bug 2] urisourcebin tried to send CUSTOM_EVENT "urisourcebin-custom-eos" toward already EOSed pad. For this, I made a change to trigger cleaning up task with pad_removed callback (without "urisourcebin-custom-eos") Please refer to following log - dashdemux sent EOS to audio path at the end of the 1st period 0:00:08.102531554 11983 0x7f8ba801e990 DEBUG GST_EVENT gstpad.c:5549:gst_pad_send_event_unchecked:<queue2-2:sink> have event type eos event: 0x7f8b7807e8e0, time 99:99:99.999999999, seq-num 4664, (NULL) - Then, audio pad of dashdemux is destroyed with pad_removed callback, and then urisourcebin tried to send CUSTOM_EVENT but it's discarded. 3329 0:00:09.036514940 ^[[332m11983^[[00m 0x7f8ba801ee30 ^[[33;01mLOG ^[[00m ^[[00m urisourcebin gsturisourcebin.c:1350:pad_removed_cb:<dashdemux0>^[[00m Pad <dashdemux0:audio_00> was removed without EOS. Sending. 3330 0:00:09.036529752 ^[[332m11983^[[00m 0x7f8ba801ee30 ^[[37mDEBUG ^[[00m ^[[00;01;34m GST_EVENT gstpad.c:5549:gst_pad_send_event_unchecked:<queue2-2:sink>^[[00m have event type custom-downstream event: 0x7f8b8c002ca0, time 9 9:99:99.999999999, seq-num 5300, urisourcebin-custom-eos; 3331 0:00:09.036537779 ^[[332m11983^[[00m 0x7f8ba801ee30 ^[[36mINFO ^[[00m ^[[00;01;34m GST_EVENT gstpad.c:5675:gst_pad_send_event_unchecked:<queue2-2:sink>^[[00m Received event on EOS pad. Discarding
Review of attachment 338244 [details] [review]: (Only a very fast review for now) Always having yet another thread when having a demuxer seems a bit heavy. Maybe you can make use of gst_element_call_async() here (in a one-shot way), which uses a thread pool? ::: gst/playback/gsturisourcebin.c @@ +754,3 @@ + g_rec_mutex_init (&urisrc->drained_lock); + + urisrc->bus = g_object_new (GST_TYPE_BUS, "enable-async", TRUE, NULL); gst_bus_new()? @@ +1864,3 @@ + /* run drained_task () */ + urisrc->drained_task = + gst_task_new ((GstTaskFunction) cleanup_slot_loop, urisrc, NULL); There can also be multiple demuxers, e.g. AVI containing DV, MXF containing MPEG-PS/TS, etc @@ +2518,3 @@ + return GST_BUS_PASS; + } else { + gst_message_unref (message); You can remove this else here and just do unref before returning DROP
(In reply to Sebastian Dröge (slomo) from comment #6) > Review of attachment 338244 [details] [review] [review]: > Thanks for very quick review !! :) > Always having yet another thread when having a demuxer seems a bit heavy. > Maybe you can make use of gst_element_call_async() here (in a one-shot way), > which uses a thread pool? I'll try to study more about "gst_element_call_async()" Thanks. > @@ +1864,3 @@ > + /* run drained_task () */ > + urisrc->drained_task = > + gst_task_new ((GstTaskFunction) cleanup_slot_loop, urisrc, NULL); > > There can also be multiple demuxers, e.g. AVI containing DV, MXF containing > MPEG-PS/TS, etc Could you kindly explain more about "AVI containing DV, MXF containing MPEG-PS/TS"? The "demux" in urisourcebin seems to be only AdaptiveDemux for DASH/HLS/Smooth streaming. Are there any case that those multiple adaptive demuxers exist at one time?
Created attachment 338263 [details] [review] urisourcebin-Cleanup-unused-output-slot.patch urisourcebin: Cleanup unused output slot Since urisourcebin cannot cleanup unused output slot in streaming thread, it will be handled in thread pool with gst_element_call_async ().
new patch was uploaded using gst_element_call_async (), The new method seems to be very useful!!
(In reply to Seungha Yang from comment #7) > > @@ +1864,3 @@ > > + /* run drained_task () */ > > + urisrc->drained_task = > > + gst_task_new ((GstTaskFunction) cleanup_slot_loop, urisrc, NULL); > > > > There can also be multiple demuxers, e.g. AVI containing DV, MXF containing > > MPEG-PS/TS, etc > > Could you kindly explain more about "AVI containing DV, MXF containing > MPEG-PS/TS"? The "demux" in urisourcebin seems to be only AdaptiveDemux for > DASH/HLS/Smooth streaming. Are there any case that those multiple adaptive > demuxers exist at one time? You're right, it's only for adaptive demuxers here. We're in urisourcebin after all :) There's also the "demuxer" field in urisourcebin btw, which is completely unused. I guess this should be used to store the demuxer and maybe also get an assertion that it is never set twice? Want to provide a patch on top of your other patches for that cleanup?
Review of attachment 338263 [details] [review]: Generally looks good to me
(In reply to Sebastian Dröge (slomo) from comment #10) > (In reply to Seungha Yang from comment #7) > There's also the "demuxer" field in urisourcebin btw, which is completely > unused. I guess this should be used to store the demuxer and maybe also get > an assertion that it is never set twice? Want to provide a patch on top of > your other patches for that cleanup? I'd like to do it, however, if my understanding is correct about "unused demuxer", it's already reported at bug #772445 :)
Created attachment 338284 [details] [review] urisourcebin-Try-to-link-output-slot-before-cleanup.patch urisourcebin: Try to link output slot before cleanup Before cleaning up output slot, check pending pads first, if available. Then, cleanup it only if linking was failed.
One more patch has been uploaded. It's more make sense that trying to link output slot first, in my opinion. Actually, in some case, adaptivedemux pushes EOS before exposing new pad during period progress.
Created attachment 338314 [details] [Dot graph] 2nd period with PATCH
Although, urisourcebin can cleanup output slot, with my patches, there are similar issues (but much more complicated) on decodebin3 and playbin3 (also maybe playsink?). <summary> [Issue 1] DecodebinInput cleanup issue [Issue 2] DecodebinOutputStream cleanup issue [Issue 3] MultiQueueSlot cleanup issue [Issue 4] Playsink reconfigure issue <details> [Issue 1] DecodebinInput cleanup issue When sinkpad of a decodebin3 was unlinked with calling gst_decodebin3_input_pad_unlink (), - decodebin3 just set the state of parsebin to NULL state. - No action for cleaning up other member (e.g., ghost sinkpad, collection, etc) of the decodebin3. (refer to attached dot graph) [Issue 2] DecodebinOutputStream cleanup issue In the multiqueue_src_probe (), there is a code for freeing output stream with detected EOS event. The issue is that, decodebin3 intercepts EOS event from parsbin's srcpad. (forward EOS event only if all streams are EOSed). So, freeing output stream cannot be triggerd. About this issue, synchronizing EOS of streams might be role of streamsynchronizer. [Issue 3] MultiQueueSlot cleanup issue freeing multiqueue slot by calling free_multiqueue_slot() does not considered in running time. It's only called during state change. [Issue 4] Playsink reconfigure issue Although any streams might be removed, playbin3 should trigger gst_play_sink_reconfigure () for actual cleanup of playsin. One of the reason seems to "decodebin3 does not signalling no-more-pads". Note that playbin2 can do this since it post the no-more-pads signal. How can we trigger reconfigure in case of playbin3? might be with stream-collection update?
Created attachment 338396 [details] [review] playback-Remove-trailing-whitespace.patch
Created attachment 338397 [details] [review] decodebin3-Fix-condition-for-merging-stream-collecti.patch decodebin3: Fix condition for merging stream collection We can not guarantee that main_input has collection or not. Collection merge decision should be done based on the number of collections.
Created attachment 338514 [details] [review] decodebin3-Cleanup-input-output-stream-and-multiqueu.patch decodebin3: Cleanup input/output stream and multiqueue slot Need to free no more used DecodebinInput, DecodebinOutputStream, and MultiQueueSlot. * About DecodebinInput, removing it from list and free it with unlinking input. Forwarding custom-downstream event in order for downstream elements to understand that input has been removed. * Although all streams are not EOSed yet, with custom-downstream event, DecodebinOutputStream and MultiQueueSlot will be removed if DecodebinInputStream has been removed.
Created attachment 338517 [details] [review] decodebin3-Cleanup-input-output-stream-and-multiqueu.patch
Created attachment 338574 [details] [review] decodebin3-Cleanup-input-output-stream-and-multiqueu.patch - Fix segfault of previous patch. - Introduce a lock for input stream list.
Comment on attachment 338397 [details] [review] decodebin3-Fix-condition-for-merging-stream-collecti.patch We don't need this patch, because main_input's collection has been cleared with other decodebin3 patch.
Comment on attachment 338245 [details] [review] urisourcebin-Remove-trailing-whitespace.patch commit 6dbfbead615389043b9422f6533591b1e9218749 Author: Seungha Yang <sh.yang@lge.com> Date: Sat Oct 22 11:08:18 2016 +0900 urisourcebin: Remove trailing whitespace https://bugzilla.gnome.org/show_bug.cgi?id=773341
Created attachment 339004 [details] [review] playback-Cleanup-input-output-stream-and-multiqueue-.patch <Modification> remove no-more-pads signal from prev. patch and reconfigure playsink with pad-removed-cb
Created attachment 341315 [details] [review] playbin3: Reconfigure playsink again with pad-removed If selected streams and actived streams are matched, do reconfigure of playsink again with pad-removed signal
Created attachment 341316 [details] [review] decodebin3: Send custom-eos event to notify drained state Likewise how urisourcebin is doing, use custom event if other streams are still alive.
Created attachment 341317 [details] [review] decodebin3: Cleanup no more used MultiQueueSlot Since MultiQueueSlot cannot be removed inside of streaming thread, use gst_element_call_async() API.
Created attachment 341318 [details] [review] decodebin3: Cleanup no more used DecodebinInput Remove DecodebinInput using gst_element_call_async() API.
Created attachment 341319 [details] [review] decodebin3: Update stream-collection with _input_pad_unlink() Since parsebin does not post new stream-collection message when it was being removed, decodebin3 should update it itself.
Created attachment 341320 [details] [review] decodebin3: Drop duration query during _input_pad_unlink () Playbin3 takes lock when querying duration and handling stream-collection message. So,to post stream-collection message, duration query should be dropped when input pad is being unlinked.
Created attachment 341321 [details] [review] decodebin3: More cleanup DecodebinOutputStream and MultiQueueSlot When removing DecodebinInputStream, cleanup DecodebinOutputStream and MultiQueueSlot also if they were drained.
Created attachment 341322 [details] [review] decodebin3: Remove unused variable
Dear all I split the last patch for easy review. Not only the initially reported mpd http://dash.edgesuite.net/dash264/TestCases/5b/nomor/3.mpd, switching "no-audio-trick" mode with any mpd causes problem with playbin3. It's due to un-removed input/output/multiqueue slot.
Thanks for those, sorry for the delay in reviewing them. They are all pushed now commit 49653b058a2a4b2093f5ce34f43d567eba51f76b Author: Seungha Yang <sh.yang@lge.com> Date: Sat Dec 3 23:01:53 2016 +0900 decodebin3: Remove unused variable https://bugzilla.gnome.org/show_bug.cgi?id=773341 commit 582c7cef18e1ed3fab1749b7b2f0180e7f259526 Author: Seungha Yang <sh.yang@lge.com> Date: Sat Dec 3 22:46:20 2016 +0900 decodebin3: More cleanup DecodebinOutputStream and MultiQueueSlot When removing DecodebinInputStream, cleanup DecodebinOutputStream and MultiQueueSlot also if they were drained. https://bugzilla.gnome.org/show_bug.cgi?id=773341 commit 6bd7a5602c52150ae163b62b2dbfa86cb1cce57e Author: Seungha Yang <sh.yang@lge.com> Date: Sat Dec 3 22:37:55 2016 +0900 decodebin3: Drop duration query during _input_pad_unlink () Playbin3 takes lock when querying duration and handling stream-collection message. So,to post stream-collection message, duration query should be dropped when input pad is being unlinked. https://bugzilla.gnome.org/show_bug.cgi?id=773341 commit 7e764058957b409200b08b4b5ec8d44cb9c18767 Author: Seungha Yang <sh.yang@lge.com> Date: Sat Dec 3 22:12:21 2016 +0900 decodebin3: Update stream-collection with _input_pad_unlink() Since parsebin does not post new stream-collection message when it was being removed, decodebin3 should update it itself. https://bugzilla.gnome.org/show_bug.cgi?id=773341 commit f08e4592a47869c0f86a8f99762b961fddad4864 Author: Seungha Yang <sh.yang@lge.com> Date: Sat Dec 3 22:28:28 2016 +0900 decodebin3: Cleanup no more used DecodebinInput Remove DecodebinInput using gst_element_call_async() API. https://bugzilla.gnome.org/show_bug.cgi?id=773341 commit bd6ec225b992340bb7535ec65726c565a0a137a1 Author: Seungha Yang <sh.yang@lge.com> Date: Sat Dec 3 21:50:47 2016 +0900 decodebin3: Cleanup no more used MultiQueueSlot Since MultiQueueSlot cannot be removed inside of streaming thread, use gst_element_call_async() API. https://bugzilla.gnome.org/show_bug.cgi?id=773341 commit 4fcbcf4e4880717fc3cb4099fdfa634c26c5b977 Author: Seungha Yang <sh.yang@lge.com> Date: Sat Dec 3 21:42:30 2016 +0900 decodebin3: Send custom-eos event to notify drained state Likewise how urisourcebin is doing, use custom event if other streams are still alive. https://bugzilla.gnome.org/show_bug.cgi?id=773341 commit f4d6aa71672f8f29fe2800ffb1473a7f16f823d9 Author: Seungha Yang <sh.yang@lge.com> Date: Sat Dec 3 20:44:21 2016 +0900 playbin3: Reconfigure playsink again with pad-removed If selected streams and actived streams are matched, do reconfigure of playsink again with pad-removed signal https://bugzilla.gnome.org/show_bug.cgi?id=773341 commit 858ee3d9138287c9634a48954a77b8907ce15cc8 Author: Seungha Yang <sh.yang@lge.com> Date: Tue Oct 25 21:06:40 2016 +0900 playback: Remove trailing whitespace https://bugzilla.gnome.org/show_bug.cgi?id=773341 commit cbd4bcd7a48b4e3cd53a594c55c68cc1b1db167c Author: Seungha Yang <sh.yang@lge.com> Date: Sun Oct 23 22:10:39 2016 +0900 urisourcebin: Try to link output slot before cleanup Before cleaning up output slot, check pending pads first, if available. Then, cleanup it only if linking was failed. https://bugzilla.gnome.org/show_bug.cgi?id=773341 commit 5760bd4543bfb86c194ed6846652f569dbc78cdb Author: Seungha Yang <sh.yang@lge.com> Date: Sat Oct 22 18:53:17 2016 +0900 urisourcebin: Cleanup unused output slot Since urisourcebin cannot cleanup unused output slot in streaming thread, it will be handled in thread pool with gst_element_call_async (). https://bugzilla.gnome.org/show_bug.cgi?id=773341