fix: calling detatchMedia() followed by attachMedia() causes audio to not play

[OrenMe]

This PR will...

Fix the detach-attach workflow when using a media with alternative audio tracks

Why is this Pull Request needed?

On re-attch the context of altAudio is not saved in buffer control so it doesn't know to create two source buffers.
Also, in context of stream controller it doesn't reset the altAudio flag.
I also noticed that stream controller is reloading the entire frags it has in fragmentTracker and it seems it needs to be reset on detach, otherwise there's a very long start delay where unnecessary buffering occurs. Unfortunately I'm not too sure what are the side effects.

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

I think there's a bigger issue here(and some smaller ones).
Big issue:
The logic seems problematic in general.
For stream with altAudio we first get the the main init segment in stream-controller and it will usually contain muxed video and audio.
The altAudio flag is undefined in this stage, which seems like a potential bug on its own, and because of this the audio track deletion doesn't happen.
The stream-controller will emit BUFFER_CODECS event that will be used in buffer-controller.
buffer-controller gets the event with payload of audio and video of main muxed content and as a side effect, cause it got altAudio flag from its manifest parsed event handler, it will create two source buffer now - it's a side effect cause it what we want but not how we want it.
from now on and BUFFER_CODECS event won't be processed by buffer-controller cause it has a flag that checks if sourceBuffer were created.
In this stage the audio-stream-controller will also get initSegement of the audio stream(not the main) and it will emit a BUFFER_CODECS event but it is already redundant as to what I wrote above.
Now, all of this seems a bit of a problem, cause I started with an assumption that first init segment is the main elementary one, but if it is the audio one then all goes down the drain cause first BUFFER_CODECS event that will arrive to buffer-controller will be audio and it will lock the creation of further sourceBuffer for video.
Before we decide if this fix is even acceptable and if my long description is even valid I will let it be reviewed.
It seems to me we need to start managing this better across controllers.
In high level:

• keep flag for altAudio from manifestParsed in streamController.
• manage the sourcebuffer creation per use case in buffer controller, by managing the source buffer object per audio and video and getting them separately from their respective controllers.

Resolves issues:

based on #2110 and fixes #2099.
@itsjamie I tried to cherry-pick your original changes but couldn't resolve the git issue of fetching from a fork.

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

[OrenMe]

So for my comment above about having unhandled cases for streams with alt audio:
There are two situations:

• main elementary stream has muxed audio and video
• main elementary stream has only video

For the later the use case is handled by

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

Lines 223 to 224 in 85c7d8f

	const pendingTracksCount = Object.keys(pendingTracks).length;
	if ((pendingTracksCount && !bufferCodecEventsExpected) || pendingTracksCount === 2) {

But for the first there's an issue cause the altAudio is only set to true after the first audio switch occurs, which is late.
The possible fix for this is to get the altAudio from the manifestParsed event already, which is early in the timeline and will always happen before the init segment parsing, and it is exactly what we do in the buffer-controller as well.
The only thing is - I see there's already a lot of logic in stream-controller around setting the altAudio so I don't know why it wasn't done in the manifestParsed originally already.

[OrenMe]

@johnBartos can we merge this? Does it need more review?

[robwalch]

Hi @OrenMe,

I will take a look!

[tchakabam]

Great track-down on the issue @OrenMe ! Looks all very good to me.

This seems like a perfect case for an additional unit test spec on buffer-controller?

[tchakabam]

I guess functionnally this works, but have a few questions on how it could be factored with the best possible streamlining of logic! see my comments

[robwalch]

Looks like a good fix. Two things:

1. Can you address the issue in buffer-controller with a private property of "total" codec events expected? This way we don't worry about two sources of truth for altAudio.
2. Not critical since subs are outside the scope of this issue, but audio-stream and subtitle-stream controllers should call fragmentTracker.removeAllFragments() and stopLoad() at the end of onMediaDetaching (maybe add this to the base class?)

[OrenMe]

There's a test failing now, but I don't understand it.
The subtitle-stream-controller has a tracks member that is initialised to empty array, and it is updated in onSubtitleTracksUpdated which gets a data with array of initialised tracks.
But there is a protection in onSubtitleTrackSwitch that checks if there's no tracks member while it seems impossible from the code.

hls.js/src/controller/subtitle-stream-controller.js

Lines 111 to 117 in fff952e

	onSubtitleTrackSwitch (data) {
	this.currentTrackId = data.id;
	if (!this.tracks || this.currentTrackId === -1) {
	this.clearInterval();
	return;
	}

There also a test checking this by setting tracks to null, which seems not necessary as tracks can't be null by the code.

hls.js/tests/unit/controller/subtitle-stream-controller.js

Lines 63 to 69 in fff952e

	it('should call clearInterval if no tracks present', function () {
	subtitleStreamController.tracks = null;
	hls.trigger(Event.SUBTITLE_TRACK_SWITCH, {
	id: 0
	});
	expect(subtitleStreamController.clearInterval).to.have.been.calledOnce;
	});

It seems we need to check if array length is 0 in the code and simulate empty array in the test. WDYT @robwalch ?

[itsjamie]

@OrenMe if you're going to be dropping the tracks inside the array of text tracks (resetting each array to be empty) can you just change the forEach line inside onMediaDetach to set this.tracks = null and accomplish the same thing?

Both cases you end up dropping the original array to the ground for GC, one works with the initial expectation of the test that sets the public property tracks to null is ok.

[itsjamie]

Just a minor fix for the null.forEach case and then I think this is good to merge.

[OrenMe]

@itsjamie the tracks and tracksBuffered are only populated in the onSubtitleTracksUpdated which is not called upon media re-attaching, so the tracksBuffered can't be set to null, and you need to go track by track and initilize it as we do in onSubtitleTracksUpdated.

hls.js/src/controller/subtitle-stream-controller.js

Lines 102 to 109 in fff952e

	onSubtitleTracksUpdated (data) {
	logger.log('subtitle tracks updated');
	this.tracksBuffered = [];
	this.tracks = data.subtitleTracks;
	this.tracks.forEach((track) => {
	this.tracksBuffered[track.id] = [];
	});
	}

and it doesn't seem correct that the tracks member can ever be null by this code, as we initialize it to empty array on start and then we add the tracks we got in onSubtitleTracksUpdated, so how do the if (!this.tracks ||... condition be correct ever and how does the test make sense? the correct behaviour is to check for 0 length tracks array.

[robwalch]

^ What @itsjamie said.

resolved