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

In a modulator, "No Controller" as Src1 shouldn't cancel Src2 #1068

Closed
davy7125 opened this issue Mar 8, 2022 · 12 comments · Fixed by #1392
Closed

In a modulator, "No Controller" as Src1 shouldn't cancel Src2 #1068

davy7125 opened this issue Mar 8, 2022 · 12 comments · Fixed by #1392
Labels
Milestone

Comments

@davy7125
Copy link

davy7125 commented Mar 8, 2022

FluidSynth version

FluidSynth runtime version 2.2.5
Copyright (C) 2000-2022 Peter Hanappe and others.
Distributed under the LGPL license.
SoundFont(R) is a registered trademark of Creative Technology Ltd.

FluidSynth executable version 2.2.5
Sample type=double

Describe the bug

I wanted to create a modulator using two CC values to compute an output, this output driving the fine tuning parameter. However it wasn't working and I discovered that fluidsynth accepts CC values in the first input of a modulator only, not the second one.

With Polyphone, here is the working configuration:

OK

This one is not ok:

KO

This could be also a bug in Polyphone when creating the file but this is unlikely since internally, both inputs of a modulator have the same structure (SFModulator):

structure

Steps to reproduce

Here is a minimal soundfont showing the problem, you will need a keyboard that can change value of CC20 to listen to the result:
example.zip

Changing CC20 can increase preset named "OK" by an octave, but preset named "KO" is not modified by CC20.

@davy7125 davy7125 added the bug label Mar 8, 2022
@davy7125
Copy link
Author

davy7125 commented Mar 8, 2022

I made other tests and it appears that using a trick, changing "no controller" by "Note-on velocity", the CC value specified at the second input is taken into account:

OK with a trick

So maybe the previous modulator, using "no controller" as input 1 and "CC" as input 2, has badly been rejected by Fluidsynth (bug 1).

And the final configuration I wanted to have is:

link

Changing CC18 to CC20 has no impact on the fine tuning (bug 2). Maybe the linked modulators are not implemented in Fluidsynth? I read some discussions on GitHub but this is not clear to me.

@jjceresa
Copy link
Collaborator

jjceresa commented Mar 8, 2022

That is a very old bug that was fixed on my branch and I completely forgot to mention this. Sorry.

The bug is in fluid_mod.c-fluid_mod_get_value(). The computation of src1 line is not identical to the computation of src2.
I don't know if there is a special reason to consider src1 different than src2.

To fix this bug:
1)fluid_mod.c-fluid_mod_get_value()-Line 456: should be (return 1.0f) instead of (return 0.0f).

2)There are a dependency in fluid_defsfont.c
fluid_defsfont.c-Line range [1605 to 1615] should be replaced by to Line range [1627 to 1637]. And of course src2 , flags2 should be replaced by src1,flags1 respectively.

@jjceresa
Copy link
Collaborator

jjceresa commented Mar 8, 2022

More notes not related to this issue:

In fluid_mod.c-fluid_mod_get_value().

  1. Line 404 fluid_real_t v1 = 0.0, v2 = 1.0; // This initialization seems useless

  2. A typo in fluid_mod.c- L211 "// an invalid portamento fromkey should be treated as 0 when it's actually used for moulating"

@jjceresa
Copy link
Collaborator

jjceresa commented Mar 8, 2022

@davy7125 My first message is related to what you call bug1.

I made other tests and it appears that using a trick, changing "no controller" by "Note-on velocity", the CC value specified at the second input is taken into account:

Yes, the bug1 is: selecting "no controller" to input 1" cancels the effect of input2 . Anything different than "no controller" to input 1 , the value specified at the second input (GC or CC) is taken into account

Maybe the linked modulators are not implemented in Fluidsynth?
Right, linked modulators are not implemented in fluidsynth master. And what you call the bug2 is probably still the bug1 that comes back.

@jjceresa
Copy link
Collaborator

jjceresa commented Mar 8, 2022

1)fluid_mod.c-fluid_mod_get_value()-Line 456: should be (return 1.0f) instead of (return 0.0f).

Oups, I made another typo mistake. 1)fluid_mod.c-fluid_mod_get_value()-Line 456: should be (v1 = 1.0f) instead of (return 0.0f)

@davy7125 davy7125 changed the title CC value as Src Amount input in modulators In a modulator, "No Controller" as Src1 cancels Src2 Mar 9, 2022
@davy7125
Copy link
Author

davy7125 commented Mar 9, 2022

Thank you for your fast and detailed answer. I changed the title of this ticket to match the actual behavior: "no controller" as source 1 shouldn't cancel source 2 in a modulator.

Right, linked modulators are not implemented in fluidsynth master. And what you call the bug2 is probably still the bug1 that comes back.

