Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Reverb wrapping distortion caused by integer overflow in calculation #16

Open
djtuBIG-MaliceX opened this issue Jan 21, 2017 · 3 comments

Comments

@djtuBIG-MaliceX
Copy link

djtuBIG-MaliceX commented Jan 21, 2017

This has been an outstanding issue for a while, but after looking at the output from a wave recording in a sound editor, the shape of a sine wave with reverb turned on and playing them at the loudest volume made it obvious.

Been trying to fix it on my own in the code but there seems to be some funny quirks with the coefficients in the calculation in synth/reverb.cpp (even after trying to simply clip to the INT_MAX/INT_MIN of possible culprits in the equations). In any case, here's a WAV recording of the symptoms in its obvious form (you can see it wrap the sine waveform in an oscilloscope): https://dl.dropboxusercontent.com/u/1287967/oxefmsynth-reverbwrappingdistiort.mp3

EDIT: on further investigation, it may well also be an issue with synthesizer.cpp unctions for SumStereoMono() and SumMonoStereo() since they sum integers without taking into account of any possible overflow wrapping?

@djtuBIG-MaliceX
Copy link
Author

I decided to have a look at this again, recompiled current sources and found the culprit. It's in the DC filter.

https://github.com/oxesoft/oxefmsynth/blob/master/src/synth/reverb.cpp#L118

The operation ou0 = b[i] - in1 + ((ou0*32674)/32768); is causing an integer overflow on calculation of ou0 * 32674. Casting ou0 with either float or double resolves the issue.

In addition, may want to consider using floating point buffers instead of integers, as DAWs can better handle 'extreme-value' loudness (eg: place a limiter in front of the signal chain), rather than have the synthesizer clip internally.

@djtuBIG-MaliceX
Copy link
Author

If anyone wants to test, try these builds out.
OxeFMSynth-7jan2021.zip

@samboy
Copy link

samboy commented Mar 29, 2021

Related: I think we can do better here than having a simple Schroeder reverb. As reverb guru Sean Costello has put it, the first thing reverb designers do is implement the Schroeder reverb. The second thing reverb designers do is realize the Schroeder reverb is no good and implement something else. A FDN is probably the easiest good sounding reverb to implement; a 4-line FDN can sound really good with careful tuning, and a 8-line or 12-line FDN can more easily sound good if we’re willing to take the CPU hit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants