http-streaming icon indicating copy to clipboard operation
http-streaming copied to clipboard

fix: resume loading on segment timeout for `experimentalBufferBasedABR`

Open gesinger opened this issue 1 year ago • 1 comments

Previously, if there was a segment timeout and experimentalBufferBasedABR was set to true, then the main segment loader would stay in a READY state and never resume loading segments. This is because the buffer based ABR path doesn't follow the same flow as non buffer based ABR, where a new playlist would be loaded immediately and it would trigger a load. Buffer based ABR may determine that no rendition change should be made, despite the timeout, leading to nothing happening. This change makes the call to load explicit, but only for buffer based ABR on timeouts.

This addresses https://github.com/videojs/http-streaming/issues/1287

Test Changes

Changed

  • Updated test to show that experimentalBufferBasedABR does make a playlist selection on bandwidthupdate
  • Updated test to remove warning logged on a bandwidthupdate when there's only one playlist. This warning doesn't always make sense, as a timeout that causes a bandwidthupdate would be a connection issue, rather than a stream issue.

Added

  • Added test that segment-loader fires timeout event on timeout
  • Added test that master-playlist-controller reloads segment loader on timeout if experimentalBufferBasedABR

Testing

  1. Open the demo page. Enable [EXPERIMENTAL] Use Buffer Level for ABR (reloads player) and set preload to none.
  2. In Chrome Devtools use a very slow connection (I manually created a 10kbps profile).
  3. Press play
  4. Without the change, after the segment request times out (16.5 seconds for the first bipbop stream, no more requests are made and the buffering spinner remains. The video never starts, even if changing the network connectivity back to something reasonable.
  5. With the change, a new segment is requested and playback resumes (if changing network connectivity to something reasonable).

Requirements Checklist

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

gesinger avatar Oct 13 '22 15:10 gesinger

Codecov Report

Merging #1333 (555cc4a) into main (66707b4) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1333      +/-   ##
==========================================
+ Coverage   86.33%   86.34%   +0.01%     
==========================================
  Files          39       39              
  Lines        9857     9859       +2     
  Branches     2298     2299       +1     
==========================================
+ Hits         8510     8513       +3     
+ Misses       1347     1346       -1     
Impacted Files Coverage Δ
src/master-playlist-controller.js 94.79% <100.00%> (+0.13%) :arrow_up:
src/segment-loader.js 96.37% <100.00%> (+<0.01%) :arrow_up:

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

codecov[bot] avatar Oct 13 '22 15:10 codecov[bot]