Improved seek behaviour

squarebracket commented 6 years ago

Description

This change requires videojs/video.js#5024, which takes care of the UI and API aspects of buffering.

SourceHandlers that use MSE have a problem: if they push a segment into a SourceBuffer and then seek close to the end, playback will stall and/or there will be a massive downswitch in quality. The general approach to fixing this that was discussed on slack was by setting the playback rate of the player to zero, buffering all that was required, and then restoring the previous playback rate. In my implementation, I've done this in the source handler.

Additionally, there is a new seekDeadline option for the source handler which controls how long a seek should take to resume. When seeking, the master playlist controller calculates the maximum bandwidth playlist for all requests needed to complete a full seek. The calculations are done using minRebufferMaxBandwidthSelector (renamed to maxBandwidthForDeadlineSelector).

Besides being a generally good change, this was somewhat needed for backwards compatibility with older video.js versions. Because of this, if you cause playback to stall after seek by seeking too close to the end of a segment, the second segment will have its spinner disappear (since you'll get a timeupdate after the waiting). This error is currently masked because seek quality is trashed when seeking, so you'll always be able to buffer the second segment before the spinner disappears from the timeupdate.

With the seek deadline enforcement, although not a real fix, it is significantly mitigated since you'll be kicked down to a lower rate for seeks which require downloading two segments. So you probably won't end up with any noticeable delay between spinner disappearing and playback resuming. For that reason, I think the "buffer appropriate number of segments" and the "seek deadline enforcement" should be considered a single PR.

When combined with #1239, this makes the seeking behavior vastly improved:

playback is on highest rate
seek is requested by client to right before the end of a segment
playback pauses, spinner appears, player.seeking() is now true
buffer is sufficiently filled at new time in reasonable timeframe, downswitching if necessary
two segments at lower rate are now in buffer
playback resumes, spinner disappears, player fires playback events, player.seeking() is now false
next segment download is at highest rate

videojs/videojs-contrib-hls#1239 has been reviewed, but I don't believe it's been tested. Let me know if you want me to stage the changes here so it can all be considered as part of a single PR.

Specific Changes proposed

Added seekDeadline option for controlling how long a seek should take to resume
seeking() is deferred from the tech to the SourceHandler
setCurrentTime stores the current playback rate and set the tech's playback rate to zero
Inside SegmentLoader#handleUpdateEnd_, if we have enough buffer that we can make another request in that time, fire buffered on this
The MasterPlaylistController receives the buffered event and restores the previous playback rate
minRebufferMaxBandwidthSelector has been renamed to maxBandwidthForDeadlineSelector, and made more generic
- it now accepts an extraRequests parameter which is added to the numRequests for the segment
- instead of returning rebufferingImpact, it now just returns requestTimeEstimate
For the deadline passed to maxBandwidthForDeadlineSelector, abortRequestEarly_ uses either the current calculation (when not seeking) or the time elapsed since seeking started
Math inside abortRequestEarly_ is now a bit simpler because it's now using the requestTimeEstimate instead of rebufferingImpact
The quality used when seeking is determined by how long it will take to get the number of segments needed to fill the buffer after the seek
The MPC's fastQualityChange_ now accepts a nextPlaylist option which will be used instead of this.selectPlaylist() when given
The playback watcher has been modified to not bork when the playback rate is zero and nothing is happening

Requirements Checklist

[x] Feature implemented / Bug fixed
[ ] If necessary, more likely in a feature request than a bug fix
- [ ] Unit Tests updated or fixed
- [ ] Docs/guides updated
- [ ] Example created (starter template on JSBin)
[ ] Reviewed by Two Core Contributors

Notes

This is implemented assuming that the video segment loader will always finish after the other segment loaders. I also made an implementation that guarantees all segment loaders have enough data, if you want to compare what that would be like. I found this to be much more concise which is why it gets the PR.

Figuring out some of the testing stuff was becoming troublesome. It'll probably take me a while to get through adding all the tests.

squarebracket commented 6 years ago

I think I've managed to nail down all the fiddly bits here.

I had some trouble with the tests, like mediachange not firing for some reason, so it'll probably take me a bit to get through making the tests, but the code itself should be feature-complete.

squarebracket commented 6 years ago

Added browser workarounds for old vjs versions

forbesjo commented 5 years ago

Thank you for your PR but we will not be accepting new changes for this repo and will be archived very soon. I would advise you to open your PR against the next iteration of this project at https://github.com/videojs/http-streaming.

squarebracket commented 5 years ago

For anyone who lands here, I opened PR videojs/http-streaming#370.

videojs / videojs-contrib-hls