From 6e6c8c212d5da1a5b5d297ff1e513f86ef46bb15 Mon Sep 17 00:00:00 2001 From: Joe Forbes Date: Mon, 1 Apr 2019 12:50:40 -0700 Subject: [PATCH] fix: clear the blacklist for other playlists if final rendition errors (#396) Ignore unsupported renditions (those with an excludeUntil of Infinity) --- src/master-playlist-controller.js | 28 +++++++-- src/playlist-loader.js | 21 +++++-- test/videojs-http-streaming.test.js | 89 ++++++++++++++++++++++------- 3 files changed, 109 insertions(+), 29 deletions(-) diff --git a/src/master-playlist-controller.js b/src/master-playlist-controller.js index 2ba4e782c..76f7b9e7c 100644 --- a/src/master-playlist-controller.js +++ b/src/master-playlist-controller.js @@ -787,15 +787,35 @@ export class MasterPlaylistController extends videojs.EventTarget { let isFinalRendition = this.masterPlaylistLoader_.master.playlists.filter(isEnabled).length === 1; + let playlists = this.masterPlaylistLoader_.master.playlists; - if (isFinalRendition) { - // Never blacklisting this playlist because it's final rendition + if (playlists.length === 1) { + // Never blacklisting this playlist because it's the only playlist videojs.log.warn('Problem encountered with the current ' + - 'HLS playlist. Trying again since it is the final playlist.'); + 'HLS playlist. Trying again since it is the only playlist.'); this.tech_.trigger('retryplaylist'); return this.masterPlaylistLoader_.load(isFinalRendition); } + + if (isFinalRendition) { + // Since we're on the final non-blacklisted playlist, and we're about to blacklist + // it, instead of erring the player or retrying this playlist, clear out the current + // blacklist. This allows other playlists to be attempted in case any have been + // fixed. + videojs.log.warn('Removing all playlists from the blacklist because the last ' + + 'rendition is about to be blacklisted.'); + playlists.forEach((playlist) => { + if (playlist.excludeUntil !== Infinity) { + delete playlist.excludeUntil; + } + }); + // Technically we are retrying a playlist, in that we are simply retrying a previous + // playlist. This is needed for users relying on the retryplaylist event to catch a + // case where the player might be stuck and looping through "dead" playlists. + this.tech_.trigger('retryplaylist'); + } + // Blacklist this playlist currentPlaylist.excludeUntil = Date.now() + (blacklistDuration * 1000); this.tech_.trigger('blacklistplaylist'); @@ -807,7 +827,7 @@ export class MasterPlaylistController extends videojs.EventTarget { (error.message ? ' ' + error.message : '') + ' Switching to another playlist.'); - return this.masterPlaylistLoader_.media(nextPlaylist); + return this.masterPlaylistLoader_.media(nextPlaylist, isFinalRendition); } /** diff --git a/src/playlist-loader.js b/src/playlist-loader.js index 2c0e313cd..4d15f4e3f 100644 --- a/src/playlist-loader.js +++ b/src/playlist-loader.js @@ -258,7 +258,7 @@ export default class PlaylistLoader extends EventTarget { this.error = { playlist: this.master.playlists[url], status: xhr.status, - message: 'HLS playlist request error at URL: ' + url, + message: `HLS playlist request error at URL: ${url}.`, responseText: xhr.responseText, code: (xhr.status >= 500) ? 4 : 2 }; @@ -339,9 +339,11 @@ export default class PlaylistLoader extends EventTarget { * * @param {Object=} playlist the parsed media playlist * object to switch to + * @param {Boolean=} is this the last available playlist + * * @return {Playlist} the current loaded media */ - media(playlist) { + media(playlist, isFinalRendition) { // getter if (!playlist) { return this.media_; @@ -352,8 +354,6 @@ export default class PlaylistLoader extends EventTarget { throw new Error('Cannot switch media playlist from ' + this.state); } - const startingState = this.state; - // find the playlist object if the target playlist has been // specified by URI if (typeof playlist === 'string') { @@ -363,6 +363,17 @@ export default class PlaylistLoader extends EventTarget { playlist = this.master.playlists[playlist]; } + window.clearTimeout(this.mediaUpdateTimeout); + + if (isFinalRendition) { + const delay = (playlist.targetDuration / 2) * 1000 || 5 * 1000; + + this.mediaUpdateTimeout = + window.setTimeout(this.media.bind(this, playlist, false), delay); + return; + } + + const startingState = this.state; const mediaChange = !this.media_ || playlist.uri !== this.media_.uri; // switch to fully loaded playlists immediately @@ -509,7 +520,7 @@ export default class PlaylistLoader extends EventTarget { if (error) { this.error = { status: req.status, - message: 'HLS playlist request error at URL: ' + this.srcUrl, + message: `HLS playlist request error at URL: ${this.srcUrl}.`, responseText: req.responseText, // MEDIA_ERR_NETWORK code: 2 diff --git a/test/videojs-http-streaming.test.js b/test/videojs-http-streaming.test.js index cc137d1f3..9f56b791f 100644 --- a/test/videojs-http-streaming.test.js +++ b/test/videojs-http-streaming.test.js @@ -570,7 +570,7 @@ function(assert) { assert.equal(error.code, 2, 'error has correct code'); assert.equal(error.message, - 'HLS playlist request error at URL: manifest/master.m3u8', + 'HLS playlist request error at URL: manifest/master.m3u8.', 'error has correct message'); assert.equal(errLogs.length, 1, 'logged an error'); @@ -1237,6 +1237,45 @@ QUnit.test('segment 404 should trigger blacklisting of media', function(assert) assert.equal(this.player.tech_.hls.stats.bandwidth, 20000, 'bandwidth set above'); }); +QUnit.test('unsupported playlist should not be re-included when excluding last playlist', function(assert) { + this.player.src({ + src: 'manifest/master.m3u8', + type: 'application/vnd.apple.mpegurl' + }); + + this.clock.tick(1); + + openMediaSource(this.player, this.clock); + + this.player.tech_.hls.bandwidth = 1; + // master + this.requests.shift() + .respond(200, null, + '#EXTM3U\n' + + '#EXT-X-STREAM-INF:BANDWIDTH=1,CODECS="avc1.4d400d,mp4a.40.2"\n' + + 'media.m3u8\n' + + '#EXT-X-STREAM-INF:BANDWIDTH=10,CODECS="avc1.4d400d,not-an-audio-codec"\n' + + 'media1.m3u8\n'); + // media + this.standardXHRResponse(this.requests.shift()); + + let master = this.player.tech_.hls.playlists.master; + let media = this.player.tech_.hls.playlists.media_; + + // segment + this.requests.shift().respond(400); + + assert.ok(master.playlists[0].excludeUntil > 0, 'original media excluded for some time'); + assert.strictEqual(master.playlists[1].excludeUntil, + Infinity, + 'blacklisted invalid audio codec'); + + assert.equal(this.env.log.warn.calls, 2, 'warning logged for blacklist'); + assert.equal(this.env.log.warn.args[0], + 'Removing all playlists from the blacklist because the last rendition is about to be blacklisted.', + 'log generic error message'); +}); + QUnit.test('playlist 404 should blacklist media', function(assert) { let media; let url; @@ -1281,7 +1320,7 @@ QUnit.test('playlist 404 should blacklist media', function(assert) { assert.ok(media.excludeUntil > 0, 'original media blacklisted for some time'); assert.equal(this.env.log.warn.calls, 1, 'warning logged for blacklist'); assert.equal(this.env.log.warn.args[0], - 'Problem encountered with the current HLS playlist. HLS playlist request error at URL: media.m3u8 Switching to another playlist.', + 'Problem encountered with the current HLS playlist. HLS playlist request error at URL: media.m3u8. Switching to another playlist.', 'log generic error message'); assert.equal(blacklistplaylist, 1, 'there is one blacklisted playlist'); assert.equal(hlsRenditionBlacklistedEvents, 1, 'an hls-rendition-blacklisted event was fired'); @@ -1292,27 +1331,36 @@ QUnit.test('playlist 404 should blacklist media', function(assert) { url = this.requests[2].url.slice(this.requests[2].url.lastIndexOf('/') + 1); media = this.player.tech_.hls.playlists.master.playlists[url]; - // media wasn't blacklisted because it's final rendition - assert.ok(!media.excludeUntil, 'media not blacklisted after playlist 404'); - assert.equal(this.env.log.warn.calls, 1, 'warning logged for blacklist'); + assert.ok(media.excludeUntil > 0, 'second media was blacklisted after playlist 404'); + assert.equal(this.env.log.warn.calls, 2, 'warning logged for blacklist'); assert.equal(this.env.log.warn.args[1], - 'Problem encountered with the current HLS playlist. Trying again since it is the final playlist.', - 'log specific error message for final playlist'); - assert.equal(retryplaylist, 1, 'retried final playlist for once'); - assert.equal(blacklistplaylist, 1, 'there is one blacklisted playlist'); - assert.equal(hlsRenditionBlacklistedEvents, 1, 'an hls-rendition-blacklisted event was fired'); + 'Removing all playlists from the blacklist because the last rendition is about to be blacklisted.', + 'log generic error message'); + assert.equal(this.env.log.warn.args[2], + 'Problem encountered with the current HLS playlist. HLS playlist request error at URL: media1.m3u8. ' + + 'Switching to another playlist.', + 'log generic error message'); + assert.equal(retryplaylist, 1, 'fired a retryplaylist event'); + assert.equal(blacklistplaylist, 2, 'media1 is blacklisted'); this.clock.tick(2 * 1000); // no new request was made since it hasn't been half the segment duration assert.strictEqual(3, this.requests.length, 'no new request was made'); this.clock.tick(3 * 1000); - // continue loading the final remaining playlist after it wasn't blacklisted + // loading the first playlist since the blacklist duration was cleared // when half the segment duaration passed + assert.strictEqual(4, this.requests.length, 'one more request was made'); + url = this.requests[3].url.slice(this.requests[3].url.lastIndexOf('/') + 1); + media = this.player.tech_.hls.playlists.master.playlists[url]; + + // the first media was unblacklisted after a refresh delay + assert.ok(!media.excludeUntil, 'removed first media from blacklist'); + assert.strictEqual(this.requests[3].url, - absoluteUrl('manifest/media1.m3u8'), + absoluteUrl('manifest/media.m3u8'), 'media playlist requested'); // verify stats @@ -1390,11 +1438,11 @@ QUnit.test('never blacklist the playlist if it is the only playlist', function(a this.requests.shift().respond(404); media = this.player.tech_.hls.playlists.media(); - // media wasn't blacklisted because it's final rendition + // media wasn't blacklisted because it's the only rendition assert.ok(!media.excludeUntil, 'media was not blacklisted after playlist 404'); assert.equal(this.env.log.warn.calls, 1, 'warning logged for blacklist'); assert.equal(this.env.log.warn.args[0], - 'Problem encountered with the current HLS playlist. Trying again since it is the final playlist.', + 'Problem encountered with the current HLS playlist. Trying again since it is the only playlist.', 'log specific error message for final playlist'); }); @@ -1417,12 +1465,12 @@ QUnit.test('error on the first playlist request does not trigger an error ' + let url = this.requests[1].url.slice(this.requests[1].url.lastIndexOf('/') + 1); let media = this.player.tech_.hls.playlists.master.playlists[url]; - // media wasn't blacklisted because it's final rendition + // media wasn't blacklisted because it's only rendition assert.ok(!media.excludeUntil, 'media was not blacklisted after playlist 404'); assert.equal(this.env.log.warn.calls, 1, 'warning logged for blacklist'); assert.equal(this.env.log.warn.args[0], - 'Problem encountered with the current HLS playlist. Trying again since it is the final playlist.', - 'log specific error message for final playlist'); + 'Problem encountered with the current HLS playlist. Trying again since it is the only playlist.', + 'log specific error message for the only playlist'); }); QUnit.test('seeking in an empty playlist is a non-erroring noop', function(assert) { @@ -1797,15 +1845,16 @@ QUnit.test('playlist blacklisting duration is set through options', function(ass assert.ok(media.excludeUntil > 0, 'original media blacklisted for some time'); assert.equal(this.env.log.warn.calls, 1, 'warning logged for blacklist'); assert.equal(this.env.log.warn.args[0], - 'Problem encountered with the current HLS playlist. HLS playlist request error at URL: media.m3u8 Switching to another playlist.', + 'Problem encountered with the current HLS playlist. HLS playlist request error at URL: media.m3u8. Switching to another playlist.', 'log generic error message'); + // this takes one millisecond + this.standardXHRResponse(this.requests[2]); - this.clock.tick(2 * 60 * 1000); + this.clock.tick(2 * 60 * 1000 - 1); assert.ok(media.excludeUntil - Date.now() > 0, 'original media still be blacklisted'); this.clock.tick(1 * 60 * 1000); assert.equal(media.excludeUntil, Date.now(), 'media\'s exclude time reach to the current time'); - assert.equal(this.env.log.warn.calls, 3, 'warning logged for blacklist'); videojs.options.hls = hlsOptions; });