Bugfix for subtitle loading after hls.stopload() was used

[netTrekfd]

This PR will...

• fix the subtitle controller to work after hls.stopLoad() and hls.startLoad() was used WebVTT subtitles broken when calling loadSource(..) inside MEDIA_ATTACHED-Event #3053
• fix an incorrect subtitle file loading order after initial seek in some very rare cases WebVTT subtitles broken when calling loadSource(..) inside MEDIA_ATTACHED-Event #3053

Why is this Pull Request needed?

The german tv-station ZDF uses the api hls.stopLoad()and hls.startLoad() in their player and needs to provide WebVTT for their clips. This also fixes the example code to work where hls.loadSource is called inside the MEDIA_ATTACHED event: https://github.com/video-dev/hls.js/blob/master/docs/API.md#third-step-load-a-manifest

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

Resolves issues:

• WebVTT subtitles broken when calling loadSource(..) inside MEDIA_ATTACHED-Event #3053

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]

Looks good. I have some concerns around stopping/starting loading on seeking/seeked which need to be addressed. I'm glad you caught that issue - we don't need a new ticket for it - but the solution needs a little more work.

[robwalch]

This is good. It makes sense and it is a step in the right direction. Here's the catch:

Aborting the request should only be done when seeking outside the range of fragCurrent. When seeking a bit within the range of this fragment, loading would just be forced to start over. The key to the logic in base-stream-controller's onMediaSeeking is that it deals with this issue for audio and video. The difference is base-stream-controller relies on BufferHelper.bufferInfo(media and here we need to use BufferHelper.bufferInfo(this._getBuffered().

[robwalch]

Suggested change

	let frag = this.fragCurrent;
	if (frag) {
	if (frag.loader) {
	frag.loader.abort();
	}
	this.fragmentTracker.removeFragment(frag);
	}
	this.fragCurrent = null;
	this.fragPrevious = null;
	if (this.state === State.FRAG_LOADING) {
	const fragCurrent = this.fragCurrent;
	const bufferInfo = BufferHelper.bufferedInfo(this._getBuffered(), media.currentTime, this.config.maxBufferHole);
	const tolerance = this.config.maxFragLookUpTolerance;
	const fragStartOffset = fragCurrent.start - tolerance;
	const fragEndOffset = fragCurrent.start + fragCurrent.duration + tolerance;
	// check if we seek position will be out of currently loaded frag range : if out cancel frag load, if in, don't do anything
	if (currentTime < fragStartOffset || currentTime > fragEndOffset) {
	if (fragCurrent.loader) {
	fragCurrent.loader.abort();
	}
	this.fragCurrent = null;
	this.fragPrevious = null;
	this.fragmentTracker.removeFragment(fragCurrent);
	// switch to IDLE state to load new fragment
	this.state = State.IDLE;
	}
	}
	}

It's quite a bit of duplicate code that could be addressed by changing _getBuffered to get mediaBuffer and using the base controller's onMediaSeeking logic, so maybe give that a try. It's certainly something I'd like to have in v1.

The suggested change above duplicates some code but that's OK. This will be addressed in v1.0.0 by exposing the buffered time ranged in the subtitle-stream-controller as a mediaBuffer property: https://github.com/video-dev/hls.js/pull/3060/files#diff-9f7fbd2b9b9e7d0824937236683db215R47-R295 which is dealt with the same for all stream controllers in base-stream-controllers onMediaSeeking handler.

[netTrekfd]

Thanks, i will take a look

[netTrekfd]

Hi @robwalch ,
i just added some changes here be2e12b.

Unfortunately the check state === stateState.FRAG_LOADING did not work for me. As soon as i check for that state i'm easily able to reproduce the issue by seeking slowly backwards. There is probably a loader running and needs to be cancelled even if the state does not reflect it.

I also did some tests with the buffering test BufferHelper.bufferInfo(this._getBuffered() but am not sure if i got your suggestion right. The root of this problem is that as soon as a loader is running, it continues to load in the order given by the last loaded segment. Because of that i think we need to cancel the fragCurrent.loader independend of the fact we are seeking to an already buffered position. The buffer will be respected inside the next tick.
As soon as i do not cancel the loader when seeking to an already buffered position, it will keep loading from the previous position which caused the problem.
I hope this is understandable. If not, i will try to explain it better ;)

Thanks
Florian

[robwalch]

LGTM - just suggested some clean up on the comments and then we can merge it.

[netTrekfd]

Thanks for reviewing, comments have been fixed

[robwalch]

Ran functional tests locally and all passed ✅

[robwalch]

woops! missed a unit test a6943fc