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: set audio status on loaders when setting up media groups #1126

Merged
merged 3 commits into from
May 19, 2021

Conversation

gesinger
Copy link
Contributor

@gesinger gesinger commented May 5, 2021

Although the audio status is set in onTrackChanged for media groups, and
the function is called when the media groups are first set up, the track
is not always considered changed. This means that for demuxed audio, the
main loader may still think it should be using its own audio itself,
leading to issues when crossing discontinuities (i.e., the main loader
will cross the discontinuity before waiting for the audio loader to be
ready, leading to audio timestamps that aren't correct).

This change ensures that the audio status is set on setup, regardless of
whether the track is considered changed. Subsequent changes are handled
in onTrackChanged.

Requirements Checklist

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

Although the audio status is set in onTrackChanged for media groups, and
the function is called when the media groups are first set up, the track
is not always considered changed. This means that for demuxed audio, the
main loader may still think it should be using its own audio itself,
leading to issues when crossing discontinuities (i.e., the main loader
will cross the discontinuity before waiting for the audio loader to be
ready, leading to audio timestamps that aren't correct).

This change ensures that the audio status is set on setup, regardless of
whether the track is considered changed. Subsequent changes are handled
in onTrackChanged.
@codecov
Copy link

codecov bot commented May 5, 2021

Codecov Report

Merging #1126 (073deaf) into main (a4af004) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1126      +/-   ##
==========================================
+ Coverage   86.20%   86.22%   +0.01%     
==========================================
  Files          39       39              
  Lines        9284     9289       +5     
  Branches     2126     2127       +1     
==========================================
+ Hits         8003     8009       +6     
+ Misses       1281     1280       -1     
Impacted Files Coverage Δ
src/media-groups.js 98.97% <100.00%> (+0.01%) ⬆️
src/source-updater.js 94.51% <0.00%> (+0.32%) ⬆️

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 a4af004...073deaf. Read the comment docs.

@brandonocasey brandonocasey merged commit a44f984 into main May 19, 2021
@brandonocasey brandonocasey deleted the fix/has-audio-demuxed branch May 19, 2021 16:39
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