GNOME Bugzilla – Bug 744362
dashdemux: Add support for live stream seeking
Last modified: 2015-02-12 21:49:18 UTC
As shown by : gst-validate-1.0 playbin \ uri=http://dev-iplatforms.kw.bbc.co.uk/dash/news24-avc3/news24.php The current behaviour was to let the following demuxer (in that case qtdemux) answer the duration query, with this stream the duration was always of 24 hours. It seems more correct to me to have the duration be now - availabilityStartTime. This patch also handles live seeking, by setting a live seek range comprised between now - timeShiftBufferDepth and now. The inteersting thing with this stream is that one can actually ask fragments up to availabilityStartTime, but it seems quite clear in the spec that content is only guaranteed to exist up to timeShiftBufferDepth. One can test live seeking this way : gst-validate-1.0 playbin \ uri=http://dev-iplatforms.kw.bbc.co.uk/dash/news24-avc3/news24.php \ --set-scenario seek_back.scenario with scenario being: description, seek=true seek, playback-time=position+5.0, start="position-600.0", flags=accurate+flush This example will play the stream, wait for five seconds, then seek back to a position 10 minutes earlier.
Created attachment 296644 [details] [review] dashdemux: Fix handling of live streams with timeshift buffers.
Review of attachment 296644 [details] [review]: Nice addition! just 2 observations below. ::: ext/dash/gstmpdparser.c @@ +3859,3 @@ + g_date_time_unref (now); + g_date_time_unref (start); + duration = stream_now * GST_USECOND; This has the side effect of making the duration query return a duration for a live stream. I don't know an use case where knowing the 'current duration' of a live stream would be useful so I'd avoid that. Some applications can query the duration and then store that as a permanent value, this can lead to issues later. Even if you wanted to do that, you'd need to remember to post a duration-changed message everytime a new fragment was generated. So, while the code above is correct I'd keep it separate from the usual 'get_duration' call to prevent problems later. ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c @@ +653,2 @@ if (first_segment) + demux->segment.start = demux->segment.position = demux->segment.time = min_pts; Why set the segment.time here? It doesn't seem related to the fix and it would deserve some testing with HLS streams to see what would be shown with this set? Anyway I think this should be in a different patch as it doesn't seem to be part of the feature you are adding.
(In reply to Thiago Sousa Santos from comment #2) > Review of attachment 296644 [details] [review] [review]: > > Nice addition! > > just 2 observations below. > > ::: ext/dash/gstmpdparser.c > @@ +3859,3 @@ > + g_date_time_unref (now); > + g_date_time_unref (start); > + duration = stream_now * GST_USECOND; > > This has the side effect of making the duration query return a duration for > a live stream. I don't know an use case where knowing the 'current duration' > of a live stream would be useful so I'd avoid that. Some applications can > query the duration and then store that as a permanent value, this can lead > to issues later. Even if you wanted to do that, you'd need to remember to > post a duration-changed message everytime a new fragment was generated. > > So, while the code above is correct I'd keep it separate from the usual > 'get_duration' call to prevent problems later. > OK, makes sense, this stream will then keep having its duration reported as 24 hours by qtdemux, I'm not against that :) > ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c > @@ +653,2 @@ > if (first_segment) > + demux->segment.start = demux->segment.position = demux->segment.time = > min_pts; > > Why set the segment.time here? > > It doesn't seem related to the fix and it would deserve some testing with > HLS streams to see what would be shown with this set? > > Anyway I think this should be in a different patch as it doesn't seem to be > part of the feature you are adding. I think this fix is related, as it makes the sink report a correct position, on all subsquent seeks time is set on the segments that are set, only the first segment has a time set to 0, which I think is a bug, I'll make a separate patch, to see what the problem is, simply run the first command line in the commit message with and without that patch.
Created attachment 296714 [details] [review] adaptivedemux: Set first segment time to segment start. Otherwise as long as a seek wasn't executed, the position was reported incorrectly: gst-validate-1.0 playbin \ uri=http://dev-iplatforms.kw.bbc.co.uk/dash/news24-avc3/news24.php
Created attachment 296715 [details] [review] dashdemux: Fix handling of live streams with timeshift buffers. By implementing get_live_seek_range. As shown by : gst-validate-1.0 playbin \ uri=http://dev-iplatforms.kw.bbc.co.uk/dash/news24-avc3/news24.php This patch handles live seeking, by setting a live seek range comprised between now - timeShiftBufferDepth and now. The inteersting thing with this stream is that one can actually ask fragments up to availabilityStartTime, but it seems quite clear in the spec that content is only guaranteed to exist up to timeShiftBufferDepth. One can test live seeking this way : gst-validate-1.0 playbin \ uri=http://dev-iplatforms.kw.bbc.co.uk/dash/news24-avc3/news24.php \ --set-scenario seek_back.scenario with scenario being: description, seek=true seek, playback-time=position+5.0, start="position-600.0", flags=accurate+flush This example will play the stream, wait for five seconds, then seek back to a position 10 minutes earlier.
Review of attachment 296715 [details] [review]: Works nicely, thanks!
Review of attachment 296714 [details] [review]: Looks good
Review of attachment 296714 [details] [review]: commit 6b864813cce885e8fb900041f73dcf4e5264d3db Author: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com> Date: Thu Feb 12 22:04:10 2015 +0100 adaptivedemux: Set first segment time to segment start. Otherwise as long as a seek wasn't executed, the position was reported incorrectly: gst-validate-1.0 playbin \ uri=http://dev-iplatforms.kw.bbc.co.uk/dash/news24-avc3/news24.php https://bugzilla.gnome.org/show_bug.cgi?id=744362 commited
Review of attachment 296715 [details] [review]: commit 7ca6d9634a8df45e9ad28f572e36a8d5e697b413 Author: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com> Date: Thu Feb 12 22:06:17 2015 +0100 dashdemux: Fix handling of live streams with timeshift buffers. By implementing get_live_seek_range. As shown by : gst-validate-1.0 playbin \ uri=http://dev-iplatforms.kw.bbc.co.uk/dash/news24-avc3/news24.php This patch handles live seeking, by setting a live seek range comprised between now - timeShiftBufferDepth and now. The inteersting thing with this stream is that one can actually ask fragments up to availabilityStartTime, but it seems quite clear in the spec that content is only guaranteed to exist up to timeShiftBufferDepth. One can test live seeking this way : gst-validate-1.0 playbin \ uri=http://dev-iplatforms.kw.bbc.co.uk/dash/news24-avc3/news24.php \ --set-scenario seek_back.scenario with scenario being: description, seek=true seek, playback-time=position+5.0, start="position-600.0", flags=accurate+flush This example will play the stream, wait for five seconds, then seek back to a position 10 minutes earlier. https://bugzilla.gnome.org/show_bug.cgi?id=744362 commited