After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 750402 - tsdemux: pads are removed before new ones are added
tsdemux: pads are removed before new ones are added
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal normal
: 1.7.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 730960
 
 
Reported: 2015-06-04 15:03 UTC by Thiago Sousa Santos
Modified: 2015-12-02 16:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
mpegtsbase: make mpegtsstream refcounted (2.86 KB, patch)
2015-06-04 15:03 UTC, Thiago Sousa Santos
none Details | Review
tsdemux: Only remove pads after adding new ones (4.72 KB, patch)
2015-06-04 15:03 UTC, Thiago Sousa Santos
none Details | Review
tsdemux: add a pads_number to avoid clashes in pad names (2.85 KB, patch)
2015-06-04 19:17 UTC, Thiago Sousa Santos
none Details | Review
tsdemux: Only remove pads after adding new ones (4.76 KB, patch)
2015-06-04 21:16 UTC, Thiago Sousa Santos
none Details | Review
tsdemux: rename pads before adding new ones to prevent clashes (2.10 KB, patch)
2015-06-05 14:16 UTC, Thiago Sousa Santos
none Details | Review
mpegtsdemux: Allow deactivation of programs to be delayed (9.26 KB, patch)
2015-11-09 17:17 UTC, Thiago Sousa Santos
none Details | Review
tsdemux: Make sure old streams are drained before switching (1.86 KB, patch)
2015-11-09 17:17 UTC, Thiago Sousa Santos
none Details | Review
tsdemux: Push GAP events *after* deactivating old programs (3.47 KB, patch)
2015-11-09 17:18 UTC, Thiago Sousa Santos
none Details | Review
tsdemux: rename pads before adding new ones to prevent clashes (1.78 KB, patch)
2015-11-09 17:18 UTC, Thiago Sousa Santos
none Details | Review

Description Thiago Sousa Santos 2015-06-04 15:03:02 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
Comment 1 Thiago Sousa Santos 2015-06-04 15:03:21 UTC
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
Comment 2 Thiago Sousa Santos 2015-06-04 15:03:27 UTC
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
Comment 3 Thiago Sousa Santos 2015-06-04 15:19:58 UTC
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?
Comment 4 Thiago Sousa Santos 2015-06-04 19:17:54 UTC
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?
Comment 5 Thiago Sousa Santos 2015-06-04 21:16:45 UTC
Created attachment 304618 [details] [review]
tsdemux: Only remove pads after adding new ones

Did a minor fix to the patch to avoid an assertion
Comment 6 Thiago Sousa Santos 2015-06-05 14:16:51 UTC
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.
Comment 7 Thiago Sousa Santos 2015-11-09 17:17:17 UTC
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.
Comment 8 Thiago Sousa Santos 2015-11-09 17:17:37 UTC
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
Comment 9 Thiago Sousa Santos 2015-11-09 17:18:16 UTC
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.
Comment 10 Thiago Sousa Santos 2015-11-09 17:18:27 UTC
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.
Comment 11 Edward Hervey 2015-12-02 16:07:59 UTC
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