Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/22acef5e2878

azimuth = -nan(0x400000) -> -nan(0x8000000000000)

This fixes the situation for the testcase in attachment, but there are some other edge cases in azimuth calculation that I want to test and deal with.

Are these patches ready for review?

I'll attach patches here. They are not top priority because the optimized builds will safely produce null output, but I want to put these up while they're fresh in my mind.

See the checkin comment. This follows the approach in http://lists.w3.org/Archives/Public/public-audio/2013JulSep/0571.html for the situations undefined in the spec. I'll come up with a tests.

The azimuth calculation in the Web Audio spec becomes undefined in this situation as it requires normalizing a zero vector. The panning effect should not depend on azimuth when the source is directly above the listener (because position does not depend on azimuth), but the specified "equalpower" panning model does depend on azimuth even at this elevation. Setting azimuth to zero produces the same result as if the normalized projection were replaced with a zero vector.

The main difference from v1 is in normalizing the supplied up vector, because both vectors in the cross-product need to be sufficiently smaller than the maximum finite value in order to know that overflow won't occur.

Comment on attachment 796382 [details] [diff] [review] part 4: handle zero front-right plane projection without NaNs Review of attachment 796382 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/PannerNode.cpp @@ +407,5 @@ > + if (projectedSource.IsZero()) { > + // source - listener direction is up or down. > + aAzimuth = 0.0; > + return; > + } We are now effectively doing the same as WebKit/Blink (in a slightly different manner), who are implementing something different than the spec. I'll go and file a bug, and quickly change the spec if nobody agrees. I was thinking it would be nice to have a code resembling what is in the spec, but your solution here is actually better than what is in the spec (since it computes less stuff for the same result). It probably does not matter too much.

Comment on attachment 796562 [details] [diff] [review] part 3 v2: normalize orientation vectors early and keep existing state if directions are undefined Review of attachment 796562 [details] [diff] [review]: ----------------------------------------------------------------- I assume the tests we have in the tree have the listener and the source on the same plane, so we weren't triggering this? It would be nice to be to have a testcase that exercise this code, and maybe give the testcase to other implementers so they can check whether their behaviour is correct. I seem to recall they just replace the NaNs/Infinity by zero and continue running the algorithm like nothing happened. Adding the testcase to our crashtest suite in content/media/test/crashtests would also be nice and quick.

Forgot to remove this in part3. Already done in the setter.

(In reply to Paul Adenot (:padenot) from comment #12) > I assume the tests we have in the tree have the listener and the source on > the same plane, so we weren't triggering this? Yes, panner-model-testing.js has source and listener on the same plane. That means that the effect of directly above case is not tested. I'm holding off writing output tests for out of plane cases for the moment because the equalpower model doesn't make much sense out of plane and perhaps that will change. I have a crashtest for the directly above case. > It would be nice to be to have a testcase that exercise this code, and maybe > give the testcase to other implementers so they can check whether their > behaviour is correct. I seem to recall they just replace the NaNs/Infinity > by zero and continue running the algorithm like nothing happened. The crashtest also exercises this code, but doesn't test that the effect uses the previous orientation. https://tbpl.mozilla.org/?tree=Try&rev=80b6bad2f9e8 https://tbpl.mozilla.org/?tree=Try&rev=8501301cd906

https://tbpl.mozilla.org/?tree=Try&rev=5f92be7b4034 https://tbpl.mozilla.org/?tree=Try&rev=acc8c3eee4e6

pushed to inbound and aurora with a=webaudio https://hg.mozilla.org/integration/mozilla-inbound/rev/3171b6ad0055 (I noticed after pushing that your patches don't have names on them, so this one got pushed with my name) https://hg.mozilla.org/releases/mozilla-aurora/rev/0177db952fd3

Other patches are still to land. I ended up rewriting the crashtest because ScriptProcessorNode isn't (and can't be) as synchronous as I was expecting. I added test_pannerNodeAbove.html with the equalpower model, but it was already passing before the patches here through min(180, max(-180, NaN) selecting azimuth = -180. https://tbpl.mozilla.org/?tree=Try&rev=acd2e5851497 https://tbpl.mozilla.org/?tree=Try&rev=c2108eee4512 https://tbpl.mozilla.org/?tree=Try&rev=b318718d606d

https://hg.mozilla.org/integration/mozilla-inbound/rev/0343a9d0f208 https://hg.mozilla.org/integration/mozilla-inbound/rev/18e8eadef06f https://hg.mozilla.org/integration/mozilla-inbound/rev/845d86b48e85 https://hg.mozilla.org/integration/mozilla-inbound/rev/084b01d1b6b3 https://hg.mozilla.org/integration/mozilla-inbound/rev/86c9a90d8408 https://hg.mozilla.org/integration/mozilla-inbound/rev/1eaccb673d33 https://hg.mozilla.org/integration/mozilla-inbound/rev/1100a5bc013b https://hg.mozilla.org/integration/mozilla-inbound/rev/c2bfb5b08f75

In reply to Christoph Diehl [:cdiehl] from comment #1) > Created attachment 793755 [details] > callstack This stack trace points to a crash after the assertion, which I can't explain nor reproduce. Do you perhaps have a different testcase that fails here? http://hg.mozilla.org/mozilla-central/annotate/4b5789932294/content/media/webaudio/blink/HRTFPanner.cpp#l224

https://hg.mozilla.org/releases/mozilla-aurora/rev/e3902838eda4 https://hg.mozilla.org/releases/mozilla-aurora/rev/b2bd6347837b https://hg.mozilla.org/releases/mozilla-aurora/rev/753d8267bc09 https://hg.mozilla.org/releases/mozilla-aurora/rev/8537c5e94952 https://hg.mozilla.org/releases/mozilla-aurora/rev/b9deaf0ea71e https://hg.mozilla.org/releases/mozilla-aurora/rev/ea6aaf786efd https://hg.mozilla.org/releases/mozilla-aurora/rev/91da1184371a https://hg.mozilla.org/releases/mozilla-aurora/rev/5397b6cbbfce

https://hg.mozilla.org/mozilla-central/rev/18e8eadef06f https://hg.mozilla.org/mozilla-central/rev/845d86b48e85 https://hg.mozilla.org/mozilla-central/rev/084b01d1b6b3 https://hg.mozilla.org/mozilla-central/rev/86c9a90d8408 https://hg.mozilla.org/mozilla-central/rev/1eaccb673d33 https://hg.mozilla.org/mozilla-central/rev/1100a5bc013b https://hg.mozilla.org/mozilla-central/rev/c2bfb5b08f75

(In reply to Karl Tomlinson (:karlt) from comment #20) > In reply to Christoph Diehl [:cdiehl] from comment #1) > > Created attachment 793755 [details] > > callstack > > This stack trace points to a crash after the assertion, which I can't > explain nor reproduce. Do you perhaps have a different testcase that fails > here? > http://hg.mozilla.org/mozilla-central/annotate/4b5789932294/content/media/ > webaudio/blink/HRTFPanner.cpp#l224 Hm, no I don't have. I will look out for it in the next runs.

Comment on attachment 793754 [details] testcase Yes, floating point arguments passed to the APIs definitely change the branching decisions.