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: remove segment loader abort in setCurrentTime #1415

Merged
merged 1 commit into from
Aug 14, 2023

Conversation

adrums86
Copy link
Contributor

@adrums86 adrums86 commented Aug 8, 2023

Description

Removing the abort call from setCurrentTime makes seeking and certain playlist switching cases more resilient. This is because setCurrentTime was essentially calling abort on all the segment loaders twice. Once in the setCurrentTime function directly, then from resetEverything which eventually calls resyncLoader which also calls abort on the segment loader.

Specific Changes proposed

Remove the abort calls on all segment loaders and fix the tests.

Requirements Checklist

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

@adrums86 adrums86 self-assigned this Aug 8, 2023
@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

Merging #1415 (fd44c5c) into main (de183c8) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1415   +/-   ##
=======================================
  Coverage   85.55%   85.56%           
=======================================
  Files          41       41           
  Lines       10145    10142    -3     
  Branches     2351     2351           
=======================================
- Hits         8680     8678    -2     
+ Misses       1465     1464    -1     
Files Changed Coverage Δ
src/playlist-controller.js 95.33% <ø> (+0.10%) ⬆️

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

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.

Looks great!

@adrums86 adrums86 merged commit 323bb32 into main Aug 14, 2023
11 checks passed
@adrums86 adrums86 deleted the fix-remove-setCurrentTime-aborts branch August 14, 2023 15:28
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