Skip to content

Commit

Permalink
fix: Exclude TIMEOUT errors when disabling streams (#7369)
Browse files Browse the repository at this point in the history
In #7368, we get stuck in a loop loading forever. This regression was
introduced in v4.4.0 and affects all v4.4, v4.5, v4.6, v4.7, and v4.8
releases, as well as v4.9.0-28, v4.9.2-caf1, v4.10.0-20, and v4.11.0-6.

The loop is composed of these elements:

1. an error that triggers disabling a stream
2. an error that doesn't resolve itself over time
3. an error that is slow enough to trigger that the first streams get
re-enabled
4. VOD content that doesn't change while we sit in the loop
5. enough streams to avoid exhausting them during the cycle

Only `TIMEOUT` errors can trigger this bug AFAICT, so we should exclude
those from the logic to disable streams. Note also that live streaming
already retries indefinitely by default, and that normal ABR logic will
change streams for us if we timeout due to a lack of bandwidth.

Disabling streams on `TIMEOUT` was suggested initially in #4764, but was
not a requirement of the OP. It was added out of caution in #4769, but
not really vetted. Because it was not ever explicitly needed, excluding
it is not a regression.

Closes #7368
  • Loading branch information
joeyparrish committed Sep 26, 2024
1 parent ea6def6 commit 00f0b06
Showing 1 changed file with 16 additions and 3 deletions.
19 changes: 16 additions & 3 deletions lib/media/streaming_engine.js
Original file line number Diff line number Diff line change
Expand Up @@ -1937,8 +1937,12 @@ shaka.media.StreamingEngine = class {
});

if (!waitingForAnotherStreamToRecover) {
const maxDisabledTime = this.getDisabledTime_(error);
if (maxDisabledTime) {
shaka.log.debug(logPrefix, 'Disabling stream due to quota', error);
}
const handled = this.playerInterface_.disableStream(
mediaState.stream, this.config_.maxDisabledTime);
mediaState.stream, maxDisabledTime);
if (handled) {
return;
}
Expand Down Expand Up @@ -2786,18 +2790,26 @@ shaka.media.StreamingEngine = class {
* @private
*/
async handleStreamingError_(mediaState, error) {
const logPrefix = shaka.media.StreamingEngine.logPrefix_(mediaState);

// If we invoke the callback right away, the application could trigger a
// rapid retry cycle that could be very unkind to the server. Instead,
// use the backoff system to delay and backoff the error handling.
await this.failureCallbackBackoff_.attempt();
this.destroyer_.ensureNotDestroyed();
// Try to recover from network errors
if (error.category === shaka.util.Error.Category.NETWORK) {

// Try to recover from network errors, but not timeouts.
// See https://github.com/shaka-project/shaka-player/issues/7368
if (error.category === shaka.util.Error.Category.NETWORK &&
error.code != shaka.util.Error.Code.TIMEOUT) {
if (mediaState.restoreStreamAfterTrickPlay) {
this.setTrickPlay(/* on= */ false);
return;
}
const maxDisabledTime = this.getDisabledTime_(error);
if (maxDisabledTime) {
shaka.log.debug(logPrefix, 'Disabling stream due to error', error);
}
error.handled = this.playerInterface_.disableStream(
mediaState.stream, maxDisabledTime);

Expand All @@ -2822,6 +2834,7 @@ shaka.media.StreamingEngine = class {

/**
* @param {!shaka.util.Error} error
* @return {number}
* @private
*/
getDisabledTime_(error) {
Expand Down

0 comments on commit 00f0b06

Please sign in to comment.