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

refactor: rename 'blacklist' to 'exclude' #1274

Merged
merged 8 commits into from
Jun 23, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/a-walk-through-vhs.md
Original file line number Diff line number Diff line change
Expand Up @@ -247,4 +247,4 @@ The video can start playing as soon as there's enough audio and video (for muxed

But once `SegmentLoader` does finish, it starts the process again, looking for new content.

There are other modules, and other functions of the code (e.g., blacklisting logic, ABR, etc.), but this is the most critical path of VHS, the one that allows video to play in the browser.
There are other modules, and other functions of the code (e.g., excluding logic, ABR, etc.), but this is the most critical path of VHS, the one that allows video to play in the browser.
55 changes: 27 additions & 28 deletions src/master-playlist-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
vhs: this.vhs_,
master: this.master(),
mediaTypes: this.mediaTypes_,
excludeCurrentPlaylist: this.excludeCurrentPlaylist.bind(this)
excludePlaylist: this.excludePlaylist.bind(this)
});

this.triggerPresenceUsage_(this.master(), media);
Expand Down Expand Up @@ -592,7 +592,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
});

this.masterPlaylistLoader_.on('error', () => {
this.excludeCurrentPlaylist(this.masterPlaylistLoader_.error);
this.excludePlaylist(this.masterPlaylistLoader_.error);
});

