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

load audio playlist on MANIFEST_PARSED #2340

Merged

Conversation

mad-gooze
Copy link
Contributor

@mad-gooze mad-gooze commented Aug 17, 2019

This PR will...

start loading level playlist on MANIFEST_PARSED event so audio will be loaded earlier

Why is this Pull Request needed?

Now hls.js starts fetching audio playlist after loading level playlist, so video start on streams with separate audio track is delayed
image
With the proposed change audio playlist will be loaded after MANIFEST_PARSED
image
With the proposed change video will start faster.
This can be tested here.

Are there any points in the code the reviewer needs to double check?

Resolves issues:

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

Copy link
Collaborator

@itsjamie itsjamie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me at a high-level approach / goal. Just a few minor details from dev to be fixed.

tests/unit/controller/audio-track-controller.js Outdated Show resolved Hide resolved
tests/unit/controller/audio-track-controller.js Outdated Show resolved Hide resolved
itsjamie
itsjamie previously approved these changes Sep 9, 2019
@itsjamie
Copy link
Collaborator

itsjamie commented Sep 9, 2019

Changes I requested were made for the unit tests.

Not waiting until level playlist is loaded to load alternative audio groups (not in the video track) makes all kinds of sense to me but would like another opinion.

src/controller/audio-track-controller.js Show resolved Hide resolved
@robwalch robwalch dismissed their stale review October 8, 2019 21:05

MANIFEST_PARSED triggers LEVEL_LOADED

@stale
Copy link

stale bot commented Dec 7, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale label Dec 7, 2019
@OrenMe
Copy link
Collaborator

OrenMe commented Dec 8, 2019

@mad-gooze will you have time to look into the comments @robwalch raised? It seems like a nice performance boost we should add.

@stale stale bot removed the Stale label Dec 8, 2019
@robwalch robwalch added this to the 0.13.2 milestone Jan 28, 2020
@robwalch robwalch added this to Top priorities in Release Planning and Backlog Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Release Planning and Backlog
  
Top priorities
Development

Successfully merging this pull request may close these issues.

None yet

4 participants