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

docs: update evaluation.md with current code analysis

Browse Source 
Co-authored-by: aider (deepseek/deepseek-reasoner) <aider@aider.chat>
This commit is contained in:
Loic Coenen
2026-05-09 23:55:07 +00:00
parent 7edd95d06e
commit 7b61384154
1 changed files with 45 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

85
evaluation.md
View File

@@ -3,62 +3,65 @@
## Summary Table ## Summary Table
| Category | Rating | Remarks | | Category | Rating | Remarks |
|--------------------------|-------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------| |--------------------------|---------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| Mocked / Left Undone | ⚠️ Partial | Control‑key modifier and bind commands are now dispatched correctly. However, note that `CMD_STOP` is defined but never triggered from MIDI or FIFO (FIFO supports `"stop"`). The MIDI code still uses raw atomic stores for `cmd_add`/`cmd_remove` instead of pushing command‑queue actions – this is a minor inconsistency but works. The test file contains many more tests than the code can actually satisfy (e.g., `test_control_key_modifier`, `test_bind_channel`, `test_bind_unbind`, `test_remove_channel`) – these tests will fail because the looper’s current mapping does not match what the tests expect (the tests use note numbers that do not map to the actual commands). | | Mocked / Left Undone | ✅ Minor | `CMD_STOP` is only reachable via the FIFO pipe (`"stop"`). The MIDI handler never sends it. This is an unused path, not a bug. The FIFO pipe is still untested in the integration suite – all tests use MIDI. No test for `"stop"` via FIFO. Everything else is implemented and matches the test suite. |
| Potential Segfaults | ✅ Good | All `jack_port_get_buffer` results are checked for NULL before dereference. No array overruns (fixed‑size loops). The SPSC queue uses modulo arithmetic within bounded capacity. | | Potential Segfaults | ✅ Good | All `jack_port_get_buffer()` calls are guarded against NULL. Array bounds respected (fixed‑size loops). SPSC queues use modulo arithmetic, no overrun possible. No dynamic memory in RT path. |
| Memory Safety | ✅ OK | No dynamic allocation in the RT path. All buffers (loop buffers, command queue) are statically sized. No use‑after‑free – the only deferred operation (port unregister) is done in the main loop after marking inactive. | | Memory Safety | ✅ OK | No leaks, no use‑after‑free. All buffers static. Deferred port unregistration waits for at least one RT cycle after `active=0` – safe. The FIFO reader thread uses stack memory for line reading. |
| Thread Safety / Race | ⚠️ Warning | The SPSC queue uses correct atomic memory ordering (`acquire`/`release`). However, the `process_callback` first calls `midi_handle_events` (which pushes to the queue), then drains the queue **in the same cycle**. This means state changes pushed by MIDI are applied *within the same audio cycle* – that is fine. **But the test code injects MIDI notes via a separate client, and the looper’s MIDI handler runs MIDI events *before* draining the queue – so a MIDI note pushed in the same cycle will be processed immediately. That is correct and expected.** No race condition there. However, there is a **potential issue with `channels[c].prev_state` being read and written from the RT thread without atomic operations** – `prev_state` is a plain `int`, not `atomic_int`. This is accessed in the process callback and nowhere else, so it is safe (single consumer). The `channel_add` and `channel_remove` functions are called from the non‑RT main loop while the RT callback may be reading `active`, `state`, `audio_in`, `audio_out` – these are all atomic, so safe. | | Thread Safety / Race | ✅ Good | SPSC queues have correct `acquire`/`release` ordering. Multi‑producer issue on main‑loop commands has been fixed by giving each producer its own queue (`cmd_queue_main_midi` for MIDI, `cmd_queue_main_fifo` for FIFO). The RT callback only writes to `cmd_queue_main_midi`; the FIFO thread only writes to `cmd_queue_main_fifo`. Both are consumed solely by the main loop, restoring SPSC safety. The deferred unregistration race is now fixed via `global_rt_cycles` counter – the main loop ensures the RT thread has completed a cycle after `active=0` before calling `jack_port_unregister()`. `prev_state` is a plain `int` but accessed only from the RT callback (single thread). All other shared state (`state`, `active`, `control_key_active`, `bind_channel`) uses atomics. |
| Performance | ✅ Good | No syscalls, no allocations, no locks in RT path. Atomic operations are cheap. Buffer accesses are linear. Queue operations are O(1). | | Performance | ✅ Good | No syscalls, locks, or dynamic allocations in the RT callback. Two queue drains (one for RT commands, one for main‑loop commands) add negligible overhead. O(1) queue operations. Linear audio processing. |
| Architectural Soundness | ✅ Good | Clean separation: MIDI handler pushes commands, RT callback applies them, main loop handles add/remove via atomic flags. The command queue is a reasonable lightweight approach. However, the mixture of atomic flags for add/remove and the command queue for state transitions is a bit inconsistent – a uniform command‑queue approach for everything would be cleaner. The FIFO pipe works well. | | Architectural Soundness | ✅ Good | Clean separation: each input source has its own SPSC queue for non‑RT commands; RT callback drains its own queue; main loop drains both auxiliary queues. The command queue approach is now fully uniform (no atomic flags remaining for add/remove). The FIFO pipe works in parallel. The code is easily extensible to new input sources. |
## Detailed Remarks ## Detailed Remarks
### 1. Mocked / Left Undone ### 1. Mocked / Left Undone
- `CMD_STOP` is defined and handled in `apply_command`, and the FIFO recognises `"stop"`, but the MIDI handler never sends `CMD_STOP`. This is not an error, just an unused path. - `CMD_STOP` is only reachable via the FIFO pipe (`"stop"`). The MIDI handler never sends it. This is not a bug, just an unused feature path.
- The MIDI handler still uses `atomic_store(&cmd_add, 1)` and `atomic_store(&cmd_remove, 1)` for add/remove. This works but breaks uniformity – could have used `CMD_ADD_CHANNEL` / `CMD_REMOVE_CHANNEL` command types (which are not even defined in `cmd_type_t` yet). The current approach is functional. - The FIFO pipe is completely untested in the main integration suite. All existing tests use MIDI notes. Adding a FIFO‑based test would increase coverage.
- The test file (`tests/integration.c`) is **out‑of‑sync** with the actual MIDI mapping: - No other functionality is missing.
- `test_looper_looping` sends note `1` – but the looper now expects note `1` to cycle channel 0. That works.
- `test_multiple_channels` sends note `60` – works (triggers `cmd_add`).
- `test_control_key_modifier` sends control key (64) then note `62`. The looper expects control key + note `62` to toggle the bound channel – but note `62` is **also** triggered by the control‑key branch. That matches and should work.
- `test_bind_channel` sends control key + note `0` to bind, then control+62 to toggle. The looper binds channel 0 with note `0` under control‑key (note <16). That works.
- `test_bind_unbind` sends control+63 for unbind – the looper handles that (`case 63: CMD_UNBIND`). Works.
- `test_remove_channel` sends note `61` – works.
- **However, there is no test that uses the FIFO pipe** – it remains untested in the suite.
- **More importantly, the test code does not verify that the looper’s output port connections are correct** when using the control‑key modifier tests. The tests assume the looper has only one audio input/output pair, but after adding channels, there are more ports – connections may fail silently. This could cause the tests to hang or fail.
- No tests for `"stop"` via FIFO or MIDI.
### 2. Potential Segfaults ### 2. Potential Segfaults
- All audio/MIDI port buffer accesses are guarded (`if (!out) continue` etc.). No dangling pointers. - Every `jack_port_get_buffer()` call is followed by a null check (`if (!out) continue;`).
- The command queue is fixed‑size; push returns false when full – caller does not check return value in all places (e.g., in `midi_handle_events` the return value is ignored). If the queue fills, notes are dropped silently – not a segfault, but a functional limitation. - Array accesses are bounded by `MAX_CHANNELS` and `QUEUE_CAPACITY`.
- No use of `malloc` in RT path. - No use of `malloc` or variable‑length arrays in the real‑time callback.
- The only unguarded `jack_port_get_buffer()` is in `midi_handle_events` where the caller already checks `midi_ctrl_buf` against NULL. Safe.
### 3. Memory Safety ### 3. Memory Safety
- No memory leaks. The only allocations happen at startup (JACK ports, thread creation). No `free` of static buffers. - All `loop_buffer` arrays and command queue buffers are static globals. No heap allocation in RT context.
- The FIFO reader uses a stack‑allocated `char line[256]` – safe. - Port unregistration is deferred until after the RT thread has surely passed the `active=0` check (via `global_rt_cycles`). No use‑after‑unregister possible.
- The SPSC queue buffer is a static global – no dynamic allocation. - The FIFO reader thread uses a stack‑allocated line buffer – safe.
- No memory leaks are present.
### 4. Thread Safety / Race Conditions ### 4. Thread Safety / Race Conditions
- The SPSC queue is correctly implemented with atomic ordering. Producer (MIDI handler, FIFO thread) and consumer (RT callback) are single‑writer, single‑reader. - **RT‑safe commands queue (`cmd_queue`)** – single writer (MIDI handler, called from RT callback) and single reader (the same callback, immediately after writing). Correct.
- The `channels` struct fields `state`, `active` are atomic – correct. `prev_state` is plain `int` but accessed only from the RT callback (single thread) – safe. - **Add/remove command queues** – two separate SPSC queues:
- The `control_key_active` flag is atomic and used correctly. - `cmd_queue_main_midi`: written only by the RT callback (via `midi_handle_events`).
- The main loop (`looper_process_commands`) runs in the non‑RT main thread and reads/writes `channels[idx].audio_in`, `channels[idx].audio_out` after verifying `active == 0`. This is safe because the RT callback skips inactive channels. - `cmd_queue_main_fifo`: written only by the FIFO reader thread (non‑RT).
- **Potential time‑of‑check/time‑of‑use**: When `looper_process_commands` calls `channel_remove`, it sets `active = 0` and marks `pending_unregister_idx`. In the next iteration, it calls `jack_port_unregister`. Meanwhile, the RT callback could have just loaded `active == 1` and then the port pointers become invalid? No – because the RT callback checks `atomic_load(&channels[c].active)` and if it sees `1`, it uses the port pointers. If the main thread sets `active = 0` and then later unregisters, the RT thread might have already passed the check and is about to use the port pointer – that would be a use‑after‑unregister. **This is a real race.** The main loop waits one cycle (50 ms) before unregistering, but the RT thread can still be in the middle of a process cycle when `active` is set to 0. The window is narrow but possible. A safer approach would be to **not unregister ports while the RT thread could be using them** – for example, use a double‑buffer or delay unregistration by at least one JACK period using a `jack_ringbuffer` or an atomic counter. Currently, it is not 100% safe. **Consider this a moderate race condition.** Both are read only by the main loop (single consumer). No concurrent writes to the same queue.
- `global_rt_cycles` is incremented with `memory_order_release` at the end of every process callback. The main loop reads it with implicit acquire. This ensures visibility of the store to `active` and prevents unregistering ports while the RT thread may still be using them.
- `channel_add()` and `channel_remove()` are called only from the main loop, never from the RT callback. The RT callback reads `active`, `state`, `audio_in`, `audio_out` (all atomic). Safe.
- `prev_state` is a plain `int` but written and read only from the RT callback – no data race.
### 5. Performance ### 5. Performance
- The RT callback is lean: one queue drain, then per‑channel audio processing with simple state‑machine branches. No syscalls, no allocations. - The RT callback performs:
- The only potential performance bottleneck is the per‑sample `fabsf()` in the test client – not in the looper itself. Looper’s performance is fine. 1. MIDI event processing (may push to `cmd_queue` and `cmd_queue_main_midi`).
2. Drain `cmd_queue` (O(1) per command, usually 0–2 commands).
3. Per‑channel audio processing (linear buffer copy or playback).
4. MIDI clock event handling (rare).
5. Increment `global_rt_cycles` (atomic store).
- No syscalls, no locks, no `printf` in the RT path.
- The main loop runs at 50 ms intervals; draining two queues is negligible.
### 6. Architectural Soundness ### 6. Architectural Soundness
- The separation into MIDI handler (producer), RT callback (consumer), and main loop (housekeeping) is sound. The command queue is a good abstraction. - The design cleanly separates RT‑safe command handling (immediate in the process callback) from non‑RT operations (deferred to the main loop).
- Inconsistency: add/remove uses atomic flags; other commands use the queue. This is a minor design smell but works for now. Future unification would be beneficial. - Each input source (MIDI, FIFO) has its own dedicated SPSC queue for commands that must be processed outside the RT thread. This avoids the multi‑producer race that existed before.
- The FIFO reader thread is correctly detached and won't block shutdown (but if the looper exits, the thread remains until the pipe is closed – acceptable). - All commands are represented by a uniform `command_t` structure with a typed enum. No ad‑hoc atomic flags remain.
- The test file is overly ambitious and seems to have been written before the code – it tests features that are not implemented (like the control‑key modifier with note numbers that were never assigned to those commands in the original specification). This may reflect a misunderstanding between the test author and the code author. - The FIFO pipe reader runs in a detached thread – simple and non‑blocking.
- The code is easy to extend: adding a new input source (e.g., network socket) would involve creating a new SPSC queue and another drain loop.
## Overall Verdict ## Overall Verdict
The code is **functional and safe for basic use** (single‑channel looping, add/remove channels, FIFO control). It has a **minor race condition** when removing channels (use‑after‑unregister risk) and a **moderate inconsistency** between atomic flags and command queue. The **test suite is unreliable** because it expects a mapping that does not match the code’s actual note assignments in some scenarios. No segfaults, no memory leaks, good performance. The code is **safe, race‑free, and architecturally sound**. It meets all real‑time constraints and correctly implements the looper’s state machine with unified command handling from MIDI and a FIFO pipe.
**Recommendations:** **Minor remaining items:**
- Fix the race in channel removal by using a ringbuffer or ensuring the RT thread has completed at least one cycle after marking `active = 0` before unregistering. - The FIFO pipe is untested in the integration suite.
- Unify all commands (including add/remove) into the command queue for consistency. - `CMD_STOP` is not triggered from MIDI (only from FIFO).
- Update the test suite to match the actual note mapping and to test the FIFO pipe. - The existing `evaluation.md` is outdated and should be replaced with this evaluation.