GNOME Bugzilla – Bug 750402
tsdemux: pads are removed before new ones are added
Last modified: 2015-12-02 16:08:16 UTC
When doing pad switching (program change), tsdemux will remove the current pads before adding the new ones and that might cause the pipeline to go EOS before the new pads are exposed
Created attachment 304589 [details] [review] mpegtsbase: make mpegtsstream refcounted Allows subclass to maintain their own reference if they need it for longer than the program active period
Created attachment 304590 [details] [review] tsdemux: Only remove pads after adding new ones Otherwise the EOS from old pads could reach sinks before the new pads are exposed and it will cause the pipeline to go EOS too early
There is still an issue here that is of new pads using the same PIDs of previous ones. This means that the new pads won't be added to the element. Is there any possible solution for this? Is changing the way the pads are named allowed at this point?
Created attachment 304610 [details] [review] tsdemux: add a pads_number to avoid clashes in pad names This patch adds a counter that will ensure pad names are unique but this might break applications that read the PID from the pad names and I'm unsure if there would be a feasible solution. Can we change the pad names once they are added to the element? Perhaps we could rename the pads just before they are to be removed to avoid clashing with the new ones to be added?
Created attachment 304618 [details] [review] tsdemux: Only remove pads after adding new ones Did a minor fix to the patch to avoid an assertion
Created attachment 304652 [details] [review] tsdemux: rename pads before adding new ones to prevent clashes Pads are named after the PIDs so when adding a new group of pads it might happen that the old pads and new pads name clash if the same PIDs are used by the new stream. This patch renames the old pads by appending '_stopping' to their names to prevent this clash.
Created attachment 315136 [details] [review] mpegtsdemux: Allow deactivation of programs to be delayed When changing programs, the order of events needs to be the following: * add pads from new program * send EOS on old pads * remove old pads * emit 'no-more-pads' Previously tsdemux was not doing that, and was first deactivating and removing old pads before adding new ones. We fix this by allowing subclasses of mpegtsbase to be able to handle themselves the deactivation of programs. In this case tsdemux will properly deactivate it once it has activated the new program.
Created attachment 315137 [details] [review] tsdemux: Make sure old streams are drained before switching Before we add any streams, make sure we drain all streams. This ensures there's consistency that only "new" data will be pushed on buffers once the new pads are added
Created attachment 315138 [details] [review] tsdemux: Push GAP events *after* deactivating old programs The order in which program switch must happen is: 1) drain all data on old pads (but don't push EOS) 2) add new pads (but don't push any data on them) 3) Push EOS and remove old pads 4) Start pushing data on new pads There was one caveat in this implementation, which is that when we activate a sparse pad (step 2) we would push a GAP event. The problem is that, while being an event, it is actually *data*. We therefore need to make sure pushing those GAP event is done at the step we start pushing data.
Created attachment 315139 [details] [review] tsdemux: rename pads before adding new ones to prevent clashes Pads are named after the PIDs so when adding a new group of pads it might happen that the old pads and new pads name clash if the same PIDs are used by the new stream. This patch renames the old pads by appending '_stopping' to their names to prevent this clash.
commit 531117b7df1002f816f2a4e3259fdcdfa470cb72 Author: Edward Hervey <edward@centricular.com> Date: Tue Oct 20 17:22:23 2015 +0200 tsdemux: Push GAP events *after* deactivating old programs The order in which program switch must happen is: 1) drain all data on old pads (but don't push EOS) 2) add new pads (but don't push any data on them) 3) Push EOS and remove old pads 4) Start pushing data on new pads There was one caveat in this implementation, which is that when we activate a sparse pad (step 2) we would push a GAP event. The problem is that, while being an event, it is actually *data*. We therefore need to make sure pushing those GAP event is done at the step we start pushing data. https://bugzilla.gnome.org/show_bug.cgi?id=750402 commit 7336294635d6066c812ca918e799a5ed21f177bb Author: Edward Hervey <edward@centricular.com> Date: Tue Sep 15 18:20:38 2015 +0200 tsdemux: Make sure old streams are drained before switching Before we add any streams, make sure we drain all streams. This ensures there's consistency that only "new" data will be pushed on buffers once the new pads are added https://bugzilla.gnome.org/show_bug.cgi?id=750402 commit 14e6d2d42736a66cd83b08113f4a067943443b11 Author: Edward Hervey <bilboed@bilboed.com> Date: Thu Sep 10 14:55:05 2015 +0200 mpegtsdemux: Allow deactivation of programs to be delayed When changing programs, the order of events needs to be the following: * add pads from new program * send EOS on old pads * remove old pads * emit 'no-more-pads' Previously tsdemux was not doing that, and was first deactivating and removing old pads before adding new ones. We fix this by allowing subclasses of mpegtsbase to be able to handle themselves the deactivation of programs. In this case tsdemux will properly deactivate it once it has activated the new program. https://bugzilla.gnome.org/show_bug.cgi?id=750402