Update fragPrevious when attempting to load an already-buffered fragment

[erkreutzer]

This PR will...

Fix #5012

Why is this Pull Request needed?

When we use maxFragLookUpTolerance: 0 and the range that's currently buffering catches up to a previously buffered range the stream-controller may get stuck in a loop trying to load a previously loaded fragment.

This adjusts the logic in the stream-controller so that if an attempt is made to load a fragment that was already successfully downloaded, the fragPrevious field is updated as if the fragment had just been downloaded.

In the case of small maxFragLookUpTolerance this will allow the logic in getFragmentAtPosition to move past the already downloaded fragment.

hls.js/src/controller/base-stream-controller.ts

Lines 1039 to 1057 in 98d4ab3

	if (fragPrevious && frag.sn === fragPrevious.sn && !loadingParts) {
	// Force the next fragment to load if the previous one was already selected. This can occasionally happen with
	// non-uniform fragment durations
	const sameLevel = fragPrevious && frag.level === fragPrevious.level;
	if (sameLevel) {
	const nextFrag = fragments[curSNIdx + 1];
	if (
	frag.sn < endSN &&
	this.fragmentTracker.getState(nextFrag) !== FragmentState.OK
	) {
	this.log(
	`SN ${frag.sn} just loaded, load next one: ${nextFrag.sn}`
	);
	frag = nextFrag;
	} else {
	frag = null;
	}
	}
	}

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

I've run the unit tests and functional tests and they all pass, but this is my first time looking at the stream-controller logic so there very well may be a situation I'm overlooking.

Resolves issues:

#5012

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]

Makes sense. Wondering if this can happen in getFragmentAtPosition so that we pick the right fragment on the first pass rather than (assuming) on the next tick.

[robwalch]

That would also move this fix to base-stream-controller so that it addresses the same lookup issue in audio playlists.

[robwalch]

If you add this to getFragmentAtPosition in base-stream-controller then it happens in place and doesn't modify the fragPrevious property (in this case I just did let { fragPrevious } = this;. I prefer that since other parts of the code we are not thinking of may depend on this.fragPrevious not being updated in this way.

   if (frag) {
      const curSNIdx = frag.sn - levelDetails.startSN;
      // Move fragPrevious forward to support forcing the next fragment to load
      // when the buffer catches up to a previously buffered range.
      if (this.fragmentTracker.getState(frag) === FragmentState.OK) {
        fragPrevious = frag;
      }
      if (fragPrevious && frag.sn === fragPrevious.sn && !loadingParts) {
        // Force the next fragment to load if the previous one was already selected. This can occasionally happen with
        // non-uniform fragment durations

[erkreutzer]

Thanks for the feedback! That makes a ton of sense and seemingly reduces the risk of my change (one of my worries of the original change not having a ton of exposure to the codebase.)

I've added the changes you proposed. Let me know if there's anything else I can do to help get this change in place.

[robwalch]

Thank you! Great catch and fix.

[robwalch]

If you add this to getFragmentAtPosition in base-stream-controller then it happens in place and doesn't modify the fragPrevious property (in this case I just did let { fragPrevious } = this;. I prefer that since other parts of the code we are not thinking of may depend on this.fragPrevious not being updated in this way.

   if (frag) {
      const curSNIdx = frag.sn - levelDetails.startSN;
      // Move fragPrevious forward to support forcing the next fragment to load
      // when the buffer catches up to a previously buffered range.
      if (this.fragmentTracker.getState(frag) === FragmentState.OK) {
        fragPrevious = frag;
      }
      if (fragPrevious && frag.sn === fragPrevious.sn && !loadingParts) {
        // Force the next fragment to load if the previous one was already selected. This can occasionally happen with
        // non-uniform fragment durations