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

Sync controller media sequence strategy #1458

Merged

Conversation

dzianis-dashkevich
Copy link
Contributor

@dzianis-dashkevich dzianis-dashkevich commented Nov 30, 2023

  • implement a new sync strategy based on the media sequence
  • implement exact match for sync controller
  • fix fast quality switch race condition
  • implement force timestamp offset after resync loader

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (98dedca) 86.12% compared to head (963caba) 85.97%.

Files Patch % Lines
src/sync-controller.js 82.08% 12 Missing ⚠️
src/playlist-controller.js 78.57% 3 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           align-3-5-2    #1458      +/-   ##
===============================================
- Coverage        86.12%   85.97%   -0.15%     
===============================================
  Files               42       42              
  Lines            10586    10673      +87     
  Branches          2441     2456      +15     
===============================================
+ Hits              9117     9176      +59     
- Misses            1469     1497      +28     

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

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.

These changes make sense to me! Approving under the assumption that we are coming back to this and cleaning up the skipped tests

src/segment-loader.js Outdated Show resolved Hide resolved
Co-authored-by: Walter Seymour <walterseymour15@gmail.com>
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 think the skips are okay here as these tests seem to be dependent on the old sync strategies throwing away a request. I do however think we should add some new sync-controller tests for the new strategy, either in this PR or a follow up.

@dzianis-dashkevich
Copy link
Contributor Author

@adrums86, I will add it in the next PRs

@dzianis-dashkevich dzianis-dashkevich merged commit 6e6e6c4 into align-3-5-2 Dec 4, 2023
13 of 15 checks passed
@dzianis-dashkevich dzianis-dashkevich deleted the sync-controller-media-sequence-strategy branch December 4, 2023 17:55
dzianis-dashkevich added a commit that referenced this pull request Dec 4, 2023
…rEachSegment and remove replaceSegmentsUntil (#1457)

* Revert "fix: check for transmuxer for vtt-segment-loader (#1452)"

This reverts commit b4dd748.

* Revert "fix: fix several issues with calculate timestamp offset for each segment (#1451)"

This reverts commit 3bbc6ef.

* Revert "fix: replaceSegmentsUntil flag resetting too early (#1444)"

This reverts commit af39ba5.

* Revert "fix: prevent wrapping in resetMainLoaderReplaceSegments (#1439)"

This reverts commit 719b7f4.

* Revert "feat: Add feature flag to calculate timestampOffset for each segment to handle streams with corrupted pts or dts timestamps (#1426)"

This reverts commit 2355ddc.

* Revert "fix: fastQualityChange refactor (#1414)"

This reverts commit 4590bdd.

* cherry-pick: use transmuxer time info instead of probeTs

* feat: sync controller media sequence strategy (#1458)

* feat: add media sequence sync strategy

* fix: fix current media sequence increment

* chore: update logs

* feat: use exact segment match in sync-controller

* fix: fix race condition for a fast quality switch

* chore: add additional logs for choose next request

* feat: force timestamp after resync

* chore: fix or skip tests

* Update src/segment-loader.js

Co-authored-by: Walter Seymour <walterseymour15@gmail.com>

---------

Co-authored-by: Dzianis Dashkevich <ddashkevich@brightcove.com>
Co-authored-by: Walter Seymour <walterseymour15@gmail.com>

---------

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

3 participants