Patch for abandonRulesCheck sudden quality drops

[robwalch]

This PR will...

Fix estimates and assumptions in _abandonRulesCheck that occur before first byte loaded.

Why is this Pull Request needed?

These changes are a subset #4825 intended improve stability until a later release focused on ABR and Low-Latency HLS improvements.

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]

Hi @Pri12890,

I'm aiming to release this smaller set of fixes to _abandonRulesCheck in the next patch. Pinging you since you originally brought these issues to my attention.

[shaharmor]

@robwalch this change, moving the bwEstimate from the stats.bwEstimate which was (optionally) passed by the loader, broke the API between hls.js & loaders.

Before this change, loaders could "control" the speed that Hls.js will use, and now they can't anymore.

[shaharmor]

In fact, I suggest that we add even additional stats.ttfbEstimate to get from the loaders and use that when possible as well.

Generally speaking, as loaders are more aware of network conditions and may do things like delay a request, they should have more control on BW estimations

[robwalch]

Hi @shaharmor,

I suggest reviewing and commenting on #4825.

Please provide more context on how you expect loader provided estimates to influence ABR decisions, especially the ones around abandonment affected by these changes.

[robwalch]

@shaharmor this change?

[shaharmor]

Yes. This was initially added (few years back) to allow for custom loaders to provide their own bandwidth estimation.
Now it is no longer possible.
I will also review the other PR you mentioned later, but this one was a breaking change

[robwalch]

Yes. This was initially added (few years back) to allow for custom loaders to provide their own bandwidth estimation.

Can you share the change that added it?

I will also review the other PR you mentioned later, but this one was a breaking change

#4825 and this change fixes issues with the abandon rules check. If the timeout occurs before any bytes are loaded and/or the loader stats do not provide a usable estimate we must use the running estimate.

[shaharmor]

This is the commit that added it: f4593a3
And this is the full PR: #587

The point of it was that custom loaders, especially p2p based ones, have a different way of doing requets which not always maps to a definitive (end-start) duration, hence the need for a more direct way of letting hls.js know what the bw was.

[robwalch]

We could amend #4825 to use stats.bwEstimate again for loadRate, but I could use some help identifying why bwEstimate is missing in the default XHR and fetch loaders. I see that loaders don't set this value and it's only set after frag buffered here overwriting any loader set value. This must be addressed first:

hls.js/src/controller/abr-controller.ts

Lines 265 to 270 in 3f9a5f4

	// Use the difference between parsing and request instead of buffering and request to compute fragLoadingProcessing;
	// rationale is that buffer appending only happens once media is attached. This can happen when config.startFragPrefetch
	// is used. If we used buffering in that case, our BW estimate sample will be very large.
	const processingMs = stats.parsing.end - stats.loading.start;
	this.bwEstimator.sample(processingMs, stats.loaded);
	stats.bwEstimate = this.bwEstimator.getEstimate();

I am open to contributions to the internal loaders and loader interface that add ttfb, but honestly it seems redundant with stats.loading.first - stats.loading.start.

For argument's sake, if you use a custom loader, don't you control all the stats including stats.loaded, stats.loading.start, and stats.loading.first (maybe that's not the case anymore; like with bwEstimate being set by a controller in an event handler)? Why not augment these measurements to produce the desired latency (ttfb) and bandwidth metrics? (This would allow you to impact the estimators sampled estimates directly instead of just this one place in abandon rules check). I know that is not the point since the a shortcut was already taken to expose bwe on stats. I am however concerned with what happens when these are inconsistent.

I am also concerned that only using the stats estimate for abandon rules is too niche and actually merits it's removal more than fixing this regression while ignoring the loader estimate for the EWMA average. Are you using another technique to modify the the bwEstimator estimate or did that also regress in v1.0?