this.masterPlaylistLoader_.on('mediachanging', () => {
Expand Down Expand Up @@ -643,7 +643,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
// exclude it and switch to another playlist in the hope that that
// one is updating (and give the player a chance to re-adjust to the
// safe live point).
this.excludeCurrentPlaylist({
this.excludePlaylist({
message: 'Playlist no longer updating.',
reason: 'playlist-unchanged'
});
Expand Down Expand Up @@ -778,7 +778,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
}

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

this.mainSegmentLoader_.on('appenderror', () => {
Expand Down Expand Up @@ -815,7 +815,7 @@ export class MasterPlaylistController extends videojs.EventTarget {

this.delegateLoaders_('all', ['abort']);

this.excludeCurrentPlaylist({
this.excludePlaylist({
message: 'Aborted early because there isn\'t enough bandwidth to complete the ' +
'request without rebuffering.'
}, ABORT_EARLY_EXCLUSION_SECONDS);
Expand Down Expand Up @@ -1143,20 +1143,20 @@ export class MasterPlaylistController extends videojs.EventTarget {
* @param {number=} playlistExclusionDuration an optional number of seconds to exclude the
* playlist
*/
excludeCurrentPlaylist(error = {}, playlistExclusionDuration) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be a bit more work, but it might be worth us having to functions: excludeCurrentPlaylist and excludePlaylist

excludeCurrentPlaylist can call excludePlaylist with an explicit playlist (this.masterPlaylistLoader_.media()), and excludePlaylist accepts a playlistToExclude param.

This would let the code read clearer for callers of the function.

excludePlaylist(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 currentPlaylist = error.playlist || this.masterPlaylistLoader_.media();
const playlistToExclude = error.playlist || this.masterPlaylistLoader_.media();

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

// If there is no current playlist, then an error occurred while we were
// trying to load the master OR while we were disposing of the tech
if (!currentPlaylist) {
if (!playlistToExclude) {
this.error = error;

if (this.mediaSource.readyState !== 'open') {
Expand All @@ -1168,16 +1168,16 @@ export class MasterPlaylistController extends videojs.EventTarget {
return;
}

currentPlaylist.playlistErrors_++;
playlistToExclude.playlistErrors_++;

const playlists = this.masterPlaylistLoader_.master.playlists;
const enabledPlaylists = playlists.filter(isEnabled);
const isFinalRendition = enabledPlaylists.length === 1 && enabledPlaylists[0] === currentPlaylist;
const isFinalRendition = enabledPlaylists.length === 1 && enabledPlaylists[0] === playlistToExclude;

// Don't exclude the only playlist unless it was excluded
// forever
if (playlists.length === 1 && playlistExclusionDuration !== Infinity) {
videojs.log.warn(`Problem encountered with playlist ${currentPlaylist.id}. ` +
videojs.log.warn(`Problem encountered with playlist ${playlistToExclude.id}. ` +
'Trying again since it is the only playlist.');

this.tech_.trigger('retryplaylist');
Expand All @@ -1194,7 +1194,7 @@ export class MasterPlaylistController extends videojs.EventTarget {

playlists.forEach((playlist) => {
// skip current playlist which is about to be excluded
if (playlist === currentPlaylist) {
if (playlist === playlistToExclude) {
return;
}
const excludeUntil = playlist.excludeUntil;
Expand All @@ -1219,25 +1219,24 @@ export class MasterPlaylistController extends videojs.EventTarget {
// Exclude this playlist
let excludeUntil;

if (currentPlaylist.playlistErrors_ > this.maxPlaylistRetries) {
if (playlistToExclude.playlistErrors_ > this.maxPlaylistRetries) {
excludeUntil = Infinity;
} else {
excludeUntil = Date.now() + (playlistExclusionDuration * 1000);
}

currentPlaylist.excludeUntil = excludeUntil;
playlistToExclude.excludeUntil = excludeUntil;

if (error.reason) {
currentPlaylist.lastExcludeReason_ = error.reason;
playlistToExclude.lastExcludeReason_ = error.reason;
}
this.tech_.trigger('blacklistplaylist');
this.tech_.trigger('excludeplaylist');
this.tech_.trigger({type: 'usage', name: 'vhs-rendition-excluded'});

// TODO: should we select a new playlist if this blacklist wasn't for the currentPlaylist?
// Would be something like media().id !=== currentPlaylist.id and we would need something
// like `pendingMedia` in playlist loaders to check against that too. This will prevent us
// from loading a new playlist on any blacklist.
// Select a new playlist
// TODO: only load a new playlist if we're excluding the playlistToExclude
// If this function was called with a playlist that's not the current active playlist
// (e.g., media().id !== playlistToExclude.id),
// then a new playlist should not be selected and loaded, as there's nothing wrong with the current playlist.
const nextPlaylist = this.selectPlaylist();

if (!nextPlaylist) {
Expand All @@ -1249,16 +1248,16 @@ export class MasterPlaylistController extends videojs.EventTarget {
const logFn = error.internal ? this.logger_ : videojs.log.warn;
const errorMessage = error.message ? (' ' + error.message) : '';

logFn(`${(error.internal ? 'Internal problem' : 'Problem')} encountered with playlist ${currentPlaylist.id}.` +
logFn(`${(error.internal ? 'Internal problem' : 'Problem')} encountered with playlist ${playlistToExclude.id}.` +
`${errorMessage} Switching to playlist ${nextPlaylist.id}.`);

// if audio group changed reset audio loaders
if (nextPlaylist.attributes.AUDIO !== currentPlaylist.attributes.AUDIO) {
if (nextPlaylist.attributes.AUDIO !== playlistToExclude.attributes.AUDIO) {
this.delegateLoaders_('audio', ['abort', 'pause']);
}

// if subtitle group changed reset subtitle loaders
if (nextPlaylist.attributes.SUBTITLES !== currentPlaylist.attributes.SUBTITLES) {
if (nextPlaylist.attributes.SUBTITLES !== playlistToExclude.attributes.SUBTITLES) {
this.delegateLoaders_('subtitle', ['abort', 'pause']);
}

Expand Down Expand Up @@ -1706,7 +1705,7 @@ export class MasterPlaylistController extends videojs.EventTarget {

// no codecs, no playback.
if (!codecs.audio && !codecs.video) {
this.excludeCurrentPlaylist({
this.excludePlaylist({
playlist: this.media(),
message: 'Could not determine codecs for playlist.',
playlistExclusionDuration: Infinity
Expand Down Expand Up @@ -1758,7 +1757,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
return acc;
}, '') + '.';

this.excludeCurrentPlaylist({
this.excludePlaylist({
playlist: this.media(),
internal: true,
message,
Expand All @@ -1783,7 +1782,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
});

if (switchMessages.length) {
this.excludeCurrentPlaylist({
this.excludePlaylist({
playlist: this.media(),
message: `Codec switching not supported: ${switchMessages.join(', ')}.`,
playlistExclusionDuration: Infinity,
Expand Down Expand Up @@ -1873,7 +1872,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
}

/**
* Blacklist playlists that are known to be codec or
* Exclude playlists that are known to be codec or
* stream-incompatible with the SourceBuffer configuration. For
* instance, Media Source Extensions would cause the video element to
* stall waiting for video data if you switched from a variant with
Expand Down
6 changes: 3 additions & 3 deletions src/media-groups.js
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ export const onError = {
const {
segmentLoaders: { [type]: segmentLoader},
mediaTypes: { [type]: mediaType },
excludeCurrentPlaylist
excludePlaylist
} = settings;

stopLoaders(segmentLoader, mediaType);
Expand All @@ -265,7 +265,7 @@ export const onError = {
if (activeTrack === defaultTrack) {
// Default track encountered an error. All we can do now is exclude the current
// rendition and hope another will switch audio groups
excludeCurrentPlaylist({
excludePlaylist({
message: 'Problem encountered loading the default audio track.'
});
return;
Expand Down Expand Up @@ -844,7 +844,7 @@ export const getActiveGroup = (type, {mediaTypes}) => () => {
* The parsed master manifest
* @param {Object} settings.mediaTypes
* Object to store the loaders, tracks, and utility methods for each media type
* @param {Function} settings.excludeCurrentPlaylist
* @param {Function} settings.excludePlaylist
* Excludes the current rendition and forces a rendition switch.
* @function setupMediaGroups
*/
Expand Down
2 changes: 1 addition & 1 deletion src/media-segment-request.js
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,7 @@ const handleSegmentBytes = ({
// TODO:
// We should have a handler that fetches the number of bytes required
// to check if something is fmp4. This will allow us to save bandwidth
// because we can only blacklist a playlist and abort requests
// because we can only exclude a playlist and abort requests
// by codec after trackinfo triggers.
if (isLikelyFmp4MediaSegment(bytesAsUint8Array)) {
segment.isFmp4 = true;
Expand Down
2 changes: 1 addition & 1 deletion src/playback-watcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ export default class PlaybackWatcher {

// TODO: should we exclude audio tracks rather than main tracks
// when type is audio?
mpc.excludeCurrentPlaylist({
mpc.excludePlaylist({
message: `Excessive ${type} segment downloading detected.`
}, Infinity);
}
Expand Down
2 changes: 1 addition & 1 deletion src/videojs-http-streaming.js
Original file line number Diff line number Diff line change
Expand Up @@ -1022,7 +1022,7 @@ class VhsHandler extends Component {

this.player_.tech_.on('keystatuschange', (e) => {
if (e.status === 'output-restricted') {
this.masterPlaylistController_.excludeCurrentPlaylist({
this.masterPlaylistController_.excludePlaylist({
playlist: this.masterPlaylistController_.media(),
message: `DRM keystatus changed to ${e.status}. Playlist will fail to play. Check for HDCP content.`,
playlistExclusionDuration: Infinity
Expand Down
20 changes: 10 additions & 10 deletions test/master-playlist-controller.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4833,7 +4833,7 @@ QUnit.module('MasterPlaylistController codecs', {
this.mpc = this.masterPlaylistController;

this.exclusionList = [];
this.mpc.excludeCurrentPlaylist = (playlist) => this.exclusionList.push(playlist);
this.mpc.excludePlaylist = (playlist) => this.exclusionList.push(playlist);

this.contentSetup = (options) => {
const {
Expand Down Expand Up @@ -5508,7 +5508,7 @@ QUnit.module('MasterPlaylistController - exclusion behavior', {

assert.equal(this.mpc.media(), this.mpc.master().playlists[0], 'selected first playlist');

this.mpc.excludeCurrentPlaylist({
this.mpc.excludePlaylist({
internal: true,
playlist: this.mpc.master().playlists[0],
playlistExclusionDuration: Infinity
Expand Down Expand Up @@ -6062,7 +6062,7 @@ QUnit.test('true if llhls playlist and we have buffered', function(assert) {
assert.ok(mpc.shouldSwitchToMedia_(nextPlaylist), 'should switch if buffered');
});

QUnit.module('MasterPlaylistController excludeCurrentPlaylist', sharedHooks);
QUnit.module('MasterPlaylistController excludePlaylist', sharedHooks);

QUnit.test("don't exclude only playlist unless it was excluded forever", function(assert) {
// expect 9 because we have a failing assertion that shouldn't run unless something is broken
Expand Down Expand Up @@ -6092,7 +6092,7 @@ QUnit.test("don't exclude only playlist unless it was excluded forever", functio

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

mpc.excludeCurrentPlaylist();
mpc.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 @@ -6128,7 +6128,7 @@ QUnit.test("don't exclude only playlist unless it was excluded forever", functio
});

// exclude forever
mpc.excludeCurrentPlaylist({}, Infinity);
mpc.excludePlaylist({}, Infinity);

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

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

mpc.excludeCurrentPlaylist();
mpc.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 @@ -6176,7 +6176,7 @@ QUnit.test('switch playlists if current playlist gets excluded and re-include if
this.standardXHRResponse(this.requests.shift());
playlist2 = mpl.master.playlists[1];

mpc.excludeCurrentPlaylist();
mpc.excludePlaylist();

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

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

mpc.excludeCurrentPlaylist();
mpc.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');

mpc.excludeCurrentPlaylist();
mpc.excludePlaylist();

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

mpc.excludeCurrentPlaylist();
mpc.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
12 changes: 6 additions & 6 deletions test/media-groups.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -926,7 +926,7 @@ QUnit.module('MediaGroups', function() {
QUnit.test(
'switches to default audio track when an error is encountered',
function(assert) {
let excludeCurrentPlaylistCalls = 0;
let excludePlaylistCalls = 0;
let onTrackChangedCalls = 0;

const type = 'AUDIO';
Expand All @@ -939,7 +939,7 @@ QUnit.module('MediaGroups', function() {
const settings = {
segmentLoaders: { AUDIO: segmentLoader },
mediaTypes: MediaGroups.createMediaTypes(),
excludeCurrentPlaylist: () => excludeCurrentPlaylistCalls++,
excludePlaylist: () => excludePlaylistCalls++,
masterPlaylistLoader
};
const mediaType = settings.mediaTypes[type];
Expand All @@ -959,7 +959,7 @@ QUnit.module('MediaGroups', function() {

onError();

assert.equal(excludeCurrentPlaylistCalls, 0, 'did not exclude current playlist');
assert.equal(excludePlaylistCalls, 0, 'did not exclude current playlist');
assert.equal(onTrackChangedCalls, 1, 'called onTrackChanged after changing to default');
assert.equal(tracks.en.enabled, true, 'enabled default track');
assert.equal(tracks.fr.enabled, false, 'disabled active track');
Expand All @@ -969,7 +969,7 @@ QUnit.module('MediaGroups', function() {

onError();

assert.equal(excludeCurrentPlaylistCalls, 1, 'excluded current playlist');
assert.equal(excludePlaylistCalls, 1, 'excluded current playlist');
assert.equal(onTrackChangedCalls, 1, 'did not call onTrackChanged after exclusion');
assert.equal(tracks.en.enabled, true, 'default track still enabled');
assert.equal(tracks.fr.enabled, false, 'disabled track still disabled');
Expand Down Expand Up @@ -1091,7 +1091,7 @@ QUnit.module('MediaGroups', function() {
requestOptions: { withCredentials: false, timeout: 10 },
master: this.master,
mediaTypes: this.mediaTypes,
excludeCurrentPlaylist() {},
excludePlaylist() {},
sourceType: 'hls'
};
}
Expand Down Expand Up @@ -1609,7 +1609,7 @@ QUnit.module('MediaGroups', function() {
requestOptions: { withCredentials: false, timeout: 10 },
master: this.master,
mediaTypes: this.mediaTypes,
excludeCurrentPlaylist() {},
excludePlaylist() {},
sourceType: 'hls'
};
}
Expand Down
2 changes: 1 addition & 1 deletion test/playlist.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -874,7 +874,7 @@ QUnit.module('Playlist', function() {
QUnit.test('determines if a playlist is incompatible', function(assert) {
// incompatible means that the playlist was excluded due to incompatible
// configuration e.g. audio only stream when trying to playback audio and video.
// incompaatibility is denoted by an excludeUntil of Infinity.
// incompatibility is denoted by an excludeUntil of Infinity.
assert.notOk(
Playlist.isIncompatible({}),
'playlist not incompatible if no excludeUntil'
Expand Down
Loading