fix: clear the blacklist for other playlists if final rendition errors (#479)

Ignore unsupported renditions (those with an excludeUntil of Infinity)

This time use a separate timer for the final rendition.

Second try at #396. Reverts #471.

Author: brandonocasey

src/master-playlist-controller.js

@@ -789,15 +789,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');
@@ -809,7 +829,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);
   }
 
   /**

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
     };
@@ -317,6 +317,7 @@ export default class PlaylistLoader extends EventTarget {
   dispose() {
     this.stopRequest();
     window.clearTimeout(this.mediaUpdateTimeout);
+    window.clearTimeout(this.finalRenditionTimeout);
   }
 
   stopRequest() {
@@ -339,9 +340,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 +355,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 +364,17 @@ export default class PlaylistLoader extends EventTarget {
       playlist = this.master.playlists[playlist];
     }
 
+    window.clearTimeout(this.finalRenditionTimeout);
+
+    if (isFinalRendition) {
+      const delay = (playlist.targetDuration / 2) * 1000 || 5 * 1000;
+
+      this.finalRenditionTimeout =
+        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 +521,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

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;
 });