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

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

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
49b97a9
resolve merge conflict
Apr 19, 2017
0c261e2
fix blacklistDuration option test case
Apr 12, 2017
f81badf
get playlist from masterPlaylistLoader
Apr 19, 2017
fd073d5
fix test case
Apr 19, 2017
02fba38
updating regarding the comments
May 23, 2017
8c431dc
test cases change
May 25, 2017
8991530
add a warning log and revise comment
May 31, 2017
83f429a
fix test case
May 31, 2017
f6a5b61
move comment and warn log out of loop
Jun 16, 2017
6275f13
change the positon if clearing warn log
Jun 16, 2017
b1443be
rephrase the comment
Jun 16, 2017
4f2c2cd
Merge remote-tracking branch 'upstream/master' into feature/un-blackl…
Jun 16, 2017
5f2aab3
resolve conflict
Jun 28, 2017
c074ec1
Merge branch 'master' into feature/un-blacklist-playlist
gesinger Jan 26, 2018
d478282
Resolve merge conflicts
gesinger Jan 26, 2018
446a46a
Move check for final playlist media change to after string playlist r…
gesinger Jan 29, 2018
09f2a6c
Fire a 'retryplaylist' event when clearing the blacklist
gesinger Jan 31, 2018
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
26 changes: 19 additions & 7 deletions src/master-playlist-controller.js
View file
Open in desktop
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,27 @@ 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;
});
}
// Blacklist this playlist
currentPlaylist.excludeUntil = Date.now() + (blacklistDuration * 1000);
this.tech_.trigger('blacklistplaylist');
Expand All @@ -888,10 +900,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
12 changes: 11 additions & 1 deletion src/playlist-loader.js
View file
Open in desktop
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,6 +319,16 @@ export default class PlaylistLoader extends EventTarget {
throw new Error('Cannot switch media playlist from ' + this.state);
}

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;

// find the playlist object if the target playlist has been
Expand Down
52 changes: 32 additions & 20 deletions test/videojs-contrib-hls.test.js
View file
Open in desktop
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, 0, 'haven\'t retried any playlist');
assert.equal(blacklistplaylist, 2, 'media1 is blacklisted');
Copy link
Contributor

Choose a reason for hiding this comment

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

Although this may be true, since it's only testing that it got a blacklist event, maybe it is worth having two different checks, one for getting the blacklist event, the other checking for the excludeUntil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we have that excludeUntil check in line 1141?


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
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to check excludeUntil was deleted

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this new response need to be added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the new response didn't be added here, it will error for the second(final) rendition. Since we clear blacklist duration for the first rendition when final rendition errors, we can't test if the blacklist duration is set through option successfully.

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