Do you confirm that:

  • solving the issue with source 1 will enable it to be driven by another modulator (enabling thus a link), or
  • solving the issue with source 1 will not allow it to be driven by another modulator since linked modulators are not implemented?

Maybe you are speaking of an intermediate state: in the previous example with a linked modulator, the input 1 that is "Modulator 2" in modulator 1 should have been considered to be "1" since linked modulators are not implemented, enabling thus the use of CC 18 to modify the fine tuning but disabling CC 19 and CC 20.

not implemented in fluidsynth master: is it implemented somewhere else? I would be very interested.

@jjceresa
Copy link
Collaborator

jjceresa commented Mar 9, 2022

I changed the title of this ticket to match the actual behavior: "no controller" as source 1 shouldn't cancel source 2 in a modulator.

That is the right title. Actually "no controller" as source 2 (amount source) behaves like 1.0f enabling the use of source 1.
But "no controller" as source 1 behaves like 0.0f disabling the use of source 2. The behaviour isn't the same on both input and this is very confusing to my taste this is why I already fixed that on my local branch as described on my previous message.

solving the issue with source 1 will not allow it to be driven by another modulator since linked modulators are not implemented.

Confirmed. Linked modulators are not implemented in master.

not implemented in fluidsynth master: is it implemented somewhere else?

I started a branch 'linked-modulators' but it is not finished. If you are interested you must be aware of lack of detail in the actual SF specs. For example if the output of a modulator 2 is connected to input 1 of modulator 1 (your example above), then the input 1 mapping stage (unipolar/bipolar, type of curve) cannot be taking account and should be bypassed. In others word only CCs source can be mapped but not 'Output modulators source`.

@jjceresa
Copy link
Collaborator

jjceresa commented Mar 9, 2022

I changed the title of this ticket to match the actual behavior: "no controller" as source 1 shouldn't cancel source 2 in a modulator.

Please you forgot "shouldn't" in the title.

@davy7125 davy7125 changed the title In a modulator, "No Controller" as Src1 cancels Src2 In a modulator, "No Controller" as Src1 shouldn't cancel Src2 Mar 9, 2022
@davy7125
Copy link
Author

davy7125 commented Mar 9, 2022

Maybe this discussion is interesting for specifications:
davy7125/polyphone#133 (comment)

And possibly how I implemented it also. Here is a modulator:
https://github.com/davy7125/polyphone/blob/master/sources/sound_engine/parametermodulator.cpp
And here is a list of modulators in a same division (of an instrument or preset, global or not):
https://github.com/davy7125/polyphone/blob/master/sources/sound_engine/modulatorgroup.cpp

When the output of a modulator A is added to input 1 of another modulator M, the possible range [minA ; maxA] is also sent.
If another modulator B adds its value to input 1 of the same modulator M, modulator M computes the full range of its input 1 which will be [minA + minB ; maxA + maxB]. Then this input is normalized between 0 and 1 and it is possible to map it according to the curve we want (unipolar / bipolar / decreasing / increasing / etc).

This is maybe an interpretation of the specifications that needs more discussion but this system is quite handy when linking modulators: the ranges are under control.

@derselbst
Copy link
Member

I just committed an incomplete fix as suggested by JJC: ef1f409

It's incomplete, because

  1. it's missing necessary changes in the sfloader, and
  2. it potentially breaks all the optimizations fluidsynth performs in fluid_voice_get_lower_boundary_for_attenuation().
  1. is not trivial to solve. I would really like to see a few unit tests for this function before we're making any changes to it. That's a big tasks. There are probably many situations to consider, and modulators as well. So, I don't think we will have this bug fixed in 2.2.6 already. It may even take until 2.3.0. Any contributions for unit tests around this function are welcome!

@derselbst derselbst added this to the 2.3 milestone Mar 15, 2022
@derselbst
Copy link
Member

I must admit that I'm lacking motivation and time to fix this properly. Couldn't we just detect this situation upon loading the SF and silently swap the first and second modulators? The worst I can think of is that someone uses the voice API to get the modulators of a voice and then finds that mod1 and mod2 have been swapped. I don't consider this to be a problem though.

@derselbst
Copy link
Member

Coming back to this, because we got a related bug report. Previously, the issue observed by Davy was caused by this code:

/* When primary source input (src1) is set to General Controller 'No Controller',
output is forced to 0.0
*/
else
{
return 0.0;
}
/* no need to go further */
if(v1 == 0.0f)
{
return 0.0f;
}

As JJC has explained earlier, it caused src2 to be not being processed. After digging through the spec again this behavior is incorrect. Also, there is no such special handling that it should return zero if both sources are set to "No Controller".

I've created a PR. I am not entirely sure whether it would cause problems for fluid_voice_get_lower_boundary_for_attenuation().

@derselbst derselbst linked a pull request Oct 3, 2024 that will close this issue
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 a pull request may close this issue.

3 participants