Bug 78051 - Simplified discreteTimeConstantForSampleRate
Summary: Simplified discreteTimeConstantForSampleRate
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Raymond Toy
URL:
Keywords:
Depends on: 77666
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-07 15:32 PST by Raymond Toy
Modified: 2012-03-03 14:30 PST (History)
2 users (show)

See Also:


Attachments
Patch (2.84 KB, patch)
2012-02-29 11:51 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (4.94 KB, patch)
2012-02-29 16:07 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (4.89 KB, patch)
2012-03-02 15:35 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 Raymond Toy 2012-02-07 15:32:54 PST
In AudioUtilities.cpp, discreteTimeConstantForSampleRate has 


    // hardcoded value is temporary build fix for Windows.
    // FIXME: replace hardcode 2.718282 with M_E until the correct MathExtras.h solution is determined.
    return 1 - powf(1 / 2.718282f, 1 / (sampleRate * timeConstant));

This can be simplified to 1 - exp(-1 / (sampleRate * timeConstant)).

There maybe some issues with roundoff if sampleRate * timeConstant is large because we compute exp(<small>) ~ 1, so 1 - exp(<small>) can have significant roundoff.
Comment 1 Raymond Toy 2012-02-09 15:22:10 PST
(In reply to comment #0)
> In AudioUtilities.cpp, discreteTimeConstantForSampleRate has 
> 
> 
>     // hardcoded value is temporary build fix for Windows.
>     // FIXME: replace hardcode 2.718282 with M_E until the correct MathExtras.h solution is determined.
>     return 1 - powf(1 / 2.718282f, 1 / (sampleRate * timeConstant));
> 
> This can be simplified to 1 - exp(-1 / (sampleRate * timeConstant)).
> 
> There maybe some issues with roundoff if sampleRate * timeConstant is large because we compute exp(<small>) ~ 1, so 1 - exp(<small>) can have significant roundoff.

If accuracy is important we can use the identity

1 - exp(-z) = 2*tanh(z/2)/(1 + tanh(z/2))

This doesn't have roundoff problem for z small because tanh(z/2) ~ z/2.
Comment 2 Chris Rogers 2012-02-09 16:11:38 PST
(In reply to comment #1)
> (In reply to comment #0)
> > In AudioUtilities.cpp, discreteTimeConstantForSampleRate has 
> > 
> > 
> >     // hardcoded value is temporary build fix for Windows.
> >     // FIXME: replace hardcode 2.718282 with M_E until the correct MathExtras.h solution is determined.
> >     return 1 - powf(1 / 2.718282f, 1 / (sampleRate * timeConstant));
> > 
> > This can be simplified to 1 - exp(-1 / (sampleRate * timeConstant)).
> > 
> > There maybe some issues with roundoff if sampleRate * timeConstant is large because we compute exp(<small>) ~ 1, so 1 - exp(<small>) can have significant roundoff.
> 
> If accuracy is important we can use the identity
> 
> 1 - exp(-z) = 2*tanh(z/2)/(1 + tanh(z/2))
> 
> This doesn't have roundoff problem for z small because tanh(z/2) ~ z/2.

Are you seeing accuracy problems with the current version?
return 1 - powf(1 / 2.718282f, 1 / (sampleRate * timeConstant));
Comment 3 Raymond Toy 2012-02-09 16:29:19 PST
(In reply to comment #2)
> (In reply to comment #1)
> > (In reply to comment #0)
> > > In AudioUtilities.cpp, discreteTimeConstantForSampleRate has 
> > > 
> > > 
> > >     // hardcoded value is temporary build fix for Windows.
> > >     // FIXME: replace hardcode 2.718282 with M_E until the correct MathExtras.h solution is determined.
> > >     return 1 - powf(1 / 2.718282f, 1 / (sampleRate * timeConstant));
> > > 
> > > This can be simplified to 1 - exp(-1 / (sampleRate * timeConstant)).
> > > 
> > > There maybe some issues with roundoff if sampleRate * timeConstant is large because we compute exp(<small>) ~ 1, so 1 - exp(<small>) can have significant roundoff.
> > 
> > If accuracy is important we can use the identity
> > 
> > 1 - exp(-z) = 2*tanh(z/2)/(1 + tanh(z/2))
> > 
> > This doesn't have roundoff problem for z small because tanh(z/2) ~ z/2.
> 
> Are you seeing accuracy problems with the current version?
> return 1 - powf(1 / 2.718282f, 1 / (sampleRate * timeConstant));

No, but I'm using fairly small time constants like .01 sec at a sample rate of 44100.
Comment 4 Chris Rogers 2012-02-09 19:18:34 PST
(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > (In reply to comment #0)
> > > > In AudioUtilities.cpp, discreteTimeConstantForSampleRate has 
> > > > 
> > > > 
> > > >     // hardcoded value is temporary build fix for Windows.
> > > >     // FIXME: replace hardcode 2.718282 with M_E until the correct MathExtras.h solution is determined.
> > > >     return 1 - powf(1 / 2.718282f, 1 / (sampleRate * timeConstant));
> > > > 
> > > > This can be simplified to 1 - exp(-1 / (sampleRate * timeConstant)).
> > > > 
> > > > There maybe some issues with roundoff if sampleRate * timeConstant is large because we compute exp(<small>) ~ 1, so 1 - exp(<small>) can have significant roundoff.
> > > 
> > > If accuracy is important we can use the identity
> > > 
> > > 1 - exp(-z) = 2*tanh(z/2)/(1 + tanh(z/2))
> > > 
> > > This doesn't have roundoff problem for z small because tanh(z/2) ~ z/2.
> > 
> > Are you seeing accuracy problems with the current version?
> > return 1 - powf(1 / 2.718282f, 1 / (sampleRate * timeConstant));
> 
> No, but I'm using fairly small time constants like .01 sec at a sample rate of 44100.

How large do the time constants have to get before things get too bad?  Is the problem that serious?

Ideally, we'd just use your simplified and clear equation:
1 - exp(-1 / (sampleRate * timeConstant))
Comment 5 Raymond Toy 2012-02-09 21:18:48 PST
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (In reply to comment #1)
> > > > (In reply to comment #0)
> > > > > In AudioUtilities.cpp, discreteTimeConstantForSampleRate has 
> > > > > 
> > > > > 
> > > > >     // hardcoded value is temporary build fix for Windows.
> > > > >     // FIXME: replace hardcode 2.718282 with M_E until the correct MathExtras.h solution is determined.
> > > > >     return 1 - powf(1 / 2.718282f, 1 / (sampleRate * timeConstant));
> > > > > 
> > > > > This can be simplified to 1 - exp(-1 / (sampleRate * timeConstant)).
> > > > > 
> > > > > There maybe some issues with roundoff if sampleRate * timeConstant is large because we compute exp(<small>) ~ 1, so 1 - exp(<small>) can have significant roundoff.
> > > > 
> > > > If accuracy is important we can use the identity
> > > > 
> > > > 1 - exp(-z) = 2*tanh(z/2)/(1 + tanh(z/2))
> > > > 
> > > > This doesn't have roundoff problem for z small because tanh(z/2) ~ z/2.
> > > 
> > > Are you seeing accuracy problems with the current version?
> > > return 1 - powf(1 / 2.718282f, 1 / (sampleRate * timeConstant));
> > 
> > No, but I'm using fairly small time constants like .01 sec at a sample rate of 44100.
> 
> How large do the time constants have to get before things get too bad?  Is the problem that serious?
> 
> Ideally, we'd just use your simplified and clear equation:
> 1 - exp(-1 / (sampleRate * timeConstant))

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (In reply to comment #1)
> > > > (In reply to comment #0)
> > > > > In AudioUtilities.cpp, discreteTimeConstantForSampleRate has 
> > > > > 
> > > > > 
> > > > >     // hardcoded value is temporary build fix for Windows.
> > > > >     // FIXME: replace hardcode 2.718282 with M_E until the correct MathExtras.h solution is determined.
> > > > >     return 1 - powf(1 / 2.718282f, 1 / (sampleRate * timeConstant));
> > > > > 
> > > > > This can be simplified to 1 - exp(-1 / (sampleRate * timeConstant)).
> > > > > 
> > > > > There maybe some issues with roundoff if sampleRate * timeConstant is large because we compute exp(<small>) ~ 1, so 1 - exp(<small>) can have significant roundoff.
> > > > 
> > > > If accuracy is important we can use the identity
> > > > 
> > > > 1 - exp(-z) = 2*tanh(z/2)/(1 + tanh(z/2))
> > > > 
> > > > This doesn't have roundoff problem for z small because tanh(z/2) ~ z/2.
> > > 
> > > Are you seeing accuracy problems with the current version?
> > > return 1 - powf(1 / 2.718282f, 1 / (sampleRate * timeConstant));
> > 
> > No, but I'm using fairly small time constants like .01 sec at a sample rate of 44100.
> 
> How large do the time constants have to get before things get too bad?  Is the problem that serious?

Probably not.  I don't have a good feel for what kind of time constant people might use.
> 
> Ideally, we'd just use your simplified and clear equation:
> 1 - exp(-1 / (sampleRate * timeConstant))

Well, assuming I did the math right, 1-exp(-1/192000) works out to be 5.185604e-6, as a float.  The true value is closer to 5.2083197e-6.  So we're only accurate to 2 digits now.

Since this is used in setTargetValueAtTime, I think it doesn't matter which value we use because it's so small that the roundoff error in computing the new values using this multiplier will be the dominating factor.  value += (target - value)*multiplier.  If value is about 1 and target is 0, we're adding a very small value.   We might want to consider doing the computation in double-float instead.  Or just accept the noisy result.
Comment 6 Chris Rogers 2012-02-19 00:52:37 PST
Generally, I'd like to see this type of math in double-precision, unless the code is inside of tight loops.
Comment 7 Raymond Toy 2012-02-21 11:20:45 PST
It would be best to update this after bug 77666 is fixed so that we get tests for this.
Comment 8 Raymond Toy 2012-02-29 11:51:45 PST
Created attachment 129486 [details]
Patch
Comment 9 Chris Rogers 2012-02-29 15:45:57 PST
The patch looks good in general, but you should check all calling sites to avoid compiler warning/error due to loss-of-precision 64bit double -> 32bit float (which can happen on certain mac-port builds)
Comment 10 Raymond Toy 2012-02-29 16:07:20 PST
Created attachment 129542 [details]
Patch
Comment 11 Raymond Toy 2012-02-29 16:09:04 PST
(In reply to comment #9)
> The patch looks good in general, but you should check all calling sites to avoid compiler warning/error due to loss-of-precision 64bit double -> 32bit float (which can happen on certain mac-port builds)

Done.  Only two places needed the cast.
Comment 12 Chris Rogers 2012-02-29 16:10:11 PST
I'm assuming this builds on Windows?
Comment 13 Raymond Toy 2012-02-29 16:22:00 PST
(In reply to comment #12)
> I'm assuming this builds on Windows?

It did when I did the first implementation, but I will check again.
Comment 14 Raymond Toy 2012-03-01 09:16:31 PST
(In reply to comment #13)
> (In reply to comment #12)
> > I'm assuming this builds on Windows?
> 
> It did when I did the first implementation, but I will check again.

Builds fine.  Don't know of any layout tests that cover this, except for the ones in bug 77666, which hasn't landed yet.
Comment 15 Chris Rogers 2012-03-01 11:45:12 PST
Does 77666 layout test run with this patch on Windows?  If so, the patch looks good, but you'll have to update to latest sources and re-upload the patch since the patch didn't apply cleanly (purple EWS)
Comment 16 Raymond Toy 2012-03-01 12:28:46 PST
(In reply to comment #15)
> Does 77666 layout test run with this patch on Windows?  If so, the patch looks good, but you'll have to update to latest sources and re-upload the patch since the patch didn't apply cleanly (purple EWS)

I believe I tried this, but I'm no longer sure because I updated my sources on Windows.  I'd like to wait for 77666 to land and then I'll try this out on Windows and let you know what happens.
Comment 17 Raymond Toy 2012-03-02 15:35:59 PST
Created attachment 129967 [details]
Patch
Comment 18 Raymond Toy 2012-03-02 15:38:26 PST
(In reply to comment #15)
> Does 77666 layout test run with this patch on Windows?  If so, the patch looks good, but you'll have to update to latest sources and re-upload the patch since the patch didn't apply cleanly (purple EWS)

Ran it (finally) on Windows and all tests pass, including the new audioparam timeline tests which does exercise discreteTimeConstantForSampleRate.

New patch uploaded too, based on current code.
Comment 19 WebKit Review Bot 2012-03-02 18:43:33 PST
Comment on attachment 129967 [details]
Patch

Rejecting attachment 129967 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Last 500 characters of output:
_by_email
    return self._reviewer_only(self.account_by_email(email))
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/common/config/committers.py", line 632, in account_by_email
    return self._email_to_account_map().get(email.lower()) if email else None
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/common/config/committers.py", line 525, in _email_to_account_map
    assert(email not in self._accounts_by_email)  # We should never have duplicate emails.
AssertionError

Full output: http://queues.webkit.org/results/11801211
Comment 20 WebKit Review Bot 2012-03-03 14:30:40 PST
Comment on attachment 129967 [details]
Patch

Clearing flags on attachment: 129967

Committed r109665: <http://trac.webkit.org/changeset/109665>
Comment 21 WebKit Review Bot 2012-03-03 14:30:46 PST
All reviewed patches have been landed.  Closing bug.