Significant overhaul of the OpenAL implementation

[jpernst]

This PR represents substantial work on improving the OpenAL implementation by @yoanlcq and myself.

First and foremost, every entry point defined by the OpenAL 1.1 specification is implemented, which is important for bindings implementations that expose the full API.

Second, error checking is now quite rigorous and conformant with the spec, which should help prevent incompatibilities with native implementations. In general, the semantics of the API conform to the spec whenever possible, including seemingly useless cases such as 0-length buffers.

Third, missing functionalities such as Doppler shift and listener-relative sources are restored.

Finally, several common extensions are implemented, including:

• "ALC_SOFT_pause_device"
• "ALC_SOFT_HRTF"
• "AL_EXT_float32"
• "AL_SOFT_loop_points"
• "AL_SOFT_source_length"
• "AL_EXT_source_distance_model"
• "AL_SOFT_source_spatialize"

I have plans to implement at least some of the EFX extension, but that one is massive, so I'm leaving it for a later PR.

Several new tests have been added to help ensure the implementation is correct, but OpenAL has a fairly large surface area, so exposing it to a few non-trivial apps to really shake it out first is probably a good idea as well.

Very little of the original implementation remains, so I understand completely if this is a controversial or time consuming PR to examine, but I hope it's found useful. I've tried to "do as the GL bindings do" for the GetProcAddress functions and such, but it's entirely possible that it will need adjustment.

[kripken]

Perhaps this should be built with -O2 or -Os?

[jpernst]

I wasn't sure what the best choice was, so I just made it exactly the same as the create_gl version. The functions contained inside are generally only called a few times at the beginning of a program and never again, so perhaps -Os would be best.

[juj]

While this has been true for OpenAL, we actually want to move away from this model and use the same linker directive that native systems use to link to OpenAL. That is, use -lopenal to link to OpenAL which I believe Linux systems do(?) The PR #4665 made this directive available, with the intent of making that mandatory in the future. It would be good to document that possibility here as the "desired" mechanism.

[yoanlcq]

Will fix! Also yes, -lopenal is the Linux way.

[juj]

