GNOME Bugzilla – Bug 729198
oggdemux: add non flushing time seeking to 0 in push mode
Last modified: 2015-03-11 14:50:40 UTC
Created attachment 275428 [details] [review] oggdemux: add non flushing time seeking to 0 in push mode Open for comments: Two main changes: - some resetting code has to be done in the NEW_SEGMENT event handler, instead of the missing FLUSH_STOP one - if we're going to 0, we bypass the bisection and seek straight to the start of the file This allows using non flushing segment seeks for looping HTML audio. Seeking to other than 0 is not supported as this would require going through the push mode bisection FSM. Maybe as a future improvement.
Created attachment 290500 [details] [review] updated to allow any target, not just start
I originally made this updated patch a while back, and while it seemed to be working, I was getting deadlocks relating to queues when testing with -base/tests/examples/playback/playback-test, maybe on a third of seeks. Testing now, however, I am unable to reproduce these deadlocks.
Created attachment 296911 [details] [review] fixes the downstream hangs This, along with another patch which I will llink shortly, fixes all the hangs AFAICT.
Created attachment 297639 [details] [review] try harder to query duration from upstream Fixed while testing push mode non flushing seeks with souphttpsrc and gst-validate
Created attachment 297640 [details] [review] set correct seqnum on new segments after a seek in push mode Fixed while testing push mode non flushing seeks with souphttpsrc and gst-validate
I'll push those in a few days if there are no comments.
Comment on attachment 297639 [details] [review] try harder to query duration from upstream This looks weird to me. You can't really assume that this is 'after preroll', because there might be live elements in the pipeline. Perhaps this should be moved somewhere into the chain function instead to be run on the first buffer received? There's also a gst_pad_peer_query_duration() fwiw.
Would an upstream query in a chainfunction be safe, locking wise ?
It should be. Everything else would be a bug in upstream elements IMO.
Created attachment 298446 [details] [review] try harder to query duration from upstream
There's some kind of race condition with HTTP that I found (unrelated to that last patch) so there'll be more patches to come too :/ Everything works fine from file://.
file:/// usually goes into pull mode, not push mode. What about pushfile:///?
Comment on attachment 298446 [details] [review] try harder to query duration from upstream Why not use the SEEKING query here?
Sorry, I meant pushfile, yes. We could use a seeking query I guess. I need the duration because I will seek to some offset before the duration in bytes in order to scan for a granpos.
Maybe both then? You can have a duration on unseekable streams :)
Seeking gets disabled if it fails to seek. But sure, I'll add a call to seeking query too.
Created attachment 298520 [details] [review] try harder to query duration from upstream With seeking query too. That leaves just the deadlock with http, which looks a lot like what I thought had been fixed by the other segment related patches :(
Created attachment 298654 [details] [review] do not send seek events from the streaming thread That takes care of that deadlock.
I'm going to push those soon, unless someone has an objection or further review comments about the patches.
Created attachment 299012 [details] [review] do not send seek events from the streaming thread Update to fix a memory corruption bug when destroying the cond var.
Created attachment 299103 [details] [review] do not send seek events from the streaming thread Updated again to fix a deadlock in unit tests on very fast state changes.
All comments addressed, remaining issues gone, make check passes. In it goes. commit e77b60a73f7a879e67d6f8c6ca69479118eb87a8 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Thu Mar 5 17:42:53 2015 +0000 oggdemux: do not send seek events from the streaming thread This will usually deadlock, despite this patch being in master for quite some time and working fine. Nevertheless, we deem it to be not working, disregarding facts. As such, we fix it by keeping track of seek events, and sending them upstream from a separate thread. Buffers are then discarded till we get a new segment with the expected seqnum. commit 5f3fc5db058b3014d966944892cf0cef4656e859 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Feb 23 13:07:41 2015 +0000 oggdemux: set correct seqnum on segment events after a seek in push mode There is already a seqnum field for this, which was used to overwrite the seqnum that was set by the push specific code. commit acaf534714e380886ffc2f17b874513dd6ddafdc Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Feb 23 11:30:36 2015 +0000 oggdemux: try harder to query duration from upstream READY->PAUSED can be too early as souphttpsrc can get the HTTP headers after this. Try again in the chain function. Also use seeking query to disable seeking if upstream reports being unseekable. commit 969cf47a82823d71f53bb576048e5d228926c783 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Fri Oct 31 10:55:14 2014 +0000 oggdemux: add non flushing time seeking in push mode Some resetting code has to be done in the NEW_SEGMENT event handler, instead of the missing FLUSH_STOP one. Segment base was also wrongly accounted for. This was hidden by the fact that flushing resets the base. A discontinuity is now also signalled on seeking. We have to also ensure that the discontinuity "sticks" till a buffer with a valid timestamp goes out, or the audio decoder base class will ignore the discontinuity for purposes of keeping track of the current time. This allows using non flushing segment seeks for looping HTML audio in particular, and more generally non flushing seeks. https://bugzilla.gnome.org/show_bug.cgi?id=729198