GNOME Bugzilla – Bug 725828
hlsdemux: reporting playlist and fragment download time stat
Last modified: 2014-07-21 07:38:30 UTC
Report playlist and fragment download time. Useful to have for statistics and QoS reasons.
Created attachment 271113 [details] [review] patch
Review of attachment 271113 [details] [review]: Please use your real name in the patch, not just your last name :) Some nitpicking, also not sure about the names really. ::: ext/hls/gsthlsdemux.c @@ +118,3 @@ G_DEFINE_TYPE (GstHLSDemux, gst_hls_demux, GST_TYPE_ELEMENT); +#define GST_HLS_DEMUX_STATISTIC_MSG_NAME "hlsdemux-statistics" MESSAGE_NAME @@ +418,3 @@ +static void +gst_hls_demux_post_stat_msg (GstHLSDemux * demux, GstStructure * structure) post_statistics_message @@ +1102,3 @@ + stat_msg = gst_structure_new (GST_HLS_DEMUX_STATISTIC_MSG_NAME, + "time-to-playlist", GST_TYPE_CLOCK_TIME, time-to-download-playlist maybe
Created attachment 277793 [details] [review] 0001-hlsdemux-provide-statistics-on-time-to-download-play.patch Just updated to apply cleanly to latest GIT master, no other changes
Gratuitous comments: I don't entirely see the value of the gst_hls_demux_post_stat_msg() helper function, might just as well do it in the code, doesn't save any lines really, does it (if you embed the gst_structure_new() without assigning it to a variable)? Regarding the names: - "hlsdemux-statistics" -> maybe "hls-statistics" ? Or maybe we want to make it generic for all these adaptive thingies from the start? Or does it not map? - "time-of-first-playlist" -> this time is absolute? why only for the first then? - "time-to-playlist" -> "playlist-download-time" ? - "time-to-download-fragment" -> "fragment-download-time" ? (Don't know really, I have no strong opinions here, I think it's all fine.)
(In reply to comment #4) > Gratuitous comments: I don't entirely see the value of the > gst_hls_demux_post_stat_msg() helper function, might just as well do it in the > code, doesn't save any lines really, does it (if you embed the > gst_structure_new() without assigning it to a variable)? > > Regarding the names: > > - "hlsdemux-statistics" -> maybe "hls-statistics" ? > Or maybe we want to make it generic for all these > adaptive thingies from the start? Or does it not map? Good point, we can generalize that for all the adaptive things. They all have some kind of playlist (usually called manifest) and fragments. And sometimes other things. > - "time-of-first-playlist" -> this time is absolute? > why only for the first then? Because the first playlist is downloaded externally, even before hlsdemux exists. So we can only report an absolute time here. The download is started as a result of something the app is doing. For the other playlist messages, here the download is started manually without the app knowing and hlsdemux knows when it was started and finished. So we can report either a duration here, or two absolute times. Maybe generalize it and always report two absolute times... and for the first playlist report -1 for the start time? And then for consistency we should report the two times instead of durations for the fragment downloads too. > - "time-to-playlist" -> "playlist-download-time" ? > > - "time-to-download-fragment" -> "fragment-download-time" ? I prefer those too, yes
Ok, let's get it in for 1.6 with: structure name=adaptive-streaming-statistics - manifest URI URI of main (!) manifest (aka playlist) - uri URI of the playlist or fragment this applies to - manifest-download-start, manifest-download-end This would have GST_CLOCK_TIME_NONE for the initial manifest and always contains gst_util_get_timestamp() based times - fragment-download-start, fragment-download-end - metadata-download-start, metadata-download-end For headers and other stuff as seen in DASH
Also hlsdemux currently reports a message when it switches bitrates. That can be generalized trivially like this too.
Created attachment 281092 [details] [review] hlsdemux: Provide statistics about time to download playlists and fragments
Created attachment 281093 [details] [review] hlsdemux: Make statistics message more generic for other adaptive streaming demuxers to reuse
commit 429f20531f03f93e95ef5395344b806c5d7b083e Author: Sebastian Dröge <sebastian@centricular.com> Date: Fri Jul 18 15:09:54 2014 +0200 hlsdemux: Make statistics message more generic for other adaptive streaming demuxers to reuse https://bugzilla.gnome.org/show_bug.cgi?id=725828 commit 7f4f9f09e3eee3fc2fc764ebae5dae14500dd4d1 Author: Alexander Zallesov <zallesov@gmail.com> Date: Tue Feb 25 11:58:57 2014 +0100 hlsdemux: Provide statistics about time to download playlists and fragments https://bugzilla.gnome.org/show_bug.cgi?id=725828