GNOME Bugzilla – Bug 707530
qtdemux: Handle segments correctly in push mode seeks
Last modified: 2013-09-18 16:51:17 UTC
Currently qtdemux fails at using the stop position of seeks in push mode. Any application that seeks with stop position set and reads from a pushfile:// uri should be enough for testing. To allow qtdemux to handle seeks in push mode correctly, it should store the seqnum of the seek and send a converted version in bytes for upstream. When receiving the segment for this seek from usptream, it should check the seqnum and use the values of the original seek as the time segment it is going to use internally.
Created attachment 254149 [details] [review] basesrc: preserve seqnum on segments after seeks The seqnum of the segment after a seek should be the same of the seek event. Downstream elements might rely on seqnums to identify events related to a seek. This is particularly important when a demuxer maps a TIME seek into a BYTES seek for upstream and it needs to identify the corresponding segment event and map it back into TIME to push downstream, possibly using the values from the original seek event.
Created attachment 254150 [details] [review] qtdemux: preserve stop of segment when doing seeks in push mode When handling seeks in push mode, qtdemux converts the seek to bytes and pushes upstream. It needs to keep track of the seek and the subsequent segment to be able to map them back to the requested seek time and properly preserve the segment stop of the seek. This is done by using the seqnum of the seek, that should be the same of the segment from upstream.
Created attachment 254151 [details] [review] qtdemux: track streams that are EOS on push mode to finish earlier When the segment has a defined stop position, qtdemux should check when streams reach this position and mark those as EOS. When all streams are EOS it will return GST_FLOW_EOS to upstream to allow the pipeline to finish instead of continuously consume buffers from upstream that are not useful for the segment.
Review of attachment 254149 [details] [review]: ::: libs/gst/base/gstbasesrc.c @@ +821,3 @@ /* Mark pending segment. Will be sent before next data */ src->priv->segment_pending = TRUE; + src->priv->segment_seqnum = 0; I think you should initialize this with gst_seqnum_next() and also call that in ::start() @@ +2678,3 @@ + GstEvent *seg_event = gst_event_new_segment (&src->segment); + + if (src->priv->segment_seqnum) { And here just always set the seqnum
Review of attachment 254150 [details] [review]: Looks sensible but: ::: gst/isomp4/qtdemux.c @@ +1907,3 @@ + GST_DEBUG_OBJECT (demux, "Stored seek seqnum: %u, received segment " + "seqnum %u", demux->push_seek_seqnum, seqnum); + if (seqnum == demux->push_seek_seqnum) { Instead of comparing the seqnum it should already be enough just compare the start position
Review of attachment 254151 [details] [review]: ::: gst/isomp4/qtdemux.c @@ +4923,3 @@ + /* check for segment end */ + if (G_UNLIKELY (demux->segment.stop != -1 + && demux->segment.stop <= pts && stream->on_keyframe)) { Why the on_keyframe here? It should do that always, independent of the seek type
(In reply to comment #5) > Review of attachment 254150 [details] [review]: > > Looks sensible but: > > ::: gst/isomp4/qtdemux.c > @@ +1907,3 @@ > + GST_DEBUG_OBJECT (demux, "Stored seek seqnum: %u, received segment " > + "seqnum %u", demux->push_seek_seqnum, seqnum); > + if (seqnum == demux->push_seek_seqnum) { > > Instead of comparing the seqnum it should already be enough just compare the > start position Just to make sure I understood your comment: Store the seek start position (in bytes) and use that instead of the seqnum? Likely because other elements might not preserve the seqnum? (In reply to comment #6) > Review of attachment 254151 [details] [review]: > > ::: gst/isomp4/qtdemux.c > @@ +4923,3 @@ > + /* check for segment end */ > + if (G_UNLIKELY (demux->segment.stop != -1 > + && demux->segment.stop <= pts && stream->on_keyframe)) { > > Why the on_keyframe here? It should do that always, independent of the seek > type This 'on_keyframe' is not related to the seek flags, this is updated for every buffer pushed and allows qtdemux to only stop pushing a stream on keyframes, allowing decoders downstream to have all data needed to reach the stop position, too.
(In reply to comment #7) > (In reply to comment #5) > > Review of attachment 254150 [details] [review] [details]: > > > > Looks sensible but: > > > > ::: gst/isomp4/qtdemux.c > > @@ +1907,3 @@ > > + GST_DEBUG_OBJECT (demux, "Stored seek seqnum: %u, received segment " > > + "seqnum %u", demux->push_seek_seqnum, seqnum); > > + if (seqnum == demux->push_seek_seqnum) { > > > > Instead of comparing the seqnum it should already be enough just compare the > > start position > > Just to make sure I understood your comment: Store the seek start position (in > bytes) and use that instead of the seqnum? Likely because other elements might > not preserve the seqnum? Yes, and also because you don't care if it's the same seqnum or not. You only really care about the start position. > (In reply to comment #6) > > Review of attachment 254151 [details] [review] [details]: > > > > ::: gst/isomp4/qtdemux.c > > @@ +4923,3 @@ > > + /* check for segment end */ > > + if (G_UNLIKELY (demux->segment.stop != -1 > > + && demux->segment.stop <= pts && stream->on_keyframe)) { > > > > Why the on_keyframe here? It should do that always, independent of the seek > > type > > This 'on_keyframe' is not related to the seek flags, this is updated for every > buffer pushed and allows qtdemux to only stop pushing a stream on keyframes, > allowing decoders downstream to have all data needed to reach the stop > position, too. Everything good then :)
Comment on attachment 254149 [details] [review] basesrc: preserve seqnum on segments after seeks Updated and pushed as 3dc8ee97e50039c73fc322f687741a5b7193b750
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #5) > > > Review of attachment 254150 [details] [review] [details] [details]: > > > > > > Looks sensible but: > > > > > > ::: gst/isomp4/qtdemux.c > > > @@ +1907,3 @@ > > > + GST_DEBUG_OBJECT (demux, "Stored seek seqnum: %u, received segment " > > > + "seqnum %u", demux->push_seek_seqnum, seqnum); > > > + if (seqnum == demux->push_seek_seqnum) { > > > > > > Instead of comparing the seqnum it should already be enough just compare the > > > start position > > > > Just to make sure I understood your comment: Store the seek start position (in > > bytes) and use that instead of the seqnum? Likely because other elements might > > not preserve the seqnum? > > Yes, and also because you don't care if it's the same seqnum or not. You only > really care about the start position. > For being absolutely sure the seek is the same, it would also be needed to compare the start and stop offsets in bytes. Given that most demuxers don't know how to convert the time into bytes for the stop position very well, I'd think it is simpler to rely on the seqnum of the events. It is usually very simple to fix events to use the same seqnum in case we find some that isn't doing it.
Sure, but why isn't just any new segment event with the right start position good enough for you?
Because it is also important to track the stop position, this information is currently being lost in most demuxers upstream
Note that *if* it worked before using the start position alone, then making it rely on sequence numbers might cause breakage if someone has a custom source that doesn't do that (but then I guess all base-src sources will automatically, so I suppose it's ok.)
(In reply to comment #12) > Because it is also important to track the stop position, this information is > currently being lost in most demuxers upstream Yes, so you keep the stop position from the seek and instead of comparing the seqnum you would just compare the seek start position :)
(In reply to comment #14) > (In reply to comment #12) > > Because it is also important to track the stop position, this information is > > currently being lost in most demuxers upstream > > Yes, so you keep the stop position from the seek and instead of comparing the > seqnum you would just compare the seek start position :) We need a plan to fix all demuxers regarding this. The only one that is correct is matroska that rejects seeks in push mode if stop != -1. All others simply set stop duration to -1 and ignore what was requested on seek. I propose the following plan: * Always store the start/stop/seqnum of the seek * If the demuxer already uses the start position to track the seek, use that to keep backwards compatibility, otherwise use the seqnum. Is there any reason not to use the seqnum other than backwards compatibility? * Make sure demuxers push buffers after stop position until they reach the next keyframe What do you think?
(In reply to comment #15) > * If the demuxer already uses the start position to track the seek, use that to > keep backwards compatibility, otherwise use the seqnum. Is there any reason not > to use the seqnum other than backwards compatibility? No, just that it's another field you need to store that seems redundant ;) Otherwise sounds like a good plan
There is some precision loss when the seek is converted from time to bytes for upstream, so we need to keep the original start and stop values in time instead of converting bytes to time back when receiving the Segment. And demuxers doesn't seem to convert stop times to bytes currently. The problem with using the bytes start to compare is that demuxers don't have the stop in bytes to make sure the seek is the same. Comparing just the start should be enough for most cases, but there might be a situation where it gets the wrong segment because the starts are the same, but the stops are different. Using the seqnum should uniquely identify the seek and segment events.
But do you even pass a stop position in bytes in the seek event? That doesn't sound like a good idea in general if you don't have an accurate bytes/time conversion. Anyway, the seqnums should make it unique... my point is just that the start position in bytes should do that too already, without having to carry around conditional code that sometimes uses the seqnum and sometimes not.
Pushed fixes, thanks for the review! commit be0eeae491eee25e80ef1116fb77ca8f9cb80842 commit 33cf8b679dee66afe927cb7e5da446563a11dec6
These two patches breaks seeking in Aurena The pattern is to preroll the pipeline, and seek to a certain position. We always get EOS instead of going to the right position. Aurena can be found and build from: https://github.com/thaytan/aurena to build: ./autogen.sh; make to run/reproduce: ./src/aurena-server &; ./src/aurena-simple-controller 1. Choose a file (in quicktime format) 2. Press next button 3. Press play button After that, you can see, or simply play/pause (wich cause a seek), and you'll get EOS with that patch.
Created attachment 254793 [details] [review] qtdemux: only update stop position if seek requests it Check for GST_SEEK_TYPE_NONE for stop poistion and only update the stop time if it is requested. Otherwise just maintain whatever was stored at the segment
Comment on attachment 254793 [details] [review] qtdemux: only update stop position if seek requests it start_type could be GST_SEEK_TYPE_NONE too btw
(In reply to comment #22) > (From update of attachment 254793 [details] [review]) > start_type could be GST_SEEK_TYPE_NONE too btw True, but I don't think it is properly handled anywhere in demuxers in push mode. I'm currently improving our gst-validate scenarios to test seeking further and check if those flags are correctly handled. For now, I've pushed the fix to this regression. Later I'll run gst-validate to find the other issues and fix them, if found. commit 566b0dce403288faf81e653705587c010ba760ca Author: Thiago Santos <thiago.sousa.santos@collabora.com> Date: Thu Sep 12 13:31:01 2013 -0300 qtdemux: only update stop position if seek requests it Check for GST_SEEK_TYPE_NONE for stop poistion and only update the stop time if it is requested. Otherwise just maintain whatever was stored at the segment https://bugzilla.gnome.org/show_bug.cgi?id=707530