GNOME Bugzilla – Bug 730960
tsdemux: allow changing program in playing
Last modified: 2018-05-06 12:28:26 UTC
The attached patches resolve a FIXME for allowing changing the program-number in tsdemux without requiring the element to go to null and back to playing.
Created attachment 277487 [details] [review] tsbase: store the program in the tsbasestream When switching streams it makes it easier to refer to the correct values if you can refer directly from the the stream to the program as the demuxer now can have 2 active programs at the same time while the switch is happening
Created attachment 277488 [details] [review] tsdemux: refactor a bit to make pads reusable Refactor some common initialization functions for pads and their data and also for cleanup to make it easier to reuse pads when adding the feature of switching program-number while in playing
Created attachment 277489 [details] [review] tsdemux: allow program-number switching in playing Use pad groups switching to add new pads for the newly selected program when then remove the old ones. Pads are kept around in case the user selects the same program again in the future.
Created attachment 277491 [details] [review] tsdemux: allow program-number switching in playing Minor update to logging
Created attachment 277492 [details] sample application Sample app to allow switching program-number in playing
Review of attachment 277487 [details] [review]: While it looks correct, I'm a bit worried about having the program shared accross so many structures. Wondering if a trivial refcounting system for programs wouldn't be needed, to ensure we only free it once nobody (whether element or pads) need it anymore
(In reply to comment #6) > Review of attachment 277487 [details] [review]: > > While it looks correct, I'm a bit worried about having the program shared > accross so many structures. Wondering if a trivial refcounting system for > programs wouldn't be needed, to ensure we only free it once nobody (whether > element or pads) need it anymore It is harmless to add that, but the programs are only freed after a call do 'deactivate_program'. but you made me realize another issue: what would happen if we are doing a program switch and the current program (still not fully switched) gets deactivated? If no pads have been added yet it will EOS prematurely. If we prevent pads from being removed from a switching program until the new program is ready (no-more-pads) we can also hang if the user selects a program that isn't available. But in this later case it would be the user's fault. What do you think?
Created attachment 278770 [details] [review] tsbase: store the program in the tsbasestream When switching streams it makes it easier to refer to the correct values if you can refer directly from the the stream to the program as the demuxer now can have 2 active programs at the same time while the switch is happening
Created attachment 278771 [details] [review] tsdemux: refactor a bit to make pads reusable Refactor some common initialization functions for pads and their data and also for cleanup to make it easier to reuse pads when adding the feature of switching program-number while in playing
Created attachment 278772 [details] [review] tsdemux: allow program-number switching in playing Use pad groups switching to add new pads for the newly selected program when then remove the old ones. Pads are kept around in case the user selects the same program again in the future. To be able to properly switch the program's exposing state is stored with the programs. The current exposed program is moved to old_program while the newly selected hasn't all its pads exposed. After that the old_program is removed
New versions of the patches that have a more robust implementation. The only failure I had so far is when switching programs so fast that h264parse didn't receive any SPS/PPS and errors out as it couldn't produce any output.
*** Bug 734394 has been marked as a duplicate of this bug. ***
Anyone would like to review this?
Review of attachment 278771 [details] [review]: Seems fine, with one typo you should fix before pushing: ::: gst/mpegtsdemux/tsdemux.c @@ +1010,3 @@ +static void +gst_ts_demux_stream_set_start_events (GstTSDemux * demux, gst_ts_demux_stream_set_start_events -> gst_ts_demux_stream_send_start_events ?
Review of attachment 278770 [details] [review]: The only problem with this is that there's nothing preventing you from having a stream belong to multiple programs - for example a set of programs that present the same video stream with a different audio track
Review of attachment 278772 [details] [review]: Again, track switching here won't work properly in the case where the new program shares streams with the old program - pads that belong to the new program will be removed.
I have tried this patchset on a local multi TS file containing 4 programs (unfortunately it's a huge 500MB file that I cannot easily share): - Contains 4 programs 1/2/3/4 - Programs are sharing some stream pids, for ex: 0x01ec, 0x1e2 (audio), so pad name unicity is needed to avoid to insert a pad with same name when switching programs (the point raised by Jan Schmidt) - Full sections details are given in README.txt in attached tar.gz (tsparser used to get DVB sections) In order to play this file and change program, I made improvements to gst-play by adding keys '+' and '-' to walkthrough programs 1/2/3/4 (patches provided in tar.gz). To overcome the problem of pad unicity I made a patch to ensure unicity of pad name adding the PMT pid to stream PID, cf 0004-tsdemux-name-pad-with-PMT-pid-for-pad-unicity.patch attached. I also noticed some problems around the fact that several audio tracks are there or subtitles are there => switch is not happening well in this case (hang), I couldn't really understand why, I miss some expertise to debug dynamic switch of pipeline. Anyway, by ensuring that only one video track pad and one audio track pad are created, switch occurs. But there are still some problems time to time when zapping fast (we ask for a new switch while the previous not completed): ERROR No valid frames found before end of stream for file:///Cornell_MPEG2_HD-SD_H264_HD-SD_PAL_QAM_474MHz_v3.trp ERROR debug information: sources/gstreamer/libs/gst/base/gstbaseparse.c(1155): gst_base_parse_sink_event_default (): /GstPlayBin:playbin/GstURIDecodeBin:uridecodebin0/GstDecodeBin:decodebin0/GstMpegvParse:mpegvparse2 I would like to know if some progress was made on those patchsets and what are your advices to move forward on this topic.
Created attachment 290836 [details] [review] Add PMT pid to pad name to ensure unicity
Created attachment 290837 [details] single video/audio hack + gst-play changes for program switch
Created attachment 290838 [details] .png pipelines with multi TS file after switch OK and when hang occurs
A quick reminder for whenever this comes back to life, remember that you can't easily re-use a pad. If you want to do it, after removing the pad from the element, you must unset the NEED_PARENT flag, but first, you need to make every pad function (chain, event, query, etc) verify if the parent is not NULL.
What's the status here?
On my side, I have abandonned the approach of dynamic reconfiguration. Instead I have used a "one playbin per channel" approach instead inside a GStreamer application. As my objective was TV playback, I only focused on DVB live, not local file containing several channels, but same strategy could be applied for this case: Each playbin is independent and use its own dvbsrc and its own audio/video sink: - DVB driver manages frontend resource (one dvbsrc per channel). - Wayland/Weston manages display resource sharing (one waylandsink per channel). - Alsa mixer (dmix) manages audio resource sharing (one alsasink per channel). This is far easier than trying to manage sources and sinks within GStreamer (either using input/output selector or dynamic linking). Zap between channels is done using cached playbin strategy to save frontend tuning, DVB tables parsing and graph negotiation/establishment.
A few other things need to be fixed before this can work properly. For example, a newly selected program can reuse the same streams PIDs causing pad names to clash. I can resume working on this after 1.6 if we want it to work with the current decodebin2 mechanisms.
Is this still relevant with the playbin3 / GstStreams related changes that landed in GIT master?
Closing this bug report as no further information has been provided. Please feel free to reopen this bug report if you can provide the information that was asked for in a previous comment. Thanks!