Fix sf2 volume control, note release, and sparse-bank patch fallback (#295)

include/sf2.h

@@ -38,7 +38,13 @@ extern int _WM_SF2_Active(void);
 /* per-mdi synth instances (voices private, sample data shared) */
 extern void *_WM_SF2_NewSynth(uint16_t rate);
 extern void _WM_SF2_FreeSynth(void *synth);
-extern void _WM_SF2_Reset(void *synth);
+extern void _WM_SF2_Reset(struct _mdi *mdi);
+
+/* re-apply every channel's volume (after a reset, or a WM_MO_LOG_VOLUME toggle) */
+extern void _WM_SF2_AdjustChannelVolumes(struct _mdi *mdi);
+
+/* send every sounding voice into its release stage */
+extern void _WM_SF2_ReleaseAll(void *synth);
 
 /* translate a wildmidi event to the synth */
 extern void _WM_SF2_Event(void *synth, struct _mdi *mdi, struct _event *event);

src/internal_midi.c

@@ -1407,6 +1407,11 @@ void _WM_do_meta_endoftrack(struct _mdi *mdi, struct _event_data *data) {
      * until key-off, so release them here too or a score that ends without
      * keying every note off rings on to the caller's cut-off. */
     if (mdi->mafm_synth) _WM_MAFM_ReleaseAll(mdi->mafm_synth);
+#ifdef WILDMIDI_SF2
+    /* Same for the soundfont engine: without this a score that ends on a
+     * still-held note sustains it until the render loop's 10s tail cap. */
+    if (mdi->sf2_synth) _WM_SF2_ReleaseAll(mdi->sf2_synth);
+#endif
     return;
 }
 

src/patches.c

@@ -80,13 +80,21 @@ _WM_get_patch_data(struct _mdi *mdi, uint16_t patchid) {
     WMIDI_UNUSED(mdi);
 
     _WM_Lock(&_WM_patch_lock);
-    search_patch = _find_nearest_patch(patchid);
+    search_patch = _find_matched_patch(patchid);
     if (search_patch == NULL && (patchid & 0xff00) != 0) {
-        /* Nothing at all in the requested bank: fall back to bank 0 rather
-         * than play silence, as a hardware synth does for an unknown bank.
-         * SMAF needs this - its scores select Yamaha's own voice banks (0x7c
-         * and friends), which no GUS/SF2 patch set defines, so without the
-         * fallback every SMAF file that has no custom FM voices is mute. */
+        /* A non-zero bank in a timidity.cfg is an overlay: it lists only the
+         * few programs that differ from bank 0 (eawpats' "bank 8" holds a
+         * single sine wave, "drumset 8" a single tambourine).  Fall back to
+         * bank 0 for everything it does not define, or the nearest-patch
+         * search below would answer every request from that bank with its one
+         * unrelated instrument.  This is also what makes SMAF audible: its
+         * scores select Yamaha's own voice banks (0x7c and friends), which no
+         * GUS/SF2 patch set defines at all. */
+        search_patch = _find_matched_patch(patchid & 0x00ff);
+    }
+    if (search_patch == NULL) {
+        /* Bank 0 has no such program either - a sparse patch set.  Nearest
+         * program is still better than silence. */
         search_patch = _find_nearest_patch(patchid & 0x00ff);
     }
     _WM_Unlock(&_WM_patch_lock);

src/sf2.c

@@ -104,13 +104,38 @@ int _WM_SF2_Active(void) {
     return (WM_sf2 != NULL);
 }
 
+/* Channel volume, using wildmidi's own curves rather than tsf's cubic
+ * default, so WM_MO_LOG_VOLUME does the same thing here as it does for the
+ * GUS mixer.  The linear curve is _WM_lin_volume[v]/1024 == v/127; the log
+ * curve is the MIDI2 table dBm_volume[v] == 40*log10(v/127), whose gain
+ * 10^(dBm/20) is just (v/127) squared. */
+static void WM_SF2_ChannelVolume(tsf *f, struct _mdi *mdi, uint8_t ch,
+                                 int volume, int expression) {
+    float gain = (float)((volume * expression) / 127) / 127.0f;
+    if (mdi->extra_info.mixer_options & WM_MO_LOG_VOLUME) {
+        gain *= gain;
+    }
+    tsf_channel_set_volume(f, ch, gain);
+}
+
 static void WM_SF2_InitChannels(tsf *f) {
     int ch;
     for (ch = 0; ch < 16; ch++) {
         tsf_channel_set_bank_preset(f, ch, (ch == 9) ? 128 : 0, 0);
     }
 }
 
+/* (Re)apply every channel's volume from the mdi's own state.  Needed after a
+ * reset and whenever WM_MO_LOG_VOLUME is toggled mid-playback. */
+void _WM_SF2_AdjustChannelVolumes(struct _mdi *mdi) {
+    uint8_t ch;
+    if (mdi->sf2_synth == NULL) return;
+    for (ch = 0; ch < 16; ch++) {
+        WM_SF2_ChannelVolume((tsf *)mdi->sf2_synth, mdi, ch,
+                             mdi->channel[ch].volume, mdi->channel[ch].expression);
+    }
+}
+
 void *_WM_SF2_NewSynth(uint16_t rate) {
     tsf *f;
     _WM_Lock(&_WM_sf2_lock);
@@ -130,14 +155,25 @@ void _WM_SF2_FreeSynth(void *synth) {
     }
 }
 
-void _WM_SF2_Reset(void *synth) {
-    tsf *f = (tsf *)synth;
+void _WM_SF2_Reset(struct _mdi *mdi) {
+    tsf *f = (tsf *)mdi->sf2_synth;
     int ch;
+    if (f == NULL) return;
     tsf_reset(f);
     for (ch = 0; ch < 16; ch++) {
         tsf_channel_midi_control(f, ch, 121, 0); /* reset controllers */
     }
     WM_SF2_InitChannels(f);
+    /* Callers reset the mdi's own channel state separately (and afterwards),
+       so seed the gains from _WM_do_sysex_gm_reset()'s defaults, not from
+       whatever mdi->channel still holds. */
+    for (ch = 0; ch < 16; ch++) {
+        WM_SF2_ChannelVolume(f, mdi, (uint8_t)ch, 100, 127);
+    }
+}
+
+void _WM_SF2_ReleaseAll(void *synth) {
+    tsf_note_off_all((tsf *)synth);
 }
 
 int _WM_SF2_ActiveVoices(void *synth) {
@@ -179,7 +215,8 @@ void _WM_SF2_Event(void *synth, struct _mdi *mdi, struct _event *event) {
         tsf_channel_midi_control(f, ch, 6, val & 0x7F);
         break;
     case ev_control_channel_volume:
-        tsf_channel_midi_control(f, ch, 7, val & 0x7F);
+        /* do_event() has not run yet, so pass the new value explicitly */
+        WM_SF2_ChannelVolume(f, mdi, ch, val & 0x7F, mdi->channel[ch].expression);
         break;
     case ev_control_channel_balance:
         tsf_channel_midi_control(f, ch, 8, val & 0x7F);
@@ -188,7 +225,7 @@ void _WM_SF2_Event(void *synth, struct _mdi *mdi, struct _event *event) {
         tsf_channel_midi_control(f, ch, 10, val & 0x7F);
         break;
     case ev_control_channel_expression:
-        tsf_channel_midi_control(f, ch, 11, val & 0x7F);
+        WM_SF2_ChannelVolume(f, mdi, ch, mdi->channel[ch].volume, val & 0x7F);
         break;
     case ev_control_data_entry_fine:
         tsf_channel_midi_control(f, ch, 38, val & 0x7F);
@@ -213,6 +250,9 @@ void _WM_SF2_Event(void *synth, struct _mdi *mdi, struct _event *event) {
         break;
     case ev_control_channel_controllers_off:
         tsf_channel_midi_control(f, ch, 121, val & 0x7F);
+        /* CC121 puts tsf's own volume back to unity; restore ours.  Like
+           _WM_do_control_channel_controllers_off(), CC7 survives, CC11 does not. */
+        WM_SF2_ChannelVolume(f, mdi, ch, mdi->channel[ch].volume, 127);
         break;
     case ev_control_channel_notes_off:
         tsf_channel_midi_control(f, ch, 123, val & 0x7F);
@@ -226,23 +266,31 @@ void _WM_SF2_Event(void *synth, struct _mdi *mdi, struct _event *event) {
     case ev_sysex_gm_reset:
     case ev_sysex_roland_reset:
     case ev_sysex_yamaha_reset:
-        _WM_SF2_Reset(f);
+        _WM_SF2_Reset(mdi);
         break;
     default: /* meta/timing events don't reach the synth */
         break;
     }
 }
 
+/* Headroom, matching VOL_DIVISOR in internal_midi.c: a soundfont renders a
+ * single note at full velocity close to full scale, so any busy score summed
+ * at unity gain clips hard. */
+#define SF2_VOL_DIVISOR 4.0f
+
 void _WM_SF2_Render(void *synth, int32_t *out, uint32_t frames) {
     tsf *f = (tsf *)synth;
-    short buf[256 * 2];
+    float buf[256 * 2];
+    /* Render float, not short: tsf_render_short() clamps to int16 itself, so
+       scaling its output afterwards would only make the clipping quieter. */
+    const float gain = (32767.0f * (float)_WM_MasterVolume / 1024.0f) / SF2_VOL_DIVISOR;
     uint32_t n, i;
 
     while (frames) {
         n = (frames > 256) ? 256 : frames;
-        tsf_render_short(f, buf, (int)n, 0);
+        tsf_render_float(f, buf, (int)n, 0);
         for (i = 0; i < n * 2; i++) {
-            out[i] += buf[i];
+            out[i] += (int32_t)(buf[i] * gain);
         }
         out += n * 2;
         frames -= n;

src/wildmidi_lib.c

@@ -2090,7 +2090,7 @@ WM_SYMBOL int WildMidi_FastSeek(midi * handle, unsigned long int *sample_pos) {
 #ifdef WILDMIDI_SF2
         /* Rewind TSF too so replayed events rebuild its state from scratch. */
         if (mdi->sf2_synth) {
-            _WM_SF2_Reset(mdi->sf2_synth);
+            _WM_SF2_Reset(mdi);
         }
 #endif
 #ifdef WILDMIDI_MAFM
@@ -2217,7 +2217,7 @@ WM_SYMBOL int WildMidi_SongSeek (midi * handle, int8_t nextsong) {
         event = mdi->events;
         _WM_ResetToStart((struct _mdi *) handle);
 #ifdef WILDMIDI_SF2
-        if (mdi->sf2_synth) _WM_SF2_Reset(mdi->sf2_synth);
+        if (mdi->sf2_synth) _WM_SF2_Reset(mdi);
 #endif
 #ifdef WILDMIDI_MAFM
         if (mdi->mafm_synth) _WM_MAFM_Reset(mdi->mafm_synth);
@@ -2254,7 +2254,7 @@ WM_SYMBOL int WildMidi_SongSeek (midi * handle, int8_t nextsong) {
         event = mdi->events;
         _WM_ResetToStart((struct _mdi *) handle);
 #ifdef WILDMIDI_SF2
-        if (mdi->sf2_synth) _WM_SF2_Reset(mdi->sf2_synth);
+        if (mdi->sf2_synth) _WM_SF2_Reset(mdi);
 #endif
 #ifdef WILDMIDI_MAFM
         if (mdi->mafm_synth) _WM_MAFM_Reset(mdi->mafm_synth);
@@ -2341,7 +2341,7 @@ static int WM_GetOutput_SF2(midi * handle, int8_t *buffer, uint32_t size) {
                 event->do_event(mdi, &event->event_data);
                 if ((mdi->extra_info.mixer_options & WM_MO_LOOP) && (event[0].evtype == ev_meta_endoftrack) && !end_encountered) {
                     end_encountered = 1; /* Avoid an infinite loop. */
-                    _WM_SF2_Reset(mdi->sf2_synth);
+                    _WM_SF2_Reset(mdi);
                     _WM_ResetToStart(mdi);
                     event = mdi->current_event;
                 } else {
@@ -2646,6 +2646,9 @@ WM_SYMBOL int WildMidi_SetOption(midi * handle, uint16_t options, uint16_t setti
     if (options & WM_MO_LOG_VOLUME) {
             _WM_AdjustChannelVolumes(mdi, 16);  /* Settings greater than 15
                                                    adjusts all channels */
+#ifdef WILDMIDI_SF2
+            _WM_SF2_AdjustChannelVolumes(mdi);
+#endif
     } else if (options & WM_MO_REVERB) {
         _WM_reset_reverb(mdi->reverb);
     }

test/CMakeLists.txt

@@ -2,6 +2,11 @@ ADD_EXECUTABLE(test_tokenize test_tokenize.c)
 TARGET_LINK_LIBRARIES(test_tokenize libwildmidi-static ${M_LIBRARY})
 ADD_TEST(NAME tokenize COMMAND test_tokenize)
 
+ADD_EXECUTABLE(test_patch_bank test_patch_bank.c)
+TARGET_INCLUDE_DIRECTORIES(test_patch_bank PRIVATE ${CMAKE_SOURCE_DIR}/include)
+TARGET_LINK_LIBRARIES(test_patch_bank libwildmidi-static ${M_LIBRARY})
+ADD_TEST(NAME patch_bank COMMAND test_patch_bank)
+
 ADD_EXECUTABLE(test_smaf_sequ test_smaf_sequ.c)
 TARGET_LINK_LIBRARIES(test_smaf_sequ libwildmidi-static ${M_LIBRARY})
 ADD_TEST(NAME smaf_sequ COMMAND test_smaf_sequ)

test/test_patch_bank.c

@@ -0,0 +1,51 @@
+/* assert-based test for _WM_get_patch_data()'s bank resolution order.
+ *
+ * Regression guard for issue #295: a non-zero bank in a timidity.cfg is a
+ * sparse overlay on bank 0 (eawpats' "bank 8" defines one program, "drumset 8"
+ * one note), so a program the overlay does not define has to come from bank 0
+ * and not from the nearest-patch search inside the overlay. */
+#include <assert.h>
+#include <stdint.h>
+#include <string.h>
+
+#include "patches.h"
+
+struct _mdi;
+extern struct _patch *_WM_get_patch_data(struct _mdi *mdi, uint16_t patchid);
+
+static struct _patch b0_p0, b0_p24, b0_d38, b8_p80, b8_d54;
+
+static void add(struct _patch *p, uint16_t patchid) {
+    memset(p, 0, sizeof(*p));
+    p->patchid = patchid;
+    p->next = _WM_patch[patchid & 0x7F];
+    _WM_patch[patchid & 0x7F] = p;
+}
+
+int main(void) {
+    add(&b0_p0,  0x0000);       /* bank 0, program 0 */
+    add(&b0_p24, 0x0018);       /* bank 0, program 24 */
+    add(&b0_d38, 0x00a6);       /* drumset 0, note 38 (0x26 | 0x80) */
+    add(&b8_p80, 0x0850);       /* bank 8, program 80 - the whole overlay */
+    add(&b8_d54, 0x08b6);       /* drumset 8, note 54 - the whole overlay */
+
+    /* exact hits win */
+    assert(_WM_get_patch_data(NULL, 0x0018) == &b0_p24);
+    assert(_WM_get_patch_data(NULL, 0x0850) == &b8_p80);
+    assert(_WM_get_patch_data(NULL, 0x08b6) == &b8_d54);
+
+    /* a program the overlay lacks comes from bank 0, not from the overlay */
+    assert(_WM_get_patch_data(NULL, 0x0818) == &b0_p24);
+    assert(_WM_get_patch_data(NULL, 0x08a6) == &b0_d38);
+
+    /* a bank nothing defines still falls back to bank 0 (SMAF selects
+       Yamaha's own 0x7c banks, which no GUS patch set has) */
+    assert(_WM_get_patch_data(NULL, 0x7c00) == &b0_p0);
+
+    /* only when bank 0 has no such program either does the nearest one
+       stand in, rather than playing silence */
+    assert(_WM_get_patch_data(NULL, 0x0017) == &b0_p24);
+    assert(_WM_get_patch_data(NULL, 0x0819) == &b0_p24);
+
+    return 0;
+}