Properly handle overlapping notes when using fluid_event_note() (#637)

src/CMakeLists.txt

@@ -204,6 +204,7 @@ set ( libfluidsynth_SOURCES
     midi/fluid_midi_router.c
     midi/fluid_midi_router.h
     midi/fluid_seqbind.c
+    midi/fluid_seqbind_notes.cpp
     midi/fluid_seq.c
     midi/fluid_seq_queue.cpp
     drivers/fluid_adriver.c

src/midi/fluid_seq.c

@@ -593,3 +593,13 @@ fluid_sequencer_process(fluid_sequencer_t *seq, unsigned int msec)
     fluid_seq_queue_process(seq->queue, seq, seq->cur_ticks);
     fluid_rec_mutex_unlock(seq->mutex);
 }
+
+
+/**
+ * @internal
+ * only used privately by fluid_seqbind and only from sequencer callback, thus lock aquire is not needed.
+ */
+void fluid_sequencer_invalidate_note(fluid_sequencer_t *seq, fluid_seq_id_t dest, fluid_note_id_t id)
+{
+    fluid_seq_queue_invalidate_note_private(seq->queue, dest, id);
+}

src/midi/fluid_seq_queue.cpp

@@ -22,6 +22,7 @@
 
 #include <deque>
 #include <algorithm>
+#include <limits>
 
 /*
  * This is an implementation of an event queue, sorted according to their timestamp.
@@ -186,6 +187,34 @@ void fluid_seq_queue_remove(void *que, fluid_seq_id_t src, fluid_seq_id_t dest,
     }
 }
 
+void fluid_seq_queue_invalidate_note_private(void *que, fluid_seq_id_t dest, fluid_note_id_t id)
+{
+    seq_queue_t& queue = *static_cast<seq_queue_t*>(que);
+
+    seq_queue_t::iterator event_to_invalidate = queue.end();
+    unsigned int earliest_noteoff_tick = std::numeric_limits<unsigned int>::max();
+
+    for (seq_queue_t::iterator it = queue.begin(); it != queue.end(); it++)
+    {
+        if((it->dest == dest) &&
+        (it->type == FLUID_SEQ_NOTEOFF) &&
+        (it->id == id) &&
+        (it->time < earliest_noteoff_tick))
+        {
+            earliest_noteoff_tick = it->time;
+            event_to_invalidate = it;
+        }
+    }
+
+    if(event_to_invalidate != queue.end())
+    {
+        // Invalidate the event, by setting invalidating its destination.
+        // Instead, removing the event from the queue would mess up the heap structure. We would need to
+        // make_heap again, which costs time, etc...
+        event_to_invalidate->dest = -1;
+    }
+}
+
 static void fluid_seq_queue_pop(seq_queue_t &queue)
 {
     std::pop_heap(queue.begin(), queue.end(), event_compare);

src/midi/fluid_seq_queue.h

@@ -28,12 +28,14 @@ extern "C" {
 #endif
 
 #include "fluid_event.h"
+#include "fluid_seqbind_notes.h"
 
 void* new_fluid_seq_queue(int nbEvents);
 void delete_fluid_seq_queue(void *queue);
 int fluid_seq_queue_push(void *queue, const fluid_event_t *evt);
 void fluid_seq_queue_remove(void *queue, fluid_seq_id_t src, fluid_seq_id_t dest, int type);
 void fluid_seq_queue_process(void *que, fluid_sequencer_t *seq, unsigned int cur_ticks);
+void fluid_seq_queue_invalidate_note_private(void *que, fluid_seq_id_t dest, fluid_note_id_t id);
 
 int event_compare_for_test(const fluid_event_t* left, const fluid_event_t* right);
 

src/midi/fluid_seqbind.c

@@ -31,6 +31,7 @@
 #include "fluid_synth.h"
 #include "fluid_midi.h"
 #include "fluid_event.h"
+#include "fluid_seqbind_notes.h"
 
 /***************************************************************
 *
@@ -43,9 +44,11 @@ struct _fluid_seqbind_t
     fluid_sequencer_t *seq;
     fluid_sample_timer_t *sample_timer;
     fluid_seq_id_t client_id;
+    void* note_container;
 };
 typedef struct _fluid_seqbind_t fluid_seqbind_t;
 
+extern void fluid_sequencer_invalidate_note(fluid_sequencer_t *seq, fluid_seq_id_t dest, fluid_note_id_t id);
 
 int fluid_seqbind_timer_callback(void *data, unsigned int msec);
 void fluid_seq_fluidsynth_callback(unsigned int time, fluid_event_t *event, fluid_sequencer_t *seq, void *data);
@@ -68,6 +71,7 @@ delete_fluid_seqbind(fluid_seqbind_t *seqbind)
         seqbind->sample_timer = NULL;
     }
 
+    delete_fluid_note_container(seqbind->note_container);
     FLUID_FREE(seqbind);
 }
 
@@ -138,12 +142,21 @@ fluid_sequencer_register_fluidsynth(fluid_sequencer_t *seq, fluid_synth_t *synth
         }
     }
 
+    seqbind->note_container = new_fluid_note_container();
+    if(seqbind->note_container == NULL)
+    {
+        delete_fluid_sample_timer(seqbind->synth, seqbind->sample_timer);
+        FLUID_FREE(seqbind);
+        return FLUID_FAILED;
+    }
+
     /* register fluidsynth itself */
     seqbind->client_id =
         fluid_sequencer_register_client(seq, "fluidsynth", fluid_seq_fluidsynth_callback, (void *)seqbind);
 
     if(seqbind->client_id == FLUID_FAILED)
     {
+        delete_fluid_note_container(seqbind->note_container);
         delete_fluid_sample_timer(seqbind->synth, seqbind->sample_timer);
         FLUID_FREE(seqbind);
         return FLUID_FAILED;
@@ -177,24 +190,63 @@ fluid_seq_fluidsynth_callback(unsigned int time, fluid_event_t *evt, fluid_seque
         break;
 
     case FLUID_SEQ_NOTEOFF:
+    {
+        fluid_note_id_t id = fluid_event_get_id(evt);
+        if(id != -1)
+        {
+            fluid_note_container_remove(seqbind->note_container, id);
+        }
         fluid_synth_noteoff(synth, fluid_event_get_channel(evt), fluid_event_get_key(evt));
-        break;
+    }
+    break;
 
     case FLUID_SEQ_NOTE:
     {
-        unsigned int dur;
-        fluid_synth_noteon(synth, fluid_event_get_channel(evt), fluid_event_get_key(evt), fluid_event_get_velocity(evt));
-        dur = fluid_event_get_duration(evt);
-        fluid_event_noteoff(evt, fluid_event_get_channel(evt), fluid_event_get_key(evt));
-        fluid_sequencer_send_at(seq, evt, dur, 0);
+        unsigned int dur = fluid_event_get_duration(evt);
+        short vel = fluid_event_get_velocity(evt);
+        short key = fluid_event_get_key(evt);
+        int chan = fluid_event_get_channel(evt);
+
+        fluid_note_id_t id = fluid_note_compute_id(chan, key);
+
+        int res = fluid_note_container_insert(seqbind->note_container, id);
+        if(res == FLUID_FAILED)
+        {
+            goto err;
+        }
+        else if(res)
+        {
+            // Note is already playing ATM, the following call to fluid_synth_noteon() will kill that note.
+            // Thus, we need to remove its noteoff from the queue
+            fluid_sequencer_invalidate_note(seqbind->seq, seqbind->client_id, id);
+        }
+        else
+        {
+            // Note not playing, all good.
+        }
+
+        fluid_event_noteoff(evt, chan, key);
+        fluid_event_set_id(evt, id);
+
+        res = fluid_sequencer_send_at(seq, evt, dur, 0);
+        if(res == FLUID_FAILED)
+        {
+            err:
+            FLUID_LOG(FLUID_ERR, "seqbind: Unable to process FLUID_SEQ_NOTE event, something went horribly wrong");
+            return;
+        }
+
+        fluid_synth_noteon(synth, chan, key, vel);
     }
     break;
 
     case FLUID_SEQ_ALLSOUNDSOFF:
+        fluid_note_container_clear(seqbind->note_container);
         fluid_synth_all_sounds_off(synth, fluid_event_get_channel(evt));
         break;
 
     case FLUID_SEQ_ALLNOTESOFF:
+        fluid_note_container_clear(seqbind->note_container);
         fluid_synth_all_notes_off(synth, fluid_event_get_channel(evt));
         break;
 

src/midi/fluid_seqbind_notes.cpp

@@ -0,0 +1,115 @@
+/* FluidSynth - A Software Synthesizer
+ *
+ * Copyright (C) 2019  Tom Moebert and others.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301, USA
+ */
+
+#include "fluid_seqbind_notes.h"
+
+#include <set>
+
+/*
+ * This is a hash container allows us to detect overlapping notes, by storing a bunch of unique integers,
+ * that allow us to track noteOn events.
+ * If an ID is part of the container, it means that we have received a noteOn on a certain channel and key.
+ * Once we receive a noteOff, we remove that ID again.
+ *
+ * Unfortunately, I can't think of a way to detect overlapping notes by using the synth only. One might
+ * think, that it's possible to use fluid_synth_get_voicelist(), in order to get a list of active voices
+ * and then detect overlaps. However, this doesn't reliably work, because voices may finish, before the
+ * noteOff is received. Think of short percussion samples spawned by long MIDI note durations.
+ *
+ * Here is an example of how it might look like. The "ticks" are equivalent to the time parameter passed
+ * into fluid_event_callback_t.
+ *
+fluidsynth: debug: Tick 1728: Note on chan 15, key 44, ends at tick 1824
+fluidsynth: debug: Tick 1825: Normal NoteOFF on chan 15, key 44
+(so far, so usual)
+
+fluidsynth: debug: Tick 1825: Note on chan 15, key 44, dur 96, ends at tick 1921
+
+oops, the voice spawned by the previous note already finished at tick 1900, but the noteOff is yet to come
+
+fluidsynth: debug: Tick 1920: Note on chan 15, key 44, dur 143, ends at tick 2063
+(Shit, we got another noteOn before the last noteOff. If we check for playing voices now, we won't find any,
+because they have all finished.)
+
+fluidsynth: debug: Tick 1921: Normal NoteOFF on chan 15, key 44
+(... which means that we cannot detect an overlap, thus we cannot remove this noteoff, thus
+this noteoff will immediately kill the voice that we've just started 1 tick ago)
+*/
+
+typedef std::set<fluid_note_id_t> note_container_t;
+
+// Compute a unique ID for a given channel-key combination. Think of it as a two-dimensional array index.
+fluid_note_id_t fluid_note_compute_id(int chan, short key)
+{
+    return 128 * chan + key;
+}
+
+void* new_fluid_note_container()
+{
+    try
+    {
+        note_container_t* cont = new note_container_t;
+        return cont;
+    }
+    catch(...)
+    {
+        return 0;
+    }
+}
+
+void delete_fluid_note_container(void *cont)
+{
+    delete static_cast<note_container_t*>(cont);
+}
+
+// Returns true, if the ID was already included in the container before, false if it was just inserted and
+// FLUID_FAILED in case of error.
+int fluid_note_container_insert(void* cont, fluid_note_id_t id)
+{
+    try
+    {
+        std::pair<note_container_t::iterator, bool> res = static_cast<note_container_t*>(cont)->insert(id);
+        // res.second tells us whether the element was inserted
+        // by inverting it, we know whether it contained the element previously
+        return !res.second;
+    }
+    catch(...)
+    {
+        return FLUID_FAILED;
+    }
+}
+
+void fluid_note_container_remove(void* cont, fluid_note_id_t id)
+{
+    try
+    {
+        static_cast<note_container_t*>(cont)->erase(id);
+    }
+    catch(...)
+    {
+        // should never happen
+    }
+}
+
+// Empties the entire collection, e.g. in case of a AllNotesOff event
+void fluid_note_container_clear(void* cont)
+{
+    static_cast<note_container_t*>(cont)->clear();
+}

src/midi/fluid_seqbind_notes.h

@@ -0,0 +1,43 @@
+/* FluidSynth - A Software Synthesizer
+ *
+ * Copyright (C) 2003  Peter Hanappe and others.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301, USA
+ */
+
+#ifndef _FLUID_SEQBIND_NOTE_H
+#define _FLUID_SEQBIND_NOTE_H
+
+#include "fluidsynth.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+typedef int fluid_note_id_t;
+
+fluid_note_id_t fluid_note_compute_id(int chan, short key);
+void* new_fluid_note_container(void);
+void delete_fluid_note_container(void *cont);
+int fluid_note_container_insert(void* cont, fluid_note_id_t id);
+void fluid_note_container_remove(void* cont, fluid_note_id_t id);
+void fluid_note_container_clear(void* cont);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _FLUID_SEQBIND_NOTE_H */

src/synth/fluid_event.c

@@ -47,6 +47,7 @@ fluid_event_clear(fluid_event_t *evt)
     evt->dest = -1;
     evt->src = -1;
     evt->type = -1;
+    evt->id = -1;
 }
 
 /**
@@ -95,6 +96,12 @@ fluid_event_set_time(fluid_event_t *evt, unsigned int time)
     evt->time = time;
 }
 
+void
+fluid_event_set_id(fluid_event_t *evt, short id)
+{
+    evt->id = id;
+}
+
 /**
  * Set source of a sequencer event. \c src must be a unique sequencer ID or -1 if not set.
  * @param evt Sequencer event structure
@@ -161,11 +168,18 @@ fluid_event_noteoff(fluid_event_t *evt, int channel, short key)
 
 /**
  * Set a sequencer event to be a note duration event.
+ *
+ * Before fluidsynth 2.2.0, this event type was naively implemented when used in conjunction with fluid_sequencer_register_fluidsynth(),
+ * because it simply enqueued a fluid_event_noteon() and fluid_event_noteoff().
+ * A handling for overlapping notes was not implemented. Starting with 2.2.0, this changes: If a fluid_event_note() is already playing,
+ * while another fluid_event_note() arrives on the same @c channel and @c key, the earlier event will be canceled.
  * @param evt Sequencer event structure
  * @param channel MIDI channel number
  * @param key MIDI note number (0-127)
  * @param vel MIDI velocity value (0-127)
  * @param duration Duration of note in the time scale used by the sequencer (by default milliseconds)
+ *
+ * @note The application should decide whether to use only Notes with duration, or separate NoteOn and NoteOff events.
  */
 void
 fluid_event_note(fluid_event_t *evt, int channel, short key, short vel, unsigned int duration)
@@ -578,6 +592,17 @@ unsigned int fluid_event_get_time(fluid_event_t *evt)
     return evt->time;
 }
 
+/**
+ * @internal
+ * Get the time field from a sequencer event structure.
+ * @param evt Sequencer event structure
+ * @return Time value
+ */
+short fluid_event_get_id(fluid_event_t *evt)
+{
+    return evt->id;
+}
+
 /**
  * Get the source sequencer client from a sequencer event structure.
  * @param evt Sequencer event structure

src/synth/fluid_event.h

@@ -41,7 +41,7 @@ struct _fluid_event_t
     short vel;
     short control;
     short value;
-    short id; //?? unused ?
+    int id;
     int pitch;
     unsigned int duration;
     void *data;
@@ -50,6 +50,9 @@ struct _fluid_event_t
 unsigned int fluid_event_get_time(fluid_event_t *evt);
 void fluid_event_set_time(fluid_event_t *evt, unsigned int time);
 
+short fluid_event_get_id(fluid_event_t *evt);
+void fluid_event_set_id(fluid_event_t *evt, short id);
+
 void fluid_event_clear(fluid_event_t *evt);
 
 #ifdef __cplusplus