[Bug 753751] Dashdemux: returned seekable range for live streams is not usable

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=753751

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #332134|none                        |needs-work
             status|                            |

--- Comment #75 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 332134:
 --> (https://bugzilla.gnome.org/review?bug=753751&attachment=332134)

::: 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)

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.