Bug 72871 - Incorrect calculation for DistanceEffect linearGain
Summary: Incorrect calculation for DistanceEffect linearGain
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Raymond Toy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-21 04:15 PST by davidgaleano
Modified: 2011-12-19 11:25 PST (History)
4 users (show)

See Also:


Attachments
Patch (1.78 KB, patch)
2011-12-12 10:14 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (2.40 KB, patch)
2011-12-12 14:22 PST, Raymond Toy
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description davidgaleano 2011-11-21 04:15:39 PST
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	}
Comment 1 Raymond Toy 2011-11-21 10:34:48 PST
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).
Comment 2 davidgaleano 2011-11-25 11:16:00 PST
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.
Comment 3 Raymond Toy 2011-11-28 10:54:28 PST
(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.
Comment 4 Chris Rogers 2011-12-11 19:16:20 PST
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?
Comment 5 Raymond Toy 2011-12-12 10:14:41 PST
Created attachment 118813 [details]
Patch
Comment 6 Raymond Toy 2011-12-12 10:52:47 PST
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?
Comment 7 Chris Rogers 2011-12-12 12:38:31 PST
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
Comment 8 Raymond Toy 2011-12-12 14:22:21 PST
Created attachment 118845 [details]
Patch
Comment 9 Raymond Toy 2011-12-12 14:24:16 PST
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.
Comment 10 Chris Rogers 2011-12-12 15:40:00 PST
Looks fine.
Comment 11 Kenneth Russell 2011-12-12 17:02:58 PST
Comment on attachment 118845 [details]
Patch

OK. rs=me
Comment 12 WebKit Review Bot 2011-12-14 12:33:23 PST
Comment on attachment 118845 [details]
Patch

Clearing flags on attachment: 118845

Committed r102810: <http://trac.webkit.org/changeset/102810>
Comment 13 WebKit Review Bot 2011-12-14 12:33:27 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Eric Seidel (no email) 2011-12-19 11:25:00 PST
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).