Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Safari backwards seek bug #2189

Merged
merged 1 commit into from
Sep 28, 2017

Conversation

robertbryer
Copy link
Contributor

There's a bug in Safari on macOS, where appending a segment to the start of a buffered range causes a portion of the original start of the buffer to be removed.

To reproduce this bug:
Seek to a point in the video, buffer for a few seconds.
Seek back 15-20 seconds, play the video forward to the first seek point.
The video stops for lack of buffer at the point first downloaded segment. Dash.js believes it has already downloaded this segment, so will fetch later ones instead.

This fixes it by watching the onSeeked event, and then removing any buffered ranges ahead of the currentTime(for macOS user agents only). There's no point just re-fetching and replacing the missing segment because the problem occurs again filling in that gap.

@epiclabsDASH
Copy link
Contributor

Hi @robertbryer. Thanks for the contribution, I could reproduce the issue very easily following your instructions.

Fix looks fine to me but I need to spend more time with it to ensure there is no regression.

Thanks!

@nicosang
Copy link
Contributor

Hi,

in my opinion, this PR is a workaround. The main question is why dash.js doesn't download the missing segment(s). Delete already downloaded segments in order to download them again is not optimal. I agree we can merge this PR to solve the problem temporarily in Safari, but keep in mind, we have to really solve it for good.

Nico

@robertbryer
Copy link
Contributor Author

There is no choice in this case except to clear all the segments ahead because appending to fill a gap at index n makes a new gap at index n+1, and so on for the entire buffer ahead.

But yes, I did look at using .buffered to see if a fragment had been downloaded, instead of using the fragmentModel class. - the issue in this case is that Safari can leave gaps in the buffer that are smaller than the tolerance that we use to grab chunks:

epsilon = (timeThreshold === undefined || timeThreshold === null) ? fd / 2 : timeThreshold;

If verifying against .buffered is wanted, I did prototype it in this commit:
bbc@d441341
But since it doesn't fix the Safari bug, and I don't know of any user agents that are clearing the buffer silently, I didn't carry on with it.

@epiclabsDASH
Copy link
Contributor

@robertbryer, do you have a reference to this issue in https://bugs.webkit.org?

@robertbryer
Copy link
Contributor Author

@epiclabsDASH
Copy link
Contributor

Thanks!

@epiclabsDASH epiclabsDASH merged commit a88e758 into Dash-Industry-Forum:development Sep 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants