iheartradio / open-m3u8

Open Source m3u8 Parser
Other
248 stars 94 forks source link

first working cut of discontinuity support #25

Closed ketankhairnar closed 8 years ago

ketankhairnar commented 8 years ago

please review and if needed suggest better approach

Wopple commented 8 years ago

Hello, and thank you for your contribution!

If we take EXT-X-DISCONTINUITY-SEQUENCE into consideration (don't need to implement yet, default value is 0), it reveals a more appropriate API:

"A segment's Discontinuity Sequence Number is the value of the EXT-X-DISCONTINUITY-SEQUENCE tag (or zero if none) plus the number of EXT-X-DISCONTINUITY tags in the Playlist preceding the URI line of the segment."

This suggests that TrackData should have a discontinuitySequenceNumber (DSN) property, not a boolean of whether or not there is a discontinuity tag. For example:

#...
#EXT-X-DISCONTINUITY-SEQUENCE:2
#EXTINF...
track1.mp3
#EXT-X-DISCONTINUITY
#EXTINF...
track2.mp3
#EXTINF...
track3.mp3
#EXT-X-DISCONTINUITY
#EXTINF...
track4.mp3

would indicate the following discontinuity sequence numbers: track1 DSN: 2 track2 DSN: 3 track3 DSN: 3 track4 DSN: 4

So all TrackData will have a DSN even if the value is 0 for all segments in the playlist (i.e. no discontinuities). The boolean becomes redundant because a change in the DSN between segments indicates there was a discontinuity.

On a more minor note, please keep the 4-spaces per indent consistent throughout. Otherwise, the coding style looks good, and thank you for including tests!

ketankhairnar commented 8 years ago

@Wopple I've limited knowledge on HLS players. Will all players support version 16 of draft-pantos-http-live-streaming? EXT-X-DISCONTINUITY-SEQUENCE got introduced in version 12 and is missing earlier. Whereas EXT-X-DISCONTINUITY in older versions too.

Let me know what you think of this backward compatibility?

Wopple commented 8 years ago

It is rough when the specification is still so volatile. It doesn't help that everyone is making their own conventions! The goal of this library is to be as up-to-date as possible with respect to the specification.

It looks like having a discontinuity sequence number will still allow old versions of the spec without EXT-X-DISCONTINUITY-SEQUENCE to work since the default value is 0 if it doesn't exist. It won't be an ideal API for supporting old versions of the spec, but it will be ideal for the newer versions and still support the old versions. It is beyond the scope of this library to support multiple APIs one for each version of the spec.

Wopple commented 8 years ago

@ketankhairnar I've got another idea. How about the following:

int TrackData.getDiscontinuitySequenceNumber();
boolean MediaPlaylist.hasDiscontinuity(int index);

The MediaPlaylist holds all the segments, so given an index, it can determine if there is a discontinuity on that segment.

Wopple commented 8 years ago

Thinking this through more, I see it as a question between which of these two approaches is better:

From the spec: "The EXT-X-DISCONTINUITY-SEQUENCE tag allows synchronization between different Renditions of the same Variant Stream or different Variant Streams that have EXT-X-DISCONTINUITY tags in their Media Playlists."

Sounds like the DSN is only useful when looking at whole playlists. Therefore, making the actual discontinuities more accessible is the better way to go. So I am changing my mind on this subject and am in favor of your original design. :)

I will try to get this merged, though I will have to make some superficial modifications.

Wopple commented 8 years ago

I amended your commit to clean up some minor issues. I also added a MediaPlaylist.getDiscontinuitySequenceNumber method. You can view the changes here:

https://github.com/iheartradio/open-m3u8/tree/discontinuity-sequence-number

Can you please confirm that this works for you before I merge?

ketankhairnar commented 8 years ago

Thanks. Looks ok.

Wopple commented 8 years ago

@ketankhairnar I released 0.2.2, please give it a couple hours before it is made available.