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

feat: add dts-based timestamp offset calculation with feature toggle.… #1251

Merged

Conversation

dzianis-dashkevich
Copy link
Contributor

@dzianis-dashkevich dzianis-dashkevich commented Feb 11, 2022

Description

Introduced DTS-based timestampOffset calculation. Can be enabled with a feature flag.
For more info please check: #1247

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 Feb 11, 2022

💖 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

@gesinger gesinger left a comment

Choose a reason for hiding this comment

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

This is amazing, thank you so much!

One possible additional test would be to use the muxedSegment, which has different times between audio and video, to ensure that video is prioritized.

test/segment-loader.test.js Show resolved Hide resolved
test/segment-loader.test.js Show resolved Hide resolved
@dzianis-dashkevich
Copy link
Contributor Author

dzianis-dashkevich commented Feb 12, 2022

This is amazing, thank you so much!

One possible additional test would be to use the muxedSegment, which has different times between audio and video, to ensure that video is prioritized.

Thank you!
here is a commit with changes addressing this code review note:
6e4bc7c

Copy link
Contributor

@gesinger gesinger left a comment

Choose a reason for hiding this comment

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

Once again, thank you @dzianis-dashkevich ! This is very much appreciated.

@codecov
Copy link

codecov bot commented Feb 14, 2022

Codecov Report

Merging #1251 (a48318c) into main (211cbe8) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head a48318c differs from pull request most recent head d1c884b. Consider uploading reports for the commit d1c884b to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1251      +/-   ##
==========================================
+ Coverage   86.31%   86.35%   +0.03%     
==========================================
  Files          39       39              
  Lines        9807     9817      +10     
  Branches     2279     2283       +4     
==========================================
+ Hits         8465     8477      +12     
+ Misses       1342     1340       -2     
Impacted Files Coverage Δ
src/master-playlist-controller.js 94.65% <ø> (ø)
src/segment-loader.js 96.36% <100.00%> (+0.02%) ⬆️
src/videojs-http-streaming.js 91.04% <100.00%> (+0.01%) ⬆️
src/transmuxer-worker.js 73.54% <0.00%> (+0.05%) ⬆️

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 42fe383...d1c884b. Read the comment docs.

@misteroneill misteroneill merged commit 450eb2d into videojs:main Mar 14, 2022
@welcome
Copy link

welcome bot commented Mar 14, 2022

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