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 repeated segments issue during bandwidth update #1477

Merged

Conversation

dzianis-dashkevich
Copy link
Contributor

Description

We may encounter duplicate segment loading during bandwidth updates.
Since we are re-calculating timestampOffset during each quality switch it results in presenting of duplicate segments.

Consider the following situation:
image

This log is placed in the chooseNextRequest_.
As you can see:

  • The player selected MediaSequence 28 from playlist A.
  • Then, the player switched quality to playlist B because of the bandwidth update.
  • Then, the player selected MediaSequence 28 despite the fact that it was already buffered from the previous playlist.
  • This happens because of the slight difference between the buffered.end and the sum of durations from the playlist.

Specific Changes proposed

Make an assumption that some segment or segment's part is already buffered if the difference between segment.end and buffered.end is less or equal to 2 * TIME_FUDGE_FACTOR (which is 0.06)
Fallback by selecting the next part or segment using buffered.end + difference padding.

Requirements Checklist

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

Copy link

codecov bot commented Jan 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5b87f69) 86.06% compared to head (3d04436) 87.13%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1477      +/-   ##
==========================================
+ Coverage   86.06%   87.13%   +1.07%     
==========================================
  Files          42       42              
  Lines       10748    11887    +1139     
  Branches     2474     2904     +430     
==========================================
+ Hits         9250    10358    +1108     
- Misses       1498     1529      +31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/playlist.js Show resolved Hide resolved
src/segment-loader.js Show resolved Hide resolved
src/segment-loader.js Show resolved Hide resolved
Copy link
Contributor

@wseymour15 wseymour15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved after nit comments are addressed.

Copy link
Contributor

@adrums86 adrums86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I do think it's probably worth adding a quick test or 2 for the <= extendedFudgeFactor and !mediaInfo cases however.

src/segment-loader.js Outdated Show resolved Hide resolved
dzianis-dashkevich and others added 2 commits January 22, 2024 12:40
Co-authored-by: Adam Waldron <awaldron@brightcove.com>
…eated-segments-issue-during-bandwidth-update
Copy link
Contributor

@adrums86 adrums86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: !

@dzianis-dashkevich dzianis-dashkevich merged commit 823f072 into main Jan 25, 2024
15 checks passed
@dzianis-dashkevich dzianis-dashkevich deleted the fix-repeated-segments-issue-during-bandwidth-update branch January 25, 2024 21:55
dzianis-dashkevich added a commit that referenced this pull request Feb 21, 2024
dzianis-dashkevich added a commit that referenced this pull request Feb 21, 2024
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