-
Notifications
You must be signed in to change notification settings - Fork 793
Clear the blacklist for other playlists if final rendition errors #1083
Changes from 11 commits
49b97a9
0c261e2
f81badf
fd073d5
02fba38
8c431dc
8991530
83f429a
f6a5b61
6275f13
b1443be
4f2c2cd
5f2aab3
c074ec1
d478282
446a46a
09f2a6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1127,35 +1127,45 @@ 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.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'); | ||
'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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -1233,11 +1243,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'); | ||
}); | ||
|
||
|
@@ -1260,12 +1270,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) { | ||
|
@@ -1529,15 +1539,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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did this new response need to be added? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
}); | ||
|
@@ -2159,7 +2171,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; | ||
|
||
|
There was a problem hiding this comment.
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.