-
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: fastQualitySwitch stability #1525
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1525 +/- ##
=======================================
Coverage 86.31% 86.31%
=======================================
Files 43 43
Lines 11097 11097
Branches 2532 2532
=======================================
Hits 9578 9578
Misses 1519 1519 ☔ View full report in Codecov by Sentry. |
this.mainSegmentLoader_.pause(); | ||
this.mainSegmentLoader_.resetEverything(() => { | ||
this.tech_.setCurrentTime(this.tech_.currentTime()); | ||
this.mainSegmentLoader_.load(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to load other loaders here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's necessary, the only reason load
was previously called on all the loaders was due to setCurrentTime
calling pause
then resetEverything
on all the loaders, including the mainSegmentLoader
a second time.
http-streaming/src/playlist-controller.js
Line 1535 in 0b4da7c
this.mainSegmentLoader_.pause(); |
Description
Currently, our fastQualitySwitch triggers a full video/main buffer clear twice. This is because we clear the buffer first with a
resetEverything
call on the mainSegmentLoader, then once that removal is complete we callsetCurrentTime
which callsresetEverything
on all active segment loaders, clearing all the other buffers as well. This is causing some instability, specifically when a quality change occurs at or near a discontinuity.Specific Changes proposed
Instead of calling
setCurrentTime
after a fastQualitySwitch, lets just callload
on the mainSegmentLoader.Note: There were 2 browser bugs that seemed to require a
setCurrentTime
call according to the comments, but after checking both URLs, those bugs seem to no longer be relevant, so we should be safe removing this part and only calling load.Requirements Checklist