Fx unit api

[jjceresa]

This PR addresses #669 point 2.1.
It proposes set/get API functions to change/read fx unit parameters.
The deprecated shell commands are updated. Now the commands line have 2 parameters:

• first parameter is the fx unit index.
• second parameter is the value to apply to the fx unit.

[jjceresa]

The following actual API:

• fluid_synth_set_chorusXX(synth, param);
• fluid_synth_get_chorus_XX(synth, param);
• fluid_synth_set_reverbXX(synth, param);
• fluid_synth_get_reverbXX(synth, param);

becomes now redundant with the new one called with unit_idx set to -1:

• fluid_synth_set_chorusXX2(synth, unit_idx,param);
• fluid_synth_get_chorus_XX2(synth,unit_idx, param);
• fluid_synth_set_reverbXX2(synth, unit_idx, param);
• fluid_synth_get_reverb_XX2(synth, unit_idx, param);

That means that actual API should not be called by new application (and perhaps should be deprecated sooner or later ?). Perhaps we could ask to the mail-list if these deprecation could cause any backward issues ?

[sonarcloud]

SonarCloud Quality Gate failed.

4 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
279 Code Smells

0.0% Coverage
0.0% Duplication

[lgtm-com]

This pull request introduces 1 alert when merging a74d792 into e5695b7 - view on LGTM.com

new alerts:

• 1 for Wrong type of arguments to formatting function

[lgtm-com]

This pull request introduces 1 alert when merging 30a1767 into e5695b7 - view on LGTM.com

new alerts:

• 1 for Wrong type of arguments to formatting function

[derselbst]

Be careful with signed shifts: https://sonarcloud.io/project/issues?id=FluidSynth_fluidsynth&open=AXVgLmC8i5dl9RN5Jo-q&pullRequest=673&resolved=false&types=BUG

I would prefer to store both parameters separately.

[derselbst]

Would compilers complain about the following?

No. From a purely technical point of view, I see no reason why casting an enum to double and back would not work. That doesn't mean that it will work fine in all scenarios. Keep in mind that fluidsynth is used by many programming languages. There is no guarantee, that there will a concellation where this does not work.

Apart from that, I feel ok with promoting a single int to double and cast it back. An enum, however, only has a few distinct values. Casting that to double feels like really poor design to me. An enum could also represent bitmasks. Would this survive an intermediate cast to double? I don't know.

Also, this approach would lead to redundant client code:

fluid_synth_chorus_on(synth, 1, true);
fluid_synth_reverb_set_param(synth, 1, FLUID_CHORUS_TYPE, FLUID_CHORUS_MOD_TRIANGLE);

rather than

fluid_synth_chorus_on(synth, 1, FLUID_CHORUS_MOD_TRIANGLE);

I think of it as two separate functions: one that affects the mode of the effect, the other function affecting its effect parameters.

Chorus can be in three different modes: Sinus, Triangle, OFF.

Likewise for reverb: FLUID_REVERB_MOD_ON, FLUID_REVERB_MOD_OFF. If we ever wanted to add another type of reverb it would nicely integrate into this "mode" API function. (We had this discussion in the past, remember: Freeverb vs. FDN reverb)

---

So, if we can't agree on this, we should fall back to separate setter functions. Without the need for an "all-in-one" setter function:

int fluid_synth_set_chorus_group_nr(fluid_synth_t *synth, int fxunit_idx, int nr);
int fluid_synth_set_chorus_group_level(fluid_synth_t *synth, int fxunit_idx, double level);
int fluid_synth_set_chorus_group_speed(fluid_synth_t *synth, int fxunit_idx, double speed);
int fluid_synth_set_chorus_group_depth(fluid_synth_t *synth, int fxunit_idx, double depth_ms);
int fluid_synth_set_chorus_group_type(fluid_synth_t *synth, int fxunit_idx, int type);

[mawe42]

Keep in mind that fluidsynth is used by many programming languages. There is no guarantee, that there will a concellation where this does not work.

So you are saying that there could be platforms or programming languages where a double/float can not accurately represent smaller (<= 16 bit) integer numbers? That would be very unusual, I think... But proving that something does not exist is hard, so I can understand if you want to be on the absolute safe side here. I guess we should move back to separate getters/setters for both effects then.

Also, this approach would lead to redundant client code:

fluid_synth_chorus_on(synth, 1, true);
fluid_synth_reverb_set_param(synth, 1, FLUID_CHORUS_TYPE, FLUID_CHORUS_MOD_TRIANGLE);

I don't agree that this example contains redundant code. I think it's perfectly fine and much cleaner than the alternative. Consider the case when you want to configure a disabled chorus unit: you would have to ensure to remember the mode for later, when the user actually wants to switch it on. Or consider the case when a user wants to switch a chorus unit off and later on again. Before switching it off, you need to save the mode param somewhere only to be able to set it back to the original value.

