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 filter processing order in DSP chain #1444

Merged
merged 5 commits into from
Dec 8, 2024
Merged

Fix filter processing order in DSP chain #1444

merged 5 commits into from
Dec 8, 2024

Conversation

derselbst
Copy link
Member

Previously, the IIR filter was applied after the gain of the volume envelope was applied to the signal. This could have caused audible clicks when a voice is turned off while its filter was using a high Q value, see #1427.

This PR fixes that by applying the filter on the interpolated signal but before the volEnv gain is applied. This also matches the processing order of the DSP chain closer to the diagram of section 9.1.8 in the sf2 specification.

fluid_iir_filter_apply() which is actually meant to process an array of samples is now used to process only a single sample. One might consider to define this function in header file to allow the compiler to inline it...

Fixes #1427

@derselbst derselbst added the bug label Dec 7, 2024
@derselbst derselbst added this to the 2.4 milestone Dec 7, 2024
Copy link

sonarqubecloud bot commented Dec 7, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
42.3% Duplication on New Code (required ≤ 5%)

See analysis details on SonarQube Cloud

@spessasus
Copy link
Contributor

fluid_iir_filter_apply() which is actually meant to process an array of samples is now used to process only a single sample.

Won't that hurt the performance? If the function was designed to handle blocks of data, won't calling it repeatedly with only one sample make it slower?

@derselbst
Copy link
Member Author

I don't think so. The function is inherently serial, i.e. it cannot be vectorized. Hence, the most expensive part is reading the samples from memory, which is (hopefully) avoided with the current approach as the sample might be ideally located in a register.

Ofc, you're free to benchmark this.

@derselbst derselbst merged commit e604d9e into master Dec 8, 2024
76 of 79 checks passed
@derselbst derselbst deleted the 1427 branch December 8, 2024 15:42
DominusExult added a commit to DominusExult/fluidsynth-sans-glib that referenced this pull request Dec 27, 2024
* master: (66 commits)
  Add portamento test files
  Restore discovery of libsndfile on Ubuntu (FluidSynth#1454)
  Fix crash on startup when there are no MIDI devices  (FluidSynth#1447)
  Replace VERSIONINFO resource cmake macro (FluidSynth#1449)
  Fix filter processing order in DSP chain (FluidSynth#1444)
  Render the nervous filter with additional soundfonts
  Add Christian's SF2 spec test to regression tests
  Bump testdata ref
  Migrate test files from LFS to submodule
  Delete test files from Git LFS
  Bump to 2.4.1
  Smooth linear filter parameter change (FluidSynth#1432)
  Reenable AWE32 NRPN 23 and 24 (FluidSynth#1430)
  indentation issue
  Disable chorus for Uplift - it's too noisy in the middle part
  Add Klerg's renderings of Altitude and Uplift
  updated cmake build system * debug output for libsndfile, quieter output for pulseaudio * enhanced build summary report * revised building win32 binaries without unicode support
  add cmake output for sndfile issues
  Fix CI builds (FluidSynth#1435)
  Add test files for AWE32 NRPN (FluidSynth#1434)
  ...
derselbst added a commit that referenced this pull request Jan 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

voice with high filter Q killed too early, causing audible click
2 participants