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

feat: Retry on error #7038

Merged
merged 9 commits into from
Mar 23, 2021
67 changes: 62 additions & 5 deletions src/js/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -3306,7 +3306,7 @@ class Player extends Component {
}

/**
* Get or set the video source.
* Executes source setting and getting logic
*
* @param {Tech~SourceObject|Tech~SourceObject[]|string} [source]
* A SourceObject, an array of SourceObjects, or a string referencing
Expand All @@ -3315,12 +3315,14 @@ class Player extends Component {
* algorithms can take the `type` into account.
*
* If not provided, this method acts as a getter.
* @param {boolean} isRetry
* Indicates whether this is being called internally as a result of a retry
*
* @return {string|undefined}
* If the `source` argument is missing, returns the current source
* URL. Otherwise, returns nothing/undefined.
*/
src(source) {
handleSrc_(source, isRetry) {
// getter usage
if (typeof source === 'undefined') {
return this.cache_.src || '';
Expand All @@ -3342,7 +3344,17 @@ class Player extends Component {
// initial sources
this.changingSrc_ = true;

this.cache_.sources = sources;
if (!isRetry) {
// Only update the cached source list if we are not retrying a new source after error,
// since in that case we want to include the failed source(s) in the cache
this.cache_.sources = sources;

// If a retry was previously started, reset it
if (this.resetRetryOnError) {
this.resetRetryOnError();
alex-barstow marked this conversation as resolved.
Show resolved Hide resolved
}
}

this.updateSourceCaches_(sources[0]);

// middlewareSource is the source after it has been changed by middleware
Expand All @@ -3351,14 +3363,17 @@ class Player extends Component {

// since sourceSet is async we have to update the cache again after we select a source since
// the source that is selected could be out of order from the cache update above this callback.
this.cache_.sources = sources;
if (!isRetry) {
this.cache_.sources = sources;
}

this.updateSourceCaches_(middlewareSource);

const err = this.src_(middlewareSource);

if (err) {
if (sources.length > 1) {
return this.src(sources.slice(1));
return this.handleSrc_(sources.slice(1));
}

this.changingSrc_ = false;
Expand All @@ -3377,6 +3392,48 @@ class Player extends Component {

middleware.setTech(mws, this.tech_);
});

// Try another available source if this one fails before playback.
if (this.options_.retryOnError && sources.length > 1) {
const retry = () => {
// Remove the error modal
this.error(null);
this.handleSrc_(sources.slice(1), true);
};
const stopListeningForErrors = () => {
this.off('error', retry);
};

this.one('error', retry);
// If we've already initiated a retry, we don't need to add this again
if (!isRetry) {
this.one('playing', stopListeningForErrors);
}

this.resetRetryOnError = () => {
alex-barstow marked this conversation as resolved.
Show resolved Hide resolved
this.off('error', retry);
this.off('playing', stopListeningForErrors);
};
}
}

/**
* Get or set the video source.
*
* @param {Tech~SourceObject|Tech~SourceObject[]|string} [source]
* A SourceObject, an array of SourceObjects, or a string referencing
* a URL to a media source. It is _highly recommended_ that an object
* or array of objects is used here, so that source selection
* algorithms can take the `type` into account.
*
* If not provided, this method acts as a getter.
*
* @return {string|undefined}
* If the `source` argument is missing, returns the current source
* URL. Otherwise, returns nothing/undefined.
*/
src(source) {
return this.handleSrc_(source);
alex-barstow marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down
170 changes: 170 additions & 0 deletions test/unit/player.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,176 @@ QUnit.test('should asynchronously fire error events during source selection', fu
log.error.restore();
});

QUnit.test('should retry setting source if error occurs and retryOnError: true', function(assert) {
const player = TestHelpers.makePlayer({
techOrder: ['html5'],
retryOnError: true,
sources: [
{ src: 'http://vjs.zencdn.net/v/oceans.mp4', type: 'video/mp4' },
{ src: 'http://vjs.zencdn.net/v/oceans2.mp4', type: 'video/mp4' },
{ src: 'http://vjs.zencdn.net/v/oceans3.mp4', type: 'video/mp4' }
]
});

assert.deepEqual(
player.currentSource(),
{ src: 'http://vjs.zencdn.net/v/oceans.mp4', type: 'video/mp4' },
'first source set'
);

player.trigger('error');

assert.deepEqual(
player.currentSource(),
{ src: 'http://vjs.zencdn.net/v/oceans2.mp4', type: 'video/mp4' },
'second source set'
);

player.trigger('error');

assert.deepEqual(
player.currentSource(),
{ src: 'http://vjs.zencdn.net/v/oceans3.mp4', type: 'video/mp4' },
'last source set'
);

// No more sources to try so the previous source should remain
player.trigger('error');

assert.deepEqual(
player.currentSource(),
{ src: 'http://vjs.zencdn.net/v/oceans3.mp4', type: 'video/mp4' },
'last source remains'
);

assert.deepEqual(
player.currentSources(),
[
{ src: 'http://vjs.zencdn.net/v/oceans.mp4', type: 'video/mp4' },
{ src: 'http://vjs.zencdn.net/v/oceans2.mp4', type: 'video/mp4' },
{ src: 'http://vjs.zencdn.net/v/oceans3.mp4', type: 'video/mp4' }
],
'currentSources() correctly returns the full source list'
);

player.dispose();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a final check that verifies that player.currentSources() still contains the initial list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good thinking

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

});

QUnit.test('should not retry setting source if retryOnError: true and error occurs during playback', function(assert) {
const player = TestHelpers.makePlayer({
techOrder: ['html5'],
retryOnError: true,
sources: [
{ src: 'http://vjs.zencdn.net/v/oceans.mp4', type: 'video/mp4' },
{ src: 'http://vjs.zencdn.net/v/oceans2.mp4', type: 'video/mp4' },
{ src: 'http://vjs.zencdn.net/v/oceans3.mp4', type: 'video/mp4' }
]
});

assert.deepEqual(
player.currentSource(),
{ src: 'http://vjs.zencdn.net/v/oceans.mp4', type: 'video/mp4' },
'first source set'
);

player.trigger('error');

assert.deepEqual(
player.currentSource(),
{ src: 'http://vjs.zencdn.net/v/oceans2.mp4', type: 'video/mp4' },
'second source set'
);

// Playback starts then error occurs
player.trigger('playing');
player.trigger('error');

assert.deepEqual(
player.currentSource(),
{ src: 'http://vjs.zencdn.net/v/oceans3.mp4', type: 'video/mp4' },
'second source remains'
);

assert.deepEqual(
player.currentSources(),
[
{ src: 'http://vjs.zencdn.net/v/oceans.mp4', type: 'video/mp4' },
{ src: 'http://vjs.zencdn.net/v/oceans2.mp4', type: 'video/mp4' },
{ src: 'http://vjs.zencdn.net/v/oceans3.mp4', type: 'video/mp4' }
],
'currentSources() correctly returns the full source list'
);

player.dispose();
});

QUnit.test('aborts and resets retryOnError behavior if new src() call made during a retry', function(assert) {
const player = TestHelpers.makePlayer({
techOrder: ['html5'],
retryOnError: true,
sources: [
{ src: 'http://vjs.zencdn.net/v/oceans.mp4', type: 'video/mp4' },
{ src: 'http://vjs.zencdn.net/v/oceans2.mp4', type: 'video/mp4' },
{ src: 'http://vjs.zencdn.net/v/oceans3.mp4', type: 'video/mp4' }
]
});

assert.deepEqual(
player.currentSource(),
{ src: 'http://vjs.zencdn.net/v/oceans.mp4', type: 'video/mp4' },
'first source set'
);

player.trigger('error');

assert.deepEqual(
player.currentSource(),
{ src: 'http://vjs.zencdn.net/v/oceans2.mp4', type: 'video/mp4' },
'second source set'
);

// Setting a new source list should reset retry behavior and enable it for the new sources
player.src([
{ src: 'http://vjs.zencdn.net/v/newSource.mp4', type: 'video/mp4' },
{ src: 'http://vjs.zencdn.net/v/newSource2.mp4', type: 'video/mp4' },
{ src: 'http://vjs.zencdn.net/v/newSource3.mp4', type: 'video/mp4' }
]);

assert.deepEqual(
player.currentSource(),
{ src: 'http://vjs.zencdn.net/v/newSource.mp4', type: 'video/mp4' },
'first new source set'
);

player.trigger('error');

assert.deepEqual(
player.currentSource(),
{ src: 'http://vjs.zencdn.net/v/newSource2.mp4', type: 'video/mp4' },
'second new source set'
);

player.trigger('error');

assert.deepEqual(
player.currentSource(),
{ src: 'http://vjs.zencdn.net/v/newSource3.mp4', type: 'video/mp4' },
'third new source set'
);

assert.deepEqual(
player.currentSources(),
[
{ src: 'http://vjs.zencdn.net/v/newSource.mp4', type: 'video/mp4' },
{ src: 'http://vjs.zencdn.net/v/newSource2.mp4', type: 'video/mp4' },
{ src: 'http://vjs.zencdn.net/v/newSource3.mp4', type: 'video/mp4' }
],
'currentSources() correctly returns the full new source list'
);

player.dispose();
});

QUnit.test('should suppress source error messages', function(assert) {
sinon.stub(log, 'error');
const clock = sinon.useFakeTimers();
Expand Down