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 755036 - mssdemux: improved live playback support
mssdemux: improved live playback support
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-09-15 07:28 UTC by Philippe Normand
Modified: 2017-03-02 00:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
mssdemux: improved live playback support (30.42 KB, patch)
2015-09-15 07:29 UTC, Philippe Normand
none Details | Review
mssdemux: improved live playback support (30.40 KB, patch)
2015-09-15 07:38 UTC, Philippe Normand
none Details | Review
rebased patch (29.96 KB, patch)
2016-06-02 06:50 UTC, Philippe Normand
none Details | Review
updated patch (30.83 KB, patch)
2016-06-07 12:39 UTC, Philippe Normand
none Details | Review
mssdemux: improved live playback support (33.77 KB, patch)
2016-11-08 17:14 UTC, Philippe Normand
none Details | Review
mssdemux: improved live playback support (32.17 KB, patch)
2016-11-11 15:09 UTC, Philippe Normand
none Details | Review
mssdemux: improved live playback support (31.87 KB, patch)
2016-11-29 10:10 UTC, Philippe Normand
committed Details | Review

Description Philippe Normand 2015-09-15 07:28:49 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.
Comment 1 Philippe Normand 2015-09-15 07:29:32 UTC
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.
Comment 2 Philippe Normand 2015-09-15 07:38:53 UTC
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 :)
Comment 3 Thiago Sousa Santos 2015-09-21 22:37:51 UTC
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.
Comment 4 Philippe Normand 2015-09-22 06:56:16 UTC
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.
Comment 5 Philippe Normand 2015-11-06 15:56:34 UTC
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.
Comment 6 Philippe Normand 2016-05-09 17:32:47 UTC
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 :)
Comment 7 Philippe Normand 2016-06-02 06:49:42 UTC
Sebastian fixed this in qtdemux, thanks :)

I'll upload a rebased patch.
Comment 8 Philippe Normand 2016-06-02 06:50:24 UTC
Created attachment 328922 [details] [review]
rebased patch
Comment 9 Philippe Normand 2016-06-07 12:39:08 UTC
Created attachment 329268 [details] [review]
updated patch
Comment 10 Philippe Normand 2016-10-12 15:22:36 UTC
Ping?
Comment 11 Philippe Normand 2016-11-08 17:14:23 UTC
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.
Comment 12 Philippe Normand 2016-11-08 17:17:29 UTC
An issue with this updated patch, sometimes the same audio/video fragments would be played twice, this still needs to be debugged.
Comment 13 Matthew Waters (ystreet00) 2016-11-10 12:18:32 UTC
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.
Comment 14 Philippe Normand 2016-11-10 12:29:41 UTC
(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.
Comment 15 Philippe Normand 2016-11-11 15:04:37 UTC
(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.
Comment 16 Philippe Normand 2016-11-11 15:09:25 UTC
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.
Comment 17 Philippe Normand 2016-11-29 10:10:34 UTC
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.
Comment 18 Philippe Normand 2016-11-29 10:11:40 UTC
In this version the live_fragments GQueue was removed, the fragment list is now used for live and non-live scenarios.
Comment 19 Matthew Waters (ystreet00) 2016-11-29 11:43:09 UTC
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?
Comment 20 Philippe Normand 2016-11-29 12:04:31 UTC
(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 21 Philippe Normand 2016-11-29 13:46:24 UTC
Comment on attachment 340963 [details] [review]
mssdemux: improved live playback support

Pushed as 73721ad4e9e2d32e1c8b6a3b4aaa98401530e58a