Bug 72871 – Incorrect calculation for DistanceEffect linearGain

• Component: Web Audio
• Status: RESOLVED FIXED

I think the following code in "WebCore/platform/audio/Distance.cpp" is incorrect:

	double DistanceEffect::linearGain(double distance)
73	{
74	    return (1.0 - m_rolloffFactor * (distance - m_refDistance)) / (m_maxDistance - m_refDistance);
75	}

When the distance is equal or lower to the reference distance the output gain should be one but if you resolve the equation for that case:

    (1.0 - m_rolloffFactor * (m_refDistance - m_refDistance)) / (m_maxDistance - m_refDistance)
    (1.0 - m_rolloffFactor * 0) / (m_maxDistance - m_refDistance)
    (1.0 - 0) / (m_maxDistance - m_refDistance)
    1.0 / (m_maxDistance - m_refDistance)

Which is not one in a majority of cases.

I think the parenthesis are wrong and the correct code should be:

	double DistanceEffect::linearGain(double distance)
73	{
74	    return (1.0 - (m_rolloffFactor * (distance - m_refDistance) / (m_maxDistance - m_refDistance));
75	}

[Raymond Toy]

Yes, I agree.  You can easily hear the effect using http://chromium.googlecode.com/svn/trunk/samples/audio/simple.html

Bring up the javascript console and enter panner.distanceModel = 0 to get the linear gain model.  Now you can't hear anything because the gain value even at the reference distance is approximately 1/10000 (because the maxDistance is 10000 and refDistance is 1).

Do you think my suggested code is correct? do you guys want a patch or something?
At the moment the linear model is a bit unusable because of this bug.

[Raymond Toy]

(In reply to comment #2)
> Do you think my suggested code is correct? do you guys want a patch or something?
> At the moment the linear model is a bit unusable because of this bug.

No, no patch is needed.  Your suggested correction looks like the correct solution.  I actually don't know exactly what linearGain is specified to do, but this looks like the most reasonable answer.

[Chris Rogers]

Hi David, sorry for jumping into this so late.  I just returned from vacation...

It looks like you're exactly right.  Thanks for catching this!

The distance models are supposed to be defined exactly as in OpenAL:
http://connect.creativelabs.com/openal/Documentation/OpenAL%201.1%20Specification.pdf

Ray, would you like to put in a patch for this?

[Raymond Toy]

Created attachment 118813 [details]
Patch

[Raymond Toy]

The patch adds a reference to the OpenAL document.  I didn't do that in this patch, but perhaps a reference to that document should also be included for the other distance formulas.  And what about the webaudio spec itself?  Should it just reference the OpenAL document or include equivalent text?

[Chris Rogers]

Comment on attachment 118813 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118813&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This line needs to be updated.  Either add a test for this, or say that tests still must be written for all distance models.  This does not add new API.

(or something like that)

> Source/WebCore/platform/audio/Distance.cpp:74
> +    // See Open AL 1.1 Specification, section 3.4.4.

If we include this comment, then we should include the URL and hoist it out of this specific function and put it somewhere else more common, like the .h or .idl

> Source/WebCore/platform/audio/Distance.cpp:77
> +    distance = std::min(std::max(distance, m_refDistance), m_maxDistance);

This clamping is not specific to linearGain.  Also, this logic is already in DistanceEffect::gain() which is taking into account m_isClamped so it should be removed from here

[Raymond Toy]

Created attachment 118845 [details]
Patch

[Raymond Toy]

Comment on attachment 118813 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118813&action=review

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> This line needs to be updated.  Either add a test for this, or say that tests still must be written for all distance models.  This does not add new API.
> 
> (or something like that)

Updated.

>> Source/WebCore/platform/audio/Distance.cpp:74
>> +    // See Open AL 1.1 Specification, section 3.4.4.
> 
> If we include this comment, then we should include the URL and hoist it out of this specific function and put it somewhere else more common, like the .h or .idl

Moved to .h file.

>> Source/WebCore/platform/audio/Distance.cpp:77
>> +    distance = std::min(std::max(distance, m_refDistance), m_maxDistance);
> 
> This clamping is not specific to linearGain.  Also, this logic is already in DistanceEffect::gain() which is taking into account m_isClamped so it should be removed from here

Removed.

[Chris Rogers]

Looks fine.

[Kenneth Russell]

Comment on attachment 118845 [details]
Patch

OK. rs=me

[WebKit Review Bot]

Comment on attachment 118845 [details]
Patch

Clearing flags on attachment: 118845

Committed r102810: <http://trac.webkit.org/changeset/102810>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Comment on attachment 118813 [details]
Patch

Cleared review? from obsolete attachment 118813 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).