Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

revert: "fix: clear the blacklist for other playlists if final rendition errors (#396)" #471

Merged
merged 1 commit into from
Apr 16, 2019
Merged

revert: "fix: clear the blacklist for other playlists if final rendition errors (#396)" #471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 4 additions & 24 deletions src/master-playlist-controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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);
}

/**
Expand Down
21 changes: 5 additions & 16 deletions src/playlist-loader.js
View file
Open in desktop
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 @@ -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_;
Expand All @@ -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') {
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
89 changes: 20 additions & 69 deletions test/videojs-http-streaming.test.js
View file
Open in desktop
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,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;
Expand Down Expand Up @@ -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');
Expand All @@ -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
Expand Down Expand Up @@ -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');
});

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