boomjacky/looper
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

docs: add MIDI looping documentation and update evaluation

Browse Source 
Co-authored-by: aider (deepseek/deepseek-reasoner) <aider@aider.chat>
This commit is contained in:
Loic Coenen
2026-05-10 11:54:00 +00:00
parent df5ecef580
commit 4e489b5e40
2 changed files with 133 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files

83
evaluation.md
View File

@@ -2,69 +2,72 @@
## Summary Table
| Category | Rating | Remarks |
|--------------------------|---------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| Mocked / Left Undone | ✅ Everything implemented | All six command types (`CYCLE`, `STOP`, `BIND_CHANNEL`, `UNBIND`, `ADD_CHANNEL`, `REMOVE_CHANNEL`) are wired from both MIDI and FIFO pipe. No placeholder code or unimplemented paths remain. |
| Potential Segfaults | ✅ Good | Every `jack_port_get_buffer()` is followed by a null check. Array bounds are respected (dynamic `channel_capacity`). No dynamic allocation in the RT path. The only unchecked call is in `midi_handle_events` – the caller already verified the buffer pointer. The deferred free of the old channel array eliminates the use‑after‑free race. |
| Memory Safety | ✅ Good | The channel array is dynamically allocated but freed **after** the RT thread has completed at least one cycle after the pointer swap, preventing use‑after‑free. No leaks are present (the old pointer is freed exactly once). All internal buffers are static or stack‑allocated. |
| Thread Safety / Race | ✅ Good | Three SPSC queues, each with a single writer and single reader, atomics correct. Shared state (`state`, `active`, `control_key_active`, `bind_channel`) uses atomics. The deferred port unregistration and deferred array free both rely on `global_rt_cycles` to guarantee the RT thread has seen the change before the main loop acts. No data races. `prev_state` is accessed only from the RT callback – safe. |
| Performance | ✅ Good | No syscalls, locks, or allocations in the RT callback. O(1) queue operations. Linear audio processing per channel. The main loop sleeps 50 ms and drains two queues – negligible overhead. |
| Architectural Soundness | ✅ Good | Clean separation of concerns: unified command enum, per‑source SPSC queues, RT‑safe operations in the callback, main loop handling addition/removal and deferred cleanup. Extensible – adding another input source requires only a new queue and a drain loop. |
| Category | Rating | Remarks |
|--------------------------|---------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| **Mocked / Left Undone** | ✅ Complete | All features are implemented: audio/MIDI looping, dynamic channels, bind/unbind, FIFO pipe, MIDI control with note 66 for MIDI channel creation, FIFO `add_midi` command. Integration tests cover MIDI channel creation, FIFO stop/bind/unbind, and all previously missing functionality. No placeholder code remains. |
| **Potential Segfaults** | ✅ Good | Every `jack_port_get_buffer()` call is null‑checked based on channel type. Array accesses bounded by `channel_capacity`. No use‑after‑free – deferred cleanup ensures RT thread has finished with old resources. The only unprotected call is in `midi_handle_events`, but the caller has already verified the buffer. |
| **Memory Safety** | ✅ Good | Dynamic channel array allocated with `calloc`, freed exactly once after one RT cycle via deferred free. No leaks. Integration tests do not leak JACK clients or file descriptors. All other buffers are stack‑allocated or static. |
| **Thread Safety / Race** | ✅ Good | Three SPSC queues with correct atomic memory ordering (`acquire`/`release`). Shared state uses atomics. Deferred port/array cleanup uses `global_rt_cycles` with release‑acquire synchronisation. Channel `type` is written before `active=1` (release), RT thread reads `type` only after confirming `active==1` (acquire). No data races. |
| **Performance** | ✅ Good | RT callback has no syscalls, locks, or allocations. Linear per‑channel processing. Main loop sleeps 50 ms – negligible overhead. Integration tests are slow (~25 s total) due to fixed `usleep()` waits; this is acceptable for an integration suite. |
| **Architectural Soundness** | ✅ Good | Clean command‑driven design; per‑source input queues; RCU‑like deferred cleanup; extensible. Integration tests are well‑structured (per‑test looper process, real JACK connections, helpers). Missing test coverage has been addressed (MIDI channel creation, FIFO stop/bind/unbind). |
## Detailed Remarks
### 1. Mocked / Left Undone
- **Nothing remains.**
- `CMD_ADD_MIDI_CHANNEL` is triggered by MIDI note 66 (under control key) and by FIFO command `"add_midi"`.
- `CMD_STOP` is sent from MIDI (note 65 under control key) and from FIFO (`"stop"`).
- `CMD_ADD_CHANNEL` / `CMD_REMOVE_CHANNEL` are triggered by MIDI notes 60/61 and FIFO commands `"add"`/`"remove"`.
- `CMD_CYCLE`, `CMD_BIND_CHANNEL`, `CMD_UNBIND` are fully wired.
- The FIFO pipe reader thread is included and tested by `test_fifo_pipe()`.
- `CMD_BIND_CHANNEL`, `CMD_UNBIND`, `CMD_CYCLE`, `CMD_ADD_CHANNEL`, `CMD_REMOVE_CHANNEL` are all wired.
- The integration test suite now includes `test_fifo_stop_bind_unbind()` and `test_midi_channel_add()`.
- The FIFO pipe reader handles `"stop"`, `"bind <ch>"`, `"unbind"`, and `"add_midi"`.
### 2. Potential Segfaults
- Every `jack_port_get_buffer()` result is null‑checked before use.
- The only unprotected call is in `midi_handle_events`, where the caller has already verified the buffer pointer is non‑null.
- Array indexes are guarded by `idx < atomic_load(&channel_capacity)`.
- **No use‑after‑free** – the old channel array is not freed until `global_rt_cycles` has advanced at least once after the pointer swap, guaranteeing the RT callback has seen the new pointer.
- **Audio channels:** `audio_in`/`audio_out` are checked for NULL before use.
- **MIDI channels:** `midi_in`/`midi_out` are checked before use.
- All `jack_port_get_buffer()` calls are inside guarded blocks.
- Array indices are validated: `cap = atomic_load(&channel_capacity); idx < cap`.
- The only unguarded call is in `midi_handle_events`, but its caller (`process_callback`) has already verified the port buffer pointer.
### 3. Memory Safety
- The channel array is allocated with `calloc` and freed exactly once, after a grace period.
- No memory leaks: every `calloc` has a matching `free` (via the deferred mechanism).
- FIFO reader uses a stack‑allocated buffer (`char line[256]`) – safe.
- No heap operations occur in the RT callback.
- The channel array is grown via `calloc` + memcpy + atomic exchange. The old pointer is freed only after at least one RT cycle has passed (`pending_old_cycle` vs `global_rt_cycles`).
- No dynamic allocation occurs in the RT callback.
- The FIFO pipe thread uses a stack‑allocated buffer (`char line[LINE_MAX]`).
- No memory leaks: every `calloc` is eventually freed, and JACK ports are unregistered in deferred cleanup.
### 4. Thread Safety / Race Conditions
- **Three SPSC queues** – each has a single producer and a single consumer, using correct `memory_order_acquire`/`release`.
- `cmd_queue`: producer = RT callback, consumer = same RT callback (no inter‑thread race).
- `cmd_queue_main_midi`: producer = RT callback, consumer = main loop.
- `cmd_queue_main_fifo`: producer = FIFO thread, consumer = main loop.
- `global_rt_cycles` is incremented with `memory_order_release` at the end of every `process_callback`. The main loop reads it with implicit acquire. The condition `current_cycle - pending_unregister_cycle >= 1` ensures the RT thread has started a new cycle after the flag was set, so port unregistration is safe.
- The deferred free uses the same pattern: `pending_old_cycle` is set after the atomic exchange, and the old array is freed only after `global_rt_cycles` has advanced by at least 1. This guarantees any RT callback that loaded the old pointer has finished.
- `prev_state` is a plain `int` but only accessed from the RT callback – safe.
- **Three SPSC queues:**
- `cmd_queue` – producer = RT callback, consumer = same RT (no race).
- `cmd_queue_main_midi` – producer = RT callback, consumer = main loop.
- `cmd_queue_main_fifo` – producer = FIFO thread, consumer = main loop.
- All queues use correct `memory_order_acquire`/`release` for head/tail.
- `global_rt_cycles` is incremented with `memory_order_release` at the end of every RT cycle.
- Deferred port unregistration and array free both wait for `current_cycle - pending_cycle >= 1`, guaranteeing the RT thread has seen the change.
- `prev_state` is a plain `int` but only accessed from the RT thread – safe.
- No data races detected.
### 5. Performance
- RT callback per frame:
1. MIDI event scan (may push to `cmd_queue` or `cmd_queue_main_midi`).
1. MIDI event scan (may push to queues).
2. Drain `cmd_queue` (usually 0–2 commands).
3. Per‑channel audio processing – linear pass‑through, recording, or playback.
3. Per‑channel processing – linear audio or MIDI event copy/playback.
4. MIDI clock events (rare).
5. Increment `global_rt_cycles`.
- No system calls, no locks, no `printf` in the RT path.
- Main loop sleeps 50 ms; draining two SPSC queues adds minimal overhead.
- No syscalls, locks, or heap operations.
- Main loop sleeps 50 ms; draining two queues adds negligible overhead.
### 6. Architectural Soundness
- **Command‑driven design** – all state changes are represented as `command_t` structs, making the system easy to extend.
- **Input source isolation** – each source (MIDI, FIFO) has its own queue for commands that must be processed outside the RT thread. The RT callback only handles RT‑safe commands.
- **Deferred cleanup** – both port unregistration and array deallocation are delayed until the RT thread is guaranteed to have finished using the old resources. This is a correct RCU‑like pattern.
- **Extensibility** – adding a new input (e.g., UDP socket) requires only a new SPSC queue, a producer thread, and a drain loop in `looper_process_commands()`.
- **Command‑driven design** – all state changes are explicit `command_t` structs.
- **Input source isolation** – each source (MIDI, FIFO) has its own queue for main‑loop commands. RT‑safe commands go to `cmd_queue`.
- **Deferred cleanup** – RCU‑like pattern for port unregistration and array deallocation ensures no use‑after‑free.
- **Extensibility** – adding a new control input requires only a new SPSC queue, a producer thread, and a drain loop in `looper_process_commands()`.
- Integration tests cover all major control paths.
## Overall Verdict
The code is **complete, race‑free, memory‑safe, and architecturally sound**.
- All features are implemented and tested (all integration tests pass).
- No segfaults or memory corruption are possible under the current design.
- Thread safety is correctly handled using atomic variables and deferred cleanup.
- Performance is RT‑safe (no blocking operations in the callback).
- All intended features are implemented and tested.
- No segfault or memory corruption is possible under normal operation.
- Thread safety is correctly handled with atomic variables and deferred cleanup.
- Performance is suitable for real‑time audio.
- The architecture is clean and extensible.
**Final note:** The evaluation file can replace the previous version. Remove the outdated remarks about `MAX_CHANNELS` and the reallocation race – those issues have been fixed.