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.

Sign up for GitHub

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

Jump to bottom

fix: Replace Loader.getResponseHeader() with Loader.getCacheAge(). #3711

Merged
merged 2 commits into from Apr 5, 2021
Merged

fix: Replace Loader.getResponseHeader() with Loader.getCacheAge(). #3711

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d520008
fix: Replace Loader.getResponseHeader() with Loader.getCacheAge().
jwalton Apr 1, 2021
ca962e5
refactor: Remove loader.getResponseHeader().
jwalton Apr 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions api-extractor/report/hls.js.api.md
View file
Open in desktop
Expand Up @@ -1555,8 +1555,9 @@ export interface Loader<T extends LoaderContext> {
context: T;
// (undocumented)
destroy(): void;
// (undocumented)
getResponseHeader(name: string): string | null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

In earlier builds under feature/v1.0.0 where we were demoing LHLS (the Bartos spec), we tried using Modified-Date and some other headers to sync playlist requests with the server.

getCacheAge?: () => number;
// @deprecated
getResponseHeader?: (name: 'age') => string | null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think getResponseHeader should either remain broad for future use (no name: 'age' required param) or be removed. What do you think?

I know I mentioned my concern for backward compatibility in an earlier review, but I also like to see changes that remove more code than adding if possible. Sorry if this is contrarian, but I appreciate the input and value your opinion on these points.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd vote in favor of removing it. Let me give you a very concrete example:

Our product is basically a viewer for networked security cameras. Our playlists are months or years long. What our product typically does is, if you want to watch video from say 3pm, we'll grab a 24 hour m3u8 file (midnight-to-midnight) and hand that off to hls.js and then seek to the appropriate point in the playlist. (There may be much better ways to do this - I've only been on this team a short while, and I'm far from an hls.js expert. ;)

So there's this problem I've run into lately - sometimes a security camera goes offline for a little while, and we end up with gaps in the m3u8 file. We have a customer with a whole bunch of such gaps, ranging from 5-30 seconds, throughout every day. hls.js does not like this at all; when we hit one of these gaps sometimes playing video just hangs, sometimes we end up showing video from a very different time than we claim to be showing from - all kinds of awful.

I've only just started investigating this - I'm a long ways from being ready to point fingers at hls.js or raise a bug report, but one thing I've noticed is that in stream-controller, hls.js seems to be seeing that the remaining buffer is too short, so it tries to grab the next segment, only the next segment is after the gap and this doesn't seem to increase the available buffer, so hls.js tries to get the next segment, and it seems like hls.js gets stuck at this point and will go off into this spiral-of-doom trying to fetch the next segment until it gets to the end of the 24 hour playlist (which could take quite a while).

One idea I had as a "quick hack" to fix this is; since we're already caching our playlists, I could go peek at the playlist, see where the next gap is, and then give hls.js a "fake" manifest URL for an m3u8 that ends at the start of the gap, and then in the loader I can generate a synthetic m3u8 based on the contents of the cache and that URL. When hls.js runs out of video, I can just give it another synthetic playlist that starts right after the gap so it keeps going. But all of these playlists I'm effectively generating on the client - once I grab that first 24 hour playlist, I never talk to the server again - I'm just cutting up this m3u8 file into pieces and delivering each piece to hls.js.

So in the loader, in this case, how would I implement getResponseHeader()? I could cache the headers I got when I fetched the 24 hour playlist and give them to hls.js, except they're mostly going to be "wrong" - the "age" header is largely meaningless here, but if I deliver an "age: 0" header that I fetched from a server 30 minutes ago, what meaningful information is hls.js going to derive from that? I could synthesize the "age" header too - I can pass in an age of 30 minutes, because that's how long this has been sitting in my cache, but that implies I need to know which headers hls.js is going to ask for, which kind of means I have to treat every patch release of hls.js as a potentially breaking change, because I'd have to review each one to make sure it's not asking for any headers I didn't have.

On the other hand, optional functions in the interface like getCacheAge() or getLastModified() are easy for me to implement. If they're optional, I don't have to implement them. If they get added and they're not optional, then it's a breaking change, but... it is a breaking change - I have to handle this new case in my loader somehow.

(It'd also be really nice to have documentation about all the values in the stats object and what hls.js actually uses them for. Right now I'm kinda filling them in with something that looks sensible, but I worry about what hls.js is actually going to do with the made up values I give it. :P)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd vote in favor of removing it. Let me give you a very concrete example:
...
On the other hand, optional functions in the interface like getCacheAge() or getLastModified() are easy for me to implement. If they're optional, I don't have to implement them. If they get added and they're not optional, then it's a breaking change, but... it is a breaking change - I have to handle this new case in my loader somehow.

(It'd also be really nice to have documentation about all the values in the stats object and what hls.js actually uses them for. Right now I'm kinda filling them in with something that looks sensible, but I worry about what hls.js is actually going to do with the made up values I give it. :P)

Let's remove it then.

// (undocumented)
load(context: LoaderContext, config: LoaderConfiguration, callbacks: LoaderCallbacks<T>): void;
// (undocumented)
Expand Down
10 changes: 8 additions & 2 deletions src/loader/playlist-loader.ts
View file
Open in desktop
Expand Up @@ -684,8 +684,14 @@ class PlaylistLoader {
}

if (levelDetails.live) {
const ageHeader = loader.getResponseHeader('age');
levelDetails.ageHeader = ageHeader ? parseFloat(ageHeader) : 0;
if (loader.getCacheAge) {
levelDetails.ageHeader = loader.getCacheAge();
} else if (loader.getResponseHeader) {
const ageHeaderStr = loader.getResponseHeader('age');
levelDetails.ageHeader = ageHeaderStr ? parseFloat(ageHeaderStr) : 0;
} else {
levelDetails.ageHeader = 0;
}
}

switch (type) {
Expand Down
25 changes: 24 additions & 1 deletion src/types/loader.ts
View file
Open in desktop
Expand Up @@ -120,7 +120,30 @@ export interface Loader<T extends LoaderContext> {
config: LoaderConfiguration,
callbacks: LoaderCallbacks<T>
): void;
getResponseHeader(name: string): string | null;
/**
* `getCacheAge()` is called by hls.js to get the duration that a given object
* has been sitting in a cache proxy when playing live. If implemented,
* this should return a value in seconds.
*
* For HTTP based loaders, this should return the contents of the "age" header.
*
* @returns time object being lodaded
*/
getCacheAge?: () => number;
/**
* `getResponseHeader()` is called by hls.js to get the duration that a given
* object has been sitting in a proxy cache when playing live.
*
* For HTTP based loaders, this should just return the "age" header. For
* non-HTTP based loaders, if the retrieved object has been cached, this
* should return the time the object has been in a cache, as a string, possibly
* with a decimal point. Otherwise implementers should return "0" or null.
*
* Note that HLS.js will never call this for a header other than "age".
*
* @deprecated - Implement `getCacheAge()` instead.
*/
getResponseHeader?: (name: 'age') => string | null;
context: T;
loader: any;
stats: LoaderStats;
Expand Down
12 changes: 5 additions & 7 deletions src/utils/fetch-loader.ts
View file
Open in desktop
Expand Up @@ -160,15 +160,13 @@ class FetchLoader implements Loader<LoaderContext> {
});
}

