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

Pr for media sequence #1204

Merged
merged 12 commits into from
Sep 22, 2021
Merged

Conversation

jontsnz
Copy link
Contributor

@jontsnz jontsnz commented Sep 16, 2021

Description

The browser tab running video.js would show a "Page Unresponsive" dialog in Chrome when the EXT-X-MEDIA-SEQUENCE tag increased by a large number between subsequent m3u8 files. This happened because the saveExpiredSegmentInfo function was iterating over this large number.

This behaviour was raised in issue 1197

Proposed Solution

The proposed solution is to avoid the browser becoming unresponsive by only iterating over the media sequence difference if it is within a reasonable threshold.

To determine a reasonable threshold, I looked at Apple's "Live Playlist (Sliding Window) Construction" page which states:

The EXT-X-MEDIA-SEQUENCE tag value must be incremented by 1 for every media URI that's removed from the playlist file.

Based on this, I have used a threshold of 1000.

Requirements Checklist

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

@welcome
Copy link

welcome bot commented Sep 16, 2021

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

Copy link
Contributor

@brandonocasey brandonocasey left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request! I think 1000 is sufficiently large here. I just have a small comment on the log message. Do you think you might have some time to write a test?

src/sync-controller.js Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 17, 2021

Codecov Report

Merging #1204 (4909989) into main (e2b46e7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1204   +/-   ##
=======================================
  Coverage   86.33%   86.34%           
=======================================
  Files          39       39           
  Lines        9691     9695    +4     
  Branches     2243     2244    +1     
=======================================
+ Hits         8367     8371    +4     
  Misses       1324     1324           
Impacted Files Coverage Δ
src/sync-controller.js 96.95% <100.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2b46e7...4909989. Read the comment docs.

src/sync-controller.js Outdated Show resolved Hide resolved
gkatsev
gkatsev previously approved these changes Sep 21, 2021
Copy link
Contributor

@brandonocasey brandonocasey left a comment

Choose a reason for hiding this comment

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

One last change brought up internally.

src/sync-controller.js Outdated Show resolved Hide resolved
test/sync-controller.test.js Outdated Show resolved Hide resolved
gesinger
gesinger previously approved these changes Sep 22, 2021
@brandonocasey brandonocasey merged commit 0dc0b61 into videojs:main Sep 22, 2021
@welcome
Copy link

welcome bot commented Sep 22, 2021

Congrats on merging your first pull request! 🎉🎉🎉

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.

4 participants