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

ModLfoToVolume incorrect - the volume doesn't rise #1374

Closed
spessasus opened this issue Sep 4, 2024 · 16 comments · Fixed by #1377
Closed

ModLfoToVolume incorrect - the volume doesn't rise #1374

spessasus opened this issue Sep 4, 2024 · 16 comments · Fixed by #1377
Labels
Milestone

Comments

@spessasus
Copy link
Contributor

FluidSynth version

Execute fluidsynth --version and provide the output.

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

FluidSynth executable version 2.3.6
Sample type=double

Describe the bug

A bit of backstory:

I've been working on making my own synth work perfectly with spec, and I've found this blogpost by Christian Collins. He also linked to his test files for testing the synths. And he also provided a rendered audio using a hardware Audigy 2 ZS.

I've ran the test in fluidsynth and it worked great, except for one part: modLfoToVolume:
image
As you can see, fluidsynth's audio is strange in this section, compared to Audigy.

From the soundfont spec:

modLfoToVolume
This is the degree, in centibels, to which a full scale excursion of the Modulation
LFO will influence volume. A positive number indicates a positive LFO excursion
increases volume; a negative number indicates a positive excursion decreases
volume. Volume is always modified logarithmically, that is the deviation is in
decibels rather than in linear amplitude. For example, a value of 100 indicates that
the volume will first rise ten dB, then fall ten dB.

So it should rise 10 dB, then fall 10 dB. Fluid only does the latter (possibly the initialAttenuation clamping to 0?)

I've managed to replicate fluidsynth's behavior in my code and it seems like:
image

  • Red is the triangle wave that should be used
  • Blue is the wave fluidsynth uses for some reason

As you can see, it ignores the negative values which should result in volume increase, like Audigy does.

Expected behavior

Fluidsynth should rause the volume when the modLfoToVol generator is used.

Steps to reproduce

  1. get the test files
  2. fluidsynth testv2_01.sf2 testv2_01.mid -F fluid_test.wav -o synth.gain=1
  3. compare the two

Additional context

I think this might have to do something with initialAttenuation being capped at 0 cB, thus ignoring negative values.

Also, modLfoToPitch is described the same (rises, then falls) and it works fine.

@spessasus spessasus added the bug label Sep 4, 2024
@spessasus spessasus changed the title ModLfoToVol violates the specification - the volume doesn't raise ModLfoToVolume incorrect - the volume doesn't rise Sep 4, 2024
@derselbst derselbst added this to the 2.4 milestone Sep 8, 2024
@derselbst
Copy link
Member

derselbst commented Sep 8, 2024

Thanks for the bug hunting. I can confirm this.

I have disabled the range checking for initialAttenuation here:

case GEN_ATTENUATION:
voice->attenuation = x;
/* Range: SF2.01 section 8.1.3 # 48
* Motivation for range checking:
* OHPiano.SF2 sets initial attenuation to a whooping -96 dB */
fluid_clip(voice->attenuation, 0.f, 1440.f);

Doing so will make test case 4 break. It has no influence on test case 5.

However, I'm observing that fluid_cb2amp() is being called with many negative values.

