62 lines
4.9 KiB
Markdown
62 lines
4.9 KiB
Markdown
# Evaluation of Looper Codebase
|
||
|
||
## Summary Table
|
||
|
||
| Category | Rating | Notes |
|
||
|-------------------------|--------|-------|
|
||
| **Mocked / left‑doing** | ⚠️ Moderate | No mock objects; real Carla dependency. Tests for Carla host are stubs. Integration test requires running JACK server. Script module test now passes 7/7 (empty macro bug fixed). |
|
||
| **Potential segfaults** | ✅ Low | `exec_command` validates channel bounds (`ch < MAX_CHANNELS`). `note_actions` is checked for NULL before use. `strdup` returns not checked but safe in practice. No out‑of‑bounds access identified. |
|
||
| **Memory management** | ✅ Good | `note_actions` strings freed via `script_cleanup()` called in `tui_cleanup()`. `strdup` allocations are freed on reload. All dynamically allocated audio buffers are freed. No leaks at exit. |
|
||
| **Thread safety** | ✅ Good | All shared scene/channel fields use C11 atomics. Logging mutex never held in audio thread. Save deactivation uses atomic active flag with two wait periods (500ms + 200ms) guaranteeing RT thread sees the change. FIFO writes are atomic (`PIPE_BUF`). No race conditions identified. |
|
||
| **Performance** | ✅ Fair | Audio path: `memcpy`, linear loops, no allocations – real‑time safe. Main loop sleeps 50 ms – fine. Save pauses audio for 500 ms (acceptable for a tool). Logging adds negligible mutex overhead outside RT thread. JACK callback time is deterministic. |
|
||
| **Architectural soundness** | ⚠️ Medium | Separation of engine and client via FIFO is clean. Orchestrator simple but effective. Three command queues still add unnecessary complexity (single queue would suffice). `prev_state` transition detection works. Carla integration remains tightly coupled to JACK client. Script/macro mechanism is extensible and well isolated. Logging design correctly avoids audio thread. |
|
||
|
||
## Detailed Commentary
|
||
|
||
### 1. Mocked / Left‑doing
|
||
- **Engine tests** require a live JACK server – no mocking.
|
||
- **Client tests** for `carla_host` and `plugins` use stubs when `TESTING` is defined; real Carla library is still linked. Integration test also requires JACK.
|
||
- **Script tests** pass 7/7 after the empty macro bug was fixed.
|
||
- **No mock for MIDI** – engine integration test uses actual MIDI events.
|
||
|
||
### 2. Potential Segfaults
|
||
- **Channel bounds** are now validated in `exec_command`: `if (ch < 0 || ch >= MAX_CHANNELS) ch = 0;`. Safe.
|
||
- **NULL pointer dereference**: `script_handle_note` checks for NULL before using `macro`. `strdup` failure would set `note_actions[note]` to NULL, then checked. Safe.
|
||
- **FIFO write errors** silently ignored – no crash.
|
||
|
||
### 3. Memory Management
|
||
- `note_actions[]` strings are freed on every `script_load` write and on cleanup (`script_cleanup` called in `tui_cleanup`). No leak.
|
||
- Audio loop buffer (`loop_data_t`) is embedded in `scene_t` – no heap allocation.
|
||
- Save path uses `malloc/free` correctly; freed after write.
|
||
- Log file pointer is closed at exit.
|
||
|
||
### 4. Thread Safety
|
||
- All `scene_t` fields accessed from both threads are atomic (`state`, `prev_state`, `record_pos`, `loop_count`, `playback_pos`). Correct.
|
||
- `channel_t.active`, `channel_t.save_ring` are atomic. Correct.
|
||
- **Save sequence**: set `active=0`, sleep 500 ms, copy buffer, set `active=1`. The sleep guarantees the RT thread has seen the deactivation. No race.
|
||
- **Logging mutex** acquired only outside audio thread – fine.
|
||
- **FIFO writes** from multiple threads are not serialized but `write` to a FIFO is atomic for writes ≤ `PIPE_BUF` (4096 bytes). Our messages are smaller. Safe.
|
||
|
||
### 5. Performance
|
||
- Audio buffer processing uses simple loops with no function calls. `nframes` is typically 64‑256 – fine.
|
||
- `state` transitions check `prev_state` each callback – cheap.
|
||
- Save mechanism: 500 ms pause may cause one xrun. Acceptable for a prototype.
|
||
- Main loop sleep 50 ms ensures low CPU usage.
|
||
|
||
### 6. Architectural Soundness
|
||
- **Good**: Clear separation between engine (real‑time audio) and client (UI, plugin management). Communication via FIFO files.
|
||
- **Weakness**: Three command queues (`cmd_queue`, `cmd_queue_main_midi`, `cmd_queue_main_fifo`) are redundant – all feed `exec_command`. Could consolidate.
|
||
- **Weakness**: Carla integration tied to the engine’s JACK client. A separate Carla engine instance would be cleaner.
|
||
- **Strength**: Script/macro system is simple text‑based and extensible. The notes FIFO allows any external controller to inject note numbers.
|
||
- **Strength**: Logging non‑intrusive and never used in real‑time path.
|
||
|
||
---
|
||
|
||
*Overall, the codebase is functional and stable.* All previously identified critical issues (channel bounds, memory leak, empty macro) have been fixed. Recommendations for further improvement:
|
||
- Replace three command queues with a single queue.
|
||
- Use a double‑buffer for save to eliminate the 500 ms pause.
|
||
- Consider mock objects for engine tests to remove JACK dependency.
|
||
- Add more unit tests for edge cases.
|
||
|
||
**Evaluation date**: 18 May 2026
|