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

feat(HLS): Lazy-load HLS media playlists #4511

Merged
merged 7 commits into from
Oct 9, 2022

Conversation

theodab
Copy link
Collaborator

@theodab theodab commented Sep 24, 2022

This changes the HLS parser so that the media playlists are only downloaded when the createSegmentIndex function for the associated stream is called.
Because there is some important information about HLS streams that is stored inside the media playlist, this also changes the player to call createSegmentIndex on the initial variant earlier in the load process, to make sure that information is available in time.
As of this change, we will now require HLS streams to be aligned (see #4308) for livestreams. VOD content can still
be unaligned.

Closes #1936

@theodab theodab added type: enhancement New feature or request component: HLS The issue involves Apple's HLS manifest format type: performance A performance issue priority: P1 Big impact or workaround impractical; resolve before feature release labels Sep 24, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 24, 2022

Incremental code coverage: 99.12%

@avelad avelad added this to the v4.3 milestone Sep 24, 2022
@avelad
Copy link
Collaborator

avelad commented Sep 30, 2022

@theodab it seems that there are some conflict with the main branch, can you rebase? Thanks!

This changes the HLS parser so that the media playlists are only
downloaded when the createSegmentIndex function for the associated
stream is called.
Because there is some important information about HLS streams that
is stored inside the media playlist, this also changes the player
to call createSegmentIndex on the initial variant earlier in the
load process, to make sure that information is available in time.

Closes shaka-project#1936
This reverts the HLS parser to using per-stream mediaSequenceToStartTime
values for VOD, which will allow us to play unaligned HLS streams on
that sort of content.

Issue shaka-project#4308
@theodab
Copy link
Collaborator Author

theodab commented Oct 3, 2022

@theodab it seems that there are some conflict with the main branch, can you rebase? Thanks!

Ok, I've merged with main. Just a small merge conflict in the tests, luckily, it wasn't anything of substance.

@avelad avelad self-requested a review October 3, 2022 16:23
avelad
avelad previously approved these changes Oct 3, 2022
Copy link
Collaborator

@avelad avelad left a comment

Choose a reason for hiding this comment

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

I have tested the change and it works fine.

The only thing that would still make sense for VOD, once a media playlist has been requested, do not request it again, right now if you load a stream and the ABR goes through all the qualities, if it goes through one that has already happened before asking for the playlist again, maybe this information could be stored in memory (maybe a config? for slow devices), what do you think?

@joeyparrish
Copy link
Member

The only thing that would still make sense for VOD, once a media playlist has been requested, do not request it again, right now if you load a stream and the ABR goes through all the qualities, if it goes through one that has already happened before asking for the playlist again, maybe this information could be stored in memory (maybe a config? for slow devices), what do you think?

It's a trade-off between memory and latency. Perhaps a config to keep VOD streams in memory might make sense for some applications, but I tend to think it's best by default to free up the memory. Shaka could be much better about memory usage in general.

Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

Only quick comment while I'm still reading hls_parser.js.

lib/player.js Outdated Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
lib/hls/hls_parser.js Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
lib/hls/hls_parser.js Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
lib/media/presentation_timeline.js Show resolved Hide resolved
lib/player.js Outdated Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
}
}
if (createSegmentIndexPromises.length > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This is fine, but JFYI, await Promise.all([]) (empty array) is equivalent to await Promise.resolve(). It will schedule a microtask and return control back to the interpreter loop, but it won't hurt anything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was worried that it might change the results of some test to have another await in the middle, so I put that in preemptively.
It's almost definitely unnecessary, I agree.

Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

Nicely done!

@theodab theodab merged commit b2f279d into shaka-project:main Oct 9, 2022
@theodab theodab deleted the lazyLoadBranch branch October 9, 2022 00:00
@hiren3897
Copy link

hello, @theodab the workaround on lazy loading feature affect playRangeEnd settings.
the player is not able to set the presentation timeline according to the playRangeEnd

I have created the issue here with my level of debugging can you please help me with that?
#4787

@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: HLS The issue involves Apple's HLS manifest format priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: enhancement New feature or request type: performance A performance issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lazy-load HLS media playlists on adaptation
4 participants