fix: sustained bad network shouldn't pause #966
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
src/master-playlist-controller.js
Outdated
| @@ -556,6 +556,10 @@ export class MasterPlaylistController extends videojs.EventTarget { | |||
| }); | |||
|
|
|||
| this.mainSegmentLoader_.on('progress', () => { | |||
| if (this.mainSegmentLoader_.earlyabortTimer) { | |||
| window.clearTimeout(this.mainSegmentLoader_.earlyabortTimer); | |||
There was a problem hiding this comment.
We have to clear this on dispose as well
src/master-playlist-controller.js
Outdated
| @@ -615,6 +619,9 @@ export class MasterPlaylistController extends videojs.EventTarget { | |||
| // if we shouldn't switch to the next playlist, do nothing | |||
| if (!this.shouldSwitchToMedia_(nextPlaylist)) { | |||
| this.logger_(`earlyabort triggered, but we will not be switching from ${currentPlaylist.id} -> ${nextPlaylist.id}.`); | |||
| this.mainSegmentLoader_.earlyabortTimer = window.setTimeout(() => { | |||
| this.mainSegmentLoader_.trigger('earlyabort'); | |||
| }, 10 * 1000); | |||
There was a problem hiding this comment.
10s might be too long, what do you think about 5s?
There was a problem hiding this comment.
Probably should be target duration dependent or something, maybe even forward buffer? Made it 10s just for starters.
src/master-playlist-controller.js
Outdated
| @@ -615,6 +619,9 @@ export class MasterPlaylistController extends videojs.EventTarget { | |||
| // if we shouldn't switch to the next playlist, do nothing | |||
| if (!this.shouldSwitchToMedia_(nextPlaylist)) { | |||
| this.logger_(`earlyabort triggered, but we will not be switching from ${currentPlaylist.id} -> ${nextPlaylist.id}.`); | |||
| this.mainSegmentLoader_.earlyabortTimer = window.setTimeout(() => { | |||
There was a problem hiding this comment.
We should clear this timer every time we come into this function right?
There was a problem hiding this comment.
Yeah, I have that as part of the TODOs. I wasn't sure whether we had a player.setTimeout method available here or whether I needed to make sure that the timer is handled fully as appropriate.
The variable should be renamed to include a _ as well.
When playing back bipbop with experimentalBufferBasedABR turned on and we switch to Slow 3G in chrome, we may time out. This is because - earlyabort is turned off - our last attempt at downswitching occurs when our forward buffer is too full To mitigate this, when we get an early abort set up a timer for 10 seconds to see if earlyabort conditions are met again. If a progress event happens in the meantime, the earlyabort timer will be cancelled.
This is because progress gets triggered after the check for earlyabort and so our timer will always get triggered *immediately*.
|
I keep finding various edge cases here. I think we need to implement the proper timer based solution for rendition switches. |
gkatsev
added a commit
that referenced
this pull request
Oct 30, 2020
… on an interval (#978) This is a followup to #886. There, I noticed that if we get sustained bad network we will time out because we will an opportunity for earlyabort. At first, I tried working around by delaying earlyabort in that case (#966) but as I was getting that ready I kept finding edge cases that we needed to account for. Specifically, the issue is that we only run our ABR algorithm on progress and bandwidthchange events from the segment loader. This means that if the download stalls, we stop run our algorithm and will eventually stall playback. Instead, we should remove earlyabort altogether (f897dde) and set up an interval (currently 250ms) to re-run our ABR algorithm. This means that if the network becomes bad for a sustained period, we will drop down once the buffer allows us but if the network recovers, we will switch up appropriately as well. Also, the interval is stopped while we're paused. Fixes #964.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When playing back bipbop with experimentalBufferBasedABR turned on and
we switch to Slow 3G in chrome, we may time out.
This is because
too full
To mitigate this, when we get an early abort set up a timer for 10
seconds to see if earlyabort conditions are met again. If a progress
event happens in the meantime, the earlyabort timer will be cancelled.
TODO:
setTimeoutis cancelled properly if the player is disposed (not sure if we have aplayer.setTimeout()like method