``` fluidsynth: fluid_cb2amp() was negative! -1.392901 fluidsynth: fluid_cb2amp() was negative! -2.785802 fluidsynth: fluid_cb2amp() was negative! -4.178703 fluidsynth: fluid_cb2amp() was negative! -5.571603 fluidsynth: fluid_cb2amp() was negative! -6.964504 fluidsynth: fluid_cb2amp() was negative! -8.357405 fluidsynth: fluid_cb2amp() was negative! -9.750305 fluidsynth: fluid_cb2amp() was negative! -11.143206 fluidsynth: fluid_cb2amp() was negative! -12.536107 fluidsynth: fluid_cb2amp() was negative! -13.929008 fluidsynth: fluid_cb2amp() was negative! -15.321908 fluidsynth: fluid_cb2amp() was negative! -16.714809 fluidsynth: fluid_cb2amp() was negative! -18.107712 fluidsynth: fluid_cb2amp() was negative! -19.500612 fluidsynth: fluid_cb2amp() was negative! -20.893515 fluidsynth: fluid_cb2amp() was negative! -22.286415 fluidsynth: fluid_cb2amp() was negative! -23.679317 fluidsynth: fluid_cb2amp() was negative! -25.072218 fluidsynth: fluid_cb2amp() was negative! -26.465120 fluidsynth: fluid_cb2amp() was negative! -27.858023 fluidsynth: fluid_cb2amp() was negative! -29.250923 fluidsynth: fluid_cb2amp() was negative! -30.643824 fluidsynth: fluid_cb2amp() was negative! -32.036724 fluidsynth: fluid_cb2amp() was negative! -33.429623 fluidsynth: fluid_cb2amp() was negative! -34.822521 fluidsynth: fluid_cb2amp() was negative! -36.215424 fluidsynth: fluid_cb2amp() was negative! -37.608322 fluidsynth: fluid_cb2amp() was negative! -39.001221 fluidsynth: fluid_cb2amp() was negative! -40.394123 fluidsynth: fluid_cb2amp() was negative! -41.787022 fluidsynth: fluid_cb2amp() was negative! -43.179920 fluidsynth: fluid_cb2amp() was negative! -44.572823 fluidsynth: fluid_cb2amp() was negative! -45.965721 fluidsynth: fluid_cb2amp() was negative! -47.358620 fluidsynth: fluid_cb2amp() was negative! -48.751518 fluidsynth: fluid_cb2amp() was negative! -50.144421 fluidsynth: fluid_cb2amp() was negative! -51.537319 fluidsynth: fluid_cb2amp() was negative! -52.930218 fluidsynth: fluid_cb2amp() was negative! -54.323120 fluidsynth: fluid_cb2amp() was negative! -55.716019 fluidsynth: fluid_cb2amp() was negative! -57.108917 fluidsynth: fluid_cb2amp() was negative! -58.501820 fluidsynth: fluid_cb2amp() was negative! -59.894718 fluidsynth: fluid_cb2amp() was negative! -58.712383 fluidsynth: fluid_cb2amp() was negative! -57.319481 fluidsynth: fluid_cb2amp() was negative! -55.926582 fluidsynth: fluid_cb2amp() was negative! -54.533684 fluidsynth: fluid_cb2amp() was negative! -53.140785 fluidsynth: fluid_cb2amp() was negative! -51.747883 fluidsynth: fluid_cb2amp() was negative! -50.354984 fluidsynth: fluid_cb2amp() was negative! -48.962086 fluidsynth: fluid_cb2amp() was negative! -47.569183 fluidsynth: fluid_cb2amp() was negative! -46.176285 fluidsynth: fluid_cb2amp() was negative! -44.783386 fluidsynth: fluid_cb2amp() was negative! -43.390484 fluidsynth: fluid_cb2amp() was negative! -41.997585 fluidsynth: fluid_cb2amp() was negative! -40.604687 fluidsynth: fluid_cb2amp() was negative! -39.211784 fluidsynth: fluid_cb2amp() was negative! -37.818886 fluidsynth: fluid_cb2amp() was negative! -36.425987 fluidsynth: fluid_cb2amp() was negative! -35.033085 fluidsynth: fluid_cb2amp() was negative! -33.640186 fluidsynth: fluid_cb2amp() was negative! -32.247288 fluidsynth: fluid_cb2amp() was negative! -30.854387 fluidsynth: fluid_cb2amp() was negative! -29.461485 fluidsynth: fluid_cb2amp() was negative! -28.068584 fluidsynth: fluid_cb2amp() was negative! -26.675682 fluidsynth: fluid_cb2amp() was negative! -25.282782 fluidsynth: fluid_cb2amp() was negative! -23.889879 fluidsynth: fluid_cb2amp() was negative! -22.496979 fluidsynth: fluid_cb2amp() was negative! -21.104076 fluidsynth: fluid_cb2amp() was negative! -19.711174 fluidsynth: fluid_cb2amp() was negative! -18.318274 fluidsynth: fluid_cb2amp() was negative! -16.925371 fluidsynth: fluid_cb2amp() was negative! -15.532471 fluidsynth: fluid_cb2amp() was negative! -14.139569 fluidsynth: fluid_cb2amp() was negative! -12.746669 fluidsynth: fluid_cb2amp() was negative! -11.353768 fluidsynth: fluid_cb2amp() was negative! -9.960868 fluidsynth: fluid_cb2amp() was negative! -8.567967 fluidsynth: fluid_cb2amp() was negative! -7.175066 fluidsynth: fluid_cb2amp() was negative! -5.782166 fluidsynth: fluid_cb2amp() was negative! -4.389265 fluidsynth: fluid_cb2amp() was negative! -2.996364 fluidsynth: fluid_cb2amp() was negative! -1.603463 fluidsynth: fluid_cb2amp() was negative! -0.210563 ```