getResponseHeader(name: string): string | null {
getCacheAge(): number {
let result: number = 0;
if (this.response) {
try {
return this.response.headers.get(name);
} catch (error) {
/* Could not get header */
}
const ageHeader = this.response.headers.get('age');
result = ageHeader ? parseFloat(ageHeader) : 0;
}
return null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

At some point in a future release, we may want to get a null value when 'age' cannot be read, is missing from headers, or simply not implemented in a custom loader. The logic that used null to backoff from even checking age was removed from playlist-loader recently for brevity, but I would still like it to be part of the interface for getCacheAge.

getCacheAge(): number | null

return result;
}

private loadProgressively(
Expand Down
13 changes: 9 additions & 4 deletions src/utils/xhr-loader.ts
View file
Open in desktop
Expand Up @@ -249,11 +249,16 @@ class XhrLoader implements Loader<LoaderContext> {
}
}

getResponseHeader(name: string): string | null {
if (this.loader && this.loader.getAllResponseHeaders().indexOf(name) >= 0) {
return this.loader.getResponseHeader(name);
getCacheAge(): number {
let result: number = 0;
if (
this.loader &&
this.loader.getAllResponseHeaders().indexOf('age') >= 0
) {
const ageHeader = this.loader.getResponseHeader('age');
result = ageHeader ? parseFloat(ageHeader) : 0;
}
return null;
return result;
}
}

Expand Down