GNOME Bugzilla – Bug 374639
GstTee doesn't handle pad list resyncs properly
Last modified: 2007-07-03 16:26:40 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:
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.
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.
I could not find any of such code in adder or in collectpads. I think it should be like this pad-addition: 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. pad-removal: 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)
Phillipe: Could you write a test case that we can put into the test suite? (I'm hoping you already have code that triggers this.)
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?
* 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), (gst_tee_chain): 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/Makefile.am: * tests/check/elements/tee.c: (handoff), (GST_START_TEST), (tee_suite): Added unit test for pad resync.