Skip to content
This repository has been archived by the owner on Jan 12, 2019. It is now read-only.

Clear the blacklist for other playlists if final rendition errors #1083

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 23 additions & 7 deletions src/master-playlist-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
// one is updating (and give the player a chance to re-adjust to the
// safe live point).
this.blacklistCurrentPlaylist({
message: 'Playlist no longer updating.'
message: 'Playlist no longer updating'
});
// useful for monitoring QoS
this.tech_.trigger('playliststuck');
Expand Down Expand Up @@ -607,7 +607,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
this.mainSegmentLoader_.on('earlyabort', () => {
this.blacklistCurrentPlaylist({
message: 'Aborted early because there isn\'t enough bandwidth to complete the ' +
'request without rebuffering.'
'request without rebuffering'
}, ABORT_EARLY_BLACKLIST_SECONDS);
});

Expand Down Expand Up @@ -871,15 +871,31 @@ 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth adding a warning log here with something like Clearing blacklist because last rendition is about to be blacklisted.

// 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');
Expand All @@ -888,10 +904,10 @@ export class MasterPlaylistController extends videojs.EventTarget {
// Select a new playlist
nextPlaylist = this.selectPlaylist();
videojs.log.warn('Problem encountered with the current HLS playlist.' +
(error.message ? ' ' + error.message : '') +
(error.message ? ' ' + error.message + '.' : '') +
' Switching to another playlist.');

return this.masterPlaylistLoader_.media(nextPlaylist);
return this.masterPlaylistLoader_.media(nextPlaylist, isFinalRendition);
}

/**
Expand Down
15 changes: 12 additions & 3 deletions src/playlist-loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ export default class PlaylistLoader extends EventTarget {
* object to switch to
* @return {Playlist} the current loaded media
*/
media(playlist) {
media(playlist, isFinalRendition) {
// getter
if (!playlist) {
return this.media_;
Expand All @@ -319,8 +319,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 @@ -330,6 +328,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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we will encounter this because of when this is called with isFinalRendition === true, playlist is an object, but we support playlist being a string as you can see just below this, so it may be better to move this block below the string check

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 updated


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
Expand Down
52 changes: 32 additions & 20 deletions test/videojs-contrib-hls.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1293,38 +1293,48 @@ 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');
assert.equal(retryplaylist, 0, 'haven\'t retried any playlist');

// request for the final available media
// request for the final media
this.requests[2].respond(404);
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 @@ -1402,11 +1412,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 @@ -1429,12 +1439,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 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.',
'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 @@ -1725,15 +1735,17 @@ 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