Skip to content
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

Clear audio tick interval when we stopLoad. #2110

Closed
wants to merge 2 commits into from
Closed

Clear audio tick interval when we stopLoad. #2110

wants to merge 2 commits into from

Conversation

itsjamie
Copy link
Collaborator

@itsjamie itsjamie commented Jan 31, 2019

This PR will...

Clears Tick interval in stop load for audio-stream-controller.

Why is this Pull Request needed?

Looking at #2099 root cause, I noticed that the audio-stream-controller does not clear its tick interval when stopLoad is called, causing it to force loop in the stopped state while the media is detached.

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

Resolves issues:

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

@itsjamie
Copy link
Collaborator Author

This doesn't solve #2099 yet. Looks like there is more to it, but this is as far as I could debug tonight.

While debugging, I was able to see that the audio track will sometimes load, and the video will stall out, and Chrome will switch to a audio only control set, and I will hear audio. But as soon as a video frag loads, I would lose audio, and gain a picture.

@itsjamie
Copy link
Collaborator Author

Closing in favor of #2123.

@itsjamie itsjamie closed this Feb 12, 2019
@itsjamie itsjamie deleted the bugfix-2099 branch February 12, 2019 21:23
@OrenMe
Copy link
Collaborator

OrenMe commented Jul 3, 2019

Hi @itsjamie as #2123 is closed and postponed 1.0.0 can we re-add this and release in next minor release? This is critical for use case where only one video element can be used but it needs to be shared with other playback sources during playback(think ads)

@johnBartos
Copy link
Collaborator

Agree with @OrenMe, audio should not reload when stopped. Let's re-open and decide if it's safe enough to go into the next patch, or should be in the minor

@OrenMe
Copy link
Collaborator

OrenMe commented Jul 3, 2019

@itsjamie do you have any lead on the issue that I can follow?

@itsjamie
Copy link
Collaborator Author

itsjamie commented Jul 3, 2019

Unfortunately, not really @OrenMe. The few things I did identify as definites were part of this PR.

One definitely was that on reattachment to a track with alternate audio, only a single buffer codec event was expected, whereas on first load, two were (correctly).

Additionally, the interval kept running in the audio controller, which is what the interval getting cleared was fixing.

I thought that the wrong amount of buffer codec events was the core issue, but when I tested it still didn't work.

OrenMe added a commit that referenced this pull request Jul 15, 2019
… 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants