GNOME Bugzilla – Bug 752413
dashdemux: add support for parsing UTCTiming elements
Last modified: 2015-08-26 06:52:07 UTC
Unless the DASH client can compensate for the difference between its clock and the clock used by the server, the client might request fragments that either not yet on the server or fragments that have already been expired from the server. This is an issue because these requests can propagate all the way back to the origin ISO/IEC 23009-1:2014/PDAM 1 defines a new UTCTiming element to allow a DASH client to track the clock used by the server generating the DASH stream. This bug ticket is a request to implement this feature in dashdemux. Given that this is a DASH specific feature, I think it belongs in dashdemux rather than adaptivedemux.
Created attachment 307466 [details] [review] dashdemux: add support for parsing UTCTiming elements This patch provides the first part of supporting the UTCTiming feature, by parsing the element and providing unit tests of this parsing.
Created attachment 308087 [details] [review] dashdemux: add support for UTCTiming elements for clock drift compensation This patch provides parsing of UTCTiming elements, unit tests of this parsing and a task to poll the server every 30 minutes. This task supports the "urn:mpeg:dash:utc:http-xsdate:2014" and "urn:mpeg:dash:utc:http-iso:2014" methods. When choosing the starting fragment number and when waiting for a fragment to become available, the difference between the server's idea of UTC and the client's idea of UTC is taken into account. For example, if the server's time is behind the client's idea of UTC, adaptivedemux waits longer before requesting a fragment.
An example stream that demonstrates the use of the UTCTiming element is available at: http://dash-live-streams.appspot.com/dash/manifest_e.mpd?drift=30 This stream will report itself as having a clock that is 30 seconds behind UTC. Without the appropriate clock drift compensation, dashdemux will request fragments that are not available from the server, causing 404 responses.
Review of attachment 308087 [details] [review]: Just a short initial review, will continue in a bit ::: ext/dash/gstdashdemux.c @@ +647,3 @@ + gst_dash_demux_clock_drift_new (dashdemux, (const gchar **) urls, + method); + gst_task_start (dashdemux->clock_drift->task); Would it be possible to reuse the mpd refresh task for this somehow? adaptivedemux already has so many threads :) @@ +1580,3 @@ + + next_update = g_get_monotonic_time (); + downloader = gst_uri_downloader_new (); You can use the adaptivedemux GstURIDownloader here instead of creating a new one. Then you can potentially reuse existing HTTP connections, reducing overhead @@ +1622,3 @@ + /* We don't know when the server sampled its clock, but we know + it must have been before "end" and probably after "start". + A reasonable estimate is to use (start+end)/2 I guess that's the best you can do here but it's not correct to assume that the server sampled the clock in the middle, especially in the presence of HTTPS and having to create a new connection... in which case the time until the server sees the request is much higher than the time until you get the reply. Shouldn't really matter much here though as this is not about millisecond-accuracy :) @@ +1675,3 @@ + /* wait for task to get a clock value from a time server */ + if (demux->clock_drift->got_clock == FALSE + && demux->clock_drift->stop_task == FALSE) { Never compare booleans with == and !=, but just check "if (x)" and "if (!x)"
Review of attachment 308087 [details] [review]: Seems generally good overall, just having yet another thread is not so nice :) ::: ext/dash/gstmpdparser.c @@ +215,3 @@ +}; + +static struct GstMpdParserUtcTimingMethod gst_mpdparser_utc_timing_methods[] = { Might want to add a const here @@ +223,3 @@ + {"urn:mpeg:dash:utc:http-ntp:2014", GST_MPD_UTCTIMING_TYPE_HTTP_NTP}, + {"urn:mpeg:dash:utc:direct:2014", GST_MPD_UTCTIMING_TYPE_DIRECT}, +#ifdef SUPPORT_DRAFT_2012_NAMES Why put an #ifdef here? It shouldn't hurt to always have it there
Created attachment 309003 [details] [review] dashdemux: add support for UTCTiming elements for clock drift compensation This patch provides parsing of UTCTiming elements, unit tests of this parsing and a function to poll a time server. This function supports the "urn:mpeg:dash:utc:http-xsdate:2014" and "urn:mpeg:dash:utc:http-iso:2014" methods. The manifest update task is used to poll the clock time server, to save having to create a new thread.
Review of attachment 309003 [details] [review]: ::: ext/dash/gstdashdemux.c @@ +1228,3 @@ GstDashDemux *dashdemux = GST_DASH_DEMUX_CAST (demux); + return MIN (dashdemux->client->mpd_node->minimumUpdatePeriod * 1000, + CLOCK_UPDATE_INTERVAL); Hm but this means that we will also update the manifest each time, right? Which might mean that we update the manifest too often, and a server might complain about that ::: ext/dash/gstmpdparser.c @@ +222,3 @@ + {"urn:mpeg:dash:utc:http-iso:2014", GST_MPD_UTCTIMING_TYPE_HTTP_ISO}, + {"urn:mpeg:dash:utc:http-ntp:2014", GST_MPD_UTCTIMING_TYPE_HTTP_NTP}, + {"urn:mpeg:dash:utc:direct:2014", GST_MPD_UTCTIMING_TYPE_DIRECT}, Who thought that it's a good idea to add that many methods for the same thing? :) Not your fault of course but it's a bit... special
(In reply to Sebastian Dröge (slomo) from comment #7) > Review of attachment 309003 [details] [review] [review]: > > ::: ext/dash/gstdashdemux.c > @@ +1228,3 @@ > GstDashDemux *dashdemux = GST_DASH_DEMUX_CAST (demux); > + return MIN (dashdemux->client->mpd_node->minimumUpdatePeriod * 1000, > + CLOCK_UPDATE_INTERVAL); > > Hm but this means that we will also update the manifest each time, right? > Which might mean that we update the manifest too often, and a server might > complain about that Yes it does. However the clock poll is set to 30 minutes, so it's not going to add much traffic. > > ::: ext/dash/gstmpdparser.c > @@ +222,3 @@ > + {"urn:mpeg:dash:utc:http-iso:2014", GST_MPD_UTCTIMING_TYPE_HTTP_ISO}, > + {"urn:mpeg:dash:utc:http-ntp:2014", GST_MPD_UTCTIMING_TYPE_HTTP_NTP}, > + {"urn:mpeg:dash:utc:direct:2014", GST_MPD_UTCTIMING_TYPE_DIRECT}, > > Who thought that it's a good idea to add that many methods for the same > thing? :) Not your fault of course but it's a bit... special The joys of standardisation. Can't agree on a timing method - let's have seven!
(In reply to A Ashley from comment #8) > (In reply to Sebastian Dröge (slomo) from comment #7) > > Review of attachment 309003 [details] [review] [review] [review]: > > > > ::: ext/dash/gstdashdemux.c > > @@ +1228,3 @@ > > GstDashDemux *dashdemux = GST_DASH_DEMUX_CAST (demux); > > + return MIN (dashdemux->client->mpd_node->minimumUpdatePeriod * 1000, > > + CLOCK_UPDATE_INTERVAL); > > > > Hm but this means that we will also update the manifest each time, right? > > Which might mean that we update the manifest too often, and a server might > > complain about that > > Yes it does. However the clock poll is set to 30 minutes, so it's not going > to add much traffic. Ah I misunderstood that part of the code... there's only going to be a problem if the minimum update period is bigger than 30 minutes, and if the server enforces that (client refreshes earlier? block it!). Seems unlikely.
(In reply to Sebastian Dröge (slomo) from comment #9) > Ah I misunderstood that part of the code... there's only going to be a > problem if the minimum update period is bigger than 30 minutes, and if the > server enforces that (client refreshes earlier? block it!). Seems unlikely. That's correct, in most cases minimumUpdatePeriod will be less than 30 minutes. minimumUpdatePeriod only defines the minimum amount of time that the manifest will stay the same (i.e. don't bother to poll more frequently than minimumUpdatePeriod). It is not meant to define the client update rate. In dashdemux we tie the two together, but that's not quite right.
Review of attachment 309003 [details] [review]: ::: ext/dash/gstdashdemux.c @@ +1534,3 @@ + + clock_drift = g_slice_new0 (GstDashDemuxClockDrift); + clock_drift->method = method; This seems to be unused, is it intentional? I don't have the spec from 2014 to check what the different methods mean. Is it how the server obtained its value of 'now'?
tldr: yes, clock_drift->method is not used and can be removed. Essentially all the methods fall into one of four categories: 1. Use an NTP/SNTP server 2. Do an HTTP HEAD request and look at the Date: field 3. Do an HTTP GET request and parse the payload 4. Parse the value directly defined in the element The patch implements 2 of the 3 payload formats defined for category 3. The other payload format (ntp format) is slightly ambiguous and I've never seen it used. I have only seen categories #3 and #4 used in streams. The reason for choosing #3 is because it provides a reasonable trade-off between client and server complexity and seems to be the most widely implemented. #4 provides a lousy time reference (it can be out by up to mininimumUpdatePeriod seconds) but could be implemented if this was considered useful. I looked at implementing #2 but it needed some surgery in gsturidownloader to pass back the HTTP response headers. Not seen anyone use it, so that was another reason for not bothering to implement it.
For NTP you could probably wrap something around the new NTP clock support in libgstnet
Created attachment 309081 [details] [review] dashdemux: add support for UTCTiming elements for clock drift compensation This patch provides parsing of UTCTiming elements, unit tests of this parsing and a function to poll a time server. This function supports the "urn:mpeg:dash:utc:http-xsdate:2014", "urn:mpeg:dash:utc:http-iso:2014" and "urn:mpeg:dash:utc:http-ntp:2014" methods.
(In reply to Sebastian Dröge (slomo) from comment #13) > For NTP you could probably wrap something around the new NTP clock support > in libgstnet Are there any examples of using the NTP clock source?
> GstClock * clock = gst_ntp_clock_new ("bla", "123.123.123.123", 123, 0); And then it works like any other GstClock.
Created attachment 309159 [details] [review] dashdemux: add support for UTCTiming elements for clock drift compensation This patch provides parsing of UTCTiming elements, unit tests of this parsing and a function to use a time server. This function supports the following methods: urn:mpeg:dash:utc:ntp:2014 urn:mpeg:dash:utc:http-xsdate:2014 urn:mpeg:dash:utc:http-iso:2014 urn:mpeg:dash:utc:http-ntp:2014
Test streams: urn:mpeg:dash:utc:ntp:2014 http://dash-live-streams.appspot.com/dash/manifest_h.mpd urn:mpeg:dash:utc:http-xsdate:2014 http://dash-live-streams.appspot.com/dash/manifest_e.mpd http://dash.bidi.int.bbc.co.uk/d/pseudolive/bbb/client_manifest.mpd urn:mpeg:dash:utc:http-ntp:2014 http://dash-live-streams.appspot.com/dash/manifest_e.mpd?time=ntp
Review of attachment 309159 [details] [review]: I think overall this looks good to go, just some minor things. Thiago? ::: ext/dash/Makefile.am @@ +28,3 @@ $(GST_BASE_LIBS) \ $(GST_LIBS) \ + -lgstnet-$(GST_API_VERSION) \ I think this has to be $(GST_NET_LIBS) ::: ext/dash/gstdashdemux.c @@ +194,3 @@ +#define FAST_CLOCK_UPDATE_INTERVAL (1000000 * 30) /* 30 seconds */ +#define SUPPORTED_CLOCK_FORMATS (GST_MPD_UTCTIMING_TYPE_NTP | GST_MPD_UTCTIMING_TYPE_HTTP_XSDATE | GST_MPD_UTCTIMING_TYPE_HTTP_ISO | GST_MPD_UTCTIMING_TYPE_HTTP_NTP) +#define NTP_TO_UNIX_EPOCH 2208988800LL /* difference (in seconds) between NTP epoch and Unix epoch */ G_GUINT64_CONSTANT(), LL is unfortunately not portable @@ +1577,3 @@ + urls[clock_drift->selected_url]); + inet_addrs = g_resolver_lookup_by_name (resolver, + urls[clock_drift->selected_url], NULL, &err); How does the NTP stuff work? I see that there's an http:// URL in the manifest, and behind that there's some non-ASCII data. E.g. http://dash-live-streams.appspot.com/time/ntp Isn't that directly the NTP timestamp? Also if the manifest specifies an IP string instead of a hostname, I think GResolver doesn't do that right thing. IIRC code usually first tries to create a GInetAddress from the string, and only if that fails goes through GResolver
(In reply to Sebastian Dröge (slomo) from comment #19) > ::: ext/dash/gstdashdemux.c > @@ +194,3 @@ > +#define FAST_CLOCK_UPDATE_INTERVAL (1000000 * 30) /* 30 seconds */ > +#define SUPPORTED_CLOCK_FORMATS (GST_MPD_UTCTIMING_TYPE_NTP | > GST_MPD_UTCTIMING_TYPE_HTTP_XSDATE | GST_MPD_UTCTIMING_TYPE_HTTP_ISO | > GST_MPD_UTCTIMING_TYPE_HTTP_NTP) > +#define NTP_TO_UNIX_EPOCH 2208988800LL /* difference (in seconds) between > NTP epoch and Unix epoch */ > > G_GUINT64_CONSTANT(), LL is unfortunately not portable > Ok, good to know. I stole that from gstrtpsession.c so I guess that also needs fixing. > @@ +1577,3 @@ > + urls[clock_drift->selected_url]); > + inet_addrs = g_resolver_lookup_by_name (resolver, > + urls[clock_drift->selected_url], NULL, &err); > > How does the NTP stuff work? I see that there's an http:// URL in the > manifest, and behind that there's some non-ASCII data. E.g. > http://dash-live-streams.appspot.com/time/ntp > There are two forms of NTP. 1. urn:mpeg:dash:utc:ntp:2014 2. urn:mpeg:dash:utc:http-ntp:2014 In case 1, the value is a list of NTP servers to contact using standard NTP protocol. In case 2, the value is a list of HTTP URLs. A GET to one of these URLs will return an 8 byte packet in NTP Timestamp format. > Also if the manifest specifies an IP > string instead of a hostname, I think GResolver doesn't do that right thing. > IIRC code usually first tries to create a GInetAddress from the string, and > only if that fails goes through GResolver According to the gio documentation: hostname may be an ASCII-only or UTF-8 hostname, or the textual form of an IP address (in which case this just becomes a wrapper around g_inet_address_new_from_string()). I wonder if a better solution would be for gst_ntp_clock_new() to support both hostnames and IP address strings?
(In reply to A Ashley from comment #20) > > According to the gio documentation: > hostname may be an ASCII-only or UTF-8 hostname, or the textual form of an > IP address (in which case this just becomes a wrapper around > g_inet_address_new_from_string()). Ah, that might be new then :) Or I just missed that before. > I wonder if a better solution would be for gst_ntp_clock_new() to support > both hostnames and IP address strings? Would be an option, you could do the name resolution synchronously in the clock thread then.
Created attachment 309243 [details] [review] Post-review fixup of patch 309159 (UTCTiming element support) Post-review fixup of patch. As it's so small I didn't squash it into the original patch, to make it easier to see the changes.
Created attachment 309244 [details] [review] dashdemux: add support for HTTP HEAD method of time sync The urn:mpeg:dash:utc:http-head:2014 method of time synchronisation uses an HTTP HEAD request to a specified URL and then parses the Date: HTTP response header. This patch adds support for this time sync method. Personally I am not convinced that the changes required to GStreamer to support this method warrants its inclusion. I am submitting this patch more as a way to archive it for posterity, in case it is ever needed. This patch also requires a change to souphttpsrc, which I will submit as a separate patch.
Created attachment 309245 [details] [review] souphttpsrc: add property to set HTTP method To allow souphttpsrc to be use HTTP methods other than GET (e.g. HEAD), add a "method" property that is a string. If this property is not set, GET is used. This patch is required by patch 309244 (dashdemux: add support for HTTP HEAD method of time sync).
Review of attachment 309245 [details] [review]: commit 9dcc7d9caca8aae32e6b9ca3dc5309d79d8e6956 Author: Alex Ashley <bugzilla@ashley-family.net> Date: Fri Aug 14 08:33:56 2015 +0100 souphttpsrc: add property to set HTTP method To allow souphttpsrc to be use HTTP methods other than GET (e.g. HEAD), add a "method" property that is a string. If this property is not set, GET is used. https://bugzilla.gnome.org/show_bug.cgi?id=752413
I had to do minor fixups because it was failing to build because of printf formats but otherwise all merged. Thanks! commit 93edd99bf7fa481b4bebc9f05ab560111eeb397c Author: Alex Ashley <bugzilla@ashley-family.net> Date: Fri Aug 14 09:44:24 2015 +0100 dashdemux: add support for HTTP HEAD method of time sync The urn:mpeg:dash:utc:http-head:2014 method of time synchronisation uses an HTTP HEAD request to a specified URL and then parses the Date: HTTP response header. This commit adds support to dashdemux for this method of time synchronisation by making a HEAD request and then parsing the Date: response. This commit adds support to gstfragment to return the HTTP headers and to uridownloader to support HEAD requests. To avoid creating a new API, the RANGE get function is re-used (abused?) with start=-1 and end=-1 to indicate a HEAD request. https://bugzilla.gnome.org/show_bug.cgi?id=752413 commit 1640ee2b339fb35cd8eb5779875242e347697265 Author: Alex Ashley <bugzilla@ashley-family.net> Date: Thu Aug 13 18:21:29 2015 +0100 dashdemux: post-review fixup of UTCTiming element This commit addresses the following items from the code review: use a portable way to define NTP_TO_UNIX_EPOCH, fix memory leak on error, and add documentation to UTCTiming parse functions Using LL is not portable, so the G_GUINT64_CONSTANT needs to be instead. If an error occurs during DNS resolution, the GError was not being released, causing a memory leak. https://bugzilla.gnome.org/show_bug.cgi?id=752413 commit 95c705ae8f48acb4504ac168b4a8c16d77cae8da Author: Alex Ashley <bugzilla@ashley-family.net> Date: Wed Jul 15 11:56:13 2015 +0100 dashdemux: add support for UTCTiming elements for clock drift compensation Unless the DASH client can compensate for the difference between its clock and the clock used by the server, the client might request fragments that either not yet on the server or fragments that have already been expired from the server. This is an issue because these requests can propagate all the way back to the origin ISO/IEC 23009-1:2014/Amd 1 [PDAM1] defines a new UTCTiming element to allow a DASH client to track the clock used by the server generating the DASH stream. Multiple UTCTiming elements might be present, to indicate support for multiple methods of UTC time gathering. Each element can contain a white space separated list of URLs that can be contacted to discover the UTC time from the server's perspective. This commit provides parsing of UTCTiming elements, unit tests of this parsing and a function to poll a time server. This function supports the following methods: urn:mpeg:dash:utc:ntp:2014 urn:mpeg:dash:utc:http-xsdate:2014 urn:mpeg:dash:utc:http-iso:2014 urn:mpeg:dash:utc:http-ntp:2014 The manifest update task is used to poll the clock time server, to save having to create a new thread. When choosing the starting fragment number and when waiting for a fragment to become available, the difference between the server's idea of UTC and the client's idea of UTC is taken into account. For example, if the server's time is behind the client's idea of UTC, we wait for longer before requesting a fragment [PDAM1]: http://www.iso.org/iso/home/store/catalogue_tc/catalogue_detail.htm?csnumber=66068 dashdemux: support NTP time servers in UTCTiming elements Use the gst_ntp_clock to support the use of an NTP server. https://bugzilla.gnome.org/show_bug.cgi?id=752413
Thanks Sebastian and Thiago for your help. We've now probably got the most complete clock drift implementation of any DASH client!
Is this working now? I'm working on a webkit patch adding the method property in the http src element we have there but the method is not set by the uridownloader because range_start is 0 in gst_uri_downloader_fetch_uri_with_range() I don't see how this could work because range_start is set to 0 and range_end to -1 in dashdemux but the uridownloader checks both variables are < 0 before setting the method on the src element.
Sorry I'm confusing range_start and range_end :)
Alex, how did you test the http-head timing method code path? Can you share a MPD url using this method?
(In reply to Philippe Normand from comment #30) > Alex, how did you test the http-head timing method code path? Can you share > a MPD url using this method? The HTTP HEAD method is the most likely to have bugs because it requires parsing the Date: string from an HTTP server. I expect there are bound to be date formats I didn't account for. I have a fake live stream that can be told to use the HTTP HEAD method: http://dash-live-streams.appspot.com/dash/manifest_e.mpd?time=head
Thank you :)