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.
(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.
(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));
(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.
(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? > > 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.
Generally, I'd like to see this type of math in double-precision, unless the code is inside of tight loops.
It would be best to update this after bug 77666 is fixed so that we get tests for this.
Created attachment 129486 [details] Patch
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)
Created attachment 129542 [details] Patch
(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.
I'm assuming this builds on Windows?
(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.
(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.
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)
(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.
Created attachment 129967 [details] Patch
(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 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 on attachment 129967 [details] Patch Clearing flags on attachment: 129967 Committed r109665: <http://trac.webkit.org/changeset/109665>
All reviewed patches have been landed. Closing bug.