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

Remove interpolation of IIR filter coefficients #1345

Merged
merged 2 commits into from
Jul 27, 2024
Merged

Conversation

derselbst
Copy link
Member

During implementation of #1342, I found the IIR filter producing strange cracks and clicks. I haven't done a deep investigation but it seems that this was caused by big and frequent changes of the cutoff frequency by fres_mod.

I found that the clicks were produced by the code that interpolates the filter coefficients. I.e. setting if(dsp_filter_coeff_incr_count > 0) to if(FALSE) didn't cause any more clicks.

Without a deeper investigation of the numeric logic I don't know how to fix this. Yet, I don't think this justifies the more complex logic, and therefore I'm suggesting to remove that interpolation logic.

Attached is a ZIP with two audio samples. DF2.ogg (Direct Form 2) is the filter as implemented before this PR. DF2-nointerp.ogg is the same filter without any interpolation.

filter-coeff-interp.zip

@derselbst derselbst added this to the 2.4 milestone Jun 29, 2024
Copy link

sonarcloud bot commented Jun 29, 2024

@klerg
Copy link

klerg commented Jun 29, 2024

Very good to see you are already trying to put this into Fluidsynth. I'm not sure what IIR means or what that does to the filter. And I know even less on what fres_mod changes to the cutoff frequency but I'm sure you got this down to a tee.

As you can tell, at this point I'm pretty much lost on what all this means but glad you were able to find the issue and stop anymore clicks and pops to the sound as well.

Wow this seems like it will not be easy to find what is behind the problem at all then. Sure, on stuff like this it will be your call to do what you assume is the best way forward. So you had to remove it.

I just listened to both samples and find it hard to hear that much of difference, in other words, to me each sample has clicks and pops, but the second has barely any less, why is that ?

@derselbst
Copy link
Member Author

You need to listen till the end. The first clicks appear in DF2.ogg after 22 sec.

@klerg
Copy link

klerg commented Jun 30, 2024

Yes I'm able to hear the clicks in DF2.ogg but also get clicks in DF2-nointerp.ogg as well

@derselbst derselbst merged commit 31fa6ea into master Jul 27, 2024
71 of 77 checks passed
@derselbst derselbst deleted the iir-filter-interpol branch July 27, 2024 18:44
DominusExult added a commit to DominusExult/fluidsynth-sans-glib that referenced this pull request Jul 29, 2024
* master: (33 commits)
  Remove interpolation of IIR filter coefficients (FluidSynth#1345)
  Add OS/2 KAI audio driver
  Fix behavior of volume envelope delay phase
  Fix chorus when compiling with single precision (FluidSynth#1339)
  Update docs about XG bank selection logic
  Fix a few compiler warnings
  Fix linkage against gobject
  Fix mixed declaration
  Log NRPNs in verbose mode
  Update documentation of synth.device-id
  Access to synth->bank_select not guarded by mutex
  when processing command line options: -o setting=value, the value is converted to UTF8
  review findings
  Documentation updated (settings, about utf-8 encoding) C++ example updated
  drivers: winmidi,waveout and dsound using utf8 options Unicode support enabled in CMake script and CLI utility
  Fixed  wasapi driver; encode device names as CP_UTF8 Solves FluidSynth#1322
  Fix an import library is treated as a static library on OS/2 (FluidSynth#1321)
  Bump the github-actions group with 2 updates
  Keep GitHub Actions up to date with GitHub's Dependabot
  Minor fixups for MSYS2 CI (FluidSynth#1317)
  ...

# Conflicts:
#	CMakeLists.txt
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

Successfully merging this pull request may close these issues.

2 participants