[mawe42]

One more comment about the double casting: LADSPA does the same, actually. All values, even control values for things that the plugin internally uses as an integer, are passed as floats. Now you could argue that LADSPA is not the nicest interface, but it's still widely used and I quite like it for it's simplicity.

[derselbst]

Or consider the case when a user wants to switch a chorus unit off and later on again. Before switching it off, you need to save the mode param somewhere only to be able to set it back to the original value.

Ok.

LADSPA does the same, actually

I don't want to compare to other interfaces, because I don't know the historical background or the motivation of their developers back then.

Here, we are designing a new interface. And I think it's hard to make decisions here, because it's hard to think of a practical or musical sense for this new interface. I have to admit, that I still only see the pure technical need for this API. I'm still not convinced that it makes sense from a musical perspective, sry.

[mawe42]

I don't want to compare to other interfaces, because I don't know the historical background or the motivation of their developers back then.

Understood, and I agree. I just wanted to point out that there is a precedence for "float-only" interfaces. And it is an interface that I myself am very comfortable with, including the "float-only" aspect.

I have to admit, that I still only see the pure technical need for this API.

Yes, I feel the same, I'm not invested in this feature. If it were up to me, then I think we should hide the fact that there are multiple effect instances used internally (or rather: keep it hidden).

But JJC said he really needs the real-time multi-channel mixing feature and also that he needs to be able to control the effect groups separately. And even though I don't understand why and why in this way, I still think if somebody has a real need, then we should be open to change. But every time I think about both of these changes more generally, I switch from "let's do it" to "I'm really not sure" and back again.

Fact is: we are currently changing or deprecating existing APIs that are used by a lot of people, to satisfy a use-case that so far only one person has expressed an interest in (or even understands why there is a need for it). I really hope it's worth it and that JJC will make good and frequent use of this feature :-)

[jjceresa]

Here, we are designing a new interface. And I think it's hard to make decisions here, because it's hard to think of a practical or musical sense for this new interface.

Yes, I feel the same, I'm not invested in this feature.

To me, having independent control fx reverb/chorus parameter on independent MIDI instruments (each on its MIDI channel) has a real musical sense. This is audible in many music.

I have to admit, that I still only see the pure technical need for this API. I'm still not convinced that it makes sense from a musical perspective,sry

we are currently changing or deprecating existing APIs that are used by a lot of people, to satisfy a use-case that so far only one person has expressed an interest in (or even understands why there is a need for it).

The "lot of people" your refer are impossible to number and never gave input when questioned on the mailing list. I hope they are happy when they change the reverb room-size and this impacts all MIDI instruments at the same time !.

I wouldn't wanna be the only person who breaks habits of lot of people. So far I would prefer to abandon this PR.

[mawe42]

To me, having independent control fx reverb/chorus parameter on independent MIDI instruments (each on its MIDI channel) has a real musical sense. This is audible in many music.

I very much agree with this, using different effects on different MIDI instruments is common, normal, makes musical sense. What I don't quite understand is why you need the solution you are proposing. Why the many other solutions that already exist are not suitable. Even solutions already existing in fluidsynth: instead of the internal reverb and chorus, you could simply use multiple external LADSPA effects and control them interactively via the API to achieve the same effect. Then use a "unity gain amp" plugin to mix your buffers any way you like.

we are currently changing or deprecating existing APIs that are used by a lot of people, to satisfy a use-case that so far only one person has expressed an interest in (or even understands why there is a need for it).

The "lot of people" your refer are impossible to number and never gave input when questioned on the mailing list. I hope they are happy when they change the reverb room-size and this impacts all MIDI instruments at the same time !.

Of course, lot of people is just a guess. But I do think that the reverb and chorus APIs are the more well known parts of fluidsynth. And I could probably name a few high profile programs that make use of it.

I wouldn't wanna be the only person who breaks habits of lot of people. So far I would prefer to abandon this PR.

Don't get me wrong... I'm not saying abandon the idea. It's not up to me anyway. I'm just saying I hope your need for this feature is real. That you will use it frequently and that it is not just a "this would make sense" idea. And these thoughts come into my head because I still (i know... I repeat myself) haven't understood where you are coming from. As Windows has only recently gained multi-channel support (thanks to you!), the use-case must be fairly new. So I'm wondering much of it is based in the real world and how much of it is a solution looking for a problem.

[jjceresa]

very much agree with this, using different effects on different MIDI instruments is common, normal, makes musical sense. What I don't quite understand is why you need the solution you are proposing. Why the many other solutions that already exist are not suitable. Even solutions already existing in fluidsynth: instead of the internal reverb and chorus, you could simply use multiple external LADSPA effects and control them interactively via the API to achieve the same effect. Then use a "unity gain amp" plugin to mix your buffers any way you like.

