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

Feature: Implement support for LL-HLS #EXT-X-PRELOAD-HINT part loading #6356

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

iamboorrito
Copy link
Collaborator

This PR will...

Add support for EXT-X-PRELOAD-HINT via state machine fragment-preloader.

Why is this Pull Request needed?

Reduce some latency for low latency mode by making preemptive requests.

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

Resolves issues:

To resolve #3988

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

@iamboorrito iamboorrito requested a review from robwalch April 15, 2024 21:04
@iamboorrito
Copy link
Collaborator Author

I started to clean up the fragment-preloader

robwalch
robwalch previously approved these changes May 10, 2024
Copy link
Collaborator

@robwalch robwalch left a comment

Choose a reason for hiding this comment

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

I'd be happy to mark this for milestone v1.6.0 if others using HLS.js for Low-Latency HLS would preview this branch and let us know what kind or results they get compared to latest. Branch preview URL: https://feature-preload-hint.hls-js-4zn.pages.dev/

I can see us using FragmentPreloader to tackle byte-range addressing (#5767 chunked-transfer interop) in the future. Thanks for laying the ground work!

@@ -349,6 +353,14 @@ export default class BaseStreamController
this.initPTS = [];
}

protected cachePreloadHint(details: LevelDetails): void {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be made private if not accessed by either sub-class (stream-controller or audio/subtitle).

@@ -284,6 +285,7 @@ export class Part extends BaseSegment {
baseurl: string,
index: number,
previous?: Part,
isPreload?: boolean,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't look like this argument is ever used. If we only set it on mergeFragData then we shouldn't add it to the constructor.

Comment on lines 382 to 383
frag.type === PlaylistLevelType.MAIN &&
!(part?.isPreload || frag.isPreload)
Copy link
Collaborator

@robwalch robwalch Jun 10, 2024

Choose a reason for hiding this comment

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

@iamboorrito what do you think about moving isPreload to stats? Or since Preload Hints only apply to parts, only on the Part subclass?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would there be any value to adding round trip time (header response) and using that in place of first body byte when dealing with blocking requests? The distinction we may want to make for stats is that this was a request where we expected the server to block until the response is available (could be on preload parts or playlists, but not on fragments via preload hints until #5767
is implemented).

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 will try moving it to stats again, I had a problem where the stats.wasBlockingLoad property I added was removed before it got checked

: this.load(frag, noop);
let loadPromise;
if (part !== undefined) {
// TODO: Use fetch loader to progressively load open-ended byterange requests
Copy link
Collaborator

Choose a reason for hiding this comment

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

cc #5767

@iamboorrito iamboorrito force-pushed the feature/preload-hint branch from e0955a8 to b3e230e Compare August 30, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

Implement support for LL-HLS #EXT-X-PRELOAD-HINT part loading
2 participants