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(live): only reset playlist loader for LLHLS #1410

Merged
merged 2 commits into from
Jul 25, 2023
Merged

Conversation

adrums86
Copy link
Contributor

Description

After a LOT of debugging with playlist changes against a linear DASH stream, I discovered we're calling resetLoader when setting a new playlist (during a rendition switch), which sets fetchAtBuffer_ to false then calls resyncLoader. This sets the mediaIndex to null, which eventually on the next chooseNextRequest_ call (which as implied, chooses the next segment to request) forces us into the else logic. Because fetchAtBuffer_ is false, we begin requesting segments from the currentTime rather than the end of the buffer. This causes the playback watcher to assume a segment download has stalled, because when we request segments at the currentTime we're either requesting cached segments or overwriting segments that were already buffered, which results in the buffer not changing and an increment of the stalled segment counter.

Specific Changes proposed

Since this logic was originally implemented as an fix for LLHLS playback (see: #1201) we can isolate it to LLHLS streams by checking for a partTargetDuration on the playlist, which is required for LLHLS streams, see: https://datatracker.ietf.org/doc/html/draft-pantos-hls-rfc8216bis#section-4.4.3.7.
This greatly reduces the re-requesting of already buffered and cached segment requests during live playback and in turn the stability of these streams as the buffer isn't being artificially starved during a rendition switch.

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 Jul 25, 2023
@codecov
Copy link

codecov bot commented Jul 25, 2023

Codecov Report

Merging #1410 (84275c1) into main (2079454) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1410   +/-   ##
=======================================
  Coverage   85.55%   85.56%           
=======================================
  Files          41       41           
  Lines       10145    10146    +1     
  Branches     2351     2352    +1     
=======================================
+ Hits         8680     8681    +1     
  Misses       1465     1465           
Files Changed Coverage Δ
src/segment-loader.js 96.48% <100.00%> (+<0.01%) ⬆️

📣 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.

LGTM!

@adrums86 adrums86 merged commit 0d8a7a3 into main Jul 25, 2023
@adrums86 adrums86 deleted the fix_live_resetLoader branch July 25, 2023 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants