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 374639 - GstTee doesn't handle pad list resyncs properly
GstTee doesn't handle pad list resyncs properly
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other All
: Normal normal
: 0.10.14
Assigned To: Wim Taymans
GStreamer Maintainers
Depends on:
Reported: 2006-11-13 11:12 UTC by Philippe Khalaf
Modified: 2007-07-03 16:26 UTC
See Also:
GNOME target: ---
GNOME version: ---

possible patch (10.43 KB, patch)
2007-07-02 18:08 UTC, Wim Taymans
committed Details | Review

Description Philippe Khalaf 2006-11-13 11:12:42 UTC
Please describe the problem:
If we are dynamically requesting pads on a tee while the pipeline is playing, it might cause the pad list iteration to return GST_ITERATOR_RESYNC. In that case the iterations will stop before going through all the pads. This means that the return value aggregation will only include the pads up until the resync happened. The tee would therefore return the wrong gst flow message since the remaining pads could be successfull.

Steps to reproduce:
1. Link a few elements to a tee
2. Play the pipeline
3. Unlink some of the elements
4. Link new elements to the tee while it is playing.
Bug reproducability depends on timing (will appear if the link happens during the iteration after the unlinked pad is passed and no other successfull push happens)

Actual results:
tee returns an internal data flow error (-1 - unlinked) because none of the pads previous to the resync have successfully pushed.

Expected results:
tee should be able to deal with new pads being added dynamically. On resyncs, it should be able to try the new pad as well as the remaining pads, or at least only the remaining pads, and then return the proper gst flow message.

Does this happen every time?
Yes if appropriate conditions are met.

Other information:
Comment 1 Stefan Sauer (gstreamer, gtkdoc dev) 2006-11-13 12:08:44 UTC
Yes, then GST_ITERATOR_RESYNC should be handled.

For pad-additions a possible fix would be to add all pads a buffer has been sent into a list and clear the list after a succesful iteration. On resync those pads that are already in the list have to be skipped.

For pad-removal care must be taken that the pad returned by the iterator can not be removed until the next iterator call or the end of iterator usage. That might pose some restrictions onto what one can do with pads while iterating.
Comment 2 Wim Taymans 2006-11-13 18:08:02 UTC
a flag for each pad would be acceptable to handle the resyncs. care to make a patch?

adder has some code to handle adding/removing pads while paused/playing.
Comment 3 Stefan Sauer (gstreamer, gtkdoc dev) 2006-11-24 09:37:02 UTC
I could not find any of such code in adder or in collectpads.

I think it should be like this
  remember pads that we went through already and do not push onto them again,
  loop on resync
  can we attach some flags on pads (g_object_set_qdata) or do we add
  a list to PushData

For this I don't yet have a solution.
  this is critical, if pad_push() in the IteratorFoldFunction goes on a pad
  that is going to be removed. The problem is the race between the point
  when the iterator returns the pad and the earliest point in the
  IteratorFoldFunction (where we could lock the pad)
Comment 4 David Schleef 2007-05-13 02:19:02 UTC

Could you write a test case that we can put into the test suite?  (I'm hoping you already have code that triggers this.)
Comment 5 Wim Taymans 2007-07-02 18:08:20 UTC
Created attachment 91047 [details] [review]
possible patch

This patch keeps track of the pads that have been pushed on. It also tries to find a pad to proxy the padalloc on. It should theoretically work when adding/removing pads while playing but that is not tested.

can somebody test this?
Comment 6 Wim Taymans 2007-07-03 16:26:40 UTC
        * plugins/elements/gsttee.c: (gst_tee_base_init),
        (gst_tee_request_new_pad), (gst_tee_release_pad),
        (gst_tee_find_buffer_alloc), (gst_tee_buffer_alloc),
        (gst_tee_do_push), (clear_pads), (gst_tee_handle_buffer),
        Be a lot smarter when deciding what srcpad to use for proxying 
        the buffer_alloc. Also handle pad added/removed when doing so.
        Fixes #357959.
        Keep track of what pads we already pushed on in case we have pads
        added/removed while pushing. Fixes #374639 

        * tests/check/
        * tests/check/elements/tee.c: (handoff), (GST_START_TEST),
        Added unit test for pad resync.