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

Clamp seekable end #1433

Merged
merged 2 commits into from
Oct 12, 2023
Merged

Clamp seekable end #1433

merged 2 commits into from
Oct 12, 2023

Conversation

dzianis-dashkevich
Copy link
Contributor

@dzianis-dashkevich dzianis-dashkevich commented Oct 11, 2023

Description

We have default liveEdgePadding as 3 * targetDuration.
Assume we have a stream with a maximum of three segments.
Each segment's actual duration is less than a targetDuration.
In this scenario, we may have a seekable.end less than a seekable.start.
This may lead to unexpected behavior.

Specific Changes proposed

Clamp seekable.end to make sure that it is more or equal to seekable start.

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 Oct 11, 2023

Codecov Report

Merging #1433 (7d1ae96) into main (dd5e2af) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1433   +/-   ##
=======================================
  Coverage   85.98%   85.99%           
=======================================
  Files          42       42           
  Lines       10428    10430    +2     
  Branches     2412     2413    +1     
=======================================
+ Hits         8967     8969    +2     
  Misses       1461     1461           
Files Coverage Δ
src/playlist.js 94.58% <100.00%> (+0.04%) ⬆️

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

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

Seems eminently reasonable to me!

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.

Great find! :shipit:

@dzianis-dashkevich dzianis-dashkevich merged commit 53edf72 into main Oct 12, 2023
14 checks passed
@dzianis-dashkevich dzianis-dashkevich deleted the clamp-seekable-end branch October 12, 2023 15:51
dzianis-dashkevich added a commit that referenced this pull request Nov 30, 2023
* fix seekable end

* fix eslint error

---------

Co-authored-by: Dzianis Dashkevich <ddashkevich@brightcove.com>
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.

5 participants