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: bad timeline changes #1526

Merged
merged 10 commits into from
Jul 22, 2024
Merged

fix: bad timeline changes #1526

merged 10 commits into from
Jul 22, 2024

Conversation

adrums86
Copy link
Contributor

Description

Sometimes when we're switching timelines, we run into a situation where the buffer is starved due to the main or audio trying to switch to separate timelines. shouldWaitForTimelineChange is called when deciding whether to wait to load or append the current segment. The conditions for this to return false are:

  • If we're calling from the audio segment loader, we need to wait for the main segment loader to change to the timeline the audio segment loader is trying to change to, before we allow further loads or appends for the audio segment loader.
if (loaderType === 'audio') {
   const lastMainTimelineChange = timelineChangeController.lastTimelineChange({
     type: 'main'
   });

   // Audio loader should wait if:
   //
   // * main hasn't had a timeline change yet (thus has not loaded its first segment)
   // * main hasn't yet changed to the timeline audio is looking to load
   return !lastMainTimelineChange || lastMainTimelineChange.to !== segmentTimeline;
 }
  • If we're calling from the main segment loader, we need to wait for audio to have that pending timeline change in order to allow further loads or appends from the main segment loader.
 const pendingAudioTimelineChange = timelineChangeController.pendingTimelineChange({
      type: 'audio'
    });

    // Main loader should wait for the audio loader if audio is not pending a timeline
    // change to the current timeline.
    //
    // Since the main loader is responsible for setting the timestamp offset for both
    // audio and video, the main loader must wait for audio to be about to change to its
    // timeline before setting the offset, otherwise, if audio is behind in loading,
    // segments from the previous timeline would be adjusted by the new timestamp offset.
    //
    // This requirement means that video will not cross a timeline until the audio is
    // about to cross to it, so that way audio and video will always cross the timeline
    // together.
    //
    // In addition to normal timeline changes, these rules also apply to the start of a
    // stream (going from a non-existent timeline, -1, to timeline 0). It's important
    // that these rules apply to the first timeline change because if they did not, it's
    // possible that the main loader will cross two timelines before the audio loader has
    // crossed one. Logic may be implemented to handle the startup as a special case, but
    // it's easier to simply treat all timeline changes the same.
    if (pendingAudioTimelineChange && pendingAudioTimelineChange.to === segmentTimeline) {
      return false;
    }

With certain live media, sometimes we'll change playlists (ABR, track change, manual quality switch) during a timeline change. This playlist change, causes a new request for a variant playlist that playlist isn't guaranteed to be the next timeline that the other loader is currently pending. For example:
On a quality change, the main segment loader is moving to an older timeline because the new variant is slightly behind.

CleanShot 2024-07-15 at 11 23 01

This causes the loader to get stuck waiting for the other loader to sync up for a timelineChange which never happens and eventually stalls playback.

Specific Changes proposed

Add checks in the loader to look for bad timeline changes, instances such as this where the loaders are both pending timeline changes to separate timelines and resetting the loader when those cases occur, in order to re-sync timelines and avoid a stall.

Requirements Checklist

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

Copy link

codecov bot commented Jul 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.33%. Comparing base (28cb9fd) to head (48551a6).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1526      +/-   ##
==========================================
+ Coverage   86.31%   86.33%   +0.02%     
==========================================
  Files          43       43              
  Lines       11097    11119      +22     
  Branches     2532     2540       +8     
==========================================
+ Hits         9578     9600      +22     
  Misses       1519     1519              

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

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

Good finding!

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.

2 participants