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

volume envelope delay is not to spec #1312

Open
mrbumpy409 opened this issue Mar 29, 2024 · 1 comment · May be fixed by #1314
Open

volume envelope delay is not to spec #1312

mrbumpy409 opened this issue Mar 29, 2024 · 1 comment · May be fixed by #1314
Labels
Milestone

Comments

@mrbumpy409
Copy link

FluidSynth's volume envelope delay does not behave according to the SoundFont specification. Currently, FluidSynth starts sample playback at the start of the attack phase rather than the start of the envelope generator. Section 9.1.7 of the SoundFont 2.04 specification defines the envelope generator as such:

[...] An envelope generates a control signal in six phases. When key-on occurs, a delay period begins during which the envelope
value is zero. The envelope then rises in a convex curve to a value of one during the attack phase. [...]

Nowhere in the document is it suggested that sample playback should start at the attack phase, and as far as I have seen, only FluidSynth behaves in this manner. In every other SoundFont synth I have tried (including Sound Blaster hardware and also other sampler formats such as SFZ), the sample is actually silently playing during the volume envelope's delay phase.

Now, here is where I have to caveat that I actually prefer FluidSynth's method here, as I have wished for the envelope delay to actually delay the start of sample playback since I started building SoundFonts back in the late '90s. From a sound design perspective, it actually makes more sense to do it this way. How often do I want to delay a sound and lose the start of the sample at the same time? If it weren't for that darned compatibility factor, I'd argue in favor of keeping things as-is. In fact, I'm kind of hating myself right now for reporting this! 😆

Here is an audio comparison of a legacy preset that becomes problematic with FluidSynth's delay method, the Roland GS preset 008:014 Church Bells, here demonstrated using GeneralUser GS. The sound you hear is a single press of the note C4. Sobanth plays this correctly, with each of the subsequent echoing bells quieter and more mellow due to the attack phase occurring later in the bell waveform. On Fluidsynth, you clearly hear each bell hit identically as each voice's attack phase is starting at the beginning of the sample.

The preset could be reprogrammed to sound more natural in FluidSynth, but that would break the playback in all other SoundFont synths. And this is the reason why—as much as it pains me to say it—we should consider changing the delay envelope behavior to match the spec.

Thoughts?

FluidSynth version

Any, as far as I know.

@mrbumpy409 mrbumpy409 added the bug label Mar 29, 2024
@derselbst
Copy link
Member

An excellent finding Christian! I was already afraid your report will cause me days of headache. But after a quick look in the code things are looking quite simple: When a voice is processed and found to be in delay, it will trigger this if clause:

if(fluid_adsr_env_get_section(&voice->envlfo.volenv) == FLUID_VOICE_ENVDELAY)
{
return -1; /* The volume amplitude is in hold phase. No sound is produced. */
}

And the -1 will cause an early return here, i.e. without updating the voice's dsp and related params down below:

count = fluid_rvoice_calc_amp(voice);
if(count <= 0)
{
return count; /* return -1 if voice is quiet, 0 if voice has finished */

So I'd guess all we need to do is to move that early return down a bit. How far down? I'd guess right before the switch case with the interpolation calls would be fine?

Doing so would mean there's no more a way to truly delay the sample in the soundfont itself. Other than delaying the noteOn. I agree that is limiting musically freedom and might be questionable. But if the spec says so ;)

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.

2 participants