videojs / http-streaming

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

fix: fastQualityChange refactor #1414

Closed adrums86 closed 11 months ago

adrums86 commented 11 months ago

Description

fastQualityChange_ was causing unnecessary rebuffering and requesting cached segments. This was resulting in slow and very buggy playlist changes. This was because it was calling resetEverything on the mainSegmentLoader, then calling setCurrentTime which calls resetEverything again on ALL the segment loaders, which removes all content from the buffer and aborts all requests.

Edit: Had to add a property replaceSegmentsUntil, which represents the end of the buffer at the moment fastQualityChange_ is called. This tells the segment loader to leave fetchAtBuffer_ as false until we've successfully replaced all the buffered segments between currentTime and bufferedEnd at the moment fastQualityChange_ is called.

Specific Changes proposed

Remove the resetEverything and setCurrentTime call from fastQualityChange_ and instead reset the next segment position to currentTime with a resetLoader call, keeping the current buffer and overwriting it.

Requirements Checklist

codecov[bot] commented 11 months ago

Codecov Report

Merging #1414 (738d2b5) into main (de183c8) will increase coverage by 0.05%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1414      +/-   ##
==========================================
+ Coverage   85.55%   85.61%   +0.05%     
==========================================
  Files          41       41              
  Lines       10145    10159      +14     
  Branches     2351     2352       +1     
==========================================
+ Hits         8680     8698      +18     
+ Misses       1465     1461       -4     
Files Changed Coverage Δ
src/playlist-controller.js 95.25% <100.00%> (+0.02%) :arrow_up:
src/segment-loader.js 96.50% <100.00%> (+0.02%) :arrow_up:
src/videojs-http-streaming.js 91.00% <100.00%> (+0.01%) :arrow_up:

... and 1 file with indirect coverage changes

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

adrums86 commented 11 months ago

I found a bug where, if there is > 1 segment buffered, the player will switch back to that segment (sometimes at reduced quality) when switching.I have a fix almost complete that I will push ASAP and tag all reviewers for another look.

adrums86 commented 11 months ago

@dzianis-dashkevich @wseymour15 This could use another look after the latest commit 4639842 (forgot to push the test fixes 😁 )

gsimko commented 8 months ago

@adrums86 is it a known bug that the new code might fail if nothing is buffered? Specifically, I get Failed to execute 'end' on 'TimeRanges': The index provided (4294967295) is greater than the maximum bound (0) due to the line const bufferedEnd = buffered.end(buffered.length - 1);.

I guess this happens because nothing is buffered. Could we early return from that function if that's the case?