feat: Retry on error #7038

alex-barstow · 2021-01-12T15:26:07Z

Description

This adds support for a retryOnError option that will have the player select a new source from the source list when the selected one fails prior to playback. If a source fails during playback, no retry will be attempted.

gkatsev

Not sure why tests are failing, I would ignore that for now.

gkatsev · 2021-01-12T17:48:33Z

src/js/player.js

+      const retry = () => {
+        // Remove the error modal
+        this.error(null);
+        this.retrying_ = true;


is it possible that we can re-use changingSrc_ here? It gets set to true when we set a new source and then is updated to false when a source gets set.

Unfortunately I don't believe so. retrying_ signals that at least one retry has been attempted, so we don't add the removeRetryHandler below more than once during any subsequent retries. At the point when we need to check whether to add that handler or not, changingSrc_ is always true so it cannot serve the same purpose.

gkatsev · 2021-01-12T17:50:09Z

test/unit/player.test.js

+    'last source remains'
+  );
+
+  player.dispose();


can we add a final check that verifies that player.currentSources() still contains the initial list?

Good thinking

gkatsev · 2021-01-12T17:55:10Z

src/js/player.js

+        // Remove the error modal
+        this.error(null);
+        this.retrying_ = true;
+        this.src(sources.slice(1));


I did notice that we do the same thing above in the mdidleware setSource callback. I assume that that only happens when a middleware handler returns an error all the way down?

No, I think that piece only is actually badly named. The err there is actually whether any techs can play the currently chosen source. So, this still makes sense. I think the other piece would trigger if we were to add say an RTMP source. Though, that's actually a good thing to verify that the two pieces don't step on each other's toes.

misteroneill

There was an offline discussion about the issue of differentiating between a src() call that comes from a user vs. one that comes from the retry process.

One potential avenue for that is to add another argument to src() but I think a preferable approach would be to use a pseudo-private method to hide the second argument. Something like:

src(source) {
  return this.src_(source);
}
src_(source, duringReset) {
  // Everything from current src() method, using duringReset as a 
  // differentiator and calling src_() internally.
}

I know src_() already exists, but I'm using it here for demonstration purposes.

…rom public source setting

gkatsev

Looks good. I did find an infinite error loop that's not great. Let me investigate it some more first.

src/js/player.js

gkatsev · 2021-01-14T23:08:56Z

So, the infinite loop happens in this type of cases:

player.src([
  {src: 'http://vjs.zencdn.net/v/oceans.foo', type: 'video/mp4'},
  {src: 'https://d2zihajmogu5jn.cloudfront.net/bipbop-advanced/bipbop_16x9_variant.m3u8', type:'application/x-mpegurl'},
  {src: 'http://vjs.zencdn.net/v/oceans.mp4', type: 'video/mp4'},
  ]);
player.on('error', () => {
  console.log('we errored');
  player.src({src:'http://vjs.zencdn.net/v/oceans.bar', type:'video/mp4'})
});

If I listen to the error with one instead of on, everything is fine. Interestingly, giving it a second source works fine.
Looking at it some more, this would be an issue even without the retry on error. So, I don't think it's something we need to worry about here.

video-archivist-bot · 2021-01-14T23:08:58Z

Hey! We've detected some video files in a comment on this issue. If you'd like to permanently archive these videos and tie them to this project, a maintainer of the project can reply to this issue with the following commands:

for https://d2zihajmogu5jn.cloudfront.net/bipbop-advanced/bipbop_16x9_variant.m3u8: say @video-archivist-bot save nwRJw2
for http://vjs.zencdn.net/v/oceans.mp4: say @video-archivist-bot save 5wq1wB

Co-authored-by: Gary Katsevman <[email protected]>

gkatsev · 2021-01-14T23:51:55Z

So, with the following thing we end up with a player that works but I think it works because our error handlers aren't getting cleared up properly:

player.src([
  {src: './oceans.foo', type: 'video/mp4'},
  {src: './oceans.mp4', type: 'video/mp4'},
  {src: 'https://d2zihajmogu5jn.cloudfront.net/bipbop-advanced/bipbop_16x9_variant.m3u8', type:'application/x-mpegurl'},
  ]);
player.on('error', () => {
  console.log('iteration %d', ++i, DomData.get(player.el()).handlers.error.length);
  player.src([
    {src:'./oceans.bar', type:'video/mp4'},
    {src:'./oceans.baz', type:'video/mp4'},
  {src: 'https://d2zihajmogu5jn.cloudfront.net/bipbop-advanced/bipbop_16x9_variant.m3u8', type:'application/x-mpegurl'},
  ]);
});

I then added this line below where we add the error handler

      console.log('retry add', retry.guid)

and then the following before removal (in both playing handler and the reset method)

      console.log('retry remove', retry.guid)

I then end up with the following output:

18:42:03.600 video.js:26905 retry add 551
18:42:03.891 video.js:26905 retry add 1019
18:42:03.892 video.js:26913 retry remove 1019
18:42:03.893 video.js:26905 retry add 1021
18:42:03.936 video.js:26913 retry remove 1021
18:42:03.937 video.js:26905 retry add 1034
18:42:03.939 video.js:26905 retry add 1037
18:42:03.954 video.js:26913 retry remove 1037
18:42:03.955 video.js:26905 retry add 1043
18:42:03.957 video.js:26905 retry add 1046

Notice that 551, 1034, and 1046 haven't been removed. I'd expect that only 1046 wouldn't have a matching remove yet.
When I hit play, it seems like all those pending ones get removed as expected:

18:42:19.803 video.js:26899 retry remove 551
18:42:19.804 video.js:26899 retry remove 1034
18:42:19.804 video.js:26899 retry remove 1043

I think these extra error handlers end up making the above snippet work.

video-archivist-bot · 2021-01-14T23:51:57Z

Hey! We've detected some video files in a comment on this issue. If you'd like to permanently archive these videos and tie them to this project, a maintainer of the project can reply to this issue with the following commands:

for https://d2zihajmogu5jn.cloudfront.net/bipbop-advanced/bipbop_16x9_variant.m3u8: say @video-archivist-bot save mj7Xwk

alex-barstow · 2021-01-15T20:51:05Z

I updated the reset logic so we remove all retry-related event handlers on every handleSrc_() call, and this seems to fix the cleanup issues.

gkatsev

Verified locally that the handlers get removed properly now and double-checked how this works for autoplay.
LGTM!

gkatsev · 2021-01-19T23:06:23Z

This PR fixes #1805!

John2397 · 2021-02-01T10:40:00Z

This PR fixes #1805!

When can we expect the new release that will include this feature?

gkatsev · 2021-05-19T00:40:57Z

@John2397 this is available now in v7.12.

gkatsev · 2021-05-19T00:41:27Z

Looks like we forgot to document this option in https://github.com/videojs/video.js/blob/main/docs/guides/options.md

Add a `retryOnError` option. When set, during source selection, if a source fails to load, we will retry the next item in the sources list. In the future, we may enable this by default. A source that fails during playback will *not* trigger this behavior. Fixes videojs#1805.

add retry logic to src() method, add tests

7d44943

gkatsev reviewed Jan 12, 2021

View reviewed changes

only update cached sourcelist if we arent retrying

1a7ee73

misteroneill reviewed Jan 14, 2021

View reviewed changes

brandonocasey mentioned this pull request Jan 14, 2021

test: update ci workflow to prevent install failures #7041

Merged

alex-barstow added 3 commits January 14, 2021 14:52

put src() logic into private method so we can differentiate private f…

efcd9d3

…rom public source setting

tweaks, add test

2d0264f

remove unused flag

ae04bed

gkatsev reviewed Jan 14, 2021

View reviewed changes

src/js/player.js Outdated Show resolved Hide resolved

gkatsev reviewed Jan 14, 2021

View reviewed changes

src/js/player.js Outdated Show resolved Hide resolved

src/js/player.js Outdated Show resolved Hide resolved

alex-barstow and others added 3 commits January 14, 2021 18:13

explicitly pass second arg to handleSrc_ call

669ad03

Co-authored-by: Gary Katsevman <[email protected]>

add underscore to private method

ba136b9

Co-authored-by: Gary Katsevman <[email protected]>

add another underscore to private method

b7b9376

Co-authored-by: Gary Katsevman <[email protected]>

fix reset logic and test

c2fcce4

gkatsev approved these changes Jan 19, 2021

View reviewed changes

gkatsev mentioned this pull request Jan 19, 2021

Retry after network error #1805

Closed

gkatsev added the confirmed label Mar 9, 2021

gkatsev merged commit 22e9843 into videojs:main Mar 23, 2021

John2397 mentioned this pull request Jun 17, 2021

How to enable retryOnError option #7277

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Retry on error #7038

feat: Retry on error #7038

alex-barstow commented Jan 12, 2021

gkatsev left a comment

gkatsev Jan 12, 2021

alex-barstow Jan 12, 2021

gkatsev Jan 12, 2021

alex-barstow Jan 12, 2021

alex-barstow Jan 14, 2021

gkatsev Jan 12, 2021

gkatsev Jan 12, 2021

misteroneill left a comment •

edited

Loading

gkatsev left a comment

gkatsev commented Jan 14, 2021 •

edited

Loading

video-archivist-bot commented Jan 14, 2021

gkatsev commented Jan 14, 2021

video-archivist-bot commented Jan 14, 2021

alex-barstow commented Jan 15, 2021

gkatsev left a comment

gkatsev commented Jan 19, 2021

John2397 commented Feb 1, 2021

gkatsev commented May 19, 2021

gkatsev commented May 19, 2021

feat: Retry on error #7038

feat: Retry on error #7038

Conversation

alex-barstow commented Jan 12, 2021

Description

gkatsev left a comment

Choose a reason for hiding this comment

gkatsev Jan 12, 2021

Choose a reason for hiding this comment

alex-barstow Jan 12, 2021

Choose a reason for hiding this comment

gkatsev Jan 12, 2021

Choose a reason for hiding this comment

alex-barstow Jan 12, 2021

Choose a reason for hiding this comment

alex-barstow Jan 14, 2021

Choose a reason for hiding this comment

gkatsev Jan 12, 2021

Choose a reason for hiding this comment

gkatsev Jan 12, 2021

Choose a reason for hiding this comment

misteroneill left a comment • edited Loading

Choose a reason for hiding this comment

gkatsev left a comment

Choose a reason for hiding this comment

gkatsev commented Jan 14, 2021 • edited Loading

video-archivist-bot commented Jan 14, 2021

gkatsev commented Jan 14, 2021

video-archivist-bot commented Jan 14, 2021

alex-barstow commented Jan 15, 2021

gkatsev left a comment

Choose a reason for hiding this comment

gkatsev commented Jan 19, 2021

John2397 commented Feb 1, 2021

gkatsev commented May 19, 2021

gkatsev commented May 19, 2021

misteroneill left a comment •

edited

Loading

gkatsev commented Jan 14, 2021 •

edited

Loading