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: fastQualityChange refactor #1414

Merged
merged 6 commits into from
Aug 14, 2023
Merged

fix: fastQualityChange refactor #1414

merged 6 commits into from
Aug 14, 2023

Conversation

adrums86
Copy link
Contributor

@adrums86 adrums86 commented Aug 8, 2023

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

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

@codecov
Copy link

codecov bot commented Aug 8, 2023

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%) ⬆️
src/segment-loader.js 96.50% <100.00%> (+0.02%) ⬆️
src/videojs-http-streaming.js 91.00% <100.00%> (+0.01%) ⬆️

... and 1 file with indirect coverage changes

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

@adrums86
Copy link
Contributor Author

adrums86 commented Aug 10, 2023

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
Copy link
Contributor Author

adrums86 commented Aug 10, 2023

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

@adrums86 adrums86 merged commit 4590bdd into main Aug 14, 2023
@adrums86 adrums86 deleted the fix-fastQualityChange branch August 14, 2023 17:27
@gsimko
Copy link

gsimko commented Oct 25, 2023

@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?

dzianis-dashkevich pushed a commit that referenced this pull request Nov 30, 2023
dzianis-dashkevich added a commit that referenced this pull request Dec 4, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…rEachSegment and remove replaceSegmentsUntil (#1457)

* Revert "fix: check for transmuxer for vtt-segment-loader (#1452)"

This reverts commit b4dd748.

* Revert "fix: fix several issues with calculate timestamp offset for each segment (#1451)"

This reverts commit 3bbc6ef.

* Revert "fix: replaceSegmentsUntil flag resetting too early (#1444)"

This reverts commit af39ba5.

* Revert "fix: prevent wrapping in resetMainLoaderReplaceSegments (#1439)"

This reverts commit 719b7f4.

* Revert "feat: Add feature flag to calculate timestampOffset for each segment to handle streams with corrupted pts or dts timestamps (#1426)"

This reverts commit 2355ddc.

* Revert "fix: fastQualityChange refactor (#1414)"

This reverts commit 4590bdd.

* cherry-pick: use transmuxer time info instead of probeTs

* feat: sync controller media sequence strategy (#1458)

* feat: add media sequence sync strategy

* fix: fix current media sequence increment

* chore: update logs

* feat: use exact segment match in sync-controller

* fix: fix race condition for a fast quality switch

* chore: add additional logs for choose next request

* feat: force timestamp after resync

* chore: fix or skip tests

* Update src/segment-loader.js

Co-authored-by: Walter Seymour <[email protected]>

---------

Co-authored-by: Dzianis Dashkevich <[email protected]>
Co-authored-by: Walter Seymour <[email protected]>

---------

Co-authored-by: Dzianis Dashkevich <[email protected]>
Co-authored-by: Walter Seymour <[email protected]>
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.

None yet

4 participants