GNOME Bugzilla – Bug 755036
mssdemux: improved live playback support
Last modified: 2017-03-02 00:34:34 UTC
Currently the demuxer refreshes the playlist periodically but this shouldn't be needed. When a MSS server hosts a live stream the fragments listed in the manifest usually don't have accurate timestamps and duration, except for the first fragment, which additionally stores timing information for the few upcoming fragments. In this scenario it is useless to periodically fetch and update the manifest and the fragments list can be incrementally built by parsing the first/current fragment. To avoid the fragments list to grow forever we could store them in a queue.
Created attachment 311325 [details] [review] mssdemux: improved live playback support When a MSS server hosts a live stream the fragments listed in the manifest usually don't have accurate timestamps and duration, except for the first fragment, which additionally stores timing information for the few upcoming fragments. In this scenario it is useless to periodically fetch and update the manifest and the fragments list can be incrementally built by parsing the first/current fragment.
Created attachment 311327 [details] [review] mssdemux: improved live playback support When a MSS server hosts a live stream the fragments listed in the manifest usually don't have accurate timestamps and duration, except for the first fragment, which additionally stores timing information for the few upcoming fragments. In this scenario it is useless to periodically fetch and update the manifest and the fragments list can be incrementally built by parsing the first/current fragment. Rebased against current master :)
Nice! It is a key missing feature for our mssdemux. Did you consider implementing the parsing at qtdemux and feeding the results with upstream events? The tricky part is how to relate the events to the fragment index/number so we know where to add on the list. I'd like other people's opinions on the preferred approach before doing a full review of the patches.
I considered it yes but since this is quite specific to MSS I thought it would be simpler to do the parsing in the smooth-streaming plugin. The PIFF box parsing is done in qtdemux though, see https://bugzilla.gnome.org/show_bug.cgi?id=753614 This isn't very consistent I admit :) Maybe I can give it a try and parse all the things in qtdemux after all.
I should get back to this patch and experiment with moving the PIFF box parsing to qtdemux. Perhaps next week I'll have some time for this.
This patch no longer works :/ The timestamps reported from the fragment tfrf and tfxd boxes are way off, and the demuxer ends up emitting erroneous segment events, I think. I'll need to debug this :)
Sebastian fixed this in qtdemux, thanks :) I'll upload a rebased patch.
Created attachment 328922 [details] [review] rebased patch
Created attachment 329268 [details] [review] updated patch
Ping?
Created attachment 339338 [details] [review] mssdemux: improved live playback support When a MSS server hosts a live stream the fragments listed in the manifest usually don't have accurate timestamps and duration, except for the first fragment, which additionally stores timing information for the few upcoming fragments. In this scenario it is useless to periodically fetch and update the manifest and the fragments list can be incrementally built by parsing the first/current fragment.
An issue with this updated patch, sometimes the same audio/video fragments would be played twice, this still needs to be debugged.
Review of attachment 339338 [details] [review]: The fragment parsing feels like it should go into qtdemux instead. Any reason why it isn't? ::: ext/smoothstreaming/gstmssmanifest.c @@ +78,3 @@ + gboolean has_live_fragments; + GQueue live_fragments; Why the separate variable instead of changing over the existing GList fragments? @@ +267,3 @@ + manifest->is_live ? "yes" : "no", manifest->look_ahead_fragment_count); + stream->has_live_fragments = manifest->is_live + && manifest->look_ahead_fragment_count; What exactly is the has_live_fragments condition signalling? The possible existence of parsing needed for the TfrfBox? @@ +1117,3 @@ + fragment = stream->current_fragment->data; + if (fragment->time <= prev_fragment->time) { + while (fragment->time <= prev_fragment->time) { double condition without an else on the if seems a little pointless no? Also, what exactly is the while loop meant to solve? When aren't the fragments that you're iterating over, not in order? In any case, this seems like it should be a separate patch. @@ +1526,3 @@ + +void +gst_mss_stream_fragment_parse (GstMssStream * stream, GstBuffer * buffer) parse_fragment() is a better name. @@ +1545,3 @@ + + for (guint8 index = 0; index < stream->fragment_parser.tfrf.entries_count; + index++) { Any particular reason you chose an 8-bit integer for an index? That would only allow 255 fragments.
(In reply to Matthew Waters (ystreet00) from comment #13) > Review of attachment 339338 [details] [review] [review]: > > The fragment parsing feels like it should go into qtdemux instead. Any > reason why it isn't? > I agree it is a bit weird indeed. It could go to qtdemux but then I think we would need relay the parsing results back to the mss demuxer somehow. > ::: ext/smoothstreaming/gstmssmanifest.c > @@ +78,3 @@ > > + gboolean has_live_fragments; > + GQueue live_fragments; > > Why the separate variable instead of changing over the existing GList > fragments? > It can be changed to use the existing list. I first thought it was better to differenciate the fragments coming from the manifest from the ones built during the current fragment parsing. > @@ +267,3 @@ > + manifest->is_live ? "yes" : "no", > manifest->look_ahead_fragment_count); > + stream->has_live_fragments = manifest->is_live > + && manifest->look_ahead_fragment_count; > > What exactly is the has_live_fragments condition signalling? > > The possible existence of parsing needed for the TfrfBox? > Yes > @@ +1117,3 @@ > + fragment = stream->current_fragment->data; > + if (fragment->time <= prev_fragment->time) { > + while (fragment->time <= prev_fragment->time) { > > double condition without an else on the if seems a little pointless no? > > Also, what exactly is the while loop meant to solve? > > When aren't the fragments that you're iterating over, not in order? > > In any case, this seems like it should be a separate patch. Yes this could be split out I think. Currently I don't remember exactly why I wrote this :) I'll check it again. > > @@ +1526,3 @@ > + > +void > +gst_mss_stream_fragment_parse (GstMssStream * stream, GstBuffer * buffer) > > parse_fragment() is a better name. > Surely can be renamed yes :) > @@ +1545,3 @@ > + > + for (guint8 index = 0; index < stream->fragment_parser.tfrf.entries_count; > + index++) { > > Any particular reason you chose an 8-bit integer for an index? > > That would only allow 255 fragments. The streams I've tested never had more than 2 look-ahead fragments. But yeah, that 8-bit integer can be changed, though I suppose it's unlikely to have more than 255 look-ahead fragments.
(In reply to Matthew Waters (ystreet00) from comment #13) > @@ +1117,3 @@ > + fragment = stream->current_fragment->data; > + if (fragment->time <= prev_fragment->time) { > + while (fragment->time <= prev_fragment->time) { > > double condition without an else on the if seems a little pointless no? > > Also, what exactly is the while loop meant to solve? > > When aren't the fragments that you're iterating over, not in order? > > In any case, this seems like it should be a separate patch. This is an issue introduced by the patch, the fragment timestamp continuity isn't respected in the parsed tfrf box. So this loop isn't needed at all and we should instead compare the fragment timestamp parsed from the tfrf box with the timestamp of the last fragment of the queue. I'll update the patch.
Created attachment 339627 [details] [review] mssdemux: improved live playback support When a MSS server hosts a live stream the fragments listed in the manifest usually don't have accurate timestamps and duration, except for the first fragment, which additionally stores timing information for the few upcoming fragments. In this scenario it is useless to periodically fetch and update the manifest and the fragments list can be incrementally built by parsing the first/current fragment.
Created attachment 340963 [details] [review] mssdemux: improved live playback support When a MSS server hosts a live stream the fragments listed in the manifest usually don't have accurate timestamps and duration, except for the first fragment, which additionally stores timing information for the few upcoming fragments. In this scenario it is useless to periodically fetch and update the manifest and the fragments list can be incrementally built by parsing the first/current fragment.
In this version the live_fragments GQueue was removed, the fragment list is now used for live and non-live scenarios.
Review of attachment 340963 [details] [review]: Looks good. Feel free to push after the change below. ::: ext/smoothstreaming/gstmssmanifest.c @@ +1626,3 @@ + gst_mss_stream_type_name (gst_mss_stream_get_type (stream)); + + for (guint8 index = 0; index < stream->fragment_parser.tfrf.entries_count; C99. Move the declaration of index out of the for loop. Also, weren't you going to make index not the size of a byte?
(In reply to Matthew Waters (ystreet00) from comment #19) > Review of attachment 340963 [details] [review] [review]: > > Looks good. Feel free to push after the change below. > Ok, thanks for the reviews! > ::: ext/smoothstreaming/gstmssmanifest.c > @@ +1626,3 @@ > + gst_mss_stream_type_name (gst_mss_stream_get_type (stream)); > + > + for (guint8 index = 0; index < stream->fragment_parser.tfrf.entries_count; > > C99. Move the declaration of index out of the for loop. > Oops > Also, weren't you going to make index not the size of a byte? Well the FragmentCount field is defined as uint8 in the spec https://msdn.microsoft.com/en-us/library/ff469518.aspx
Comment on attachment 340963 [details] [review] mssdemux: improved live playback support Pushed as 73721ad4e9e2d32e1c8b6a3b4aaa98401530e58a