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: Replace Loader.getResponseHeader() with Loader.getCacheAge(). #3711

Merged
merged 2 commits into from Apr 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 1 addition & 2 deletions api-extractor/report/hls.js.api.md
Expand Up @@ -1555,8 +1555,7 @@ 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 | null;
// (undocumented)
load(context: LoaderContext, config: LoaderConfiguration, callbacks: LoaderCallbacks<T>): void;
// (undocumented)
Expand Down
8 changes: 6 additions & 2 deletions src/loader/playlist-loader.ts
Expand Up @@ -684,8 +684,12 @@ class PlaylistLoader {
}

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

switch (type) {
Expand Down
11 changes: 10 additions & 1 deletion src/types/loader.ts
Expand Up @@ -120,7 +120,16 @@ 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 | null;
context: T;
loader: any;
stats: LoaderStats;
Expand Down
12 changes: 5 additions & 7 deletions src/utils/fetch-loader.ts
Expand Up @@ -160,15 +160,13 @@ class FetchLoader implements Loader<LoaderContext> {
});
}

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

Expand Down
3 changes: 0 additions & 3 deletions tests/unit/loader/fragment-loader.ts
Expand Up @@ -36,9 +36,6 @@ class MockXhr implements Loader<LoaderContext> {

abort() {}
destroy(): void {}
getResponseHeader(name: string): string | null {
return null;
}
}

describe('FragmentLoader tests', function () {
Expand Down