957024 - Web Audio sources don't play when connected to PannerNodes with position 0,0,0

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P1)

Description

[Rob Manson]

Attachment: wa2.html shows two separate source/panner graphs. To test changing the position enter "panner1.setPosition(1,0,0)" and "panner2.setPosition(1,0,0)" or similar into the javascript console - wav files not included.

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131205075310

Steps to reproduce:

Created an audio context, a buffer source and a panner node and connected them together (e.g. source -> panner -> context.destination). Then loaded a sound file into a buffer via xhr2 and then onload poured the loaded data into the source's buffer, set it to loop and called start(0.0).

Tested on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0

See attached example files.
wa2.html shows two separate source/panner graphs. To test changing the position enter "panner1.setPosition(1,0,0)" and "panner2.setPosition(1,0,0)" or similar into the javascript console - wav files not included.


Actual results:

Silence.

But if you call panner.setPosition() with anything but 0 for any of the x,y,z params (within a reasonable distance range) then the sound starts playing - beware low values blowing your ears out if you have headphones in 8)

Even more frustratingly, if you have more than one panner node, then if ANY of them are set to position 0,0,0 then all sounds connected to panner nodes stop playing. The panner positions can be updated independently and work correctly as long as they all have non 0,0,0 values.



Expected results:

The sounds connected to panners at position 0,0,0 should play and sound like they are at the same position the context.listener is at (default 0,0,0). And if any of them go to position 0,0,0 then the others should still keep playing like normal and not go silent.

This behaviour works as expected in Chrome.

Comment 1

[Karl Tomlinson (:karlt)]

The default PannerNode distanceModel and rolloffFactor treats sources as point sources.  In order for point sources to be heard at a distance > 0, the volume at the point must be ∞.  That means the expected result for a panner at the listener point would be sound with infinite volume.  In floating point arithmetic, that would be a signal with +inf, -inf, and NaNs at the zero crossings.

The silence implies that a conversion from infs and NaNs to zero is happening, which is probably better than a full volume square wave from clipping.

The influence of a single panner node at zero distance on the whole graph implies that the conversion is happening after the mixing.  I guess it could be silenced before mixing, but I don't see much point as there is still going to be a full volume clipped signal if any panner node has distance close to zero.

Comment 3

[Karl Tomlinson (:karlt)]

The spec has changed so that distances less than refDistance should be treated as refDistance when calculating distanceGain.

Comment 4

[Karl Tomlinson (:karlt)]

This can cause significant problems with clipping, and shouldn't be too hard to fix.

Comment 6

[Dan Minor [:dminor]]

Attachment: Bug 957024 - Update PannerNode distance calculations to match the web audio spec;

Distances less than refDistance should be treated as refDistance when calculating
the gain.

Review commit: https://reviewboard.mozilla.org/r/71978/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71978/

Comment 7

[Dan Minor [:dminor]]

Strangely enough, I wasn't able to reproduce this failure. The test case seems simple enough but maybe I'm missing something. Regardless, we should update the gain equations to match the spec. I've also added a test to catch future regressions.

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8781580 [details]
Bug 957024 - Update PannerNode distance calculations to match the web audio spec;

https://reviewboard.mozilla.org/r/71978/#review84958

::: dom/media/webaudio/PannerNode.cpp:374
(Diff revision 1)
>  
>  // Those three functions are described in the spec.
>  float
>  PannerNodeEngine::LinearGainFunction(float aDistance)
>  {
> -  return 1 - mRolloffFactor * (aDistance - mRefDistance) / (mMaxDistance - mRefDistance);
> +  return 1 - mRolloffFactor * (std::max(std::min((double)aDistance, mMaxDistance), mRefDistance) - mRefDistance) / (mMaxDistance - mRefDistance);

PannerNodeEngine::ComputeDistanceGain() converts a double sqrt() to a float
|distance| before passing to one of these functions.  Can you change
|distance| and |aDistance| to double type in order to remove that implicit
cast, please, and remove the explicit C-style (double) casts introduced in
this patch?

::: dom/media/webaudio/test/mochitest.ini:88
(Diff revision 1)
>  [test_bug867203.html]
>  [test_bug875221.html]
>  [test_bug875402.html]
>  [test_bug894150.html]
>  [test_bug956489.html]
> +[test_bug957024.html]

If you can call this something like test_pannerNodeAtZeroDistance.html, then what is wrong in a test failure would be obvious from the output.

::: dom/media/webaudio/test/test_bug957024.html:4
(Diff revision 1)
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test PannerNode produces output even when the position is 0, 0, 0</title>

"... even when the distance is from the listener is zero" describes the relevant situation more precisely.

::: dom/media/webaudio/test/test_bug957024.html:71
(Diff revision 1)
> +    var silence = true;
> +    var array = buffer.getChannelData(0);
> +    for (var i = 0; i < buffer.length; i++) {
> +      if (array[i] != 0) {
> +        silence = false;
> +      }
> +    }
> +    ok(!silence, "The buffer is not silent.");

This won't detect a revert to previous behavior because currently output is infinite, which is not = 0.  Better to compare against the precise expected output.  Is that monoBuffer?

Comment 9

[Dan Minor [:dminor]]

Comment on attachment 8781580 [details]
Bug 957024 - Update PannerNode distance calculations to match the web audio spec;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71978/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3687c50bc52e
Update PannerNode distance calculations to match the web audio spec; r=karlt

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3687c50bc52e