From 9075e7567da542f467cb996a4db601299ee83c1a Mon Sep 17 00:00:00 2001 From: Garrett Singer Date: Mon, 1 Aug 2022 16:25:04 -0400 Subject: [PATCH 1/3] refactor: clean up parameters of excludePlaylist Rename the parameters passed to excludePlaylist to make the logic easier to follow. --- src/media-groups.js | 2 +- src/playback-watcher.js | 5 +- src/playlist-controller.js | 72 +++++++++++++--------- src/videojs-http-streaming.js | 7 ++- test/playlist-controller.test.js | 92 ++++++++++++++++++----------- test/videojs-http-streaming.test.js | 7 ++- 6 files changed, 115 insertions(+), 70 deletions(-) diff --git a/src/media-groups.js b/src/media-groups.js index 21772375a..31588481f 100644 --- a/src/media-groups.js +++ b/src/media-groups.js @@ -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; } diff --git a/src/playback-watcher.js b/src/playback-watcher.js index cf36d11b8..17380cc3d 100644 --- a/src/playback-watcher.js +++ b/src/playback-watcher.js @@ -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 + }); } /** diff --git a/src/playlist-controller.js b/src/playlist-controller.js index 03ac9df26..fca23a734 100644 --- a/src/playlist-controller.js +++ b/src/playlist-controller.js @@ -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', () => { @@ -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'); @@ -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', () => { @@ -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 = () => { @@ -1122,25 +1131,30 @@ 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.masterPlaylistLoader_.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 @@ -1694,8 +1708,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; @@ -1746,9 +1760,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; @@ -1771,10 +1787,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; } diff --git a/src/videojs-http-streaming.js b/src/videojs-http-streaming.js index cf464a84c..5f14bf2e5 100644 --- a/src/videojs-http-streaming.js +++ b/src/videojs-http-streaming.js @@ -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 }); } diff --git a/test/playlist-controller.test.js b/test/playlist-controller.test.js index 4866e0d20..b73117d46 100644 --- a/test/playlist-controller.test.js +++ b/test/playlist-controller.test.js @@ -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 { @@ -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'); }); @@ -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'); }); @@ -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'); }); @@ -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'); }); @@ -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'); }); @@ -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'); }); @@ -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'); }); @@ -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'); @@ -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'); }); @@ -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.master().playlists[0], + error: { internal: true }, playlistExclusionDuration: Infinity }); @@ -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'); @@ -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'); @@ -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'); @@ -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'); @@ -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'); @@ -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'); diff --git a/test/videojs-http-streaming.test.js b/test/videojs-http-streaming.test.js index 4e7d92736..5cf6b97e3 100644 --- a/test/videojs-http-streaming.test.js +++ b/test/videojs-http-streaming.test.js @@ -4322,8 +4322,11 @@ QUnit.test('eme handles keystatuschange where status is output-restricted', func assert.deepEqual(excludes, [{ playlistExclusionDuration: Infinity, - message: 'DRM keystatus changed to output-restricted. Playlist will fail to play. Check for HDCP content.', - playlist: undefined + error: { + message: 'DRM keystatus changed to output-restricted. Playlist will fail to play. ' + + 'Check for HDCP content.' + }, + playlistToExclude: undefined }], 'excluded playlist'); }); From a689041c90c552f7c92e81bf517c8f65ba8dfa6d Mon Sep 17 00:00:00 2001 From: Garrett Singer Date: Thu, 4 Aug 2022 16:32:04 -0400 Subject: [PATCH 2/3] Remove unnecessary comment in excludePlaylist --- src/playlist-controller.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/playlist-controller.js b/src/playlist-controller.js index fca23a734..75e6f9939 100644 --- a/src/playlist-controller.js +++ b/src/playlist-controller.js @@ -1146,6 +1146,7 @@ export class PlaylistController extends videojs.EventTarget { 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 From c6b54bbe2628ee2d1b388a143e23127ffbe500ec Mon Sep 17 00:00:00 2001 From: Pat O'Neill Date: Fri, 19 Aug 2022 11:16:15 -0400 Subject: [PATCH 3/3] fix post-rebase failures --- src/playlist-controller.js | 2 +- test/playlist-controller.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/playlist-controller.js b/src/playlist-controller.js index 75e6f9939..d4d40862c 100644 --- a/src/playlist-controller.js +++ b/src/playlist-controller.js @@ -1142,7 +1142,7 @@ export class PlaylistController extends videojs.EventTarget { * an optional number of seconds to exclude the playlist */ excludePlaylist({ - playlistToExclude = this.masterPlaylistLoader_.media(), + playlistToExclude = this.mainPlaylistLoader_.media(), error = {}, playlistExclusionDuration }) { diff --git a/test/playlist-controller.test.js b/test/playlist-controller.test.js index b73117d46..1364135eb 100644 --- a/test/playlist-controller.test.js +++ b/test/playlist-controller.test.js @@ -5447,7 +5447,7 @@ QUnit.module('PlaylistController - exclusion behavior', { assert.equal(this.pc.media(), this.pc.main().playlists[0], 'selected first playlist'); this.pc.excludePlaylist({ - playlistToExclude: this.pc.master().playlists[0], + playlistToExclude: this.pc.main().playlists[0], error: { internal: true }, playlistExclusionDuration: Infinity });