This would be good to be clarified to refer to synchronously blocking on long running calls to be more clear. (accessing OpenAL calls in a loop is fine, one just can't block for long periods)

[juj]

This second point would probably be good to be removed altogether. Referencing poor garbage collection implementations in browsers can read a bit FUD-mongering. In general we do not find OpenAL garbage collection to be a big performance problem, but alBufferData is the problematic one. Referring to the usual "do resource loading in cold paths" optimization would be ok however - that is also advice that is sound for native code.

[juj]

This browser specific note is a candidate to get out of date on the long run, and does not seem to carry particularly important value, so would be cleaner to avoid this? (or perhaps remark above asking for the user's permission, and on some browsers, the capture device to choose).

[juj]

Change must to should?

[juj]

Does "type conversion passes" need double quotes?

[yoanlcq]

I had put them because it felt like this was a term I just pulled of nowhere and needed to show that I was aware of it. It's hard to put into words, but not important anymore since I'll change the wording to be more to the point, like :

Thus, ``AL_FORMAT_MONO_FLOAT32`` and ``AL_FORMAT_STEREO_FLOAT32`` 
are the only formats which do not require converting acquired samples from their
initial type to another.

[juj]

Good note!

[juj]

Let's remove all these four notes, they don't seem to carry particular value.

[juj]

Change is unpredictable to something like is currently dictated by the browser and hardware, instead of user code, since unpredictable might read "random", but the behavior is very specific and deterministic on a specific browser and OS. See also #5349, which is a new update to Web Audio spec, though not sure if it will apply to microphone captures.

[yoanlcq]

Agreed for the rewording.
Also, interesting! Looks like something worth adding to the Improving section at the end.

[juj]

Does this reference to asking for the actual sampling rate of an already created context, or the default sampling rate of created contexts? Adding functions for either as needed sounds good.

[yoanlcq]

That sample rate [...] refers to the sampling rate of already created contexts. However I'm a bit confused by the question because it's not clear what we mean by "context" (which I assume is AudioContext).
The thing is, it just so happens that the sample rate is the same across all AudioContext on the same setup (browser, OS, hardware, etc). This is not guaranteed anywhere, it's just a (naïve?) observation which I still wanted to document as such.

Adding functions for either as needed sounds good.

I believe the "standard" way would be to register a constant for use by alcGetIntegerv(). Adding new dedicated functions which directly return a single int would also be fine I guess, but I can't decide which way is better.
The reason I (we?) didn't bother registering this functionality as an extension yet is that this is an implementation detail not that many apps actually care about, because AFAIK it's not provided in native OpenAL impls anyway.

[juj]

In these types of loops, use for (var i = 0; ...)

[juj]

This looks like a source of per-frame generated garbage. Not a big deal, though it would be good to avoid temporary garbage where possible.

[jpernst]

Removed the slicing here since the destination arrays are already guaranteed to exist.

[juj]

The kind of generic sample format conversion mechanism below looks clean, though these kinds of loops have been profiled to be the biggest performance bottleneck in audio in the past. See https://github.com/kripken/emscripten/blob/incoming/src/library_openal.js#L857 for how these were optimized down to the tightest form or JS loops for best performance. Not sure if the performance issue might carry over to microphone, but might be good to keep in mind.

[yoanlcq]

I'll look into it. I'm a bit afraid this might end up less easy to read, but at the same time it doesn't look hard to do and the benefits are clear.

[yoanlcq]

Fixed (?) by commit e344c (edit: and 90b3f62. Whoops).

Should I also replace the setSample() mechanism in alcCaptureSamples() to use heap views instead, and unroll loops the same way ?
I personally would rather avoid this task because, since we handle the "resampling" and "no resampling" cases separately, it would cause a local explosion of code, which would harm readability (and maintenance) really bad this time - all for the name of better performance, but do we need to go that far? Aren't today's JS engines good enough for this ? (genuine question)

IMO it would be best to leave it as-is (or close to "as-is") for the time being, and if it (alcCaptureSamples()) actually shows up in profilers, people can always improve it after our PR (this time with real-world performance data in their hands).
As I see it right now, the added burden simply outweights the hypothetical benefits.

[juj]

Convention is to have a space after if: if (.

[juj]

ScriptProcessorNode is being deprecated in Web Audio, so it would be good to not rely on this (https://www.w3.org/TR/webaudio/#the-scriptprocessornode-interface---deprecated). Is there other ways to implement this, or do you think this will need upcoming AudioWorklets?

[yoanlcq]

I was actually asked the same question before (here), and basically according to the MDN doc on ScriptProcessorNode the functionality is to be replaced by Audio Workers.
Following the link in the deprecation message, I saw that the latter was not implemented yet (even though it dates back to 2014), so I didn't go any further, not even considering opening the spec to know more. (not claiming it was the right thing to do, it's just how it happened)

I also believe that ScriptProcessorNode won't be an easy one to bury (edit: I mean, by the Web Audio API, not our implementation).

Also, AudioWorklets, Web Workers, etc. are more involved and quite another plane in our problem-space if I may say.

[juj]

Space after for, for (

[juj]

In general, for(var i = 0; I < srcArray.length; ++i) {

[juj]

Either

if (condition) statement;

or

if (condition) {
  statement;
}

[juj]

Let's remove the word helper from all these functions, it's a bit redundant, and just have AL.getGlobal et al.?, (or something like AL.getGlobalParameter and AL.setGlobalParameter?)

[yoanlcq]

(If we do it, I'd be in favor ingetGlobal, it's short and no less clear. @jpernst ?)

[jpernst]

i think getGlobalParam/etc is a nice compromise between brevity and clarity

[juj]

This kind of pattern is explicitly reverting this earlier PR: https://github.com/kripken/emscripten/pull/4275/files#diff-36fcd036fa88446c972f56ca850c357fL1387. I see your documentation did have a mention about being sensitive to garbage collection pressure, so it would be good to retain the garbage free operation from PR #4275?

[jpernst]

Passing an array is very useful to maintain the separation of the core param setter methods from the entrypoints, so what I've done is create a recycled param array that will be filled before calls to the real setters, then passed in, This should hopefully have the desired effect of eliminating that source of per-frame garbage.

[juj]

AL.setListenerParameter instead of AL.listenerHelper? The word helper reads a bit ambiguous about what it is exactly helping.

[yoanlcq]

Also agreed, waiting for @jpernst's opinion on this

[jpernst]

Same. I think Param would do fine, but if we want the full word that's OK too.

[juj]

With this pattern I see that there's implicit casting that occurs to convert the parameter type to the type that was requested. In https://www.openal.org/documentation/OpenAL_Programmers_Guide.pdf pages 12&13, I understand it is not allowed to query an integer parameter via a float get function, and vice versa. Does that seem correct to you?

[yoanlcq]

(I didn't work on this part of the code, just wanted to add that the spec seems to agree, and it's probably expected to fail with ALC_INVALID_ENUM in this case.)

[jpernst]

Yeah, it was probably a copy/paste. I'll tackle this and the other main AL semantic issues.

[jpernst]

Actually, upon closer review, the 1.1 specification does expilcitly allow int/float queries for AL_POSITION and such, while the Programming Guide does not. I chose to adhere to the spec document in cases where they contradict.

[juj]

Very little of the original implementation remains, so I understand completely if this is a controversial or time consuming PR to examine, but I hope it's found useful.

Thanks. This PR is appreciated, but agreed that it is quite difficult to review, because there are so many conceptually unrelated changes that the diffs no longer line up on functions even. Added some superficial comments, I'll schedule time to review this closer with tests shortly.

[yoanlcq]

@juj, starting from your first review on line 1605 at src/library_openal.js, all diffs in your comments show up as going from lines 1605 to 1608, so I (we?) can't tell precisely which part of the code your comments actually refer to. I suspect (and hope) that I'm the only one experiencing this, but this is how it appears both in my mails and on GitHub, so there's something weird going on.

Right now I'm off to make changes to the docs and fixing style issues as per your comments.

[jpernst]

I've rebased on incoming again to eliminate the merge commits since the emscripten contributor guide says to avoid those.

[juj]

Ok, I've now got a backlog of other work items cleared, so I should be able to focus on this with better responsiveness - apologies for the wait. Running tests locally, at revision

commit 450f18452642d3c15474a092c73f02f30fca3a7a
Author: Jameson Ernst <jameson@jpernst.com>
Date:   Wed Aug 30 08:52:50 2017 -0700

    Fix `AL.buffer` typo

I am seeing the following:

• python tests/runner.py interactive.test_openal_al_soft_loop_points

fails with

uncaught exception: Assertion failed: state == AL_PLAYING, at: c:/users/clb/appdata/local/temp\emscripten_test_interactive_afurz9\openal_playback.cpp,251,main at jsStackTrace@http://localhost:8888/test.js:1312:13
stackTrace@http://localhost:8888/test.js:1329:12
___assert_fail@http://localhost:8888/test.js:1870:210
_main@http://localhost:8888/test.js:8543:3
asm._main@http://localhost:8888/test.js:15837:10
callMain@http://localhost:8888/test.js:16053:15
doRun@http://localhost:8888/test.js:16117:42
run/<@http://localhost:8888/test.js:16128:7

• python tests/runner.py interactive.test_openal_animated_looped_distance_playback has some kind of odd bug that in addition to fading the volume in and out, the pitch of the audio clip changes up and down as well. Reading the test code file, it should not be changing AL_PITCH, so I think this pitch change should not be happening in this test?

• python tests/runner.py interactive.test_openal_animated_looped_doppler_playback plays back corrupted/glitchy sound stream. Are you able to reproduce?

• python tests/runner.py interactive.test_openal_capture: There was another PR that landed in between, and this needs a small change from REPORT_RESULT(); to REPORT_RESULT(result); to compile. After that fix, I am getting an issue with the capture not running, and code throwing a stream of exceptions

ReferenceError: srcBuf is not defined
	onSuccess/newCapture.scriptProcessorNode.onaudioprocess

to web console. I wonder if the capture device I am using might be somehow incompatible, I realize I only have a Oculus Rift headset with its microphone connected, and no other microphones. Or does the issue reproduce for you?

[juj]

What remains is to decide if the while (true) is acceptable or needs an emergency exit

Could the loop condition read for (var i = 0; i < src.bufQueue.length; ++i)? That way it will be explicit that there's only at most that many iterations.

if the listener relative math is correct

Re-reading, the code checks out now in my mind as well. A sound source at relative location (0, 0, -1.0) should map to world position lPos + 1.0 * lDirection, which matches going through the code. Great work adding relative audio support btw, this was missing for a long time.

[jpernst]

Ok, I can repro all these problems. Odd, there must have been a regression at some point. I'll bisect and see if I can track it down.

[jpernst]

Ok, I nailed most of those problems, they were all caused by the same bug. The shared param array was accidentally getting cached into some internal fields, so when it was later reused for other param settings, unrelated params would suddenly change, which naturally caused some super wonky breakage.

The capture problem is still an open question, @yoanlcq can probably speak to that better than I.

Anyway, per the loop condition thing: The reason I didn't use that as the loop condition is that if there's a long stall from a blocking operation or something, a looping buffer queue could theoretically have looped many times over by that point, so there's no statically known upper bound to the number of iterations required.

[jpernst]

Actually, now that I think about it some more, we can probably determine the maximum number of iterations required by calculating the duration of the buffer queue, then dividing the delta time by that. From a strictly semantic standpoint that won't gain us anything, but it may make the loop more clear and further guard against possible non-termination. I'll tinker with it a bit.

[yoanlcq]

Apologies for the late reply; I'll see about the capture bug as soon as I can (within the next few days).

[jpernst]

I've refactored the update loop to remove the unbounded condition, and generally made the update method more clear. This version will probably be faster in some cases as well, since the new control flow lets it skip some unnecessary work in certain cases.

[yoanlcq]

There we go (hopefully)! For some reason there were two variables (c and srcBuf) which had vanished (at the same spot), and putting them back just did it.
How this could have happened is beyond me, but hopefully everything's back to normal now.

EDIT: There's one thing, but I suspect this might be my configuration, but I find that the playback of recordings is abnormally low in volume. Is it the case for you too ?

[juj]

Perfect, tests all pass now. I am still having issues with microphone recording as well, though my both mic-capable setups are not that great, so I'm not sure. Given that it's a new feature, I'm ok to merge nevertheless, so we can resolve further fixes in the tree.

Thanks for all the work here, and for the great patience to wait for the reviews.