Add sf2 (soundfont) support to wildmidi - #270
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds optional SF2 playback support via TinySoundFont, gated by a new build option. It introduces the SF2 API and implementation, wires SF2 synth instances into MIDI handling and output, hardens several realloc-based paths, and updates version metadata and documentation. ChangesSF2 Support
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant WildMidi
participant SF2
participant TinySoundFont
App->>WildMidi: WildMidi_Init(config_file)
WildMidi->>SF2: _WM_SF2_Load(data)
SF2->>TinySoundFont: tsf_load_memory / tsf_copy
App->>WildMidi: WildMidi_GetOutput()
WildMidi->>SF2: WM_GetOutput_SF2()
loop per event
WildMidi->>SF2: _WM_SF2_Event(synth, mdi, event)
SF2->>TinySoundFont: tsf_channel_* / tsf_note_on/off
end
WildMidi->>SF2: _WM_SF2_Render(synth, out, frames)
SF2->>TinySoundFont: tsf_render_short(chunked)
App->>WildMidi: WildMidi_Shutdown()
WildMidi->>SF2: _WM_SF2_Unload()
Related Issues: Suggested labels: enhancement, build Suggested reviewers: none 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@sezero should I include the lib in /extern or just leave it to cmake to fetch it for us? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
CMakeLists.txt (1)
308-318: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
WANT_SF2defaults to ON, making a network fetch mandatory for default builds.With
WANT_SF2defaultingON, every default configure now performs aFetchContentgit clone of TinySoundFont from GitHub. This silently introduces a hard network dependency for a build that previously worked fully offline, which breaks sandboxed CI, air-gapped builds, and distro packaging pipelines (Debian/Fedora and similar policies explicitly forbid network access during package builds). All of the project's other optional audio backends (WANT_ALSA,WANT_OSS, etc.) rely onFIND_PACKAGE(... REQUIRED)against locally installed dependencies rather than fetching source over the network, so this is a new class of build dependency for the project.Consider defaulting
WANT_SF2toOFF, or at least tryingfind_package/an existing local checkout first and falling back toFetchContentonly when explicitly requested.♻️ Suggested default change
-OPTION(WANT_SF2 "SoundFont2 (SF2) support via TinySoundFont" ON) +OPTION(WANT_SF2 "SoundFont2 (SF2) support via TinySoundFont" OFF)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CMakeLists.txt` around lines 308 - 318, The default SF2 backend setup in the CMake configuration currently forces a network FetchContent download through WANT_SF2, which should not happen in default builds. Update the WANT_SF2 handling in the CMakeLists logic around FetchContent_Declare(tinysoundfont) and FetchContent_MakeAvailable so default configuration does not require GitHub access; either switch WANT_SF2 to OFF by default or make the download path conditional on an explicit opt-in and prefer a local find_package or existing source checkout first.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sf2.c`:
- Around line 70-72: _WM_SF2_Active is reading the shared WM_sf2 pointer without
the WM_sf2_lock used by _WM_SF2_Load, _WM_SF2_Unload, and _WM_SF2_NewSynth.
Update _WM_SF2_Active to take the same lock around the WM_sf2 NULL check, then
release it before returning, so the active-state check follows the file’s
existing locking discipline and cannot race with unload.
- Around line 47-59: Reject oversized soundfont buffers in _WM_SF2_Load before
passing the length to tsf_load_memory(). Add a check on the uint32_t size
argument to ensure it does not exceed INT_MAX, and return an error if it does,
then only cast to int after that guard. Keep the fix localized to _WM_SF2_Load
in src/sf2.c so the tsf_load_memory call never receives a wrapped negative
length.
In `@src/wildmidi_lib.c`:
- Around line 543-552: The config parsing failure path in the soundfont load
flow leaves the global SF2 state active after a successful _WM_SF2_Load(), so
add _WM_SF2_Unload() before returning from the failure cleanup in the config
loader logic. Update the failure branch around the _WM_SF2_Load call in the
config parsing routine so any later directive parse error also unloads the SF2
font, alongside the existing frees for sf2_buffer, sf2_path, patches,
config_dir, line_tokens, and config_buffer.
- Around line 2079-2084: Read mutable handle state only after acquiring the MIDI
lock in the output path. In the code around the current_event handling, move the
assignment from mdi->current_event to event so it happens after
_WM_Lock(&mdi->lock), keeping the state access inside the protected section and
preventing races with WildMidi_GetOutput() and seek-style operations.
- Around line 2088-2095: The realloc handling in the mix-buffer growth logic can
lose the original buffer if allocation fails. Update the `wildmidi` mix buffer
resize path around `mdi->mix_buffer` to store `realloc()` in a temporary
pointer, check for failure, and avoid overwriting the existing buffer when
allocation returns null. If the resize fails, unlock any held state and return
an error before any later use of `tmp_buffer` or `memset`, so the existing
buffer remains valid.
- Around line 1648-1667: The SF2 initialization path in WM_InitPatches() returns
early on _WM_BufferFile(), _WM_SF2_Load(), and WM_LoadConfig() failures without
cleaning up globals initialized earlier. Update the SF2 branch to perform the
same teardown on every failure path: free patch state initialized by
WM_InitPatches(), unload any SF2 data already loaded from a prior soundfont
directive, and release cfg_buffer before returning. Keep the cleanup centralized
around the WM_InitPatches()/WM_LoadConfig() flow and the _WM_SF2_Load() error
handling so all exit paths leave the library in a consistent state.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 308-318: The default SF2 backend setup in the CMake configuration
currently forces a network FetchContent download through WANT_SF2, which should
not happen in default builds. Update the WANT_SF2 handling in the CMakeLists
logic around FetchContent_Declare(tinysoundfont) and FetchContent_MakeAvailable
so default configuration does not require GitHub access; either switch WANT_SF2
to OFF by default or make the download path conditional on an explicit opt-in
and prefer a local find_package or existing source checkout first.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1650a9a1-93b9-41be-8558-3e23c70b8b6a
📒 Files selected for processing (11)
CMakeLists.txtdocs/SF2.mdinclude/config.h.cmakeinclude/internal_midi.hinclude/sf2.hsrc/CMakeLists.txtsrc/internal_midi.csrc/sf2.csrc/wildmidi_lib.ctest/CMakeLists.txttest/test_sf2.c
| int _WM_SF2_Active(void) { | ||
| return (WM_sf2 != NULL); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
_WM_SF2_Active reads the shared WM_sf2 pointer without the lock used everywhere else.
_WM_SF2_Load/_WM_SF2_Unload/_WM_SF2_NewSynth all guard access to WM_sf2 with WM_sf2_lock, but _WM_SF2_Active reads it unguarded. This is inconsistent with the locking discipline established in this same file and can race with a concurrent _WM_SF2_Unload, yielding a stale/torn read of the "is SF2 active" status.
🔒 Suggested fix
int _WM_SF2_Active(void) {
- return (WM_sf2 != NULL);
+ int active;
+ _WM_Lock(&WM_sf2_lock);
+ active = (WM_sf2 != NULL);
+ _WM_Unlock(&WM_sf2_lock);
+ return active;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int _WM_SF2_Active(void) { | |
| return (WM_sf2 != NULL); | |
| } | |
| int _WM_SF2_Active(void) { | |
| int active; | |
| _WM_Lock(&WM_sf2_lock); | |
| active = (WM_sf2 != NULL); | |
| _WM_Unlock(&WM_sf2_lock); | |
| return active; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sf2.c` around lines 70 - 72, _WM_SF2_Active is reading the shared WM_sf2
pointer without the WM_sf2_lock used by _WM_SF2_Load, _WM_SF2_Unload, and
_WM_SF2_NewSynth. Update _WM_SF2_Active to take the same lock around the WM_sf2
NULL check, then release it before returning, so the active-state check follows
the file’s existing locking discipline and cannot race with unload.
| sf2_buffer = (uint8_t *) _WM_BufferFile(sf2_path, &sf2_size); | ||
| if ((sf2_buffer == NULL) || (_WM_SF2_Load(sf2_buffer, sf2_size) == -1)) { | ||
| _WM_GLOBAL_ERROR(WM_ERR_INVALID_ARG, "(unable to load soundfont)", 0); | ||
| _WM_FreeBufferFile(sf2_buffer); | ||
| free(sf2_path); | ||
| WM_FreePatches(); | ||
| free(config_dir); | ||
| free(line_tokens); | ||
| _WM_FreeBufferFile(config_buffer); | ||
| return (-1); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Unload SF2 state when config parsing fails after loading a soundfont.
A config can load a soundfont successfully, then fail on a later directive. The failure paths free patches/config buffers but leave the global SF2 font active, which can leak state into a subsequent init attempt. Add _WM_SF2_Unload() on config-load failure paths that occur after _WM_SF2_Load() may have succeeded.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/wildmidi_lib.c` around lines 543 - 552, The config parsing failure path
in the soundfont load flow leaves the global SF2 state active after a successful
_WM_SF2_Load(), so add _WM_SF2_Unload() before returning from the failure
cleanup in the config loader logic. Update the failure branch around the
_WM_SF2_Load call in the config parsing routine so any later directive parse
error also unloads the SF2 font, alongside the existing frees for sf2_buffer,
sf2_path, patches, config_dir, line_tokens, and config_buffer.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/bsd.yml (1)
27-37: 🧹 Nitpick | 🔵 TrivialCorrectly adds
gitfor FetchContent, but note new network dependency in CI.Adding
gitalongsidecmakeis necessary sinceWANT_SF2defaults toONand TinySoundFont is fetched viaFetchContent_Declare(... GIT_REPOSITORY ...). Withoutgit, these BSD builds would fail to configure.One operational note: this makes previously self-contained BSD CI jobs depend on outbound network access to
github.comduring the build step (cloning TinySoundFont inside the cross-platform-actions VM). If those sandboxed BSD VMs have restricted or flaky egress, this could introduce new CI flakiness/timeouts that didn't exist before.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/bsd.yml around lines 27 - 37, The BSD workflow change for the OpenBSD and NetBSD matrix jobs now introduces a runtime dependency on outbound GitHub access because WANT_SF2 pulls TinySoundFont via FetchContent_Declare using a GIT_REPOSITORY. Keep git installed, and update the BSD CI setup around the matrix entries so this network requirement is explicitly handled or mitigated (for example by prefetching or caching the dependency in the workflow) to avoid flaky configure-time failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/bsd.yml:
- Around line 27-37: The BSD workflow change for the OpenBSD and NetBSD matrix
jobs now introduces a runtime dependency on outbound GitHub access because
WANT_SF2 pulls TinySoundFont via FetchContent_Declare using a GIT_REPOSITORY.
Keep git installed, and update the BSD CI setup around the matrix entries so
this network requirement is explicitly handled or mitigated (for example by
prefetching or caching the dependency in the workflow) to avoid flaky
configure-time failures.
yes: that way, we can customize it as we see fit as necessary
please no |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/internal_midi.c`:
- Around line 552-565: The event allocation failure handling in
_WM_CheckEventMemoryPool is currently local-only and lets the setup path
continue with stale capacity, so thread the failure back through the event
creation flow instead of just returning. Update the midi_setup_* call chain and
any helper that writes into mdi->events so they can detect a failed resize from
_WM_CheckEventMemoryPool, stop advancing event_count, and abort the current
setup cleanly before any write occurs. Ensure the failure is propagated from the
allocation check through the event-setup helpers that consume it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c3b6eda-e3e3-4f2a-b88e-1426d81fd2f5
📒 Files selected for processing (4)
src/internal_midi.csrc/patches.csrc/sf2.csrc/wildmidi_lib.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sf2.c
Okay, vendered in /extern I think this is worthy of a version bump; 0.5.0 or 1.0 or 1.5 ? :P |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/internal_midi.c (1)
2172-2174: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCheck the
realloclength math before appending copyright data.old_len + 1 + tmp_length + 1can wrapsize_t, which would under-allocate the buffer before the latermemcpy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/internal_midi.c` around lines 2172 - 2174, The copyright append logic in the internal MIDI handling path should guard against size_t overflow before calling realloc. In the code that computes the new buffer size using old_len, tmp_length, and the extra separator/terminator space, add an explicit overflow check before the realloc on mdi->extra_info.copyright so the allocation cannot wrap and under-allocate. Keep the fix localized to the copyright append block and preserve the existing memcpy flow after the safe allocation succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CMakeLists.txt`:
- Around line 304-310: Guard the WANT_SF2 branch in CMakeLists.txt so
WILDMIDI_SF2 is only enabled on little-endian targets; this SF2 path uses
TinySoundFont via tsf.h and will misparse data on WORDS_BIGENDIAN builds. Update
the IF block around WANT_SF2 to check endianness before setting WILDMIDI_SF2,
and either disable SF2 with a fatal configuration error on big-endian systems or
add the required byte-swapping support in the TinySoundFont integration.
In `@src/internal_midi.c`:
- Line 1430: The terminal-event write in _WM_CheckEventMemoryPool is using the
growth helper too early, which blocks a valid final write when only the spare
terminator slot is missing. Update this path so it only requires
events[event_count] to be available, and preserve the existing valid slot when
event_count == events_size - 1 instead of returning early; keep the fix
localized around the _WM_CheckEventMemoryPool check in the terminal-event append
logic.
- Around line 555-558: The event pool growth in the internal MIDI path needs
overflow guards before resizing. In the `mdi->event_count`/`mdi->events_size`
expansion block inside the MIDI event handling code, check that `event_count +
1`, `events_size + MEM_CHUNK`, and the `realloc` byte-size calculation cannot
wrap before using them. If any arithmetic would overflow, fail safely instead of
allocating a smaller buffer and then writing to `mdi->events[mdi->event_count]`.
---
Outside diff comments:
In `@src/internal_midi.c`:
- Around line 2172-2174: The copyright append logic in the internal MIDI
handling path should guard against size_t overflow before calling realloc. In
the code that computes the new buffer size using old_len, tmp_length, and the
extra separator/terminator space, add an explicit overflow check before the
realloc on mdi->extra_info.copyright so the allocation cannot wrap and
under-allocate. Keep the fix localized to the copyright append block and
preserve the existing memcpy flow after the safe allocation succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b6da9af-116b-46bd-a2fd-92ce6fe8ef71
📒 Files selected for processing (7)
CMakeLists.txtdocs/SF2.mdextern/TinySoundFont/LICENSEextern/TinySoundFont/README.mdextern/TinySoundFont/tsf.hsrc/internal_midi.ctest/CMakeLists.txt
✅ Files skipped from review due to trivial changes (3)
- extern/TinySoundFont/README.md
- extern/TinySoundFont/LICENSE
- docs/SF2.md
🚧 Files skipped from review as they are similar to previous changes (1)
- test/CMakeLists.txt
|
@sezero please create a PR for this PR? :D I wasn't done working on this branch ;) |
OK, sorry. Here are four patches (in |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Strange, I thought you could open a PR to merge into sf2-support branch; that would have worked too. What do you think of a major or minor version bump? |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CMakeLists.txt (1)
71-71: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider defaulting
WANT_SF2to OFF.This enables a brand-new, untrusted-binary-parsing feature (TinySoundFont/SF2) by default. Given the multiple portability/robustness fixes already needed for this vendored parser (per the commit history: missing
SIZE_MAX,TSF_SQRTFcheck, array address-of misuse, missingpowf/expf/sqrtf), it may be safer to ship this as opt-in until it has more field exposure, especially since it also introduces a new mandatoryFATAL_ERRORdependency check (Line 322) for builds that previously had none.diff
-OPTION(WANT_SF2 "SoundFont2 (SF2) support via TinySoundFont" ON) +OPTION(WANT_SF2 "SoundFont2 (SF2) support via TinySoundFont" OFF)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CMakeLists.txt` at line 71, Default the WANT_SF2 option to OFF in the CMakeLists.txt configuration so TinySoundFont/SF2 support is opt-in rather than enabled by default. Update the OPTION declaration for WANT_SF2 accordingly, and make sure any SF2-related enablement paths still work when users explicitly turn it on, including the existing fatal dependency check tied to the SF2 build path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@CMakeLists.txt`:
- Line 71: Default the WANT_SF2 option to OFF in the CMakeLists.txt
configuration so TinySoundFont/SF2 support is opt-in rather than enabled by
default. Update the OPTION declaration for WANT_SF2 accordingly, and make sure
any SF2-related enablement paths still work when users explicitly turn it on,
including the existing fatal dependency check tied to the SF2 build path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 159724fe-42f6-4b09-9cef-51b97e0e43ab
📒 Files selected for processing (5)
CMakeLists.txtextern/TinySoundFont/tsf.hinclude/config.h.cmakesrc/internal_midi.csrc/sf2.c
🚧 Files skipped from review as they are similar to previous changes (3)
- extern/TinySoundFont/tsf.h
- src/sf2.c
- src/internal_midi.c
|
Here is the second set of patches, again in |
0.5.0 seems reasonable to me. (Even 6c40712 (PR/254) might be considered enough of a behavior change..) |
Do you plan on upstreaming these to the project? |
Haven't considered it. Should I do it? |
Yes please :) Let's be good neighbours. |
|
Ready for merge? |
I guess. I can update standalone makefiles+configs later. |
|
Cheers @sezero ! :) We can do a few more cleanups, then launch 0.5 when ready |
| #else | ||
| #define _TSFREGIONOFFSET(TYPE, FIELD) (unsigned char)(offsetof(struct tsf_region,FIELD) / sizeof(TYPE)) | ||
| #define _TSFREGIONENVOFFSET(TYPE, ENV, FIELD) (unsigned char)((offsetof(struct tsf_region,ENV) + offsetof(struct tsf_envelope,FIELD)) / sizeof(TYPE)) | ||
| #endif |
There was a problem hiding this comment.
@psi29a (and any others): I'm not 100.000% certain of this fix: Can you think of any cases where it can output different offsets compared to original code?
|
@psi29a: Do we want schellingb/TinySoundFont#110 and maybe schellingb/TinySoundFont#118 from among mainstream tsf P/Rs? |
Yes of course. I had hoped to let cmake handle this, but now we have to manually keep things up to date. 😬 |
Our changes submitted to mainstream as schellingb/TinySoundFont#125 |
3 participants
The existing -c / --config option accepts the soundfont directly, because detection happens by file magic (RIFF/sfbk), not extension or option:
Shouldu close #8
Summary by CodeRabbit
WANT_SF2build option and asoundfontconfiguration directive to load an SF2.ENOMEM-style error handling; strengthened initialization and shutdown cleanup when SF2 is enabled.