The solution you propose is to get rid of internal functionalities that already exist and replace these by external effects. For now I would prefer application the more simple as possible. This gives me more time as I prefer playing on piano rather then computer keyboard.

As Windows has only recently gained multi-channel support (thanks to you!), the use-case must be fairly new.

This PR has nothing to do with audio multi-channels, and corresponds only to the real need already expressed in the first message.

[mawe42]

This PR has nothing to do with audio multi-channels, and corresponds only to the real need already expressed in the first message.

Sorry, but the first message does not express a need, it simply states what you want to change... not why. The only thing that could be taken as a motivation for this PR is the link to #669. And that issue is about the multi-channel real-time control you proposed. So I assumed that this PR here is simply the ground work for #669.

Ok, so you are saying that you need to control separate fx per MIDI channel even if the real-time multi-channel feature does not make it into FluidSynth. I hadn't understood that until now.

[jjceresa]

So I assumed that this PR here is simply the ground work for #669.

This PR is the ground work for #669 point 2.1 only. #669 point 1 is addressed by PR 672.
For clarity, I should have to split this issue 669 in two. Also 669 issue title (that I cannot update anymore) doesn't reflect point 2. Sorry.

[jjceresa]

So, if we can't agree on this, we should fall back to separate setter functions. Without the need for an "all-in-one" setter function

The all-in-one setter (param enum) function is really a nice idea as it allows to add parameters in the future without requiring new API. For example the reverb unit actually has only a "late stage" but we could add the missing "early stage" controlled by new enum values.

To avoid double/int values casting, I proposed a compromise to split the interface like this:

• fluid_synth_xxxx_set_param_num( ..,int param, double value)
• fluid_synth_xxxx_set_param_int(..,int param, int value)).
• fluid_synth_xxxx_get_param_num( ..,int param, double value)
• fluid_synth_xxxx_get_param_int(..,int param, int value)).
  Opinions ?.

[mawe42]

Thanks JJC, also a good approach. And similar to the way we currently handle the synth settings.

Even though Tom already said choices by other people are of no concern to us, I think its still interesting to see how other people have approached this problem.

• LADSPA: uses floats for all parameters, see fluid_ladspa_effect_set_control().
• VST2(and 3?): also only use floats, but with a twist: here it is always 32-bit float params in the range 0.0 - 1.0.
• LV2: uses unions containing many different types, including public factory methods to create the correct union instance from a basic type.

But of course they have to use something generic because they can't know what people might want as parameters in their plugins. We know exactly which parameters our two effects accept.

So, my preference would be (in order):

1. generic setters/getters with floats
2. JJC new suggestion with typed generic setters/getters
3. The current style of specific setters/getters and one all-in-one getter/setter.

Options 2. and 3. can easily switch places for me, though. It depends on how likely it is that we will add or change the parameters of the internal effects. If it's not likely, then 3. would switch place with 2.

[derselbst]

My problem with

fluid_synth_xxxx_set_param_num( ..,int param, double value)
fluid_synth_xxxx_set_param_int(..,int param, int value)).
fluid_synth_xxxx_get_param_num( ..,int param, double value)
fluid_synth_xxxx_get_param_int(..,int param, int value)).

is that people need to look up the enum param and see which typed function to use. I, personally, consider this to be uncomfortable. However, if we really wanted to do this, we could also use a variable argument list:

int fluid_synth_xxxx_set_param(synth, int param, ...);

Depending on param, variable argument number one will be treated as int or double. libtiff actually uses that approach: TIFFGetField()

[jjceresa]

My problem with
fluid_synth_xxxx_set_param_num( ..,int param, double value)
fluid_synth_xxxx_set_param_int(..,int param, int value)).
is that people need to look up the enum param and see which typed function to use.

No because, I forgot to mention that there would be also 2 distincts param enum type.
enum fluid_xxxx_param_num
{
....
};

enum fluid_xxxx_param_int
{
....
};

Anyway, the enum param idea requires the user to look for the function name first then to the typed enum, which is still a bit uncomfortable compared to actual specific function for each param. I think that it worth to:
1)come back using actual specific function for each param which is easier to use (less parameters) and clean API.
2)Internally we can keep the "enum param- all-in-one" setter function (not public ofc), because, (a) it allows to check param values range, (b) it requires little overload when called from public API, (c) It makes quite easy the addition of future specific function for new param.

[derselbst]

I think that it worth to come back using actual specific function for each param

Ok.

[derselbst]

Should be good now. Waiting for the CI to complete. Thanks!

[jjceresa]

Waiting for the CI to complete. Thanks!

Thanks for reviewing.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
22 Code Smells

0.0% Coverage
0.0% Duplication