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

fix: reduce playlist exclusion defaults #1413

Merged
merged 2 commits into from
Aug 14, 2023
Merged

Conversation

adrums86
Copy link
Contributor

@adrums86 adrums86 commented Aug 7, 2023

Description

The default playlist exclusion duration seemed arbitrarily long, especially for live playback. Lets reduce the default and the earlyAbortWhenNeeded_ exclusion to seconds instead of minutes to allow for more live resiliency.

Specific Changes proposed

Reduce the default playlistExclusionDuration value from 5 minutes to 10 seconds. Also reduce the ABORT_EARLY_EXCLUSION_SECONDS from 2 minutes to 10 seconds.

Requirements Checklist

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

@adrums86 adrums86 self-assigned this Aug 7, 2023
@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Merging #1413 (db2462d) into main (4153b8a) will increase coverage by 0.23%.
Report is 3 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1413      +/-   ##
==========================================
+ Coverage   85.56%   85.80%   +0.23%     
==========================================
  Files          41       41              
  Lines       10147    10297     +150     
  Branches     2353     2425      +72     
==========================================
+ Hits         8682     8835     +153     
+ Misses       1465     1462       -3     
Files Changed Coverage Δ
src/playlist-controller.js 96.28% <100.00%> (+1.05%) ⬆️
src/videojs-http-streaming.js 90.98% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@harisha-swaminathan harisha-swaminathan dismissed their stale review August 7, 2023 18:27

May require more investigation as discussed with Adam!

@dzianis-dashkevich
Copy link
Contributor

Should we mark it as a draft if it requires more investigation? @adrums86 @harisha-swaminathan

@adrums86
Copy link
Contributor Author

adrums86 commented Aug 8, 2023

@dzianis-dashkevich yeah good question. The original PR cites preventing a cache loop, but the time still seems arbitrary. I haven't seen any adverse effects lowering this value while testing. I'm starting to think this is likely a safe change that will net more stability for the average user than any risk assumed by lowering the values significantly.

@adrums86
Copy link
Contributor Author

adrums86 commented Aug 8, 2023

@gesinger I know the original fix was an eternity ago, but I figure it's worth pinging you to see if you have any thoughts on reducing these values?

@dzianis-dashkevich
Copy link
Contributor

Maybe we should consider some hybrid approach: like introducing min=10 and max=Infinity values.
On each failed load - increase exclude by some factor excludeTime = previousExcludeTime * factor
and on every success load - reset previousExcludeTime back to min

@adrums86
Copy link
Contributor Author

I like the hybrid approach for the playback watcher, which I think we can eventually change, as it currently excludes for Infinity. I think the most important piece here is bringing ABORT_EARLY_EXCLUSION_SECONDS down to less than a value in minutes. As this is currently excluding a playlist for 2 minutes whenever our default ABR aborts requests when it needs to step down due to network conditions.

@adrums86
Copy link
Contributor Author

@dzianis-dashkevich @harisha-swaminathan I think we should move ahead with this change. Reducing the constant and the default value will only effect cases where our default ABR aborts a request due to poor network conditions OR if a playlist is not updated for an unknown reason. All other behavior (unsupported playlists or the playback-watcher stalled downloads functionality) uses the exclusion duration of Infinity, not the configurable value or the constant.

@dzianis-dashkevich
Copy link
Contributor

sure, lets merge it

@adrums86 adrums86 merged commit bf0a300 into main Aug 14, 2023
14 checks passed
@adrums86 adrums86 deleted the reduceExclusionDefault branch August 14, 2023 17:25
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.

3 participants