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 775665 - hlsdemux: problems with broken iHeartRadio streams
hlsdemux: problems with broken iHeartRadio streams
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.11.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-12-05 20:46 UTC by voltzz
Modified: 2017-01-18 11:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Small player to repro issue (1.03 KB, text/plain)
2016-12-05 20:46 UTC, voltzz
  Details
hlsdemux: Detect and work around media sequence number inconsistencies (8.75 KB, patch)
2017-01-05 17:13 UTC, Sebastian Dröge (slomo)
rejected Details | Review

Description voltzz 2016-12-05 20:46:38 UTC
Created attachment 341430 [details]
Small player to repro issue

When playing the following iHeartRadio stream: https://c5.prod.playlists.ihrhls.com/1469/playlist.m3u8 the following problems occur:
- Start up time is a few seconds even on a fast connection
- Playback stutters and buffers very often
- Playback stops often
- After a certain point, playback discontinues indefinitely

See attached file for code reference. This was tested on a Raspberry Pi 3 device using Raspbian (Debian) OS using alsa drivers.

Other audio-only HLS streams were tested (e.g. Groove music) and they seemed to work fine. This looks specific to iHeartRadio streams.
Comment 1 Bruce Tsai 2016-12-06 14:22:51 UTC
It works by using "playbin3" to play given url.
gst-launch-1.0 playbin3 uri=https://c5.prod.playlists.ihrhls.com/1469/playlist.m3u8
Comment 2 Sebastian Dröge (slomo) 2016-12-23 16:57:33 UTC
This commit makes it work in playbin / decodebin:
https://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=991758c3d68a0a7131df4e6244fe3ce29f2b9f7f

And this fixes another problem with it:
https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=ee647ca4be8b5beecada6ebc130df5d376a1ad23


However the stream is broken in general and not according to the spec. Usage of the media sequence (ids) is inconsistent, and the spec recommends in this case to even halt playback. Which is what happens with this stream after a while in Safari on an iPad.

We can probably work around this, but this stream should really be fixed.
Comment 3 Sebastian Dröge (slomo) 2017-01-05 17:13:15 UTC
Created attachment 342974 [details] [review]
hlsdemux: Detect and work around media sequence number inconsistencies

And also handle live streams without any media sequence numbers at all.

Partially works around brokenness with iHeartRadio streams, which have
 a) inconsistent media sequence numbers, that just jump back and forth
    all the time and wrap around at 24 or something
 b) broken CDN caching or similar, which causes older versions of the
    playlist to be fetched sometimes.

This works well now unless the situation happens, that a playlist is
fetched that has *only* fragments that are before the previous playlist.
Which unfortunately happens with the iHearRadio streams (meaning:
playlist is 30s shifted into the past!).
Comment 4 Sebastian Dröge (slomo) 2017-01-05 17:14:28 UTC
These streams are indeed impressively broken, and they don't play in any software I have here. Does anybody have contacts there so they can fix their stuff? Is any software known that can actually play these streams for a while?
Comment 5 Sebastian Dröge (slomo) 2017-01-05 17:23:23 UTC
And even when played like this, without running into the remaining problem, it sounds like fragments are not audible contiguous: the audio just jumps forwards or backwards by a few seconds on a segment boundary sometimes.
Comment 6 Thiago Sousa Santos 2017-01-05 18:02:38 UTC
Did we ever see part of those spec breakages in other streams? It seems like we add quite some code to handle something that is broken beyond repair. Is it worth adding more complexity in this case? It would be better to try and contact the maintainers of the service.
Comment 7 Thiago Sousa Santos 2017-01-05 18:03:39 UTC
Review of attachment 342974 [details] [review]:

::: ext/hls/m3u8.c
@@ +684,3 @@
+    g_list_free (previous_files);
+    previous_files = NULL;
+  }

I'd move this to a separate function to avoid making the _update() function longer.

@@ +711,3 @@
+      } else {
+        mediasequence = file->sequence;
+      }

This could be a separate patch. Does the spec mention what to do in this case?
Comment 8 Sebastian Dröge (slomo) 2017-01-06 10:24:12 UTC
(In reply to Thiago Sousa Santos from comment #7)
> 
> This could be a separate patch. Does the spec mention what to do in this
> case?

Yes, halt playback like with all media sequence ID inconsistencies.
Comment 9 Sebastian Dröge (slomo) 2017-01-06 10:24:18 UTC
For the original issue with the iHeartRadio streams. The problem is application-side here as Jan noticed. Just try playing the stream in a browser and look at how it requests the m3u8 playlist: it adds lots of query parameters, including sessions IDs and other stuff that somehow comes from the Javascript. If you pass that full URL with the query parameters to GStreamer, the stream plays just fine without any code changes at all. The media sequence ids are continuous, playlists are always updated and never go back in time, there's no buffering, no anything.
Also note that there are RTMP and Icecast variants of the streams available, which might or might not work better without the query parameters.


For the attached patch. I agree that it adds a lot of complexity and the code is also not very nice. Maybe we should not merge it then :)

However it implements two parts: 1) detecting of inconsistent media sequence IDs (the first part) and ignoring them then, and 2) handling media sequence IDs correctly for live playlists that don't specify them inside the playlist (it's not mandatory).

1) is something that the spec suggests, detecting these scenarios and then just stopping playback. 2) is something we need to implement if we want to support such (valid) streams.
Comment 10 Thiago Sousa Santos 2017-01-09 01:05:33 UTC
I guess for 2 the extra work to also ignore broken media sequence and try to play is minimal, right?

I'd still prefer to abort as the spec suggests, using some meaningful error. But, as this was already implemented, feel free to merge it too if you see some use to it. Aside my minor suggested refactoring, it looks good to me.
Comment 11 Sebastian Dröge (slomo) 2017-01-09 11:17:16 UTC
(In reply to Thiago Sousa Santos from comment #10)
> I guess for 2 the extra work to also ignore broken media sequence and try to
> play is minimal, right?

The "if (have_mediasequence && !self->broken_media_sequence) {}" is for 1), the "if (!have_mediasequence || self->broken_media_sequence) {}" is for 2). Or what did you mean?
Comment 12 Thiago Sousa Santos 2017-01-10 09:21:55 UTC
I just meant that for 2, officially according to the spec, you wouldn't need the 'broken_media_sequence' because we would have aborted earlier.

My question is: shouldn't we just should abort when 'broken_media_sequence' happens or we have a good reason to support those streams? Widely available kind of broken?
Comment 13 Tim-Philipp Müller 2017-01-10 12:34:39 UTC
I think we should error out, just like apple does.
Comment 14 Sebastian Dröge (slomo) 2017-01-10 15:02:22 UTC
(In reply to Thiago Sousa Santos from comment #12)
> I just meant that for 2, officially according to the spec, you wouldn't need
> the 'broken_media_sequence' because we would have aborted earlier.

That's correct, yes

> My question is: shouldn't we just should abort when 'broken_media_sequence'
> happens or we have a good reason to support those streams?

We could, yes. The only difference in the patch would be the removal of the new boolean in the struct and that the first "if" just errors out instead of falling back to the second "if"

> Widely available kind of broken?

I'm not aware of any
Comment 15 Sebastian Dröge (slomo) 2017-01-10 15:02:47 UTC
If that's the consensus, I can update the patch accordingly (and move it to a new function too).
Comment 16 Sebastian Dröge (slomo) 2017-01-12 15:33:34 UTC
Yes, no? :)
Comment 17 Thiago Sousa Santos 2017-01-13 12:07:06 UTC
I agree with Tim. Let's do it.
Comment 18 Sebastian Dröge (slomo) 2017-01-18 11:38:01 UTC
commit 075ceffd9b21339b50130a5207a0df4ce9f1f5a7
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Thu Jan 5 19:10:52 2017 +0200

    hlsdemux: Detect media sequence number inconsistencies and fail
    
    Without failing, we would play back random parts of the stream which is
    arguably a worse user experience, and failing is also recommended by the
    spec here.
    
    And also handle live streams without any media sequence numbers at all
    properly, that is, make sure the sequence numbers are increasing instead
    of starting again at 0 every time.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=775665