It's called from here:

* fluid_cb2amp(FLUID_PEAK_ATTENUATION * (1.0f - fluid_adsr_env_get_val(&voice->envlfo.volenv))
+ fluid_lfo_get_val(&voice->envlfo.modlfo) * -voice->envlfo.modlfo_to_vol);

I need to investigate...

@derselbst
Copy link
Member

derselbst commented Sep 8, 2024

Test case 4 is generally broken on recent master. I need to investigate.

The ModToLFO can be fixed by allowing negative values in fluid_cb2amp(). I need to check the spec and the history, if negative attenuation values are actually allowed. In the meantime, you can try the issue1374 branch.

Edit: Negative values are only forbidden for the initial attenuation. But as the spec clearly says, that the volume will rise, we need to permit negative values in this case. I'll open a PR, however, for upcoming fluidsynth 2.4, as this is an audible change and I would like to avoid audible changes for 2.3.

@mrbumpy409
Copy link

mrbumpy409 commented Sep 9, 2024

@spessasus: This issue is made more confusing by an oversight in the old version of my test. I have created a new test suite with an update to the LFO test (test no. 5) that properly accounts for the difference in volume modulation of attenuated vs. unattenuated voices. Essentially, each voice has a 0 dB ceiling, which—at least on Sound Blaster Hardware—also means the volume LFO cannot push the voice volume beyond this point. Whether or not FluidSynth should emulate this restriction is up for debate.

Please read my notes for both tests no. 5 and no. 12 and have a look at the provided audio renderings for Audigy 2 ZS and FluidSynth 2.3.6. Here is test no. 5 compared between the two:

image

FluidSynth 2.3.6 is correct for test 5A (left), but incorrect for 5B (right). What does this test result look like after patch #1377?

@spessasus
Copy link
Contributor Author

@spessasus: This issue is made more confusing by an oversight in the old version of my test. I have created a new test suite with an update to the LFO test (test no. 5) that properly accounts for the difference in volume modulation of attenuated vs. unattenuated voices. Essentially, each voice has a 0 dB ceiling, which—at least on Sound Blaster Hardware—also means the volume LFO cannot push the voice volume beyond this point. Whether or not FluidSynth should emulate this restriction is up for debate.

Please read my notes for both tests no. 5 and no. 12 and have a look at the provided audio renderings for Audigy 2 ZS and FluidSynth 2.3.6. Here is test no. 5 compared between the two:

FluidSynth 2.3.6 is correct for test 5A (left), but incorrect for 5B (right). What does this test result look like after patch #1377?

Hi Christian,
thanks for the updated tests! Though Audigy caps out at 0 dB attenuation, the spec doesn't mention anything about a 0 dB cap. The modLfoToVolume says that the volume will raise, no exceptions are specified.

So as you've said, it's up to debate to choose either to emulate hardware or stick to spec. Either way, the current behavior is incorrect (always capping out at 0 dB, regardless of initial attenuation).

PS: A bit off topic, but I've read your 2016 blogpost and your results were very detailed. I'm currently working on a soundfont synth myself, could you run it through your tests? I ran it myself and it sounded fine, but I don't have a whole lot of experience with music or a particularly good pair of headphones so I'd really appreciate that :-)

@mrbumpy409
Copy link

mrbumpy409 commented Sep 9, 2024

I agree that a 0dB cap for the LFO is not only not needed, but a bit weird when you think about it. Clamping to 0 dB for the attenuation values (as FluidSynth currently does) makes sense for compatibility reasons, but I don't think that is needed for the LFO.

Regarding your SoundFont synth, I would love to test it out! I'm also about to release a major GeneralUser GS upgrade which will be a good test for any SoundFont synth.

@spessasus
Copy link
Contributor Author

spessasus commented Sep 9, 2024

I agree that a 0dB cap for the LFO is not only not needed, but a bit weird when you think about it. Clamping to 0 dB for the attenuation values (as FluidSynth currently does) makes sense for compatibility reasons, but I don't think that is needed for the LFO.

