fix: stutter after fast quality change in IE/Edge #213
Conversation
src/master-playlist-controller.js
Outdated
|
|
||
| if (media === this.masterPlaylistLoader_.media()) { | ||
| return; | ||
| } | ||
|
|
||
| this.tech_.pause(); |
There was a problem hiding this comment.
Is it necessary for us to pause? I think that may have inadvertent side effects.
There was a problem hiding this comment.
The rendition switch occurs without a stutter even without pausing/playing, but for some reason timeupdate events continue to fire even while playback has stopped and the loading spinner isn't shown. This only happens in IE/Edge.
There was a problem hiding this comment.
We may want to look into that further. I'm hesitant for us to initiate a pause, as it could have inadvertent behavior with user integrations. For instance, if someone is listening to paused events to display an overlay or make another action happen, then this will cause that behavior to trigger.
src/master-playlist-controller.js
Outdated
| this.mainSegmentLoader_.resetEverything(() => { | ||
| // Since this is not a typical seek, we avoid the seekTo method which can cause | ||
| // segments from the previously enabled rendition to load before the new playlist | ||
| // has finished loading | ||
| this.tech_.setCurrentTime(this.tech_.currentTime()); | ||
| this.tech_.setCurrentTime(this.tech_.currentTime() + .04); |
There was a problem hiding this comment.
Should we be seeking forwards for all browsers, or just those that are problematic?
There was a problem hiding this comment.
I was on the fence about whether it's better to minimize the scope of the forward seeking, or to omit any user agent checks and have uniform quality switch behavior across browsers. I'll defer to your judgement on this.
There was a problem hiding this comment.
I'd be interested to hear others' thoughts as well, but I think minimizing the scope may be better, just to limit potential impact, especially when it's only one set of browsers (IE/Edge).
|
Added the IE/Edge browser checks, removed the pause/play, and added additional tests |
| }; | ||
|
|
||
| // makes sure the resetEverything callback is queued when sourceUpdater_.remove() gets called | ||
| this.masterPlaylistController.mainSegmentLoader_.sourceUpdater_.processedAppend_ = true; |
There was a problem hiding this comment.
For this and below, instead of calling into private properties, would it make sense to append a segment first in the test. This might also reflect real-world usage better, as I don't believe we will switch renditions until we are in "walking forwards" mode after a segment download.
|
|
||
| this.masterPlaylistController.mediaSource.sourceBuffers[0].trigger('updateend'); | ||
|
|
||
| assert.equal(this.player.currentTime(), timeBeforeSwitch, 'seeks in place on fast quality switch'); |
There was a problem hiding this comment.
Although this verifies the time, I don't believe a seek is verified to have happened.
|
|
||
| const timeBeforeSwitch = this.player.currentTime(); | ||
|
|
||
| this.masterPlaylistController.fastQualityChange_(); | ||
|
|
||
| this.masterPlaylistController.mediaSource.sourceBuffers[0].trigger('updateend'); |
There was a problem hiding this comment.
Is this responding to an actual "append?" I think if the fastQualityChange happened, there would still have to be a segment request before an append would happen.
|
|
||
| const timeBeforeSwitch = this.player.currentTime(); | ||
|
|
||
| videojs.browser.IE_VERSION = null; | ||
| videojs.browser.IS_EDGE = true; | ||
|
|
||
| this.masterPlaylistController.fastQualityChange_(); | ||
|
|
||
| this.masterPlaylistController.mediaSource.sourceBuffers[0].trigger('updateend'); |
|
|
||
| const timeBeforeSwitch = this.player.currentTime(); | ||
|
|
||
| videojs.browser.IE_VERSION = 11; | ||
| videojs.browser.IS_EDGE = false; | ||
|
|
||
| this.masterPlaylistController.fastQualityChange_(); | ||
|
|
||
| this.masterPlaylistController.mediaSource.sourceBuffers[0].trigger('updateend'); |
Description
This tweaks the changes in #113 to fix a stutter in IE/Edge after the buffer is cleared. In the initial PR, seeking in place was sufficient to address this issue in Chrome, but we need to seek further behind/ahead of the current time in order to also fix the issue in IE/Edge.