GNOME Bugzilla – Bug 753751
Dashdemux: returned seekable range for live streams is not usable
Last modified: 2016-08-02 11:56:49 UTC
For live streams in dashdemux (adaptivedemux) seeking query returns a time range that is based on the wall clock. This makes it impossible to seek the very beginning of the stream because by the time I execute seek, seekable range is already updated (because wall clock/server clock moves on). This wouldn't be a problem if seeking before the start of the live stream would just clamp to the current start. Unfortunately in that case seek fails. I'd appreciate any though.
dashdemux should probably clamp the start time to be a multiple of segment duration, in a manner that rounds up to the next available segment. GstClockTime duration = gst_mpd_client_get_segment_duration (client, stream, NULL); start += start % duration; .. and round down the end time ... end -= end % duration; This is still racy if you happen to call gst_dash_demux_get_live_seek_range() and then wait segment_duration before issuing the seek.
That's right. It still can be racy. That's why I feel it might be better to allow seek before the start of seekable range.
Created attachment 310799 [details] [review] allow seeking before start in live streams I think this takes care of it, but it's not yet tested because I don't have a live DASH stream around at the moment. The one I knew of (http://rdmedia.bbc.co.uk/dash/testmpds/multiperiod/bbb.php) seems to point to 404 streams. Parenthetically, this stream triggered https://bugzilla.gnome.org/show_bug.cgi?id=754668.
You could try: http://dash.bidi.int.bbc.co.uk/d/pseudolive/bbb/client_manifest.mpd
Using gst-launch and navseek, I can seek back to "past the beginning". In this case, it loops backwards. The original report mentions the beginning of the stream, which I'm not sure what it is with a live stream. In any case, I tested this with the ACCURATE flag check removed, as navseek sends ACCURATE seeks. I'm not sure what's the best here. It feels a bit weird to allow this adjustment when ACCURATE isn't set.
For a live DASH stream, you can access fragments between "now" and "now - timeshiftBufferDepth". The GST_QUERY_SEEKING query for a DASH stream will respond with start and end position based upon the current time of day. For example if the query arrived at 10:00:00am and the timeshiftBufferDepth was 60 seconds, the response from dashdemux would be: start = 09:59:00 stop = 10:00:00 One second later, a call to seek is made using the start value from the query response. By this time, the seek range has moved to: start = 09:59:01 stop = 10:00:01 Therefore the seek fails, because the requested seek position is before the earliest possible seek position.
Created attachment 311386 [details] Python script for seeking with live DASH streams This is the script I have been using when testing seeking for DASH streams.
Created attachment 311628 [details] Python script for seeking with live DASH streams Updated script with goto-start and goto-end buttons. On a live stream the goto-start button does not work because by the time the seek is issued, the start value from seeking query is already invalid. As GStreamer times are in nanoseconds, this issue even occurs in native code.
From what I can see, on the stream you posted in comment #4 and your script, the server 404s on the segment that's requested (because it has presumably just dumped that segment a fraction of a second ago. The two ways I can see to get round this are (1) get dashdemux to spot a 404 error when seeking and try the next segment instead, and (2) always restrict the seekable range's start. (1) doesn't seem like a good idea, and (2) seems racy too.
For the record, I left the python test case above run on the pseudo live stream mentioned above for more than 35 minutes (the mpd file states timeShiftBufferDepth="PT35M"), then seeked to start. I can see that the stream that 404s was indeed requested succesfully early in the playback.
One other possibility is to remember where we were when the seek was initiated, and seek back there if we get a 404 error back. That error is received after the seek event has "succeeded", asynchronously. There is still a possibility that this fails, if the previous position was close to the range start, though.
(In reply to Vincent Penquerc'h from comment #10) > For the record, I left the python test case above run on the pseudo live > stream mentioned above for more than 35 minutes (the mpd file states > timeShiftBufferDepth="PT35M"), then seeked to start. I can see that the > stream that 404s was indeed requested succesfully early in the playback. That is the expected behaviour. Once the fragment has fallen outside of timeshiftBufferDepth, the server is allowed to delete the fragment. In the case of this test stream, it generates the fragments on-the-fly and will return 404 errors for any requests before "now" or after "now - timeshiftBufferDepth"
When I looked at this, I thought I traced it down to discarding the seek request in the GST_EVENT_SEEK handler in gstadaptivedemux.c: if (gst_adaptive_demux_is_live (demux)) { gint64 range_start, range_stop; if (!gst_adaptive_demux_get_live_seek_range (demux, &range_start, &range_stop)) { gst_event_unref (event); return FALSE; } if (start < range_start || start >= range_stop) { GST_WARNING_OBJECT (demux, "Seek to invalid position"); gst_event_unref (event); return FALSE; } } If an app issues a GST_QUERY_SEEKING and then a GST_EVENT_SEEK with the start position set to the start value from the GST_QUERY_SEEKING call, then the (start < range_start) condition triggers. This is because range_start is calculated from a call to gst_adaptive_demux_get_live_seek_range() that occurs after the call to gst_adaptive_demux_get_live_seek_range() call that responded to the query. For DASH, gst_adaptive_demux_get_live_seek_range() is calculated by sampling current time of day. It only takes one tick of the RTC between GST_QUERY_SEEKING and GST_EVENT_SEEK to make start<range_start.
This was indeed the case, but my patch above fixes this by adjusting the seek target if the ACCURATE flag is not set and the source is live. The remaining problem is that the server itself will 404 a request for the very start timestamp we know about, and we can't know exactly what the server's idea of the (changing) start timestamp will be at the time it considers our request.
Alex, do you see what I'm saying, or am I missing the point you were making ? And if so, do you see another way out than the ones in comment #9, which aren't great ?
There was a problem with the BBC example stream that was causing it to 404 when it shouldn't be. It should be ok now. We probably need another ticket to track what the behaviour should be for 404 errors on a live stream. Moving on to the next segment is probably a reasonable option. The DVB DASH specification defines quite a few rules for this, involving manifest refreshes and refreshing clock drift compensation. I could live with making the ACCURATE flag having a slightly different meaning for live streams, but it does seem a bit ugly. For any "sliding window" live stream (e.g. DASH or HLS) we have the situation that the values returned by GST_QUERY_SEEKING are only valid for short period of time, because fragments are being added and removed by the server. In the case of HLS, this isn't normally an issue because the values returned by GST_QUERY_SEEKING are valid until the playlist is reloaded. In the case of DASH this doesn't work because it is based upon wall clock time. One nanosecond after the response to GST_QUERY_SEEKING, the start value can no longer be used. There is another issue in that it will take time to download a fragment. There is no point returning a position in GST_QUERY_SEEKING that refers to a fragment that cannot be downloaded before it will have been deleted from the server. Another problem is that GST_QUERY_SEEKING returns a stop value that is beyond the available range. If you have a look at the gst_mpd_client_check_time_position() and gst_mpd_client_get_next_segment_availability_end_time() functions in gstmpdparser.c, they include the segment duration when checking if a segment is available. The gst_dash_demux_get_live_seek_range() function in gstdashdemux.c ignores the segment duration.
gst_mpd_client_check_time_position() seems wrong, no ? If you pass it "now", it wlil test a future value. Also, it seems likely that the available range jumps as segments are removed and added, rather than flow smoothly. Or maybe I just don't understand how DASH works.
Created attachment 312366 [details] [review] dashdemux: include segment duration when calculating seek range The gst_dash_demux_get_live_seek_range () function returns a stop value that is beyond the available range. The functions gst_mpd_client_check_time_position() and gst_mpd_client_get_next_segment_availability_end_time() in gstmpdparser.c include the segment duration when checking if a segment is available. The gst_dash_demux_get_live_seek_range() function in gstdashdemux.c ignores the segment duration. This patch subtracts maxSegmentDuration from the stop time, which should be a conservative approach to calculating stop time. A more accurate value for segment duration could be used if GstActiveStream* was available to the gst_dash_demux_get_live_seek_range() function.
Ah, it's live, so that explains the weirdness. You're expected to always be at least one segment's duration late, and it makes sense.
(In reply to Vincent Penquerc'h from comment #17) > gst_mpd_client_check_time_position() seems wrong, no ? If you pass it "now", > it wlil test a future value. Also, it seems likely that the available range > jumps as segments are removed and added, rather than flow smoothly. Or maybe > I just don't understand how DASH works. The gst_mpd_client_check_time_position() function is not actually used and should be deleted. Yes, you are right that available range jumps as segments are removed and added. Fragments are atomic - they are either fully available or not available.
Created attachment 312473 [details] [review] retry once on fragment download error This allows surviving 404 by retrying once. There will be a pause which depends on the size of the fragment and where we seeked within this fragment.
Under normal circumstances a 404 is not a temporary error and therefore should not be retried. An error in the 500 range (e.g. 503, 504) should cause a retry. A retry of a 404 (or 410) error is particularly bad because these requests tend to cause a request to make it all the way back to the origin. However, in the case of a DASH live stream, a 404 might indicate that the segment is not yet available on the server. This probably indicates a clock drift error or a server-side error. The DVB DASH specification defines two different behaviours, depending upon whether the stream is a live stream or an on-demand stream. On-demand: The Player shall change BaseURL as specified in clause 10.8.2.3. Live: The Player shall reload the MPD. If the MPD indicates a source of time as specified in clause 4.7.3 the Player shall resynchronise to one of the time sources as described in clause 4.7.3. If as a result of reloading the MPD and carrying out time synchronisation the Player determines the request is no longer appropriate, it shall adjust its position in the media to reflect the new MPD and time. If the request is still valid the Player may retry the request up to the max number of retries specified. If trying the above does not result in success the Player shall change BaseURL as specified in clause 10.8.2.3. I don't think we're quite ready to implement the full DVB approach, but we do need to be careful about creating code that acts like a denial of service attack on the origin server. My suggestion is: For 5xx errors: /* Check if requested segment still exists (i.e. it hasn't fallen out of the timeshift buffer).*/ if( segment_no_longer_exists() ) { move_play_position_forward_until_valid_segment_is_found(); load_segment(); } else { retry_segment(); /* with some suitable retry limit */ } For 4xx errors: now = server_utc_time_of_day() if (current_position >= now ) { /* maybe check should be if pos close to now?*/ /* maybe poll clock server */ wait(); retry_segment(); /* with some suitable retry limit */ } else{ move_play_position_forward_one_segment(); load_segment(); }
This will go on to the next segment, though, as finish_segment and _advance_segment are called. Though I suppose this is an assumption that the error is due to a 404 from obsolete segment. I'd originally started by hooking in the error message handler (where we have to strstr the debug message to get at the HTTP error code, AFAICT), but this become too hairy as it was in a different thread. Maybe we just have to do it anyway after all.
Created attachment 312504 [details] [review] uridownloader: expose http status
Created attachment 313470 [details] [review] souphttpsrc: Expose HTTP status for the last error. Patch to souphttpsrc to allow the HTTP status code to be queried.
Created attachment 313471 [details] [review] souphttpsc: new property for last status code
Created attachment 313472 [details] [review] retry once on fragment download error
The step to next segment is now done only on 4xx/5xx, if the current time is before the visible portion of a live stream. I can't get an error to happen when seeking forward, so the wait part of the test is not tested. I'm not sure if it can happen. Maybe after long playback time if clock rates are different.
Created attachment 313474 [details] [review] adaptivedemux: expose HTTP status To allow adaptivedemux to make retry decisions, it needs to know what sort of HTTP error has occurred. For example, the retry logic for a 410 error is different from a 504 error.
Maybe I should change my patch to only do the retry code on 404. That's the only error I saw returned on the test BBC stream when seeking back slightly before the cutoff, but I'm not sure whether this is the case with all other servers.
(In reply to Vincent Penquerc'h from comment #28) > The step to next segment is now done only on 4xx/5xx, if the current time is > before the visible portion of a live stream. > I can't get an error to happen when seeking forward, so the wait part of the > test is not tested. I'm not sure if it can happen. Maybe after long playback > time if clock rates are different. Patch looks good. One improvement would be to allow one retry in the case of a 5xx error for a non-live stream.
(In reply to Vincent Penquerc'h from comment #30) > Maybe I should change my patch to only do the retry code on 404. That's the > only error I saw returned on the test BBC stream when seeking back slightly > before the cutoff, but I'm not sure whether this is the case with all other > servers. A 500, 503 or 504 error could occur on any stream at any time, so we should probably keep support for retrying if they occur. The BBC stream should be behaving itself now, there was a problem with it that required it to be restarted.
Created attachment 313653 [details] [review] retry once on fragment download error Changed to allow one retry (without changing segment) on 5xx if not live.
(In reply to Vincent Penquerc'h from comment #33) > Created attachment 313653 [details] [review] [review] > retry once on fragment download error > > Changed to allow one retry (without changing segment) on 5xx if not live. It might be nicer to (optionally) do that inside souphttpsrc, see bug #756318 which does it for the sink.
Review of attachment 313470 [details] [review]: ::: ext/soup/gstsouphttpsrc.c @@ +463,3 @@ g_free (src->iradio_url); src->iradio_url = NULL; + src->last_http_status_code = 0; Is this called when going back to NULL state? @@ +1558,3 @@ _("Could not resolve server name.")); src->ret = GST_FLOW_ERROR; + src->last_http_status_code = msg->status_code; Can't we just set it once at the end of the switch statement?
Comment on attachment 313471 [details] [review] souphttpsc: new property for last status code Wouldn't it be nicer to put it into the ERROR messages as another field?
(In reply to Sebastian Dröge (slomo) from comment #36) > Comment on attachment 313471 [details] [review] [review] > souphttpsc: new property for last status code > > Wouldn't it be nicer to put it into the ERROR messages as another field? ... as that would also allow applications in other situations (playbin!) to do something useful based on the error if they want to.
Looking at the code, I'd add a gst_element_message_full_param, which takes an extra GstStructure with fields to be copied in the new error structure, then a GST_ELEMENT_ERROR_PARAM. Or did you have another way to do it in mind before I do that ?
I didn't, but I'm not sure that API is great. Let me create another bug about that, wanted to do that a while ago already.
(In reply to Sebastian Dröge (slomo) from comment #39) > I didn't, but I'm not sure that API is great. Let me create another bug > about that, wanted to do that a while ago already. See bug #756806
Right. That's the only to access the internal GstStructure though. I'll leave that as is for now then.
(In reply to Sebastian Dröge (slomo) from comment #34) > > Changed to allow one retry (without changing segment) on 5xx if not live. > > It might be nicer to (optionally) do that inside souphttpsrc, see bug > #756318 which does it for the sink. The disadvantage of that approach is that dashdemux could use DASH-specific behaviour (e.g. switch to a different BaseURL) in response to a 5xx error, which would be lost if it was all handled in souphttpsrc. Having said that, no DASH-specific code exists at the moment, it's all in adaptivedemux.
What was the conclusion about seeking before start on DASH live streams? I guess if the seek is flagged as non-accurate we could skip to the first available segment and download that. Should we do the same for accurate seeks? I think this is the first time we have to handle this scenario in gstreamer, so I don't know.
Under normal circumstances, it is reasonable for a user of the GST_QUERY_SEEKING API to be able to seek anywhere within the returned range. However, in the case of live streams, the range returned by GST_QUERY_SEEKING is only valid for a limited amount of time. There is no way to know how long the returned range from GST_QUERY_SEEKING is valid. Not sure if it would be possible to add such information in an ABI compatible manner. In the case of hlsdemux, the returned range is valid until the next playlist refresh. In the case of dashdemux, the returned range is valid for 1 nanosecond. Selecting the earliest available segment for a non-accurate seek seems reasonable. Not so sure about the accurate case. I would be inclined to fail the seek request if the accurate flag is specified and the value is before the seek range.
Review of attachment 312366 [details] [review]: ::: ext/dash/gstdashdemux.c @@ +315,3 @@ + GstClockTime seg_duration; + + /* TODO: use gst_mpd_client_get_segment_duration() to get real segment duration */ Be careful that gst_mpd_client_get_segment_duration uses base->duration without testing that it actually exists. For example, if segment timeline is being used, duration will not be set. @@ +316,3 @@ + + /* TODO: use gst_mpd_client_get_segment_duration() to get real segment duration */ + seg_duration = self->client->mpd_node->maxSegmentDuration * GST_MSECOND; Currently maxSegmentDuration could be undefined (set to -1). You need to test for that. The standard says "If not present, then the maximum Segment duration shall be the maximum duration of any Segment documented in this MPD". Ideally, the code should update maxSegmentDuration every time a segment duration is parsed/computed.
Created attachment 319447 [details] [review] dashdemux: include segment duration when calculating seek range Moved code into a gst_mpd_client_get_maximum_segment_duration() function in gstmpdparser and added a fallback if MPD@maxSegmentDuration is not present.
Created attachment 319448 [details] [review] tests: dashdemux: add test for gst_mpd_client_get_maximum_segment_duration() function Add a test of the gst_mpd_client_get_maximum_segment_duration() function. See patch 319447 for implementation of gst_mpd_client_get_maximum_segment_duration() function.
slomo, are you OK with the HTTP error code patch here, which may end up replaced by something more generic in souphttpsrc later ? Or do you want 756806 to be done first, then use this for the patches here ?
No, I'd like to have it done properly first :) Also in any case after 1.8.0
Created attachment 322979 [details] [review] allow seeking before start in live streams
Created attachment 322980 [details] [review] adaptivedemux: expose HTTP status
Comment on attachment 313471 [details] [review] souphttpsc: new property for last status code Replaced by allowing optional arbitrary data in error messages.
Created attachment 322981 [details] [review] retry once on fragment download error
Created attachment 322982 [details] [review] dashdemux: include segment duration when calculating seek range
Created attachment 322983 [details] [review] tests: dashdemux: add test for gst_mpd_client_get_maximum_segment_duration() function
All patches ported to recent master, and to replace use of souphttpsrc error code property with use of the new details structure in error messages. Passes dash unit tests.
Review of attachment 322981 [details] [review]: ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c @@ +2440,3 @@ +static const char * +uritype (GstAdaptiveDemuxStream * s) +{ Does this function need to encased within #ifndef GST_DISABLE_GST_DEBUG to avoid an unused function error when compiling with debugging disabled?
Created attachment 322988 [details] [review] retry once on fragment download error Only define uritype when debugging isn't disabled.
Review of attachment 322979 [details] [review]: You have a stray 'common' submodule change in there
Created attachment 323400 [details] [review] allow seeking before start in live streams
Review of attachment 322988 [details] [review]: My eyes hurt. Can we make gst_adaptive_demux_stream_download_fragment() *simpler* and not unreadable like this ? I'll provide more feedback once I can read it with going cross-eyed :D ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c @@ +2653,3 @@ url = stream->fragment.uri; GST_DEBUG_OBJECT (stream->pad, "Got url '%s' for stream %p", url, stream); if (url) { switch around the condition. if (!url) goto ... @@ +2670,3 @@ /* TODO check if we are truly stopping */ + live = gst_adaptive_demux_is_live (demux); + if (ret == GST_FLOW_CUSTOM_ERROR && live) { this massive if/else/if/else/if/else... *always* uses "ret == GST_FLOW_CUSTOM_ERROR", can we move that check a bit further up ?
(I meant "without" going cross-eyed :) )
Created attachment 324286 [details] [review] retry once on fragment download error With requested changes.
This now needs updates against the proper API and the change in souphttpsrc :) Can you update, Vincent?
I've ported those patches to current master and changed to match souphttpsrc, but there's a regression in one of the unit tests which I'm trying to work out (error recovery is ending up looping retrying forever: one retry in the function it's meant to do it, but that function is being called again indefinitely). So I'll push that when I got that figured out.
And by push, I mean post, of course :)
Created attachment 332132 [details] [review] allow seeking before start in live streams
Created attachment 332133 [details] [review] adaptivedemux: expose HTTP status
Created attachment 332134 [details] [review] retry once on fragment download error
Created attachment 332135 [details] [review] dashdemux: include segment duration when calculating seek range
Created attachment 332136 [details] [review] tests: dashdemux: add test for gst_mpd_client_get_maximum_segment_duration() function
With test regression fixed.
Comment on attachment 332132 [details] [review] allow seeking before start in live streams Not sure if this is the nicest approach. We basically will shift stream time 0 into the future now to make the current seek start position 0. And if there is yet another seek to an earlier point, it will shift 0 again. That's very confusing from a user point of view. Maybe instead of that, we can have the stream time be something different for live streams? Like having it the current wallclock time at the moment when the stream is started. For running time you can obviously do whatever you want ;)
Review of attachment 332133 [details] [review]: ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c @@ +2682,3 @@ + if (g_object_class_find_property (gobject_class, + "last-http-error-code")) { + g_object_get (stream->src, "last-http-error-code", http_status, NULL); This property does not exist though? You have to get it from the error message now, and without error assume 200 (which might not be true, could as well be 206).
Review of attachment 332134 [details] [review]: ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c @@ +2596,3 @@ + if (s->downloading_header) + return "header"; + if (s->downloading_index) I always wondered why these are not an enum :) @@ +2705,3 @@ + "last-http-statuscode")) { + g_object_get (stream->src, "last-http-status-code", http_status, + NULL); Same here, this does not exist @@ +2854,3 @@ + GstAdaptiveDemuxClass *klass = + GST_ADAPTIVE_DEMUX_GET_CLASS (stream->demux); + if (last_status_code / 100 == 4 || last_status_code / 100 == 5) { /* 4xx/5xx */ For non-live cases we probably also want to retry at least once on 5xx @@ +2860,3 @@ + if (gst_adaptive_demux_get_live_seek_range (demux, &range_start, + &range_stop)) { + if (demux->segment.position < range_start) { Nice if-cascade :) Can this be refactored a bit to not have 6 new levels of indentation? @@ +2868,3 @@ + gst_flow_get_name (ret)); + gst_adaptive_demux_stream_fragment_download_finish (stream, + ret, NULL); This is basically the EOS case, maybe refactor that to a function and use it in both places @@ +2900,3 @@ + } + g_cond_wait_until (&stream->fragment_download_cond, + &stream->fragment_download_lock, end_time); Condition variables can have spurious wakeups @@ +2935,3 @@ + /* retry once (same segment) for 5xx (server errors) */ + if (!retried_once) { + retried_once = TRUE; You probably want to wait a short and random amount of time before retrying. Also "network errors" are useful for retries (DNS failure for example, soup also reports them)
(In reply to Sebastian Dröge (slomo) from comment #73) > Maybe instead of that, we can have the stream time be something different > for live streams? Like having it the current wallclock time at the moment > when the stream is started. Note that I propose that for adaptivedemux live streams in general. And the SEEKING query would then return the valid seekable range in that stream time (you obviously can't seek to 0, which would be in 1970 or so). You would also have to "resync" the stream time whenever you jump ahead a fragment because we got out of sync with the server.
(In reply to Sebastian Dröge (slomo) from comment #73) > Comment on attachment 332132 [details] [review] [review] > allow seeking before start in live streams > > Not sure if this is the nicest approach. We basically will shift stream time > 0 into the future now to make the current seek start position 0. And if > there is yet another seek to an earlier point, it will shift 0 again. That's > very confusing from a user point of view. It's the seek target that's being modified, so it behaves as if the seek had originally been to dt after the actual target.
Created attachment 332216 [details] [review] adaptivedemux: expose HTTP status
To distinguish about HTTP status, I'd have to get souphttpsrc to send an info message at each time there's an transfer. But actually, it doesn't sound too bad now that I'm typing it I guess.
To distinguish between various 20x HTTP status, I meant.
(In reply to Vincent Penquerc'h from comment #80) > To distinguish between various 20x HTTP status, I meant. Why do you need to distinguish between them? :) (In reply to Vincent Penquerc'h from comment #77) > (In reply to Sebastian Dröge (slomo) from comment #73) > > Comment on attachment 332132 [details] [review] [review] [review] > > allow seeking before start in live streams > > > > Not sure if this is the nicest approach. We basically will shift stream time > > 0 into the future now to make the current seek start position 0. And if > > there is yet another seek to an earlier point, it will shift 0 again. That's > > very confusing from a user point of view. > > It's the seek target that's being modified, so it behaves as if the seek had > originally been to dt after the actual target. Can you explain? How does this look exactly from a user point of view, maybe with an example?
Created attachment 332225 [details] [review] expose HTTP status
Created attachment 332226 [details] [review] souphttpsrc: post info messages with HTTP status codes
(In reply to Sebastian Dröge (slomo) from comment #81) > (In reply to Vincent Penquerc'h from comment #80) > > To distinguish between various 20x HTTP status, I meant. > > Why do you need to distinguish between them? :) I thought that's what you asked (do not assume a non failing HTTP status code is 200 - so we need to know which one it is). But see the last two patches, which do that. Was this what you had in mind ?
(In reply to Sebastian Dröge (slomo) from comment #81) > (In reply to Vincent Penquerc'h from comment #77) > > (In reply to Sebastian Dröge (slomo) from comment #73) > > > Comment on attachment 332132 [details] [review] [review] [review] [review] > > > allow seeking before start in live streams > > > > > > Not sure if this is the nicest approach. We basically will shift stream time > > > 0 into the future now to make the current seek start position 0. And if > > > there is yet another seek to an earlier point, it will shift 0 again. That's > > > very confusing from a user point of view. > > > > It's the seek target that's being modified, so it behaves as if the seek had > > originally been to dt after the actual target. > > Can you explain? How does this look exactly from a user point of view, maybe > with an example? A program using gst decides to seek to the beginning of the current valid range, and the range it thinks is current is 74 to 259. It will ask to seek to 74. The patch requests the current range, and sees it's now 75 to 260, so adjusts the requested target to 75, and seeks there instead.
(In reply to Vincent Penquerc'h from comment #84) > (In reply to Sebastian Dröge (slomo) from comment #81) > > (In reply to Vincent Penquerc'h from comment #80) > > > To distinguish between various 20x HTTP status, I meant. > > > > Why do you need to distinguish between them? :) > > I thought that's what you asked (do not assume a non failing HTTP status > code is 200 - so we need to know which one it is). But see the last two > patches, which do that. Was this what you had in mind ? Oh, no, I was just commented that. No action required, for our purposes all 200 status codes should just be fine and handled the same. Posting an info message in every case seems a bit overkill.
(In reply to Vincent Penquerc'h from comment #85) > A program using gst decides to seek to the beginning of the current valid > range, and the range it thinks is current is 74 to 259. It will ask to seek > to 74. The patch requests the current range, and sees it's now 75 to 260, so > adjusts the requested target to 75, and seeks there instead. Ah I completely misunderstood the intention of the patch then! So it only updates the seek position if they are outside the currently valid range. And at no time you can actually seek before the position where dashdemux started (i.e. stream time 0). Seems good then
Comment on attachment 332226 [details] [review] souphttpsrc: post info messages with HTTP status codes This seems a bit extreme and a lot of overhead, applications might also show INFO messages to the user.
Vincent, are you updating the remaining patch? Then this is good to go IMHO
I will when I get a moment for it. Also bilboed wanted to review and wasn't quite happy with one of the large functions.
Created attachment 332344 [details] [review] expose HTTP status
Created attachment 332345 [details] [review] retry once on fragment download error
I think that also takes care of the "large function" thing (well, mostly indentation levels).
Review of attachment 332344 [details] [review]: Looks good except for one minor thing ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c @@ +2678,3 @@ uri, stream->last_ret, gst_flow_get_name (stream->last_ret)); + if (stream->last_ret != GST_FLOW_OK && http_status) { + *http_status = stream->last_status_code; For consistency, this function should *always* set http_status to something before returning. It's too easy to later forget this in the callers
Review of attachment 332345 [details] [review]: Seems mostly good now ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c @@ +819,3 @@ + if (details) { + gst_structure_get_uint (details, "http-status-code", + &stream->last_status_code); This seems like something that should go together with the other patch (expose http status code) @@ +2855,3 @@ + + live = gst_adaptive_demux_is_live (demux); + if (live) { Why only for live? It seems like retry for 5xx errors should be done in all cases, just for non-live you don't want to do the segment skipping
(In reply to Sebastian Dröge (slomo) from comment #95) > Review of attachment 332345 [details] [review] [review]: > > Seems mostly good now > > ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c > @@ +819,3 @@ > + if (details) { > + gst_structure_get_uint (details, "http-status-code", > + &stream->last_status_code); > > This seems like something that should go together with the other patch > (expose http status code) The whole thing ? This sets a new boolean that's not in the previous patch, and if it moves, a number of things also move (and then i can just merge both patches I think).
Created attachment 332514 [details] [review] expose HTTP status
Created attachment 332515 [details] [review] retry once on fragment download error
Review of attachment 332515 [details] [review]: ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c @@ +2858,3 @@ + + live = gst_adaptive_demux_is_live (demux); + if (!retried_once && (last_status_code / 100 == 4 || last_status_code / 100 == 5)) { /* 4xx/5xx */ For non-live, 4xx seems fatal but not too important I guess
Created attachment 332540 [details] [review] retry once on fragment download error 4xx for live only, 5xx for all
Review of attachment 332540 [details] [review]: Go go go :)
Thanks for the reviews! commit dc6e4ccbf9d23962cf39ce7bd9f9152e9d41a6e3 Author: Alex Ashley <bugzilla@ashley-family.net> Date: Wed Jan 20 16:42:24 2016 +0000 tests: dashdemux: add test for gst_mpd_client_get_maximum_segment_duration Add a test of the gst_mpd_client_get_maximum_segment_duration() function to check that it first checks the MPD@maxSegmentDuration and then falls back to checking all of the segment durations. https://bugzilla.gnome.org/show_bug.cgi?id=753751 commit d9bcf4dbd93df05255feb6f52dec68dbe13622ac Author: Alex Ashley <bugzilla@ashley-family.net> Date: Wed Feb 24 15:54:54 2016 +0000 dashdemux: include segment duration when calculating seek range The gst_dash_demux_get_live_seek_range () function returns a stop value that is beyond the available range. The functions gst_mpd_client_check_time_position() and gst_mpd_client_get_next_segment_availability_end_time() in gstmpdparser.c include the segment duration when checking if a segment is available. The gst_dash_demux_get_live_seek_range() function in gstdashdemux.c ignores the segment duration. According to the DASH specification, if maxSegmentDuration is not present, then the maximum Segment duration is the maximum duration of any Segment documented in the MPD. https://bugzilla.gnome.org/show_bug.cgi?id=753751 commit 535f10b61d0c45c8e7f73de298432b5e97385e05 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Wed Feb 24 15:52:41 2016 +0000 adaptivedemux: retry once on 4xx/5xx in certain conditions This helps catch those 404 server errors in live streams when seeking to the very beginning, as the server will handle a request with some delay, which can cause it to drop the fragment before sending it. https://bugzilla.gnome.org/show_bug.cgi?id=753751 commit 341cdb198f4f1a8da8e85bbf2a7da75a7ee3ab6b Author: Alex Ashley <bugzilla@ashley-family.net> Date: Wed Feb 24 15:47:09 2016 +0000 adaptivedemux: expose HTTP status To allow adaptivedemux to make retry decisions, it needs to know what sort of HTTP error has occurred. For example, the retry logic for a 410 error is different from a 504 error. https://bugzilla.gnome.org/show_bug.cgi?id=753751 commit 5b7f60dada60ce1b8dfcddc25012a906357e92e3 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Mar 7 17:04:33 2016 +0000 adaptivedemux: allow seeking before start in live streams Some derived classes (at least dashdemux) expose a seeking range based on wall clock. This means that a subsequent seek to the start of this range will be before the allowed range. To solve this, seeks without the ACCURATE flag are allowed to seek before the start for live streams, in which case the segment is shifted to start at the start of the new seek range. If there is an end position, is is shifted too, to keep the duration constant. https://bugzilla.gnome.org/show_bug.cgi?id=753751