Bug 78051 – Simplified discreteTimeConstantForSampleRate

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Raymond Toy]

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.

[Raymond Toy]

(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.

[Chris Rogers]

(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));

[Raymond Toy]

(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.

[Chris Rogers]

(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))

[Raymond Toy]

(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.

[Chris Rogers]

Generally, I'd like to see this type of math in double-precision, unless the code is inside of tight loops.

[Raymond Toy]

It would be best to update this after bug 77666 is fixed so that we get tests for this.

[Raymond Toy]

Created attachment 129486 [details]
Patch

[Chris Rogers]

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)

[Raymond Toy]

Created attachment 129542 [details]
Patch

[Raymond Toy]

(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.

[Chris Rogers]

I'm assuming this builds on Windows?

[Raymond Toy]

(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.

[Raymond Toy]

(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.

[Chris Rogers]

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)

[Raymond Toy]

(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.

[Raymond Toy]

Created attachment 129967 [details]
Patch

[Raymond Toy]

(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.

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 129967 [details]
Patch

Clearing flags on attachment: 129967

Committed r109665: <http://trac.webkit.org/changeset/109665>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.