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

Maximum excursion of modulators is incorrect #1073

Closed
rianhunter opened this issue Mar 22, 2022 · 6 comments · Fixed by #1152
Closed

Maximum excursion of modulators is incorrect #1073

rianhunter opened this issue Mar 22, 2022 · 6 comments · Fixed by #1152
Labels
Milestone

Comments

@rianhunter
Copy link

rianhunter commented Mar 22, 2022

FluidSynth version

FluidSynth runtime version 2.1.7
Copyright (C) 2000-2021 Peter Hanappe and others.
Distributed under the LGPL license.
SoundFont(R) is a registered trademark of E-mu Systems, Inc.

FluidSynth executable version 2.1.7
Sample type=double

Describe the bug

In SF 2.04 specification, section 8.4.4 it says:

The MIDI Continuous Controller 1 data value is used as a Positive Unipolar source; thus the input value of 0 is mapped to a value of 0, an input value of 127 is mapped to 127/128 and all other values are mapped between 0 and 127/128 in a linear fashion. The MIDI Continuous Controller 33 data value may be optionally used for increased resolution of the controller input.

...

The amount of this modulator is 50 cents/max excursion of vibrato modulation.

The product of these values is passed through a Linear Transform (or is left uninhibited) and is added to the Vibrato LFO to Pitch generator summing node.

I take this to mean that when the CC#1 has a value of 127, then the total value of the modulator should be 50 * 127/128. Instead 127 is used as the range and results in the value of the modulator being 50 * 127/127, which is just 50.

fluid_real_t range1 = 127.0, range2 = 127.0;

Expected behavior

I expect the value of the modulator to be 50 * 127 / 128 when the value of CC#1 is 127.

@rianhunter rianhunter added the bug label Mar 22, 2022
@jjceresa
Copy link
Collaborator

jjceresa commented Apr 1, 2022

Instead 127 is used as the range and results in the value of the modulator being 50 * 127/127, which is just 50.

Right. The CC value source in the range [0 (min) ..127max] need to be normalized to the range [0..1.0f] for unipolar (or [-1.0f..+1.0f]) for bipolar). Please, have a look to the spec chapter 9.5.1 p 53 fig 3 and fig 4. and pay attention to the sentence:

"The SoundFont controller model is NOT designed to accommodate the MIDI controller data values, rather the MIDI controller values should be translated to accommodate the SoundFont controller model.".

@rianhunter
Copy link
Author

rianhunter commented Apr 1, 2022

Right. The CC value source in the range [0 (min) ..127max] need to be normalized to the range [0..1.0f] for unipolar (or [-1.0f..+1.0f]) for bipolar). Please, have a look to the spec chapter 9.5.1 p 53 fig 3 and fig 4. and pay attention to the sentence:

I get that but how do you reconcile that with the language in 8.4.4? It specifically states that 127 is mapped to 127/128 in the positive unipolar map, whereas the normal max for the positive unipolar map is 1.0f as you said.

@jjceresa
Copy link
Collaborator

jjceresa commented Apr 1, 2022

how do you reconcile that with the language in 8.4.4?

In 8.4.4, having a modulator that provides at its output a 50 cents/max excursion of vibrato modulation when the CC is maximum is only possible if we follow what the specs says in chapter 9.5.1 p 53 fig 3 and fig 4.

When a SF2 sound designer/musician is editing a modulator and chooses an amount value of X cents, it is likely he expects that any SF2 synthesizer will be able to reproduce a max excursion of X cents if the musician sets the CC to the max position.

@rianhunter
Copy link
Author

In 8.4.4, having a modulator that provides at its output a 50 cents/max excursion of vibrato modulation when the CC is maximum is only possible if we follow what the specs says in chapter 9.5.1 p 53 fig 3 and fig 4.

In my interpretation the "max excursion" is the reference value used by the positive unipolar curve, i.e. the value it scales. It seems plausible that the max excursion is not intended to be attainable with this CC given the reference to 127/128. Otherwise I don't see why they would reference 127/128 at all. At the very least it seems like this behavior is ill-defined.

When a SF2 sound designer/musician is editing a modulator and chooses an amount value of X cents, it is likely he expects that any SF2 synthesizer will be able to reproduce a max excursion of X cents if the musician sets the CC to the max position.

I originally believed this to be the case until I investigated how other MIDI synths handle this CC. From the few I surveyed, the MIDI CC value of 127 maps to 127/128 * 50 ~= 49.6 cents vibrato depth.

@derselbst
Copy link
Member

Sorry for the delay. I now had a chance to look into this.

The wording of the other default modulators also refers to 127/128. And the table in section 9.5.3 suggests, that this mapping should be applied to all unipolar and bipolar mappings respectively.

Nice finding, Rian!

However, I consider this change too intrusive and little beneficial for fluidsynth 2.2.x. But we could have it in fluidsynth 2.3.0, though.

One would have to implement a few unit tests to assert the expected behavior before actually changing the implementation. Special care has to be taken for PAN and Balance modulators, as they are implemented according to MIDI RP-036... And while thinking about it, it seems that using 127/128 with a CC of 64 would indeed result in a value of 63.5, i.e. the exact center of the range [0;127], possibly making the RP-036 workaround redundant, wouldn't it?

In any case, all those issues should be thoroughly unit-tested.

@rianhunter
Copy link
Author

Thanks for taking a look! I consider FluidSynth a reference implement of SF 2.04 so just wanted to bring it to your guys attention because I wasn't sure if I was going crazy. The table on 9.5.3 does clarify things for me.

I don't expect this change to be made lightly since it seems like the code has been this way for quite a few years so I'll leave it up to you how to change it. This will have a material effect on how FluidSynth renders MIDIs but qualitatively a small one.

@derselbst derselbst added this to the 2.3 milestone Jun 22, 2022
@derselbst derselbst changed the title Potentially Incorrect Mapping of CC#1 to Vibrato LFO Pitch Depth Maximum excursion of modulators is incorrect Jun 22, 2022
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