Created attachment 307468 [details] [review]
dashdemux: provide a default suggestedPresentationDelay

If the MPD@suggestedPresentationDelay attribute is not present, provide a suitable default value as a property of the dashdemux element.

Review of attachment 307468 [details] [review]:

::: ext/dash/gstdashdemux.c
@@ +188,3 @@
 #define DEFAULT_BANDWIDTH_USAGE         0.8     /* 0 to 1     */
 #define DEFAULT_MAX_BITRATE        24000000     /* in bit/s  */
+#define DEFAULT_PRESENTATION_DELAY       12     /* in seconds */

This seems rather arbitrary. I'd prefer having it as 0 as default to just go to the current time rather than being 12s behind.
Another option is try to base on the fragment durations, like going 1 fragment behind or 3 (like HLS does).

Also, how is the application supposed to know what value to use here? Maybe just having a better heuristic would be enough or do you have an use case where the application actually suggest a value?

(In reply to Thiago Sousa Santos from comment #2)
> Review of attachment 307468 [details] [review] [review]:
> 
> ::: ext/dash/gstdashdemux.c
> @@ +188,3 @@
>  #define DEFAULT_BANDWIDTH_USAGE         0.8     /* 0 to 1     */
>  #define DEFAULT_MAX_BITRATE        24000000     /* in bit/s  */
> +#define DEFAULT_PRESENTATION_DELAY       12     /* in seconds */
> 
> This seems rather arbitrary. I'd prefer having it as 0 as default to just go
> to the current time rather than being 12s behind.
> Another option is try to base on the fragment durations, like going 1
> fragment behind or 3 (like HLS does).

Yes, it's an arbitrary value. The problem is that for a live stream, once the client has chosen its start position, it can't adjust this position without impacting the viewer (e.g. by pausing the playback to build up a buffer).

My reasoning for choosing seconds rather than fragments is because different streams might have quite different sized fragments. I am using streams with segment durations between 2 and 5 seconds. Using number of fragments would cause a large variance in start position. If we're feeling really keen, the property could be a string so that we can specify either fragments or seconds (e.g. "3f", "12s") depending upon the use case.

I don't think one or zero fragments behind is sufficient, because it means the DASH client can never buffer more than a fragment. Three fragments (like HLS) is a reasonable value. However, if it makes this patch more acceptable, I'm ok with making the default zero.

> 
> Also, how is the application supposed to know what value to use here? Maybe
> just having a better heuristic would be enough or do you have an use case
> where the application actually suggest a value?

The tricky thing is that the start position has to be decided before downloading any fragments, so the adaptive bitrate algorithm doesn't help. In theory an application could use something like prior history, but in practice it is most likely to be a fixed value.

Created attachment 310020 [details] [review]
dashdemux: provide a default suggestedPresentationDelay

Changed the property to be a string, so that either seconds or fragments can be specified.

Changed the default value to be zero, to match current dashdemux behaviour.

Review of attachment 310020 [details] [review]:

::: ext/dash/gstdashdemux.c
@@ +372,3 @@
+      g_param_spec_string ("presentation-delay", "Presentation delay",
+          "Default presentation delay (in seconds or fragments) (e.g. 12s, 3f)",
+          DEFAULT_PRESENTATION_DELAY,

Should mention 'M' which the parsing function understands

@@ +456,3 @@
+    case PROP_PRESENTATION_DELAY:
+      g_free (demux->default_presentation_delay);
+      demux->default_presentation_delay = g_value_dup_string (value);

Seems to leak on object destruction

Created attachment 311288 [details] [review]
dashdemux: provide a default suggestedPresentationDelay

If the MPD@suggestedPresentationDelay attribute is not present, provide a suitable default value as a property of the dashdemux element. To allow the default presentation delay to be specified either using fragments or seconds, the property is a string that contains a number and a unit (e.g. "10 seconds", "4 fragments", "2500ms").

Updated patch following review and re-based to current master.

I just added "= NULL" at the initialization of endptr, since it could technically be equual to default_presentation_delay, and thus fail the test even though strtol did its thing without trouble.


commit c8ef39cac7337b55901bfb38ca5a8ecda07376a8
Author: Alex Ashley <bugzilla@ashley-family.net>
Date:   Tue Jul 7 15:38:08 2015 +0100

    dashdemux: provide a default suggestedPresentationDelay
    
    If MPD@suggestedPresentationDelay is not present in the manifest,
    dashdemux selects the fragment closest to the most recently generated
    fragment. This causes a playback issue because this choice does not allow
    the DASH client to build up any buffer of downloaded fragments without
    pausing playback. This is because by definition new fragments appear on
    the server in real-time (e.g. if segment duration is 4 seconds, a new
    fragment will appear on the server every 4 seconds). If the starting
    playback position was n*segmentDuration seconds behind "now", the DASH
    client could download up to 'n' fragments faster than realtime before it
    reached the point where it needed to wait for fragments to appear on the
    server.
    
    The MPD@suggestedPresentationDelay attribute allows a content publisher
    to provide a suggested starting position that is behind the current
    "live" position.
    
    If the MPD@suggestedPresentationDelay attribute is not present, provide
    a suitable default value as a property of the dashdemux element. To
    allow the default presentation delay to be specified either using
    fragments or seconds, the property is a string that contains a number
    and a unit (e.g. "10 seconds", "4 fragments", "2500ms").