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

[jwalton]

This PR will replace Loader.getResponseHeader() with Loader.getCacheAge(), in a backward-compatibly fashion.

Why is this Pull Request needed?

Loader has a function called getResponseHeader() (added in 1.0.0), which returns the value of a response header. It's used to find the "age" header to see if a request has been in a proxy for a while, when playing live.

But, this API is way too broad - first it assumes the underlying transport is HTTP based. If the underlying transport is something else, like WebRTC or websocket, what should getResponseHeader() return? Or, in my case, I have a caching loader which caches playlists locally, and my HTTP headers are long gone (and if I wanted to save them explicitly for the loader, it wouldn't even be useful, because the "age" header would be out of date by the time my loader fetches them).

You could argue it should just return undefined for WebRTC and websocket, and maybe in my local cache case I should construct an "age" based on how long I've cached it, but making such an argument implies that getResponseHeader() will only ever be used to fetch the "age" header. But there's nothing about this API which restricts hls.js from calling it and asking for other headers. As an implementer, you're left trying to work out what possible HTTP headers hls.js might want in this or future versions, and trying to reconstruct them from whatever data you have available to you from your transport.

This PR replaces getResponseHeader() with getCacheAge(), which is a much nicer API to try to implement as someone writing a non-HTTP loader. This PR leaves getResponseHeader() in place as a @deprecated API, so that existing implmentations won't break with this change, and further specifies it as getResponseHeader(name: 'age'), which guarantees that hls.js will never call it with something other than "age", but still allows existing implementations with (name: string) to continue to work.

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

No.

Resolves issues:

None.

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

[robwalch]

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.

[robwalch]

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

[robwalch]

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.

[jwalton]

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)

[robwalch]

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.