Regarding your SoundFont synth, I would love to test it out! I'm also about to release a major GeneralUser GS upgrade which will be a good test for any SoundFont synth.

Thanks, Christian! About my synth: it's called SpessaSynth. I'd really appreciate you testing its features :-) And looking forward to your update to GeneralUser, my synth uses it as the default, so it will definitely improve the sound!

PS: About the LFO, I agree with you, doing it the proper way makes more sense than emulating Audigy. It makes more sense spec-wise and sounds better (at least to my ears).

@spessasus
Copy link
Contributor Author

@spessasus: This issue is made more confusing by an oversight in the old version of my test. I have created a new test suite with an update to the LFO test (test no. 5) that properly accounts for the difference in volume modulation of attenuated vs. unattenuated voices. Essentially, each voice has a 0 dB ceiling, which—at least on Sound Blaster Hardware—also means the volume LFO cannot push the voice volume beyond this point. Whether or not FluidSynth should emulate this restriction is up for debate.

Please read my notes for both tests no. 5 and no. 12 and have a look at the provided audio renderings for Audigy 2 ZS and FluidSynth 2.3.6. Here is test no. 5 compared between the two:

image

FluidSynth 2.3.6 is correct for test 5A (left), but incorrect for 5B (right). What does this test result look like after patch #1377?

One more question, what happens if you leave the initialAttenuation at 0dB but halve the sample's gain (i.e. multiply every sample by 0.5 so the sine wave ranges from -0.5 to 0.5 rather than -1 to 1? Because this 0dB ceiling looks like just clipping. If the quieter sample does get louder like the normal sample with attenuation, this means that there's no ceiling; audigy tried to make the sample louder but it simply got clipped.

@mrbumpy409
Copy link

You can verify that it is not clipping by zooming into the waveform. Here, you can see that none of the sine wave peaks are clipped:
image

@derselbst
Copy link
Member

derselbst commented Sep 17, 2024

FluidSynth 2.3.6 is correct for test 5A (left), but incorrect for 5B (right). What does this test result look like after patch #1377?

I'll look like the one at the bottom.

Screenshot_20240917_111748

fluid_master_1374.zip

To me it seems that #1377 would correct the behavior for 5b, but instead mess up 5a?

@derselbst
Copy link
Member

PS: The broken test case 4 I mentioned earlier as well as similar audible glitches found on recent master (and also in my previous audio sample) should be solved by #1381.

@spessasus
Copy link
Contributor Author

spessasus commented Sep 17, 2024

To me it seems that #1377 would correct the behavior for 5b, but instead mess up 5a?

Remember: 5a on audigy is not a defined behavior. Sf spec doesn't mention any 0dB ceiling or explicitly state that you can't make a sample louder under ant condition. So technically fluid passes both. The 0dB ceiling is just another undocumented Feature of hardware (like the 0.4 attenuation multiplier).

If fluid's goal is to emulate hardware however, then yes, it does fail.

@derselbst
Copy link
Member

5a on audigy is not a defined behavior. [...] So technically fluid passes both.

True. So I guess #1377 would be fine then. I'll wait a bit for Christian to reply or intervene.

@mrbumpy409
Copy link

mrbumpy409 commented Sep 17, 2024

I have no issue with the LFO being able to amplify the voice above 0 dB. I always found it weird that an instrument with LFO-to-volume would sound different depending on the amount of voice attenuation. This is one of those Creative hardware limitations that I don't think needs to be emulated.

The only argument I can think of for keeping the Audigy behavior would be the case where somebody provides a high dB value for the LFO, expecting that dB 0 cap to keep it from becoming painful to listen to. I'm not aware of any such cases out in the wild, though.

@derselbst
Copy link
Member

Ok, thanks for the feedback. I once again ran the test suite and it now sounds quite fine to me. There are minor differences audible in test case 14 in how the IIR filter behaves. If someone feels that's worth investigating, pls file another issue. Really nice test suite Christian! I'll go ahead and merge the PR now.

@mrbumpy409
Copy link

Ok, thanks for the feedback. I once again ran the test suite and it now sounds quite fine to me. There are minor differences audible in test case 14 in how the IIR filter behaves. If someone feels that's worth investigating, pls file another issue. Really nice test suite Christian! I'll go ahead and merge the PR now.

Please do not change anything about FluidSynth's filter implementation. It is perfect. :)

@derselbst
Copy link
Member

Ok. FYI: #1345

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