Implement per-channel fluid_synth_all_notes_off when seeking/stopping player

[albedozero]

I'm working on an app that uses FluidSynth to play backing tracks from a MIDI file on some channels while the user can also trigger notes on other channels with an attached MIDI controller. The features added to fluid_player are great for this, particularly the ability to seek within a midi file, which makes it possible to set up loop points, jump to verse/chorus, etc. However, seeking calls fluid_synth_all_sounds_off(synth, -1), which creates an audible click as all playing sounds are cut off without being allowed to complete their release phase, and also stops any notes the user is playing on other MIDI channels.

I propose two things:

• Use fluid_synth_all_notes_off above instead, so that notes can release smoothly when seeking.
• Since fluid_synth_all_notes_off can already be called for specific channels, add a way of setting in the player which channels should receive an all_notes_off when seeking or stopping the player.

This involves adding an array of type int to _fluid_player_t and a function to the API for setting the array in a player object. I've created PR #980 that demonstrates/accomplishes this. The default behavior remains the same - if the user doesn't set the array, its initial value is -1, and all channels receive the all_notes_off call.

[derselbst]

Ok for me in general.

Use fluid_synth_all_notes_off above instead, so that notes can release smoothly when seeking.

Pls. test how the behavior is when notes in the backing tracks are sustained: I.e. NoteOn, press sustain pedal, seek to a location where
a) the sustain pedal is no longer depressed, and
b) the sustain pedal is still depressed.

There shouldn't be any hanging notes in a), but for b) I'm not entirely sure.

[mawe42]

Ok, I could have read the source before commenting. It seems we do exactly that, go though the file from the beginning and play everything apart from note on/off until the point we seek to. Sorry for the noise.

[albedozero]

Can do @derselbst - I find there are hanging notes in b), also sometimes a), but also there are hanging notes if the player is stopped when the sustain pedal is down, because it also calls fluid_synth_all_notes_off - this doesn't seem to be desired behavior. If there was a function that called fluid_voice_release for all the voices on the requested channel it seems like it would deal with the hanging notes, but since that's not the behavior of All Notes Off or All Sounds Off according to the soundfont spec (9.2), perhaps a function fluid_synth_all_notes_release could be added to fluid_synth.c?

Here's the rundown of what I found using this test_mid.zip file, included for reference (I could try to whip up some example C++ code too if that helps).

Seek from a point in the song where playing notes have been released (i.e. noteoff) but sustain pedal is depressed so that notes continue to sound, e.g. from tick 6200, to
a) point where sustain pedal has been released (tick 4200) - no hanging notes
b) point where sustain pedal is being held (tick 2300) - sustained notes continue to hang
c) point in song were sustain pedal is released but has never been pressed (CC64 has never changed - tick 0) - hanging notes
d) player is stopped at any point when sustain pedal is down - hanging notes

@mawe42 your comment actually made me look at the code a bit closer to figure out what was going on in c), so noise appreciated :)

[albedozero]

With the latest commits to #980 adding fluid_synth_all_notes_release, the same tests as in my last reply give no hanging notes for a), b), or d). Seeking back to a point in the song before the sustain pedal has ever been pressed or released c) still produces hanging notes, because there is no CC64=0 message in the track up to that point for the player to use to set the sustain pedal state, so it remains at what it was. This seems like it would be rare/edge case however, and could be gotten around by programmatically sending the sustain pedal release message right after seeking, or putting a CC64=0 message in the MIDI file at the beginning of the song.

[mawe42]

The problem with c) could be handled by always issuing an "All Controllers Off" message before starting the seek. However... I think I remember us discussing if we should issue a controller reset between songs and decided against it. But I can't remember why, to be honest.

[derselbst]

I don't think that we need a fluid_synth_all_notes_release, because the client is free to "unpress" sustain or sostenuto pedal via fluid_synth_cc. Or ControllersOff as Marcus said. I'll try to find some time next weekend to have a closer listen to you provided example MIDI.

I think I remember us discussing if we should issue a controller reset between songs and decided against it. But I can't remember why, to be honest.

IIRC it would break seamless looping, or so. On the other hand, we could also simply risk it and see if somebody complains. But let's first investigate the "clicks". Then we can think about how the player should behave when stopping and when seeking.

[mawe42]

Just one more general thought about what you want to achieve. If you don't stop notes immediately before the seek but let them transition into their release phase and die out, you might get weird effects for sounds with a long-ish release phase, especially if they where affected by any controllers. Say a glockenspiel with a long release tail, which the song pitched down using a controller. You seek to a different position in the song, arrive at a different controller value and your (still audible) glockenspiel release phase suddenly jumps to a new pitch.

It seems to me that letting the sounds go into release instead of cutting them off opens up quite a few new edge cases.

[derselbst]

Is there a reason to send an ALL_NOTES_OFF CC when stopping instead?

I think it's because the fluid_player_stop is actually a misnamed "pause" function. And when pressing the pause button it seemed more natural for a MIDI player to send out ALL_NOTES_OFF. I have no strong preference though. We could probably live with ALL_SOUND_OFF as well. @mawe42 ?

[mawe42]

If fluid_player_stop is really just a pause, and if the recommended way to implement a proper stop is to fluid_player_stop(); fluid_player_seek(0), then I would vote to leave stop as ALL_NOTES_OFF and make seek use ALL_SOUNDS_OFF and ALL_CTRL_OFF.

[albedozero]

If fluid_player_stop is really just a pause, and if the recommended way to implement a proper stop is to fluid_player_stop(); fluid_player_seek(0), then I would vote to leave stop as ALL_NOTES_OFF

Even if fluid_player_stop is a pause, I feel like most users expect it should halt sound from the player, even if the sustain pedal is active at that moment, without having to additionally call fluid_player_seek(0). This setup also makes it cumbersome if a user really does just want to pause playback and recommence from the same spot - they have to call fluid_player_stop, record the current tick position, call fluid_player_seek(0) to silence any sustained notes, then to recommence call fluid_player_seek(tick) to get the player back where it was and finally fluid_player_play().

and make seek use ALL_SOUNDS_OFF and ALL_CTRL_OFF.

@mawe42 I think you suggested ALL_CTRL_OFF above as a way to solve hanging notes in case c) where a player did a seek from a point where the sustain pedal is active to a point earlier in the song where it has never been pressed, and fluid_track_send_events thus has no CC64=0 event to process. But, the code I was testing was sending notes_off when seeking - if we're sending ALL_SOUND_OFF then controls_off shouldn't be needed to solve that problem. In fact, it might reset controllers that the user has set for specific reasons - I'm thinking of MIDI_VOLUME in particular, but there could be others.

So, I feel like sending ALL_SOUND_OFF when stopping produces the more expected behavior, and refraining from sending ALL_CTRL_OFF when seeking gives the user more flexibility.

[albedozero]

Another problem I realized with ALL_CTRL_OFF is that when seeking forward this would change the current state of the player, so fluid_track_send_events() would have to be rewritten to read from the beginning of the track whether seeking forward or backward.

Let me know what you think either way - I won't argue any more 🙂

[derselbst]

So, I feel like sending ALL_SOUND_OFF when stopping produces the more expected behavior, and refraining from sending ALL_CTRL_OFF when seeking gives the user more flexibility.

Agreed. Let's do it that way: ALL_SOUND_OFF for both: stop and seek. And no ALL_CTRL_OFF for now. That should be good enough. I'll give your PR a closer review this weekend.