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

reorder fx mixing #1117

Closed
wants to merge 2 commits into from
Closed

reorder fx mixing #1117

wants to merge 2 commits into from

Conversation

albedozero
Copy link
Contributor

This PR moves mixing of LADSPA effects in fluid_rvoice_mixer_process_fx() before mixing of the internal reverb and chorus effects.

Currently, if a user sets synth.audio-groups >1, LADSPA effects can be applied to specific MIDI channels only. The user can then follow @mawe42's great suggestion from the mailing list and use a zero-gain amp effect to mix the LADSPA buffers back to the first host port for output to the sound card (This seems like the most proper way to do this IMO - the LADSPA subsystem shouldn't have to figure out what FluidSynth's audio output channels are on its own).

However, since the internal reverb and chorus are applied first and don't care about synth.audio-groups, the LADSPA effects will appear in the internal effects buffers regardless of which audio_group the currently-playing MIDI channels are part of. Increasing synth.effects-groups is not ideal since it adds a lot of processing load, and also would seem to require that effects-groups always be equal to audio-groups. A user could disable the internal reverb and chorus and add them as LADSPA effects, but it's nice to be able to have backing instruments or tracks from MIDI files play "normally" (i.e. according to the soundfonts' reverb and chorus settings), while a user plays lead with interesting effects that don't alter the backing voices.

I don't think this PR alters the way the buffers are set up, since that's done by fluid_rvoice_mixer_set_ladspa(), and my tests with ALSA and JACK on Linux seem to work fine, but feel free to close if this breaks something I don't understand.

mix ladspa fx buffers before internal reverb and chorus
@derselbst
Copy link
Member

@mawe42 You are more familiar with LADSPA than I. Would it be safe to have it in the next bugfix release or could it break anything so that we should better postpone it to 2.3.0 ?

@mawe42
Copy link
Member

mawe42 commented Jun 19, 2022

I don't think it would break anything, but I do thing that it's a behavioural change that is more than a bugfix. And there might be valid use-cases for both behaviours (LADSPA running before or after the internal effects).

Hm... a safe way to change this might be to introduce (yet another) config switch that controls if when the LADSPA effects are called and set the default to the current behaviour.

Alternatively, maybe we could extend the LADSPA commands in such a way that it would be possible to call the internal chrous and reverb? That way the user would have total control over if and when the internal effects should affect the output.

@albedozero
Copy link
Contributor Author

I was hoping @mawe42 you'd have some thoughts about why/what use-cases for which LADSPA effects are mixed in after the internal effects, but I agree it is a behavior change that might produce different sound output for some users. I could add a precedence flag to fluid_rvoice_mixer_t that would tell fluid_rvoice_mixer_process_fx() whether to mix them before or after internal effects, and a synth.ladspa.precedence setting that defaults to zero (=mix after internal effects) - would that make sense?

Your second option might be a bit beyond my skill, but also another way to give the user that kind of control would be to follow the second part of your suggestion from the mailing list and just disable the internal effects and use tap_reverb and tap_chorus (or similar) in the LADSPA effects chain. If this pull doesn't get merged I can just do that myself to get around the issue.

Incidentally, consider this my +1 for keeping the LADSPA subsystem in- it's great to be able to add custom effects on a per-midi-channel basis within FluidSynth itself, as opposed to the overhead of sending multi-channel audio output to an external host.

@mawe42
Copy link
Member

mawe42 commented Jun 19, 2022

I could probably construct some use-cases where it would be useful to have the internal effects run before the LADSPA effects chain. But they would be exactly that: constructed use-cases with no real foundation. The LADSPA effects were already running after the internal effects when I rewrote the LADSPA subsystem, so I'm not quite sure why it was set up that way. I personally never used LADSPA and internal effects togehter, only one or the other.

Even with moving LADSPA before internal effects, I don't think it would fully solve your use-case (assuming I have actually understood your goal). From what I understand, you wanted most channels to run with default effects as intended by the Soundfont designer, but have a few select channels with your own special effects. The problem that I see is that the LADSPA system currently only provides a single Reverb:Send and Chorus:Send port. So if you keep the internal effects on or if you put LADSPA reverb and chorus effects on the Reverb|Chrous:Send ports, the input to reverb and chorus would contain all channels, even the selected special ones.

So maybe instead of changing the precedent of LADSPA effects, we should make the LADSPA interface more powerful by honoring the effects-groups setting. Just like audio-groups affects how many Main:L and Main:R ports get created, effects-channels could affect how many Reverb:Send and Chorus:Send ports get created.

That way you could disable internal effects, set effects-channels = audio-groups = 16 and then copy all "normal" effect sends into two temporary buffers, which would then act as the input to the LADSPA reverb and chorus effects. The "special" channels would get their special treatment and could either also be copied into the temp buffers or directly to an output channel, as desired.

And maybe, instead of having to do this copying with 0-gain amp plugins, we could add a new LADSPA command that does this: ladspa_copy for example.

Hm... but then again: is that really worth it? Setting up effects chains like this would be quite involved, lots of typing and probably too difficult to understand for most users. If you want such an elaborate setup, wouldn't it be much easier to use FluidSynth in a multi-channel setup and use something like Carla to setup all this with a nice graphical interface and even have the possibility of controlling the effects via MIDI signals?

@albedozero
Copy link
Contributor Author

albedozero commented Jun 20, 2022

In my case I'm actually fine with the internal reverb and chorus effects being applied to all MIDI channels/audio_groups, even those that have LADSPA effects connected to their host ports. The problem is in setups where the effects are mixed into the internal dry audio buffers (mix_fx_to_out = TRUE e.g. ALSA, JACK with multi=0), the LADSPA effects are audible even if the instrument playing doesn't have them connected to its host ports.

Example with synth.audio-groups=2, audio.driver=alsa

  • Rhodes is on MIDI channel 0 with tap_echo connected to Main:L1/R1
  • Clavinet is on MIDI channel 1 with no LADSPA effects
    Sending notes on MIDI channel 1 will cause the Clavinet to play, with no immediate echo since the voice is in the buffer for Main:L2/R2, but the reverb/chorus effect from the Clav is already mixed into all the LADSPA buffers and thus gets processed by tap_echo on Main:L1/R1, so the Clav's reverb has an unwanted echo effect added to it.

This seems like buggy behavior to me. I'd wager the reason LADSPA effects are mixed in after internal effects is that it usually doesn't matter - it only causes an issue when trying to have multiple audio groups for LADSPA effects but mix them back to a single stereo audio output. I agree it seems like it would be good to maybe modify fluid_rvoice_mixer_set_ladspa() to respect synth.effects-groups and add additional Reverb:Send and Chorus:Send channels, but that's not the problem I was trying to solve with this PR. A ladspa_copy function might be useful, but for now I've created a dirt-simple LADSPA effect patchcord.so that just copies audio from one port to another and has run_adding, and it works great for mixing the audio down to the first host port.

As for skipping all this and just using JACK with Carla or something similar - I acknowledge that's a way to do things, but I've put in a lot of time creating a wrapper for FluidSynth so it can easily be run as a headless sound module, with all functions being managed from a MIDI controller. I'm biased, but I think my system for allowing the user to set up LADSPA fx chains is pretty slick, and it does allow you to control ports and connections using MIDI messages! That's why I'm trying to leverage the LADSPA subsystem like this - it works really well to be able to handle everything within FluidSynth on an embedded system like the Raspberry Pi.

@sonarcloud
Copy link

sonarcloud bot commented Jun 20, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mawe42
Copy link
Member

mawe42 commented Jun 20, 2022

Thanks, that makes your use-case and the reasoning behind this PR pretty clear. And fluidpatcher looks like quite a nifty tool!

I guess I was overthinking this, so lets keep it simple: I don't know any use-case where running the internal effects before LADSPA would be important. So a +1 from me for this change.

And yes, one could consider the current processing order a bug, which would make this change ok for a bugfix release. On the other hand... it's also a documented behaviour, which might be a reason to make this a feature change in 2.3.0. @derselbst Your call?

@albedozero
Copy link
Contributor Author

Thanks @mawe42. I'd also be happy to add a synth.ladspa.precedence flag if that's desired. It'd be easy if it doesn't have to be real-time, but doable either way.

@mawe42
Copy link
Member

mawe42 commented Jun 20, 2022

I would be happy with this change without precedence flag. We could add a flag if somebody comes forward with a use-case for the original ordering :-)

@derselbst
Copy link
Member

I have no preference on whether to mix it before or after. I would just apply your commits to master. And if somebody complains we can still think about whether it's worth introducing an extra setting for it.

@derselbst derselbst closed this in 57554ee Jun 27, 2022
@albedozero albedozero deleted the reorder-fx-mixing branch June 27, 2022 17:48
derselbst added a commit that referenced this pull request Sep 21, 2022
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.

None yet

3 participants