GNOME Bugzilla – Bug 706441
adder: Rework/review the way we handle flushing
Last modified: 2014-11-16 17:00:19 UTC
We have been quite a few issue with the current implementation of the flush handling in videomixer (most of those also apply to adder btw) and we should review it and rework it once for all instead of keeping adding stuff around to fix issues as they come if needed. Currently we have the following code: * When getting a flushing seek, we set flush_stop_pending=TRUE which means that the next time collectpad->collected is called, we send a flush_stop ourself before pushing a buffer. * When we get a flush_start, we set waiting_flush_stop and we forward the flush_stop once and set that variable back to FALSE. Also there is a bug where we send 2 times flush_stop Mathieu Duponchelle has a patch for it.
Created attachment 252471 [details] [review] fixes quite a few issue we have been
I think this should work the following way: 1) flush-start/flush-stop received from upstream without a pending seek: Reset the GstVideoMixer2Pad segment on flush-stop and don't forward anything downstream. Never. 2) flush-start/flush-stop received from downstream: Reset on flush-stop and forward upstream 3) flushing seek: Forward to all upstreams, don't send flush-start/stop ourselves downstream. Forward the *first* flush-start and flush-stop that happens because of the seek, drop all others. Reset the GstVideoMixer2Pad segment when receiving the flush-stops (all of them for the corresponding pad).
Also see bug #706779, same issue in oggmux.
Comment on attachment 252471 [details] [review] fixes quite a few issue we have been This was committed already
(In reply to comment #2) > I think this should work the following way: > > > 3) flushing seek: Forward to all upstreams, don't send flush-start/stop > ourselves downstream. Forward the *first* flush-start and flush-stop that > happens because of the seek, drop all others. Reset the GstVideoMixer2Pad > segment when receiving the flush-stops (all of them for the corresponding pad). Isn't this racy if you have two upstreams where the 1st flushes, sending flush start/stop and the 2nd still sends some data belonging to the old segment after the 1st has sent flush_stop (and flush_stop has been propagated downstream)?
It is, new solution that shouldn't be racy is in bug #706779
Created attachment 254746 [details] [review] adder: Rework the way we handle flushes and flushing seeks
OK, this one was about adder and is not finnished, but I wanted to check if it corresponds to what you have in mind? Also there are still some races in that patch, it is just a first version and I wanted to see if you have ideas about what is wrong etc... with that basis.
Created attachment 255147 [details] [review] adder: Rework the way we handle flushes and flushing seeks The new strategy is that we forward dowstream flush_stop only once and that after seeking. We now always forward flush start and stop comming from dowstream to all our upstream pads We flush ourself right when receiving a flushing seek and send flush_start downstream to make sue the collectpad is flushing and we can send our seek events without fearing to get collected call meanwhile.
Comment on attachment 252471 [details] [review] fixes quite a few issue we have been Merged
Review of attachment 255147 [details] [review]: ::: tests/check/elements/adder.c @@ +1235,3 @@ adder_src = gst_element_get_static_pad (adder, "src"); fail_if (GST_PAD_IS_FLUSHING (adder_src)); + gst_pad_send_event (adder_src, gst_event_new_flush_start ()); you're changing the nature of this test with this. Shouldn't you add a different unit test ?
(In reply to comment #11) > Review of attachment 255147 [details] [review]: > > ::: tests/check/elements/adder.c > @@ +1235,3 @@ > adder_src = gst_element_get_static_pad (adder, "src"); > fail_if (GST_PAD_IS_FLUSHING (adder_src)); > + gst_pad_send_event (adder_src, gst_event_new_flush_start ()); > > you're changing the nature of this test with this. I actually changed the behaviour of Adder concerning flush event comming from upstream, as now, I do not foward dowstream them unless they are coming from the result of a flushing seek. That test doesn't make sense anymore in that new context. > Shouldn't you add a different unit test ?
What if the seek event went through another branch ? You're not guaranteed you'll see the seek event. tee -- adder1 -- sink \----adder2 -- sink
Btw, this patch should no be merged anyway as this is now implemented in collectpads directly.
(In reply to comment #13) > What if the seek event went through another branch ? You're not guaranteed > you'll see the seek event. > > > > tee -- adder1 -- sink > \----adder2 -- sink In that particular case, the flush should no be forwarded on the branch where the seek did not happen I would say. If we seek on the adder2 branch, the flushes should be only forwarded to adder2 not adder 1.
I disagree, if you would do it that way the non-flushed branches of tee would get wrong running times.
Should that bug simply be closed as OBSELETE now that we have audiomixer?
WONTFIX or OBSOLETE seems best to me. I think it's difficult to change adder anyway because of the way it handles (or didn't handle) timestamps etc.
Wontfix it is.