[PATCH]: Adding Sostenuto code

[derselbst]

This a patch for Fluidsynth v 1.1.6, to: 1) Fix a minor bug. 2) Add code to support Sostenuto pedal. All is explained in Sostenuto.pdf. Please See attachement.

Reported by: jjceresa

Original Ticket: fluidsynth/tickets/136

[derselbst]

Hi, and many thanks for your patch as well as your detailed pdf description!

I'm attaching a unified diff with all the files in one. (If you're not using the git version control system, you can construct one using this command "diff -Nur old_directory new_directory".)

Original comment by: diwic

[derselbst]

I have never used sostenuto myself. Looking at the soundfont spec, it seems to specify sostenuto as CC 67 instead of CC 66. After looking around a bit, it seems like this is just typo in the soundfont spec and that CC 66 is the correct one.

I also read the following: "All Notes Off - When received with any data value, all notes playing in the key-on state immediately enter release phase, pending their status in SUSTAIN or SOSTENUTO state." I think this means that Sustain/Sostenuto should still be respected when this CC is sent? It looks like fluid_synth_all_notes_off_LOCAL() should remain calling fluid_voice_noteoff, not fluid_voice_release.

Original comment by: diwic

[derselbst]

Hi, thanks for useful tips about diff.

Original comment by: jjceresa

[derselbst]

Hi, David

it seems like this is just typo in the soundfont spec and that CC 66 is >the correct one. After reading MIDI specifications on the Web, sostenuto seems to be CC 66. Furthermore, after trying CC 66 on/off on other synthesizer, it reacts as expected.

It looks like fluid_synth_all_notes_off_LOCAL() should remain calling >fluid_voice_noteoff, not fluid_voice_release. After other reading, I think you are right, "All notes Off" MIDI message behaves like "noteOff" messages on alls notes actually sounding.So fluid_synth_all_notes_off_LOCAL() should remain calling fluid_voice_noteoff.(In fact, i was confused by "All Sounds Off" message !).

So, take this new patch (see attachement) that must one

Please see attached file : 0002-Add-sostenuto-pedal-functionality.patch.zip that replace the previous one (0001-Add-sostenuto-pedal-functionality.patch.zip).

Original comment by: jjceresa

[derselbst]

Please ignore this, as it is just a test to see how notifications works.

Original comment by: jjceresa

[derselbst]

Thanks, the new patch is much more readable than the old one!

I have another question though. The construct of adding a new variable (has_released_same_note) to fluid_synth_t seems wrong to me. Sostenuto functionality is never per synth, but per channel or per note/voice. I think this has something to do with dual noteOn events without a noteOff inbetween, but could you motivate why this is needed? Maybe one could instead use some local variable to get the information from where it's calculated to fluid_voice_init instead of using something that looks like a global synth state?

Original comment by: diwic

[derselbst]

Sostenuto functionality is never per synth, but per channel or per note/voice. Of course, Sostenuto functionality is per channel (i.e alls voices for the note on a channel intended to be sustained by sostenuto on this channel).

I think this has something to do with dual noteOn events without a noteOff inbetween, but could you motivate why this is needed?

Really, "has_released_same_note" has to do ONLY with Sostenuto.

"has_released_same_note" has something to do with Specification 4 (here extracted from FluidSostenuto.pdf chapter 1.2.1)

"Specification 4: When a note is currently sustained by sostenuto, playing a same note stop the previous note and start a new note. The new note will keep a sustained state (even if Sostenuto pedal is depressed before the new note)."

Note that stopping a previous sustained note and restart a new note is useful no matter if the note was sustained by sostenuto or sustain.

The detection of a note "currently sustained by sostenuto" is done by fluid_synth_release_voice_on_same_note_LOCAL(), at fluid_synth_noteon_LOCAL() time. The note currently sustained (by sustain or sostenuto) is stopped and when the note was sustained by sostenuto it is memorized in "has_released_same_note" to be passed later to the local variable in fluid_voice_t by fluid_voice_init(). The local variable will be used later on noteOff to keep a sustained state (even if Sostenuto pedal is depressed before the new note).

Maybe one could instead use some local variable to get the information from where it's calculated to fluid_voice_init instead of using something that looks like a global synth state? May be an other way to pass this parameter would be to move "has_released_same_note" from fluid_synth_t to fluid_channel_t

That seems more readable as "has_released_same_note" is a sostenuto functionality per channel.

The following patch (0003-Add-sostenuto-pedal-functionality.zip) implement this and is a replacement of the previous one (0002-Add-sostenuto-pedal-functionality.patch.zip).

Original comment by: jjceresa

[derselbst]

Hi, David Please ignore 0003-Add-sostenuto-pedal-functionality.zip and take the new one 0004-Add-sostenuto-pedal-functionality.zip.

This new patch is a more better implementation of specification 4 without the need of "has_released_same_note variable".

See attachement.

Original comment by: jjceresa

[derselbst]

Thanks, are we missing the 0004-Add-sostenuto-pedal-functionality.zip attachment? I see only a pdf document.

Original comment by: diwic

[derselbst]

Oups!, here is 0004-Add-sostenuto-pedal-functionality.zip.

Original comment by: jjceresa

[derselbst]

Your code has now been committed - with some changes - to FluidSynth. Thanks for the contribution!

Apart from trivial fixes (whitespace, spelling fixes etc) there were two functional changes backed out, because they did not seem to make sense to me:

1) fluid_synth_stop_local still calls fluid_voice_noteoff. fluid_synth_stop is an API function not related to any midi specification. It is uncertain whether any application depends on the current behaviour, so this remains the way it is out of carefulness.

2) there was a line removed by your patch: "&& (fluid_voice_get_id(voice) != synth->noteid))" in fluid_synth_release_voice_on_same_note_LOCAL. The check needs to be there as explained in the comment above the function: "Note: One noteon event can trigger several voice processes, for example a stereo sample. Don't release those...". I hope this does not disturb your sostenuto functionality in any way.

Original comment by: diwic

[derselbst]

Here's a link to the commit: https://sourceforge.net/p/fluidsynth/code-git/ci/048c51c4ab55fc8bbf04816ff8b85c93fd238b5f/

Original comment by: diwic

[derselbst]

• status: new --> closed
• Resolution: --> fixed

Original comment by: diwic

[derselbst]

1> It is uncertain whether any application depends on the current behaviour ... Yes, i understand it is very important to remain compatible.

2> The presence of this expression does not disturb sostenuto functionality. Simply at fluid_synth_release_voice_on_same_note_LOCAL() time , any curently running voice have an id that is always lower than synth->noteid. So the expression will be always true. Note this remark is ONLY valid because fluid_synth_release_voice_on_same_note_LOCAL() is call only once (at noteOn time). Anyway if i am wrong, it is prudent that the expression remains.

Thanks for your energy to maintain this software. Good afternoon !

Original comment by: jjceresa

[derselbst]

If you want, we can remove string "fluid_synth_stop_LOCAL()" from this comment.

+/** fluid_voice_release

• Force the voice into release stage. Usefuf anywhere a voice
• needs to be damped even if pedals (sustain sostenuto) are depressed.
• See fluid_synth_damp_voices_LOCAL(), fluid_synth_damp_voices_by_sostenuto_LOCAL,
• fluid_voice_noteoff(), fluid_synth_stop_LOCAL(). +*/

Original comment by: jjceresa