After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 725828 - hlsdemux: reporting playlist and fragment download time stat
hlsdemux: reporting playlist and fragment download time stat
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.x
Other All
: Normal enhancement
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-03-06 15:01 UTC by Alexander Zallesov
Modified: 2014-07-21 07:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (3.77 KB, patch)
2014-03-06 15:01 UTC, Alexander Zallesov
needs-work Details | Review
0001-hlsdemux-provide-statistics-on-time-to-download-play.patch (3.58 KB, patch)
2014-06-03 12:15 UTC, Sebastian Dröge (slomo)
needs-work Details | Review
hlsdemux: Provide statistics about time to download playlists and fragments (3.41 KB, patch)
2014-07-18 13:10 UTC, Sebastian Dröge (slomo)
committed Details | Review
hlsdemux: Make statistics message more generic for other adaptive streaming demuxers to reuse (8.11 KB, patch)
2014-07-18 13:10 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Alexander Zallesov 2014-03-06 15:01:05 UTC
Report playlist and fragment download time. Useful to have for statistics and QoS reasons.
Comment 1 Alexander Zallesov 2014-03-06 15:01:52 UTC
Created attachment 271113 [details] [review]
patch
Comment 2 Sebastian Dröge (slomo) 2014-04-08 08:19:30 UTC
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
Comment 3 Sebastian Dröge (slomo) 2014-06-03 12:15:36 UTC
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
Comment 4 Tim-Philipp Müller 2014-06-23 11:00:36 UTC
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.)
Comment 5 Sebastian Dröge (slomo) 2014-06-23 11:09:14 UTC
(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
Comment 6 Sebastian Dröge (slomo) 2014-07-03 08:36:40 UTC
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
Comment 7 Sebastian Dröge (slomo) 2014-07-03 08:40:10 UTC
Also hlsdemux currently reports a message when it switches bitrates. That can be generalized trivially like this too.
Comment 8 Sebastian Dröge (slomo) 2014-07-18 13:10:52 UTC
Created attachment 281092 [details] [review]
hlsdemux: Provide statistics about time to download playlists and fragments
Comment 9 Sebastian Dröge (slomo) 2014-07-18 13:10:56 UTC
Created attachment 281093 [details] [review]
hlsdemux: Make statistics message more generic for other adaptive streaming demuxers to reuse
Comment 10 Sebastian Dröge (slomo) 2014-07-21 07:38:23 UTC
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