Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow clients to limit the number of times a playlist attempts to reload following an error #1098

Merged

Conversation

evanfarina
Copy link
Contributor

Description

Addresses the issue of unlimited retries mentioned in issue #983 by allowing clients to control how often a playlist will try to reload following an error. Thanks to @gkatsev for the suggestion of using the playlist object to keep track of number of retries.

Specific Changes proposed

Introduce a new option maxPlaylistRetries which sets a limit on the number of times a playlist will be retried. Once the number of retries is greater than (>) the value set by maxPlaylistRetries, playlist.excludeUntil is set to Infinity.

Note: A property retryCount is set and incremented on Playlist objects. I've found that playlist errors and segment errors follow different paths (playlistRequestError and blacklistCurrentPlaylist, respectfully) so I'm incrementing this property in each of these places. I'd appreciate it if a reviewer could confirm that no error will result in both of these methods being called as that would cause a single error to increment the retryCount twice.

Requirements Checklist

  • [ x ] Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

@codecov
Copy link

codecov bot commented Mar 24, 2021

Codecov Report

Merging #1098 (298508d) into main (5405c18) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1098      +/-   ##
==========================================
+ Coverage   86.41%   86.44%   +0.02%     
==========================================
  Files          39       39              
  Lines        9465     9471       +6     
  Branches     2182     2183       +1     
==========================================
+ Hits         8179     8187       +8     
+ Misses       1286     1284       -2     
Impacted Files Coverage Δ
src/segment-loader.js 96.18% <ø> (ø)
src/manifest.js 93.06% <100.00%> (+0.06%) ⬆️
src/master-playlist-controller.js 94.49% <100.00%> (+0.03%) ⬆️
src/playlist-loader.js 93.73% <0.00%> (+0.62%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5405c18...298508d. Read the comment docs.

@evanfarina evanfarina force-pushed the efarina/configure_playlist_retry_count branch from 4360dc0 to 67bb93c Compare April 15, 2021 11:59
Copy link
Contributor

@brandonocasey brandonocasey left a comment

Choose a reason for hiding this comment

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

Sorry it took us so long to review. There has been a ton on our plate internally.

README.md Show resolved Hide resolved
src/manifest.js Outdated Show resolved Hide resolved
src/master-playlist-controller.js Outdated Show resolved Hide resolved
test/master-playlist-controller.test.js Outdated Show resolved Hide resolved
brandonocasey
brandonocasey previously approved these changes Apr 29, 2021
gkatsev
gkatsev previously approved these changes May 5, 2021
Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Aside from the weird test whitespace, LGTM

test/playlist-loader.test.js Outdated Show resolved Hide resolved
test/playlist-loader.test.js Outdated Show resolved Hide resolved
test/playlist-loader.test.js Outdated Show resolved Hide resolved
@gkatsev
Copy link
Member

gkatsev commented May 5, 2021

The playback tests look a bit suspicious, they don't seem like the standard timeout issue we see.

@evanfarina evanfarina dismissed stale reviews from gkatsev and brandonocasey via 402157f May 12, 2021 15:20
gkatsev
gkatsev previously approved these changes Jun 9, 2021
@gkatsev gkatsev merged commit 44905d4 into videojs:main Jun 11, 2021
gkatsev pushed a commit that referenced this pull request Jun 22, 2021
Followup to #1098 where the default should've been Infinity, but it got lost in translation.

Co-authored-by: Evan farina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants