-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: calling detatchMedia() followed by attachMedia() causes audio to not play #2303
Conversation
… not play 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 unncessery buffering occurs. based on #2110 and fixes #2099.
save altAudio flag in stream controer from the manifest parsed
So for my comment above about having unhandled cases for streams with alt audio:
For the later the use case is handled by hls.js/src/controller/buffer-controller.ts Lines 223 to 224 in 85c7d8f
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. |
@johnBartos can we merge this? Does it need more review? |
Hi @OrenMe, I will take a look! |
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good fix. Two things:
- 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
. - Not critical since subs are outside the scope of this issue, but audio-stream and subtitle-stream controllers should call
fragmentTracker.removeAllFragments()
andstopLoad()
at the end ofonMediaDetaching
(maybe add this to the base class?)
There's a test failing now, but I don't understand it. hls.js/src/controller/subtitle-stream-controller.js Lines 111 to 117 in fff952e
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 seems we need to check if array length is 0 in the code and simulate empty array in the test. WDYT @robwalch ? |
@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 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor fix for the null.forEach
case and then I think this is good to merge.
@itsjamie the hls.js/src/controller/subtitle-stream-controller.js Lines 102 to 109 in fff952e
and it doesn't seem correct that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ What @itsjamie said.
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 isundefined
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 emitBUFFER_CODECS
event that will be used inbuffer-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 bybuffer-controller
cause it has a flag that checks ifsourceBuffer
were created.In this stage the
audio-stream-controller
will also get initSegement of the audio stream(not the main) and it will emit aBUFFER_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 tobuffer-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:
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