From dd55028b58c7afd7bf6d79d32a9eba5381e02f52 Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Tue, 16 Apr 2019 15:38:35 -0400 Subject: [PATCH] revert: "fix: clear the blacklist for other playlists if final rendition errors (#396)" (#471) This reverts commit 6e6c8c212d5da1a5b5d297ff1e513f86ef46bb15. --- src/master-playlist-controller.js | 28 ++------- src/playlist-loader.js | 21 ++----- test/videojs-http-streaming.test.js | 89 +++++++---------------------- 3 files changed, 29 insertions(+), 109 deletions(-) diff --git a/src/master-playlist-controller.js b/src/master-playlist-controller.js index c5678d87f..1e743b794 100644 --- a/src/master-playlist-controller.js +++ b/src/master-playlist-controller.js @@ -789,35 +789,15 @@ export class MasterPlaylistController extends videojs.EventTarget { let isFinalRendition = this.masterPlaylistLoader_.master.playlists.filter(isEnabled).length === 1; - let playlists = this.masterPlaylistLoader_.master.playlists; - if (playlists.length === 1) { - // Never blacklisting this playlist because it's the only playlist + if (isFinalRendition) { + // Never blacklisting this playlist because it's final rendition videojs.log.warn('Problem encountered with the current ' + - 'HLS playlist. Trying again since it is the only playlist.'); + 'HLS playlist. Trying again since it is the final 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'); @@ -829,7 +809,7 @@ export class MasterPlaylistController extends videojs.EventTarget { (error.message ? ' ' + error.message : '') + ' Switching to another playlist.'); - return this.masterPlaylistLoader_.media(nextPlaylist, isFinalRendition); + return this.masterPlaylistLoader_.media(nextPlaylist); } /** diff --git a/src/playlist-loader.js b/src/playlist-loader.js index 4d15f4e3f..2c0e313cd 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,11 +339,9 @@ 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, isFinalRendition) { + media(playlist) { // getter if (!playlist) { return this.media_; @@ -354,6 +352,8 @@ 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,17 +363,6 @@ 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 @@ -520,7 +509,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 eb522fd7c..7a74ead33 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,45 +1237,6 @@ 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; @@ -1320,7 +1281,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'); @@ -1331,36 +1292,27 @@ 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]; - assert.ok(media.excludeUntil > 0, 'second media was blacklisted after playlist 404'); - assert.equal(this.env.log.warn.calls, 2, 'warning logged for blacklist'); + // 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.equal(this.env.log.warn.args[1], - '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'); + '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'); 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); - // loading the first playlist since the blacklist duration was cleared + // continue loading the final remaining playlist after it wasn't blacklisted // 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/media.m3u8'), + absoluteUrl('manifest/media1.m3u8'), 'media playlist requested'); // verify stats @@ -1438,11 +1390,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 the only rendition + // media wasn't blacklisted because it's final 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 only playlist.', + 'Problem encountered with the current HLS playlist. Trying again since it is the final playlist.', 'log specific error message for final playlist'); }); @@ -1465,12 +1417,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 only rendition + // media wasn't blacklisted because it's final 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 only playlist.', - 'log specific error message for the only playlist'); + 'Problem encountered with the current HLS playlist. Trying again since it is the final playlist.', + 'log specific error message for final playlist'); }); QUnit.test('seeking in an empty playlist is a non-erroring noop', function(assert) { @@ -1845,16 +1797,15 @@ 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 - 1); + this.clock.tick(2 * 60 * 1000); 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; });