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

reexecute enableChromeAEC if network changed #3604

Merged
merged 3 commits into from
Feb 26, 2021

Conversation

vincentfretin
Copy link
Contributor

@vincentfretin vincentfretin commented Dec 19, 2020

When the network change from wired to Wi-Fi, the RTCPeerConnection loopback stays disconnected, recreate it after 10s if it didn't get back to connected.

Capture d’écran de 2020-12-19 14-40-31

Note : this is only a partial fix. There is still an issue with the websocket and Send Transport that doesn't reconnect. It just stays disconnected on Chrome.

@keianhzo keianhzo self-assigned this Jan 5, 2021
@keianhzo keianhzo self-requested a review January 5, 2021 17:26
@vincentfretin
Copy link
Contributor Author

Even with this fix in my app, a user reported to me that on Chrome macOS he saw the other participant avatar talking (so the audio-analyzer is working) but couldn't hear anything. I don't have much details about it... I'm wondering if the Audio object created in enableChromeAEC can be paused by the browser (when taking a Skype or Teams call like it's the case for Safari iOS) or any other event...
I added the following code in my app, but I don't know if it helps or not, I don't have sentry integrated yet.

  const audioEl = new Audio();
  audioEl.setAttribute("autoplay", "autoplay");
  audioEl.setAttribute("playsinline", "playsinline");
  window.app.audioEl = audioEl;
  // https://developer.mozilla.org/en-US/docs/Web/Guide/Audio_and_video_delivery/Cross-browser_audio_basics
  audioEl.addEventListener("pause", (e) => {
    console.error("enableChromeAEC: audioEl pause", e);
    performDelayedReconnect(gainNode);
  });
  audioEl.addEventListener("ended", (e) => {
    console.error("enableChromeAEC: audioEl ended", e);
    performDelayedReconnect(gainNode);
  });
  audioEl.addEventListener("suspend", (e) => {
    console.error("enableChromeAEC: audioEl suspend", e);
    performDelayedReconnect(gainNode);
  });
  audioEl.addEventListener("abort", (e) => {
    console.error("enableChromeAEC: audioEl abort", e);
    performDelayedReconnect(gainNode);
  });
  audioEl.addEventListener("error", (e) => {
    console.error("enableChromeAEC: audioEl error", e);
    performDelayedReconnect(gainNode);
  });
  audioEl.addEventListener("emptied", (e) => {
    console.error("enableChromeAEC: audioEl emptied", e);
    performDelayedReconnect(gainNode);
  });
  audioEl.addEventListener("stalled", (e) => {
    console.error("enableChromeAEC: audioEl stalled", e);
    performDelayedReconnect(gainNode);
  });

@vincentfretin
Copy link
Contributor Author

Please forget my last comment. The issue I was having is actually the enableChromeAEC function that is sometime not called because of my alreadyExecuting boolean. :D Sometimes the setTimeout 0 to execute the callback on next tick is not enough, the audioContext is not in the running state yet.

@robertlong
Copy link
Contributor

@keianhzo I think we're ready to work towards merging this now pending a review. Thanks again @vincentfretin!

@keianhzo
Copy link
Contributor

Looks great, thanks a lot for this @vincentfretin! Now that we are doing retries for other RTC relates services it's great to have this aligned. I've just pointed out a couple of things. Let me know what your thoughts are.

@keianhzo keianhzo merged commit e83d75c into Hubs-Foundation:master Feb 26, 2021
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