-
Notifications
You must be signed in to change notification settings - Fork 425
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
Conversation
Codecov Report
@@ 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
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
May require more investigation as discussed with Adam!
Should we mark it as a draft if it requires more investigation? @adrums86 @harisha-swaminathan |
@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. |
@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? |
Maybe we should consider some hybrid approach: like introducing |
I like the hybrid approach for the playback watcher, which I think we can eventually change, as it currently excludes for |
@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 |
sure, lets merge it |
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 theABORT_EARLY_EXCLUSION_SECONDS
from 2 minutes to 10 seconds.Requirements Checklist