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

zhuangs · 2017-04-11T22:01:42Z

Description

Before the behavior is we never blacklist the final request and wait up to 5 minutes for other playlists' blacklist duration to be cleared. Change the behavior as follow:

If there is only one rendition and it errors, never blacklist it
if there are multiple renditions and we are down to the last one (the previous renditions are all blacklisted) and it errors, we clear the blacklist duration for the other playlists, and retry the best available playlist after a delay.

mjneil · 2017-04-14T13:51:14Z

src/master-playlist-controller.js

@@ -1064,14 +1064,22 @@ export class MasterPlaylistController extends videojs.EventTarget {
    }

    let isFinalRendition = this.masterPlaylistLoader_.isFinalRendition_();
+    let playlists = this.tech_.hls.playlists.master.playlists;


This can just be this.masterPlaylistLoader_.master.playlists

gesinger · 2017-05-19T18:32:50Z

src/playlist-loader.js

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


This should probably be moved up

gesinger · 2017-05-19T18:40:08Z

test/videojs-contrib-hls.test.js

-              '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.',


I also just realized that there is no period between media1.m3u8 Switching (from before, but might be nice to fix.

gesinger · 2017-05-19T18:52:35Z

test/videojs-contrib-hls.test.js

@@ -1153,9 +1152,11 @@ QUnit.test('playlist 404 should blacklist media', function(assert) {
  this.clock.tick(3 * 1000);
  // continue loading the final remaining playlist after it wasn't blacklisted


Technically, I think this comment might need to change

gesinger · 2017-05-19T19:01:56Z

test/videojs-contrib-hls.test.js

@@ -1531,13 +1532,14 @@ QUnit.test('playlist blacklisting duration is set through options', function(ass
  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.',
              'log generic error message');
+  // this takes one millisecond


Why did this new response need to be added?

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.

gesinger · 2017-05-19T19:03:06Z

src/master-playlist-controller.js

+      playlists.forEach((playlist) => {
+        // clear the blacklist duration for the other playlists when
+        // the final playlist errors
+        playlist.excludeUntil = Date.now();


It might be clearer to delete playlist.excludeUntil for each.

gesinger · 2017-05-24T20:01:28Z

test/videojs-contrib-hls.test.js

@@ -1529,15 +1532,16 @@ 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.',


gesinger · 2017-05-24T20:58:54Z

test/videojs-contrib-hls.test.js

+              'Switching to another playlist.',
+              'log generic error message');
+  assert.equal(retryplaylist, 0, 'haven\'t retried any playlist');
+  assert.equal(blacklistplaylist, 2, 'media1 is blacklisted');


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

I think we have that excludeUntil check in line 1141?

gesinger · 2017-05-24T20:59:43Z

test/videojs-contrib-hls.test.js


  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 is cleared


gesinger · 2017-05-24T21:00:12Z

test/videojs-contrib-hls.test.js

  assert.strictEqual(4, this.requests.length, 'one more request was made');
+  // the first media was unblacklisted after a refresh delay


Would be good to check excludeUntil was deleted

gesinger · 2017-05-30T19:55:26Z

src/master-playlist-controller.js


      this.tech_.trigger('retryplaylist');
      return this.masterPlaylistLoader_.load(isFinalRendition);
    }
+    if (isFinalRendition) {
+      playlists.forEach((playlist) => {
+        // clear the blacklist duration for the other playlists when


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.

gesinger · 2017-05-30T19:58:32Z

test/videojs-contrib-hls.test.js

+  media = this.player.tech_.hls.playlists.master.playlists[url];
+
+  // the first media was unblacklisted after a refresh delay
+  assert.ok(!media.excludeUntil, 'excludeUntil was deleted after cleared the blacklist duration for the first media');


Can just be removed first media from blacklist

gesinger · 2017-05-30T20:00:41Z

src/master-playlist-controller.js


      this.tech_.trigger('retryplaylist');
      return this.masterPlaylistLoader_.load(isFinalRendition);
    }
+    if (isFinalRendition) {


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

gesinger · 2017-06-16T17:25:08Z

src/master-playlist-controller.js

+        // 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.
+        videojs.log.warn('Clearing blacklist because last rendition is about to be blacklisted.');


This will get logged for every playlist. May just want to log it once.

gesinger · 2017-06-16T17:25:24Z

src/master-playlist-controller.js


      this.tech_.trigger('retryplaylist');
      return this.masterPlaylistLoader_.load(isFinalRendition);
    }
+    if (isFinalRendition) {
+      playlists.forEach((playlist) => {
+        // Since we're on the final non-blacklisted playlist, and we're about to blacklist it,


We probably want this comment outside of the loop as well.

gesinger · 2017-06-16T19:09:17Z

src/master-playlist-controller.js

+      playlists.forEach((playlist) => {
+        delete playlist.excludeUntil;
+      });
+      videojs.log.warn('Clearing blacklist for every playlist because last rendition ' +


Minor, but putting it just before the loop might be better for a couple reasons: (1) if something breaks (throws an exception) during the loop, we know the last warning log and it can help us determine what broke and (2) we already cleared everything, so Clearing might be more accurate before

Makes sense! Changed as suggested

…ist-playlist

andybarron · 2017-10-24T18:13:32Z

Any updates on this PR? The blacklisting is actively affecting a project I'm working on, would love to see this fixed :)

mjneil · 2018-01-26T21:00:29Z

src/playlist-loader.js

+    window.clearTimeout(this.mediaUpdateTimeout);
+
+    if (isFinalRendition) {
+      const delay = (playlist.targetDuration / 2) * 1000 || 5 * 1000;


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

👍 updated

…esolver

forbesjo · 2018-08-20T15:22:37Z

Let's open this against VHS

forbesjo · 2019-01-11T18:58:30Z

Closing and opening against VHS in the future

zhuangs force-pushed the feature/un-blacklist-playlist branch from 3fabb71 to cbffa84 Compare April 11, 2017 22:23

mjneil reviewed Apr 14, 2017

View reviewed changes

Shan Zhuang added 3 commits April 19, 2017 10:48

resolve merge conflict

49b97a9

fix blacklistDuration option test case

0c261e2

get playlist from masterPlaylistLoader

f81badf

zhuangs force-pushed the feature/un-blacklist-playlist branch from b384f69 to f81badf Compare April 19, 2017 14:49

fix test case

fd073d5

mjneil added the needs:testing label Apr 20, 2017

imbcmdth added the needs testing label May 9, 2017

imbcmdth assigned gesinger May 9, 2017

gesinger reviewed May 19, 2017

View reviewed changes

updating regarding the comments

02fba38

gesinger reviewed May 24, 2017

View reviewed changes

test cases change

8c431dc

gesinger reviewed May 30, 2017

View reviewed changes

Shan Zhuang added 2 commits May 31, 2017 10:50

add a warning log and revise comment

8991530

fix test case

83f429a

gesinger reviewed Jun 16, 2017

View reviewed changes

move comment and warn log out of loop

f6a5b61

zhuangs force-pushed the feature/un-blacklist-playlist branch from fd5548f to f6a5b61 Compare June 16, 2017 17:47

gesinger reviewed Jun 16, 2017

View reviewed changes

Shan Zhuang added 2 commits June 16, 2017 15:31

change the positon if clearing warn log

6275f13

rephrase the comment

b1443be

zhuangs force-pushed the feature/un-blacklist-playlist branch from 5a70507 to b1443be Compare June 16, 2017 20:49

gesinger approved these changes Jun 16, 2017

View reviewed changes

Shan Zhuang added 2 commits June 16, 2017 16:58

Merge remote-tracking branch 'upstream/master' into feature/un-blackl…

4f2c2cd

…ist-playlist

resolve conflict

5f2aab3

gesinger added 2 commits January 26, 2018 15:03

Merge branch 'master' into feature/un-blacklist-playlist

c074ec1

Resolve merge conflicts

d478282

gesinger approved these changes Jan 26, 2018

View reviewed changes

mjneil reviewed Jan 26, 2018

View reviewed changes

Move check for final playlist media change to after string playlist r…

446a46a

…esolver

mjneil approved these changes Jan 30, 2018

View reviewed changes

gesinger removed the needs testing label Jan 30, 2018

Fire a 'retryplaylist' event when clearing the blacklist

09f2a6c

forbesjo unassigned gesinger Oct 3, 2018

forbesjo closed this Jan 11, 2019

forbesjo mentioned this pull request Feb 7, 2019

fix: Clear the blacklist for other playlists if final rendition errors videojs/http-streaming#396

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zhuangs commented Apr 11, 2017

mjneil Apr 14, 2017

gesinger May 19, 2017

gesinger May 19, 2017

gesinger May 19, 2017

gesinger May 19, 2017

gesinger May 19, 2017

zhuangs May 24, 2017

gesinger May 19, 2017

gesinger May 24, 2017

gesinger May 24, 2017

zhuangs May 25, 2017

gesinger May 24, 2017

gesinger May 24, 2017

gesinger May 30, 2017

gesinger May 30, 2017

gesinger May 30, 2017

gesinger Jun 16, 2017

gesinger Jun 16, 2017

gesinger Jun 16, 2017

zhuangs Jun 16, 2017

andybarron commented Oct 24, 2017

mjneil Jan 26, 2018

gesinger Jan 29, 2018

forbesjo commented Aug 20, 2018

forbesjo commented Jan 11, 2019

		@@ -1153,9 +1152,11 @@ QUnit.test('playlist 404 should blacklist media', function(assert) {
		this.clock.tick(3 * 1000);
		// continue loading the final remaining playlist after it wasn't blacklisted

		assert.strictEqual(4, this.requests.length, 'one more request was made');
		// the first media was unblacklisted after a refresh delay

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

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

Conversation

zhuangs commented Apr 11, 2017

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andybarron commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forbesjo commented Aug 20, 2018

forbesjo commented Jan 11, 2019