Skip to content

Commit

Permalink
refactor: clean up parameters of excludePlaylist (#1304)
Browse files Browse the repository at this point in the history 
Co-authored-by: Pat O'Neill <[email protected]>

`PlaylistController#excludePlaylist` now takes a single object with 3 properties:

* `playlistToExclude`, defaults to the currently selected playlist
* `error`
* `playlistExclusionDuration`, defaults to the value passed in the constructor

BREAKING CHANGE: This changes the arguments for the `PlaylistController#excludePlaylist` method to take a single object instead of multiple arguments.
  • Loading branch information
@gesinger
gesinger authored Aug 19, 2022
1 parent 02c3c77 commit ca3162b
Show file tree
Hide file tree
Showing 6 changed files with 116 additions and 70 deletions.
2 changes: 1 addition & 1 deletion src/media-groups.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ export const onError = {
// Default track encountered an error. All we can do now is exclude the current
// rendition and hope another will switch audio groups
excludePlaylist({
message: 'Problem encountered loading the default audio track.'
error: { message: 'Problem encountered loading the default audio track.' }
});
return;
}
Expand Down
5 changes: 3 additions & 2 deletions src/playback-watcher.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,8 +233,9 @@ export default class PlaybackWatcher {
// TODO: should we exclude audio tracks rather than main tracks
// when type is audio?
pc.excludePlaylist({
message: `Excessive ${type} segment downloading detected.`
}, Infinity);
error: { message: `Excessive ${type} segment downloading detected.` },
playlistExclusionDuration: Infinity
});
}

