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

Add AudioContext workaround #10700

Open
wants to merge 6 commits into
base: master
from
Open

Conversation

@fluffrabbit
Copy link

fluffrabbit commented Mar 17, 2020

First attempt. Fixes #6511

fluffrabbit
@welcome

This comment has been minimized.

Copy link

welcome bot commented Mar 17, 2020

Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS.

Copy link
Member

kripken left a comment

Good idea about adding these as separate files!

Please add some docs that mention the new files, and explain the issue, in site/source/docs/porting/Audio.rst. That page seems to currently focus on OpenAL so perhaps the intro section to it could be reworded to be more general (if you don't have time to improve those docs generally I understand of course, but if you can that would be great!)

It would be good to have some testing of this. We have the "interactive" test suite which is relevant here, see tests/test_interactive.py. Some of the audio tests might have '--shell-file', path_from_root('src', 'shell_audio.html') added to the args so that the new shell file is used. You can run those with e.g. ./tests/runner.py interactive.test_openal_buffers. I verified that one fails currently so hopefully it will pass with the new shell.

src/shell_audio.html Outdated Show resolved Hide resolved
src/shell_audio.html Outdated Show resolved Hide resolved
@kripken

This comment has been minimized.

Copy link
Member

kripken commented Mar 17, 2020

@Beuc

This comment has been minimized.

Copy link
Contributor

Beuc commented Mar 17, 2020

I was asked to move my comment from the issue to the PR:

Nice. This looks like a small snippet of javascript so I would just add it to shell.html (not "minimal", nor another "audio" variant).

More generally it's about ease of use for Emscripten users, and I believe we have other such work-arounds consolidated in shell.html.

fluffrabbit
@fluffrabbit

This comment has been minimized.

Copy link
Author

fluffrabbit commented Mar 17, 2020

More generally it's about ease of use for Emscripten users, and I believe we have other such work-arounds consolidated in shell.html.

I agree, and I think it should be in shell_minimal.html as well. I don't know how this is going to work with multiple files, but it probably will somehow.

fluffrabbit added 4 commits Mar 18, 2020
fluffrabbit
fluffrabbit
fluffrabbit
fluffrabbit
@fluffrabbit

This comment has been minimized.

Copy link
Author

fluffrabbit commented Mar 18, 2020

I don't know how to improve the audio documentation besides adding that little note about the workaround. Anyways, check it out @kripken

// Work around browser autoplay policy
// https://github.com/emscripten-core/emscripten/issues/6511
// An array of all contexts to resume on the page
const audioContexList = [];

This comment has been minimized.

Copy link
@juj

juj Mar 18, 2020

Collaborator

Typo audioContexList -> audioContextList

This comment has been minimized.

Copy link
@fluffrabbit

fluffrabbit Mar 18, 2020

Author

raysan5 came up with the name. I think it's fine because it avoids potential future name collisions and can still be abbreviated as ctx.

@juj

This comment has been minimized.

Copy link
Collaborator

juj commented Mar 18, 2020

LGTM. The only thing that is a bit unfortunate is that the shells are now duplicated more. (I wonder if a --post-js src/shell_audio.js approach might be able to achieve the same, for orthonogality? WFM either way)

Have to say I really wish that Google and other browsers would have followed the standards process here. We have been struggling with audio unlocks as well, and it creates quite a lot of annoying churn to deal with minute Chrome vs Firefox vs Safari behavioral differences on this feature.

@fluffrabbit

This comment has been minimized.

Copy link
Author

fluffrabbit commented Mar 18, 2020

@juj You're saying some very interesting things. I know very little about Emscripten internals, but macro substitution does sound like a solution, however it is implemented.

Google puts business before standards, and other browsers follow the leader (for the most part). You know how it is. The situation is a lot better than it was 10 years ago. If you have a solution that deals with behavioral differences between those major browsers, that is an area that I have not tested with this patch.

@floooh

This comment has been minimized.

Copy link
Contributor

floooh commented Mar 19, 2020

I use the same workaround in my own code (but have it wrapped via embedded EM_JS() in C code instead of putting it into the shell.html). It's good that it's optional though, because a "proper" application would want an "unmute" button somewhere overlayed.

I'd probably prefer if the fix isn't automatically included into the standard shell.html files, for instance in my case this would clash with my own solution.

If emscripten had an audio-equivalent to the emscripten_webgl_* helper function the audio context resume functionality should probably better be handled there.

FWIW this is how I'm doing it in my own code (as part of the general WebAudio context setup happening in an embedded EM_JS() Javascript function:

https://github.com/floooh/sokol/blob/82707c2dd90b4086341c7e4171909cf0bb2753bb/sokol_audio.h#L1333-L1343

@floooh

This comment has been minimized.

Copy link
Contributor

floooh commented Mar 19, 2020

@fluffrabbit one potential simplification to your solution would be to use { once:true } in the addEventListener instead of removing the handler in the callback function like I'm doing here:

https://github.com/floooh/sokol/blob/82707c2dd90b4086341c7e4171909cf0bb2753bb/sokol_audio.h#L1341-L1343

@fluffrabbit

This comment has been minimized.

Copy link
Author

fluffrabbit commented Mar 19, 2020

@floooh I gave you commit access to my fork of emscripten. I haven't used JavaScript in a few years so I don't know best practices.

You're welcome to update this PR however you like. God knows I don't know what I'm doing with this.

@kripken

This comment has been minimized.

Copy link
Member

kripken commented Mar 23, 2020

If emscripten had an audio-equivalent to the emscripten_webgl_* helper function the audio context resume functionality should probably better be handled there.

That makes me think, perhaps this could go in the HTML C API. That has event handlers for keypresses and mouse clicks, and this workaround is tied closely to that, so it feels like it might be the right place.

More specifically, maybe when a user sets a keyboard or mouse event handler they could set a flag to handle auto contexts. Or, if that doesn't fit, maybe it could be a new API.

@floooh

This comment has been minimized.

Copy link
Contributor

floooh commented Mar 24, 2020

More specifically, maybe when a user sets a keyboard or mouse event handler they could set a flag to handle auto contexts. Or, if that doesn't fit, maybe it could be a new API.

I think (if this route is taken) it should be a new API, mixing WebAudio stuff into the "generic" input handlers seems a bit random.

@fluffrabbit

This comment has been minimized.

Copy link
Author

fluffrabbit commented Mar 24, 2020

@kripken

This comment has been minimized.

Copy link
Member

kripken commented Mar 26, 2020

@floooh Makes sense, yeah, maybe this should be a new API then.

@fluffrabbit What do you mean by unclean?

To be clear, what I am thinking of is a C API like this:

emscripten_add_audio_context_resuming();

A user that wants this feature would call that, and then the code would only be included for those users.

@fluffrabbit

This comment has been minimized.

Copy link
Author

fluffrabbit commented Mar 26, 2020

@kripken I want to give this library a spin in Emscripten. Would this workaround API allow that to work unimpeded? It lists Emscripten support, but I wonder about how an audio context resume workaround would work at the emulated OpenAL level or whatever the libraries use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.