From 5b67f7194c1bc5e69abef09f5f53db7e03a07fa9 Mon Sep 17 00:00:00 2001 From: zhuangs Date: Thu, 7 Feb 2019 11:24:22 -0500 Subject: [PATCH 1/4] fix: Clear the blacklist for other playlists if final rendition errors --- src/master-playlist-controller.js | 26 ++++++++++++--- src/playlist-loader.js | 21 +++++++++--- test/videojs-http-streaming.test.js | 50 +++++++++++++++++------------ 3 files changed, 68 insertions(+), 29 deletions(-) diff --git a/src/master-playlist-controller.js b/src/master-playlist-controller.js index 99c933692..8b7940181 100644 --- a/src/master-playlist-controller.js +++ b/src/master-playlist-controller.js @@ -765,15 +765,33 @@ 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) => { + 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'); @@ -785,7 +803,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 f8e994219..192a1ed7c 100644 --- a/src/playlist-loader.js +++ b/src/playlist-loader.js @@ -254,7 +254,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 }; @@ -335,9 +335,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_; @@ -348,8 +350,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') { @@ -359,6 +359,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 @@ -503,7 +514,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 de706c2a1..7cffe1981 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'); @@ -1281,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'); @@ -1292,27 +1292,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 +1399,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 +1426,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) { @@ -1713,15 +1722,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; }); From f805916bf15393d12da6ec52d23c1c5633a3ce2f Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Mon, 1 Apr 2019 14:47:03 -0400 Subject: [PATCH 2/4] add test for re-adding unsupported (excludedUtil === Infinity) playlists on final playlist --- test/videojs-http-streaming.test.js | 39 +++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/test/videojs-http-streaming.test.js b/test/videojs-http-streaming.test.js index 7cffe1981..8fc801668 100644 --- a/test/videojs-http-streaming.test.js +++ b/test/videojs-http-streaming.test.js @@ -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; From b37e4fc1bf678c3bb079f37237af1d26285a17b5 Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Mon, 1 Apr 2019 14:51:58 -0400 Subject: [PATCH 3/4] fix: ignore exlcudeUtil === Infinity --- src/master-playlist-controller.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/master-playlist-controller.js b/src/master-playlist-controller.js index 8b7940181..f6c2baa2d 100644 --- a/src/master-playlist-controller.js +++ b/src/master-playlist-controller.js @@ -784,7 +784,9 @@ export class MasterPlaylistController extends videojs.EventTarget { videojs.log.warn('Removing all playlists from the blacklist because the last ' + 'rendition is about to be blacklisted.'); playlists.forEach((playlist) => { - delete playlist.excludeUntil; + 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 From 26b76f269ba9f63fba52bb092dccc82210cce246 Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Mon, 1 Apr 2019 15:07:11 -0400 Subject: [PATCH 4/4] seeking to duration is failing in Firefox --- test/playback.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/playback.test.js b/test/playback.test.js index 97dc7f2de..7257ea009 100644 --- a/test/playback.test.js +++ b/test/playback.test.js @@ -184,7 +184,7 @@ QUnit.test('loops', function(assert) { done(); }); }); - player.currentTime(player.duration()); + player.currentTime(player.duration() - 1); }); player.play(); });