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

Safari mobile audiocontext suspended fix #3969

Merged
merged 2 commits into from
Apr 6, 2021

Conversation

keianhzo
Copy link
Contributor

@keianhzo keianhzo commented Feb 26, 2021

Fixes #338 Hack for a Safari Mobile issue where unplugging/plugging headphones may leave the Audio context in a suspended state.

Based on this contribution from @vincentfretin: #338 (comment)

I couldn't test this on a physical device so iOS Safari testing is needed.

@keianhzo
Copy link
Contributor Author

@vincentfretin This PR is based on a contribution of yours in case you want to take a look, review or test.

Copy link
Contributor

@vincentfretin vincentfretin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need to check for browser here. You should always add the this.audioContext.onstatechange listener. It's harmless for other browsers.

src/systems/audio-system.js Outdated Show resolved Hide resolved
src/systems/audio-system.js Outdated Show resolved Hide resolved
@keianhzo keianhzo force-pushed the safari-mobile-audio-context-fix branch from 9910206 to 4ce1725 Compare March 1, 2021 17:37
@keianhzo
Copy link
Contributor Author

keianhzo commented Mar 1, 2021

@vincentfretin I have removed the iOS specific check. Still can't test so if anybody can please do and report.

@vincentfretin
Copy link
Contributor

Yes, better. I'll try to test that soon with #3944

Copy link
Contributor

@brianpeiris brianpeiris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one minor comment. LGTM otherwise.

src/systems/audio-system.js Outdated Show resolved Hide resolved
@vincentfretin
Copy link
Contributor

Test on iPad with iOS 14.3

Scenario:

  • insert jack on iPad, all is working fine
  • unplug jack

With master:
Results: mic volume indicator doesn't work, we don't hear others (the head doesn't scale)

With this PR:
Results:

  • mic volume indicator doesn't work, we don't hear others (the head doesn't scale)
  • "Audio has been resumed" log shows up (instead of "Audio has been suspended, click somewhere in the room to resume the audio.", weird)
  • I click and move in the scene
  • "Audio has been resumed" log shows up, the mic volume works, I can hear others.

The changes works but with the wrong first message, weird.

@vincentfretin
Copy link
Contributor

I don't know what going on with LogMessageType.audioSuspended why it doesn't show the correct message
If I replace
document.getElementById("avatar-rig").messageDispatch.log(LogMessageType.audioSuspended)
by for example
document.getElementById("avatar-rig").messageDispatch.log(LogMessageType.audioNormalizationNaN)
I see the "audioNormalization command needs a valid number parameter." message, so good.

@vincentfretin
Copy link
Contributor

vincentfretin commented Mar 20, 2021

If I add a new message in const logMessages = defineMessages in ChatSidebar.js
I actually see this new message for both LogMessageType.audioSuspended and LogMessageType.audioResumed
so it seems that document.getElementById("avatar-rig").messageDispatch.log doesn't take into account the argument and just use the last message in the logMessages list.

@vincentfretin
Copy link
Contributor

I quickly looked at src/message-dispatch.js I don't understand how the translation machinery works. So I'll let you look into this.

@keianhzo
Copy link
Contributor Author

keianhzo commented Apr 1, 2021

Thanks for the review @vincentfretin! Good catch, I was missing adding the messages to the LogMessageType object. I've added that and now it should be working fine.

@vincentfretin
Copy link
Contributor

Yes! This is good now.
IMG_0083
You can merge.

@keianhzo
Copy link
Contributor Author

keianhzo commented Apr 1, 2021

Great, I'll merge then. Thanks a lot for your support!

@keianhzo keianhzo merged commit bff3f0e into master Apr 6, 2021
@keianhzo keianhzo deleted the safari-mobile-audio-context-fix branch April 6, 2021 12:27
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.

iPhone: unplugging/replugging headphones causes audio problems
4 participants