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 6 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
20 changes: 14 additions & 6 deletions src/master-playlist-controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,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 @@ -1064,26 +1064,34 @@ export class MasterPlaylistController extends videojs.EventTarget {
}

let isFinalRendition = this.masterPlaylistLoader_.isFinalRendition_();
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.

playlists.forEach((playlist) => {
// clear the blacklist duration for the other playlists when
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than describing what the code is doing, it might make more sense to have a comment about the reason we are doing it. Something like 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, we clear out the current blacklist so the other playlists may be attempted in case any have been fixed.

// the final playlist errors
delete playlist.excludeUntil;
});
}
// Blacklist this playlist
currentPlaylist.excludeUntil = Date.now() + this.blacklistDuration * 1000;
this.tech_.trigger('blacklistplaylist');

// 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
11 changes: 10 additions & 1 deletion src/playlist-loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ const PlaylistLoader = function(srcUrl, hls, withCredentials) {
* object to switch to
* @return {Playlist} the current loaded media
*/
loader.media = function(playlist) {
loader.media = function(playlist, isFinalRendition) {
let startingState = loader.state;
let mediaChange;

Expand All @@ -300,6 +300,15 @@ const PlaylistLoader = function(srcUrl, hls, withCredentials) {
throw new Error('Cannot switch media playlist from ' + loader.state);
}

window.clearTimeout(mediaUpdateTimeout);

if (isFinalRendition) {
let refreshDelay = (playlist.targetDuration / 2) * 1000 || 5 * 1000;

mediaUpdateTimeout = window.setTimeout(loader.media.bind(loader, playlist, false), refreshDelay);
return;
}

// find the playlist object if the target playlist has been
// specified by URI
if (typeof playlist === 'string') {
Expand Down
50 changes: 30 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 @@ -1127,35 +1127,42 @@ 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(blacklistplaylist, 1, 'media is blacklisted');
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.ok(media.excludeUntil > 0, 'second media was blacklisted after playlist 404');
assert.equal(this.env.log.warn.calls, 1, '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');
'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, 'excludeUntil was deleted after cleared the blacklist duration for the first media');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can just be 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 @@ -1233,11 +1240,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 @@ -1260,12 +1267,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 @@ -1529,15 +1536,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 Expand Up @@ -2159,7 +2168,8 @@ QUnit.test('adds audio tracks if we have parsed some from a playlist', function(
assert.equal(vjsAudioTracks[0].enabled, false, 'main track is disabled');
});

QUnit.test('when audioinfo changes on an independent audio track in Firefox 48 & below, the enabled track is blacklisted and removed', function(assert) {
QUnit.test('when audioinfo changes on an independent audio track in Firefox 48 & below, ' +
'the enabled track is blacklisted and removed', function(assert) {
let audioTracks = this.player.audioTracks();
let oldLabel;

Expand Down