Skip to content
This repository has been archived by the owner on Jan 12, 2019. It is now read-only.

Fixes for HLS #279

Closed
wants to merge 1 commit into from
Closed

Fixes for HLS #279

wants to merge 1 commit into from

Conversation

mikrohard
Copy link
Contributor

Let me explain what (and why) these fixes are necessary.

All these issues can be reproduced and checked with this sample content:
https://dl.dropboxusercontent.com/u/8261657/hls-vod-sample.zip

1.) There could be content with only one program (PMT) and with a non-standard network PID (NIT). In these cases there can be more than one PAT entry and I don't see why we shouldn't be able to play such content. In the first commit I check wether the PAT entry is a PMT or NIT pid and throw an error only if there are multiple PMT-s.

2.) I'm not sure why... but it appears that finishFrame can be called when the stream extraData hasn't been parsed yet. The issue has been introduced with the commit ce689ae The additional check for existing extra data prevents crashing at playback start.

3.) If playlists contain "\r\n" line endings or even whitespace lines the m3u8 parser falsely detects those lines as "empty" segment URI-s. This results in faulty duration calculation and possible failure at playback start. By trimming line data before feeding it into the parser this issue is fixed.

4.) Once again... I'm not sure why. But sometimes during seeking within the sample content provided above there are no tags with desired pts values. This commit prevents crashing and fixes seeking in the provided sample. But there may be a problem in the logic somewhere else.

@mikrohard mikrohard force-pushed the hls-fixes branch 2 times, most recently from f4f9045 to e14622f Compare May 26, 2015 13:08
@mikrohard
Copy link
Contributor Author

I see that some unit tests fail... probably due to mikrohard@f90651b Guess it'll have to be fixed in a different way.

@dmlap
Copy link
Member

dmlap commented May 26, 2015

@mikrohard thanks! I think I have fixes for the tests worked out. I'd like to spend a little bit of time trying to work out the cause behind 4). Any suggestions on how best to reproduce it?

@mikrohard
Copy link
Contributor Author

Great news about the tests...

About issue no. 4... I can't give you a 100% reproducible case. But if you take the sample content I provided above and start seeking to a random position the moment is starts playing (sometimes even before it starts) it should be quite easy reproducible. I reverted my last commit and it took me about a minute to get this: https://dl.dropboxusercontent.com/u/8261657/videojs-hls-seeking-crash.png

@dmlap dmlap mentioned this pull request May 27, 2015
dmlap added a commit to dmlap/videojs-contrib-hls that referenced this pull request May 27, 2015
@dmlap dmlap mentioned this pull request May 27, 2015
@mikrohard mikrohard force-pushed the hls-fixes branch 2 times, most recently from e50fc82 to 9e9fed1 Compare May 29, 2015 08:41
@mikrohard
Copy link
Contributor Author

Ok... I did a little digging about issue no. 4.

I found out that seek always fails if seeking in between two segments where the preciseDuration of the segment is shorter of the duration provided by the playlist.

Here is a list of preciseDuration vs. duration reported by playlist for the above sample:
vod-01.ts: 9.548822222221643 vs. 9.873
vod-02.ts: 9.664244444443845 vs. 9.706
vod-03.ts: 9.70351111111138 vs. 9.707
vod-04.ts: 9.081799999999815 vs. 9.124
vod-05.ts: 9.473755555556156 vs. 9.457
vod-06.ts: 9.54339999999944 vs. 9.54
vod-07.ts: 9.914911111112684 vs. 9.915
vod-08.ts: 9.613066666666418 vs. 9.623
vod-09.ts: 9.891711111110636 vs. 9.873
vod-10.ts: 9.86848888888862 vs. 9.873
vod-11.ts: 6.246177777779288 vs. 6.248

What happens:
1.) User seeks to 19.3 seconds (this is between the preciseDuration 19.213 and playlist duration 19.579)
2.) The previous logic downloaded the segment at index 1 (because the seek time is below 19.579)
3.) The logic expected to find the PTS for time 19.3 (which was in segment at index 2 because the seek time is above 19.213)
4.) It crashed in the while loop

What I did to fix this:
1.) Use preciseDuration whenever possible. This selects a correct seek media index if preciseDurations are already known.
2.) Fallback to closest timestamp if desired PTS not found. I think this is a reasonable fallback because the error should never exceed a few hundred milliseconds. We could correct the mediaIndex and load the correct segment... but this would unnecessarily increase the seeking time.

@mikrohard
Copy link
Contributor Author

@dmlap Any comments about my findings on issue no. 4?

@dmlap
Copy link
Member

dmlap commented Jun 1, 2015

@mikrohard That must have been fun to track down :/ I'm a little busy working on some live enhancements this week. If you could write up a test case for this, I can get it pulled in quicker.

@mikrohard
Copy link
Contributor Author

Sorry... I did try to get grunt working but it just gets stuck at "Running "jshint:test" (jshint) task" and using 100% CPU. I don't have the time to debug this... I guess it's just gonna have to wait.

@mikrohard mikrohard force-pushed the hls-fixes branch 2 times, most recently from 74f57b3 to 1fcdef2 Compare June 18, 2015 07:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants