Skip to content

Commit

Permalink
fix: clear the blacklist for other playlists if final rendition errors (
Browse files Browse the repository at this point in the history 
#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.
  • Loading branch information
@brandonocasey @gkatsev
brandonocasey authored and gkatsev committed Apr 26, 2019
1 parent faf1198 commit fe3b378
Show file tree
Hide file tree
Showing 3 changed files with 110 additions and 29 deletions.
28 changes: 24 additions & 4 deletions src/master-playlist-controller.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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);
}

/**
Expand Down
22 changes: 17 additions & 5 deletions src/playlist-loader.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
};
Expand Down Expand Up @@ -317,6 +317,7 @@ export default class PlaylistLoader extends EventTarget {
dispose() {
this.stopRequest();
window.clearTimeout(this.mediaUpdateTimeout);
window.clearTimeout(this.finalRenditionTimeout);
}

stopRequest() {
Expand All @@ -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_;
Expand All @@ -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') {
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
89 changes: 69 additions & 20 deletions test/videojs-http-streaming.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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');
Expand All @@ -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
Expand Down Expand Up @@ -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');
});

Expand All @@ -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) {
Expand Down Expand Up @@ -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;
});
Expand Down

0 comments on commit fe3b378

Please sign in to comment.