videojs / http-streaming

HLS, DASH, and future HTTP streaming protocols library for video.js
https://videojs-http-streaming.netlify.app/
Other
2.49k stars 422 forks source link

fix: reduce playlist exclusion defaults #1413

Closed adrums86 closed 1 year ago

adrums86 commented 1 year ago

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

codecov[bot] commented 1 year ago

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%) :arrow_up:
src/videojs-http-streaming.js 90.98% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

dzianis-dashkevich commented 1 year ago

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

adrums86 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

sure, lets merge it