SoundFonts cannot be unloaded if polyphony is ever exceeded

[dwhinham]

FluidSynth version

v2.1.5

Describe the bug

If polyphony is exceeded and FluidSynth has to allocate a voice by calling fluid_synth_free_voice_by_kill_LOCAL(), two problems occur:

• The value returned by fluid_synth_get_active_voice_count() becomes inaccurate - the active voice count never returns back to 0 because the counter is not decremented in the usual way.
• SoundFont samples are not unref'd, and therefore if an attempt is made to unload the SoundFont, the deferred unload timer is started, and fluid_synth_sfunload_callback() unsuccessfully tries to unload the SoundFont forever.

Expected behavior

• fluid_synth_get_active_voice_count() should provide an accurate value even if polyphony was exceeded during the session.
• The SoundFont should successfully unload, either immediately or eventually (via the deferred timer mechanism), if polyphony was exceeded during the session.

Steps to reproduce

• Start FluidSynth and load a SoundFont.
• Set polyphony to a low value (for ease of testing), e.g. 2.
• Play more notes than the polyphony permits.
• Attempt to unload the SoundFont.
• Set a breakpoint inside fluid_synth_sfunload_callback() and notice that it continues to be called repeatedly long after the request was made, even though all voices may be idle.

Additional context

As a quick hack to monitor the active voice count, I added the following code just before the call to fluid_istream_readline(), near line 510 of fluid_cmd.c:

printf("%d\n", fluid_synth_get_active_voice_count(shell->handler->synth));

This is just so I could hit enter and see the result. After polyphony is exceeded, this value never returns to zero.

Thanks for your hard work on FluidSynth!

[derselbst]

Yeah, I know. That bug already exists since at least fluidsynth 1.1.6. The problem is that the call to fluid_voice_off() in fluid_synth_free_voice_by_kill_LOCAL() triggers a delayed fluid_voice_stop() which postpones it until the next rendering call is made, where we're being told that the rvoice has indeed finished, so we can safely call fluid_voice_stop() now (in which all the sample unref is performed). However, fluid_synth_alloc_voice_LOCAL() doesn't wait for the stop, it just calls fluid_voice_init() right away. Within there, this "overflow rvoice" is supposed to handle the premature off, but it seems broken.

During all this time I haven't come up with an idea... this is not trivial to solve. The timer unloading mechanism is garbage as well. And I'm afraid I don't really have motiviation to look into this. @jjceresa is that something for you?

[dwhinham]