/**
Expand Down
73 changes: 46 additions & 27 deletions src/playlist-controller.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -592,7 +592,9 @@ export class PlaylistController extends videojs.EventTarget {
});

this.mainPlaylistLoader_.on('error', () => {
this.excludePlaylist(this.mainPlaylistLoader_.error);
const error = this.mainPlaylistLoader_.error;

this.excludePlaylist({ playlistToExclude: error.playlist, error });
});

this.mainPlaylistLoader_.on('mediachanging', () => {
Expand Down Expand Up @@ -644,8 +646,10 @@ export class PlaylistController extends videojs.EventTarget {
// one is updating (and give the player a chance to re-adjust to the
// safe live point).
this.excludePlaylist({
message: 'Playlist no longer updating.',
reason: 'playlist-unchanged'
error: {
message: 'Playlist no longer updating.',
reason: 'playlist-unchanged'
}
});
// useful for monitoring QoS
this.tech_.trigger('playliststuck');
Expand Down Expand Up @@ -778,7 +782,9 @@ export class PlaylistController extends videojs.EventTarget {
}

this.mainSegmentLoader_.on('error', () => {
this.excludePlaylist(this.mainSegmentLoader_.error());
const error = this.mainSegmentLoader_.error();

this.excludePlaylist({ playlistToExclude: error.playlist, error });
});

this.mainSegmentLoader_.on('appenderror', () => {
Expand Down Expand Up @@ -816,9 +822,12 @@ export class PlaylistController extends videojs.EventTarget {
this.delegateLoaders_('all', ['abort']);

this.excludePlaylist({
message: 'Aborted early because there isn\'t enough bandwidth to complete the ' +
'request without rebuffering.'
}, ABORT_EARLY_EXCLUSION_SECONDS);
error: {
message: 'Aborted early because there isn\'t enough bandwidth to complete ' +
'the request without rebuffering.'
},
playlistExclusionDuration: ABORT_EARLY_EXCLUSION_SECONDS
});
});

const updateCodecs = () => {
Expand Down Expand Up @@ -1122,25 +1131,31 @@ export class PlaylistController extends videojs.EventTarget {
}

/**
* Exclude a playlist when an error occurs for a set amount of time
* making it unavailable for selection by the rendition selection algorithm
* and then forces a new playlist (rendition) selection.
* Exclude a playlist for a set amount of time, making it unavailable for selection by
* the rendition selection algorithm, then force a new playlist (rendition) selection.
*
* @param {Object=} error an optional error that may include the playlist
* to exclude
* @param {number=} playlistExclusionDuration an optional number of seconds to exclude the
* playlist
* @param {Object=} playlistToExclude
* the playlist to exclude, defaults to the currently selected playlist
* @param {Object=} error
* an optional error
* @param {number=} playlistExclusionDuration
* an optional number of seconds to exclude the playlist
*/
excludePlaylist(error = {}, playlistExclusionDuration) {
excludePlaylist({
playlistToExclude = this.mainPlaylistLoader_.media(),
error = {},
playlistExclusionDuration
}) {

// If the `error` was generated by the playlist loader, it will contain
// the playlist we were trying to load (but failed) and that should be
// excluded instead of the currently selected playlist which is likely
// out-of-date in this scenario
const playlistToExclude = error.playlist || this.mainPlaylistLoader_.media();
playlistToExclude = playlistToExclude || this.mainPlaylistLoader_.media();

playlistExclusionDuration = playlistExclusionDuration ||
error.playlistExclusionDuration ||
this.playlistExclusionDuration;
error.playlistExclusionDuration ||
this.playlistExclusionDuration;

// If there is no current playlist, then an error occurred while we were
// trying to load the main OR while we were disposing of the tech
Expand Down Expand Up @@ -1694,8 +1709,8 @@ export class PlaylistController extends videojs.EventTarget {
// no codecs, no playback.
if (!codecs.audio && !codecs.video) {
this.excludePlaylist({
playlist: this.media(),
message: 'Could not determine codecs for playlist.',
playlistToExclude: this.media(),
error: { message: 'Could not determine codecs for playlist.' },
playlistExclusionDuration: Infinity
});
return;
Expand Down Expand Up @@ -1746,9 +1761,11 @@ export class PlaylistController extends videojs.EventTarget {
}, '') + '.';

this.excludePlaylist({
playlist: this.media(),
internal: true,
message,
playlistToExclude: this.media(),
error: {
internal: true,
message
},
playlistExclusionDuration: Infinity
});
return;
Expand All @@ -1771,10 +1788,12 @@ export class PlaylistController extends videojs.EventTarget {

if (switchMessages.length) {
this.excludePlaylist({
playlist: this.media(),
message: `Codec switching not supported: ${switchMessages.join(', ')}.`,
playlistExclusionDuration: Infinity,
internal: true
playlistToExclude: this.media(),
error: {
message: `Codec switching not supported: ${switchMessages.join(', ')}.`,
internal: true
},
playlistExclusionDuration: Infinity
});
return;
}
Expand Down
7 changes: 5 additions & 2 deletions src/videojs-http-streaming.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1019,8 +1019,11 @@ class VhsHandler extends Component {
this.player_.tech_.on('keystatuschange', (e) => {
if (e.status === 'output-restricted') {
this.playlistController_.excludePlaylist({
playlist: this.playlistController_.media(),
message: `DRM keystatus changed to ${e.status}. Playlist will fail to play. Check for HDCP content.`,
playlistToExclude: this.playlistController_.media(),
error: {
message: `DRM keystatus changed to ${e.status}. Playlist will fail to play. ` +
'Check for HDCP content.'
},
playlistExclusionDuration: Infinity
});
}
Expand Down
92 changes: 56 additions & 36 deletions test/playlist-controller.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4753,7 +4753,7 @@ QUnit.module('PlaylistController codecs', {
this.pc = this.playlistController;

this.exclusionList = [];
this.pc.excludePlaylist = (playlist) => this.exclusionList.push(playlist);
this.pc.excludePlaylist = (options) => this.exclusionList.push(options);

this.contentSetup = (options) => {
const {
Expand Down Expand Up @@ -4988,8 +4988,8 @@ QUnit.test('excludes playlist without detected audio/video', function(assert) {

assert.deepEqual(this.exclusionList, [{
playlistExclusionDuration: Infinity,
message: 'Could not determine codecs for playlist.',
playlist: {attributes: {}}
playlistToExclude: { attributes: {} },
error: { message: 'Could not determine codecs for playlist.' }
}], 'excluded playlist');
assert.deepEqual(codecs, void 0, 'no codecs returned');
});
Expand All @@ -5009,9 +5009,11 @@ QUnit.test('excludes unsupported muxer codecs for ts', function(assert) {

assert.deepEqual(this.exclusionList, [{
playlistExclusionDuration: Infinity,
playlist: {attributes: {}},
internal: true,
message: 'muxer does not support codec(s): "hvc1.2.4.L123.B0,ac-3".'
playlistToExclude: {attributes: {}},
error: {
internal: true,
message: 'muxer does not support codec(s): "hvc1.2.4.L123.B0,ac-3".'
}
}], 'excluded playlist');
assert.deepEqual(codecs, void 0, 'codecs returned');
});
Expand All @@ -5035,9 +5037,11 @@ QUnit.test('excludes unsupported browser codecs for muxed fmp4', function(assert

assert.deepEqual(this.exclusionList, [{
playlistExclusionDuration: Infinity,
playlist: {attributes: {}},
internal: true,
message: 'browser does not support codec(s): "hvc1.2.4.L123.B0,ac-3".'
playlistToExclude: {attributes: {}},
error: {
internal: true,
message: 'browser does not support codec(s): "hvc1.2.4.L123.B0,ac-3".'
}
}], 'excluded playlist');
assert.deepEqual(codecs, void 0, 'codecs returned');
});
Expand All @@ -5058,9 +5062,11 @@ QUnit.test('excludes unsupported muxer codecs for muxed ts', function(assert) {

assert.deepEqual(this.exclusionList, [{
playlistExclusionDuration: Infinity,
playlist: {attributes: {}},
internal: true,
message: 'muxer does not support codec(s): "hvc1.2.4.L123.B0,ac-3".'
playlistToExclude: {attributes: {}},
error: {
internal: true,
message: 'muxer does not support codec(s): "hvc1.2.4.L123.B0,ac-3".'
}
}], 'excluded playlist');
assert.deepEqual(codecs, void 0, 'codecs returned');
});
Expand All @@ -5083,9 +5089,11 @@ QUnit.test('excludes unsupported browser codecs for fmp4', function(assert) {

assert.deepEqual(this.exclusionList, [{
playlistExclusionDuration: Infinity,
playlist: {attributes: {}},
internal: true,
message: 'browser does not support codec(s): "hvc1.2.4.L123.B0,ac-3".'
playlistToExclude: {attributes: {}},
error: {
internal: true,
message: 'browser does not support codec(s): "hvc1.2.4.L123.B0,ac-3".'
}
}], 'excluded playlist');
assert.deepEqual(codecs, void 0, 'codecs returned');
});
Expand Down Expand Up @@ -5113,9 +5121,12 @@ QUnit.test('excludes unsupported codecs video ts, audio fmp4', function(assert)

assert.deepEqual(this.exclusionList, [{
playlistExclusionDuration: Infinity,
playlist: {attributes: {AUDIO: 'low-quality'}},
internal: true,
message: 'muxer does not support codec(s): "hvc1.2.4.L123.B0", browser does not support codec(s): "ac-3".'
playlistToExclude: {attributes: {AUDIO: 'low-quality'}},
error: {
internal: true,
message: 'muxer does not support codec(s): "hvc1.2.4.L123.B0", ' +
'browser does not support codec(s): "ac-3".'
}
}], 'excluded playlist');
assert.deepEqual(codecs, void 0, 'codecs returned');
});
Expand Down Expand Up @@ -5143,9 +5154,12 @@ QUnit.test('excludes unsupported codecs video fmp4, audio ts', function(assert)

assert.deepEqual(this.exclusionList, [{
playlistExclusionDuration: Infinity,
playlist: {attributes: {AUDIO: 'low-quality'}},
internal: true,
message: 'browser does not support codec(s): "hvc1.2.4.L123.B0", muxer does not support codec(s): "ac-3".'
playlistToExclude: {attributes: {AUDIO: 'low-quality'}},
error: {
internal: true,
message: 'browser does not support codec(s): "hvc1.2.4.L123.B0", ' +
'muxer does not support codec(s): "ac-3".'
}
}], 'excluded playlist');
assert.deepEqual(codecs, void 0, 'codecs returned');
});
Expand Down Expand Up @@ -5173,9 +5187,11 @@ QUnit.test('excludes all of audio group on unsupported audio', function(assert)

assert.deepEqual(this.exclusionList, [{
playlistExclusionDuration: Infinity,
playlist: {attributes: {AUDIO: 'low-quality'}, id: 'bar'},
internal: true,
message: 'muxer does not support codec(s): "hvc1.2.4.L123.B0,ac-3".'
playlistToExclude: {attributes: {AUDIO: 'low-quality'}, id: 'bar'},
error: {
internal: true,
message: 'muxer does not support codec(s): "hvc1.2.4.L123.B0,ac-3".'
}
}], 'excluded playlist');
assert.deepEqual(codecs, void 0, 'codecs returned');
assert.equal(this.main.playlists[2].id, 'foo', 'playlist 3 is the one we added');
Expand Down Expand Up @@ -5218,9 +5234,11 @@ QUnit.test('excludes on codec switch if codec switching not supported', function

assert.deepEqual(this.exclusionList, [{
playlistExclusionDuration: Infinity,
playlist: {attributes: {AUDIO: 'low-quality'}},
internal: true,
message: 'Codec switching not supported: "avc1.4c400d" -> "hvc1.2.4.L123.B0", "mp4a.40.2" -> "ac-3".'
playlistToExclude: {attributes: {AUDIO: 'low-quality'}},
error: {
internal: true,
message: 'Codec switching not supported: "avc1.4c400d" -> "hvc1.2.4.L123.B0", "mp4a.40.2" -> "ac-3".'
}
}], 'excluded playlist');
assert.deepEqual(codecs, void 0, 'codecs returned');
});
Expand Down Expand Up @@ -5429,8 +5447,8 @@ QUnit.module('PlaylistController - exclusion behavior', {
assert.equal(this.pc.media(), this.pc.main().playlists[0], 'selected first playlist');

this.pc.excludePlaylist({
internal: true,
playlist: this.pc.main().playlists[0],
playlistToExclude: this.pc.main().playlists[0],
error: { internal: true },
playlistExclusionDuration: Infinity
});

Expand Down Expand Up @@ -6012,7 +6030,7 @@ QUnit.test("don't exclude only playlist unless it was excluded forever", functio

mpl.load = (delay) => (shouldDelay = delay);

pc.excludePlaylist();
pc.excludePlaylist({});

assert.notOk('excludeUntil' in playlist, 'playlist was not excluded since excludeDuration was finite');
assert.ok(shouldDelay, 'we delay retry since it is the final rendition');
Expand Down Expand Up @@ -6048,7 +6066,9 @@ QUnit.test("don't exclude only playlist unless it was excluded forever", functio
});

// exclude forever
pc.excludePlaylist({}, Infinity);
pc.excludePlaylist({
playlistExclusionDuration: Infinity
});

assert.ok('excludeUntil' in playlist, 'playlist was excluded');
assert.notOk(shouldDelay, 'value was not changed');
Expand Down Expand Up @@ -6084,7 +6104,7 @@ QUnit.test('switch playlists if current playlist gets excluded and re-include if

mpl.load = (delay) => (shouldDelay = delay);

pc.excludePlaylist();
pc.excludePlaylist({});

assert.ok('excludeUntil' in playlist, 'playlist was excluded since there is another playlist');
assert.notOk(shouldDelay, 'we do not delay retry since it is not the final rendition');
Expand All @@ -6096,7 +6116,7 @@ QUnit.test('switch playlists if current playlist gets excluded and re-include if
this.standardXHRResponse(this.requests.shift());
playlist2 = mpl.main.playlists[1];

pc.excludePlaylist();
pc.excludePlaylist({});

assert.ok('excludeUntil' in playlist2, 'playlist2 was excluded');
assert.notOk('excludeUntil' in playlist, 'playlist was re-included');
Expand Down Expand Up @@ -6133,13 +6153,13 @@ QUnit.test('Playlist is excluded indefinitely if number of playlistErrors_ excee

assert.equal(playlist.playlistErrors_, 0, 'playlistErrors_ starts at zero');

pc.excludePlaylist();
pc.excludePlaylist({});

assert.ok('excludeUntil' in playlist, 'playlist was excluded');
assert.equal(playlist.playlistErrors_, 1, 'we incremented playlistErrors_');
assert.notEqual(playlist.excludeUntil, Infinity, 'The playlist was not excluded indefinitely');

pc.excludePlaylist();
pc.excludePlaylist({});

assert.equal(playlist.playlistErrors_, 2, 'we incremented playlistErrors_');
assert.equal(playlist.excludeUntil, Infinity, 'The playlist was excluded indefinitely');
Expand Down Expand Up @@ -6179,7 +6199,7 @@ QUnit.test('should delay loading of new playlist if lastRequest was less than ha
};
playlist2.lastRequest = Date.now() - 1000;

pc.excludePlaylist();
pc.excludePlaylist({});

assert.ok('excludeUntil' in playlist, 'playlist was excluded since there is another playlist');
assert.ok(shouldDelay, 'we delay retry since second rendition was loaded less than half target duration ago');
Expand Down
Loading

0 comments on commit ca3162b

Please sign in to comment.