Understood - yes; normally I like to try and provide a patch/PR but this one looked very tricky (and I don't know much about Fluid internals).

My apologies if you already knew about it - I can work around the problem for my use case in the meantime. 🙂

[jjceresa]

@dwhinham Thanks for reporting this bug.

This should be fixed in next PR.

[jjceresa]

@dwhinham, Please, could you check that PR #729 on branch fix-issue-727 is Ok for your use case ?.

[dwhinham]

@jjceresa Thankyou! I just tested your PR and it does indeed appear to fix this bug.

The SoundFont is now unloaded as expected and fluid_synth_get_active_voice_count() returns to 0 successfully.

Thankyou for addressing it so quickly! 😀

[derselbst]

It looks better, yes. But I'm observing that I have to send two NoteOffs to make fluidsynth unload the font:

$ fluidsynth -v -la alsa -o synth.polyphony=2 /usr/share/sounds/sf2/FluidR3_GM.sf2
> noteon 0 60 127 # voice count is 2
> unload 1
> noteoff 0 60 # voice count is still 2
> noteoff 0 60 # voice count is 0
fluidsynth: debug: Unloaded SoundFont
fluidsynth: debug: Timer thread finished

It works fine when the unload comes after the noteoff:

> noteon 0 60 127
> noteoff 0 60
> unload 1

[jjceresa]

But I'm observing that I have to send two NoteOffs to make fluidsynth unload the font:

Using FluidR3_GM.sf2 and other sfonts I cannot reproduce this here. After noteoff the voice count is always 0.

Did your observations always true at each try or is it random ?

[jjceresa]

But I'm observing that I have to send two NoteOffs to make fluidsynth unload the font
It works fine when the unload comes after the noteoff

Your are right, and this is normal. Please remember, after sending the first noteoff command (which call the noteoff function API), the voices are finished (and returned to the MIDI side via the FIFO) but the sample's reference count isn't yet decremented. So the sfont remain loaded. We remain in this state until the next call to any synth API function. Then when the next call to a function API occurs, the function API first look for any voices finished via fluid_synth_check_finished_voices() which in turn call fluid_voice_stop() or fluid_voice_overflow_rvoice_finished(). At this point the sample's reference count are decremented which leads the sfont to be unloaded.

So the sequence: noteon 0 60 127, unload 1, noteoff 0 60 is not sufficient to unload the sfont. We must use any command that calls any function API. This command could be a second noteoff command or simply a voice_count command.

[derselbst]

Your are right, and this is normal. We remain in this state until the next call to any synth API function.

Indeed, you're correct. Client apps will most likely make another call to the synth API, so this would be acceptable.

But, let's assume that this "next API call" never happens and instead delete_fluid_synth is being called. I've assumed that the soundfont would be deleted in any case after the synth has been deleted. But it seems this is not always true:

1. The fluid_voice_sample_unref function only unrefs if the sample pointed to is not NULL
2. rvoice->dsp.sample and overflow_rvoice->dsp.sample stay NULL after fluid_voice_init, until someone makes a call to the rendering API where the synth thread assigns a sample with fluid_rvoice_set_sample
3. If noone calls the rendering API (at the right time), dsp.sample always stays NULL.
4. Nothing will ever decrement the sample->refcount
5. Nothing will ever free the soundfont, after the synth has been deleted.

This topic seems important enough to construct some unit tests. I'll see what can be done.

... In an ideal world, we would put all the soundfonts which have been requested for unloading to a
fluid_list soundfonts_to_be_unloaded;. And whenever a sample is unref'ed we would get a callback and see if we can delete any of those fonts. (Without this kind of callback we could just perform that check directly within in fluid_synth_check_finished_voices()). And if we ever reach delete_fluid_synth and still find those soundfonts_to_be_unloaded we could just delete them right away.

[jjceresa]

In an ideal world, .....we could just delete them right away.

yes, understood.

There is another solution that is to make fluid_synth_sfunload_callback(void *data, unsigned int msec) to monitor fluid_synth_check_finished_voices(). This can be done inside fluid_synth_sfunload_callback() by calling this pair of function:
fluid_synth_api_enter(synth); fluid_synth_api_exit(synth);

Inside fluid_synth_sfunload_callback(), this requires to have access to the synth variable through the parameter data which is a pointer to fluid_sfont_t structure. That means that when the timer is created in fluid_synth_sfont_unref(), we need to pass the synth variable to a new field fluid_synth_t *synth in the fluid_sfont_t struct. Unfortunately this requires to add a new field in fluid_sfont_t which seems to be public. This will will break the API.

Do you see another way to pass the synth variable via the parameter data without changing fluid_sfont_t ?

[derselbst]

fluid_synth_sfunload_callback(void *data, unsigned int msec) to monitor fluid_synth_check_finished_voices(). This can be done inside fluid_synth_sfunload_callback() by calling this pair of function:
fluid_synth_api_enter(synth); fluid_synth_api_exit(synth);

No. The synth may have already been deleted. Calling any synth fields from the timer thread would result in a use-after free.

[derselbst]

Ok, I've set up two unit tests on the test-unload branch. This issue is actually bigger than I initially thought. There really is no guarantee the soundfont will ever be unloaded. Thus the branch is based on 2.1.x.

For now, I've added two test-cases:

test_after_polyphony_exceeded() is basically this issue. This function will succeed, if we apply JJCs patch a4ac565 (which is a squashed version of PR #729 and rebased against 2.1.x, found on this branch). But! Notice the number of rendering calls I have to make to really unload the soundfont. Even the number of rendered samples is important, which somewhat surprised me, although when thinking about it, it's plausible.

The second test case is test_without_rendering(). No rendering calls --> no success so far.

So, JJC if you want to continue with this, you may do so on the fix-sfunload branch. If you have already started on the other branch, no problem, I'll rebase it later.

[jjceresa]

No. The synth may have already been deleted.

Yes, of course. Stupid of me!

[jjceresa]

Sorry I have problem trying to pull fix-sfunload branches locally. I got

$ git pull
Auto-merging src/synth/fluid_voice.c
Auto-merging src/synth/fluid_synth.c
Auto-merging src/synth/fluid_event.c
CONFLICT (content): Merge conflict in src/synth/fluid_event.c
Auto-merging include/fluidsynth/event.h
CONFLICT (content): Merge conflict in include/fluidsynth/event.h
Automatic merge failed; fix conflicts and then commit the result.

Same problems trying to pull test-unload branch.

I don't know how to do to obtain the 2 unit tests test_after_polyphony_exceeded() and test_without_rendering() ?.

[jjceresa]

I have some some changes on branch fix-issue-727-1 which is a sub-branch of fix-issue-727. May be could you apply the unit test described above ?

[derselbst]

git pull performs a merge. Perhaps you've chosen the wrong base branch (master instead of 2.1.x). It's ok, just keep working with the branches you have. To apply my unit test, just execute the following command on your branch

git fetch origin
git cherry-pick e67f23eb11e926dadaefc51090f8b5940a1ef7e2

[jjceresa]

Perhaps you've chosen the wrong base branch (master instead of 2.1.x).

yes, I made the wrong git commands. Thanks for your advise.

To apply my unit test, just execute the following command on your branch

Thanks for these useful units tests. Indeed test_without_rendering() shows that we are waiting for soundfont 1 to unload forever after calling delete_fluid_synth() which is a big failure.

I've added these unit tests into fix-issue-727-1 (which is a sub-branch of fix-issue-727). Both tests test_after_polyphony_exceeded() and test_without_rendering() are success.

[derselbst]

I've added these unit tests into fix-issue-727-1. Both tests test_after_polyphony_exceeded() and test_without_rendering() are success.

Looks good, thanks JJC! I'll sleep over it again, maybe I'll come up with another test case. But at the moment it looks safe to me.

[jjceresa]

maybe I'll come up with another test case.

Perhaps, we should add 2 tests case to complete the ones you already provided ?:

• test without polyphony exceeded (same as tests test_after_polyphony_exceeded() but with polyphony keep to default 128).
• test without polyphony exceeded without rendering (same as test_without_rendering() but with polyphony keep to default 128).

[derselbst]

Perhaps, we should add 2 tests case to complete the ones you already provided ?:

Done, see PR #735. I also found another use-after-free bug, which I've fixed as well.

I found that the patch from issue #151 introduced this particular unloading bug when polyphony has been exceeded: 2f3a1a9, that was your patch, JJC :)

However, the "missing next API call issue" from my comment above seems to date back much longer. Looking at the code, even fluidsynth 1.1.4 had a sample!=NULL guard that prevented decrementing the sample refcount when the voice hasn't yet been processed by the DSP loop. I haven't run the new regression test against this old version though... source code has diverged too much.

@dwhinham Thanks for the report!

[dwhinham]

@derselbst My pleasure, I appreciate you and @jjceresa looking into this so quickly.

Thanks again and Happy New Year! 😃

[jjceresa]

I found that the patch from issue #151 introduced this particular unloading bug when polyphony has been exceeded: 2f3a1a9, that was your patch, JJC :)

Right. I remember now that in this patch I forgot to check the case where polyphony has been exceeded. Thanks to @dwhinham
and Happy New Year too.