summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorskal <pascal.massimino@gmail.com>2026-02-05 18:44:31 +0100
committerskal <pascal.massimino@gmail.com>2026-02-05 18:44:31 +0100
commit137727c4e12a4ad2350d1ce22d45e03fec6e510a (patch)
tree19558f608401ffa35e3b2fb710cfe0c29caf0de4
parent7721f574fad99949f85b4caf1e196b957fc0cf51 (diff)
docs: Add Audio Lifecycle Refactor plan (Task #56)
Created comprehensive design document for refactoring the tracker-synth relationship to eliminate initialization order dependencies. Proposed solution: AudioEngine class that manages both synth and tracker as private members, providing order-independent initialization and clear ownership semantics. Three options analyzed: - Option A: Unified AudioEngine (recommended) - Option B: Registration Handle System - Option C: Reference-Counted Resources Estimated effort: 2-3 weeks with incremental migration path. Binary size impact: ~500 bytes (acceptable). See doc/AUDIO_LIFECYCLE_REFACTOR.md for complete design rationale, implementation plan, and decision matrix. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
-rw-r--r--TODO.md12
-rw-r--r--doc/AUDIO_LIFECYCLE_REFACTOR.md316
2 files changed, 328 insertions, 0 deletions
diff --git a/TODO.md b/TODO.md
index 3322304..fbe4060 100644
--- a/TODO.md
+++ b/TODO.md
@@ -113,6 +113,18 @@ This file tracks prioritized tasks with detailed attack plans.
- [x] **Enhanced Procedural Noise:** Implemented a multi-octave Value Noise generator for higher-quality skybox textures.
- [x] **Scene Integrity:** Restored proper object indexing and removed redundant geometry, ensuring the floor grid and objects render correctly.
+## Priority 1.5: Audio System Architecture (Task #56)
+**Goal:** Refactor tracker-synth relationship to eliminate fragile initialization order dependency.
+
+- [ ] **Task #56: Audio Lifecycle Refactor**
+ - **Problem:** Current system requires synth_init() before tracker_init() or spectrograms get cleared
+ - **Impact:** Fragile, hard to test, prevents composition, causes subtle bugs
+ - **Proposed Solution:** Unified AudioEngine class that manages synth and tracker as members
+ - **See:** `doc/AUDIO_LIFECYCLE_REFACTOR.md` for detailed design options and implementation plan
+ - **Effort:** 2-3 weeks (incremental migration path available)
+ - **Size Impact:** ~500 bytes (acceptable for robustness gain)
+ - **Priority:** Medium (current workaround functional but fragile)
+
## Priority 2: 3D System Enhancements (Task #18)
**Goal:** Establish a pipeline for importing complex 3D scenes to replace hardcoded geometry.
- [ ] **Task #18.0: Basic OBJ Asset Pipeline** (New)
diff --git a/doc/AUDIO_LIFECYCLE_REFACTOR.md b/doc/AUDIO_LIFECYCLE_REFACTOR.md
new file mode 100644
index 0000000..28de5a2
--- /dev/null
+++ b/doc/AUDIO_LIFECYCLE_REFACTOR.md
@@ -0,0 +1,316 @@
+# Audio System Lifecycle Refactor Plan
+
+## Problem Statement
+
+The current audio system has a fragile initialization order dependency between `synth_init()` and `tracker_init()`:
+
+- `synth_init()` clears ALL registered spectrograms (global state reset)
+- `tracker_init()` registers spectrograms with the synth
+- If called in wrong order → spectrograms cleared → no audio / test failures
+
+**Current Workarounds:**
+- Tests must call `synth_init()` before `tracker_init()`
+- `tracker_init()` re-registers everything on every call (memory overhead)
+- Hidden bugs when `audio_init()` internally calls `synth_init()`
+
+**Why This is Bad:**
+- Brittle: Easy to break by calling init functions in wrong order
+- Non-obvious: No compile-time or runtime checks for correct usage
+- Wasteful: Re-allocates and re-registers on every init
+- Hard to test: Tests must know internal implementation details
+- Prevents composition: Can't have multiple tracker instances
+
+---
+
+## Design Goals
+
+1. **Order Independence:** Init functions should work in any order
+2. **Explicit Ownership:** Clear who owns what resources
+3. **Testability:** Easy to mock, inject, and test components
+4. **Memory Safety:** No leaks, no dangling pointers
+5. **Size Conscious:** Minimal overhead for 64k binary goal
+
+---
+
+## Proposed Solutions (3 Options)
+
+### Option A: Unified AudioEngine Class (Recommended)
+
+**Concept:** Single top-level class that manages synth and tracker as private members.
+
+```cpp
+class AudioEngine {
+ public:
+ // Lifecycle
+ void init();
+ void shutdown();
+ void reset(); // Clear all state
+
+ // Synth interface (delegates to private synth_)
+ int register_spectrogram(const Spectrogram* spec);
+ void trigger_voice(int spec_id, float volume, float pan);
+ void render(float* output, int num_frames);
+
+ // Tracker interface (delegates to private tracker_)
+ void load_music_data(const TrackerScore* score);
+ void update_tracker(float music_time);
+
+ private:
+ Synth synth_; // Embedded instance
+ Tracker tracker_; // Embedded instance
+ bool initialized_ = false;
+};
+```
+
+**Pros:**
+- Single initialization point: `engine.init()` sets up everything
+- Order independence: Internal init sequence is correct by design
+- Clear ownership: Engine owns both synth and tracker
+- Easy to test: Mock the entire engine or inject test data
+
+**Cons:**
+- Larger refactor: Need to update all callsites
+- Increased coupling: Synth and tracker bundled together
+- Slightly larger binary (vtable overhead if using virtual methods)
+
+**Implementation Steps:**
+1. Create `AudioEngine` class in `src/audio/audio_engine.h/cc`
+2. Move synth and tracker global state into class members
+3. Update `audio.cc` to use `AudioEngine` internally
+4. Refactor tests to use `AudioEngine` API
+5. Gradually migrate production code
+6. Remove old `synth_init()` / `tracker_init()` globals
+
+---
+
+### Option B: Registration Handle System
+
+**Concept:** Tracker holds registration handles and manages synth resources explicitly.
+
+```cpp
+class Tracker {
+ public:
+ void init(Synth* synth); // Takes synth pointer, registers spectrograms
+ void shutdown(); // Unregisters spectrograms
+ void update(float music_time);
+
+ private:
+ Synth* synth_ = nullptr;
+ std::vector<int> registered_spec_ids_; // Tracks what we registered
+};
+
+// Usage:
+Synth synth;
+synth.init();
+
+Tracker tracker;
+tracker.init(&synth); // Registers spectrograms
+
+// Later, if synth is reset:
+synth.reset();
+tracker.re_register(); // Tracker knows what to re-register
+```
+
+**Pros:**
+- Explicit dependencies: Tracker knows it depends on Synth
+- Order enforced by API: Can't init tracker without synth pointer
+- Resource cleanup: Tracker can unregister on shutdown
+- Smaller refactor: Only changes tracker implementation
+
+**Cons:**
+- Still has global synth state (just less fragile)
+- Tracker needs to track registered IDs (extra bookkeeping)
+- Multiple trackers would need careful coordination
+
+**Implementation Steps:**
+1. Add `Synth* synth_` member to Tracker
+2. Change `tracker_init()` to `tracker_init(Synth* synth)`
+3. Store registered spec IDs in tracker
+4. Add `tracker_re_register()` to handle synth resets
+5. Update all callsites to pass synth pointer
+
+---
+
+### Option C: Synth Resource Manager with Reference Counting
+
+**Concept:** Spectrograms are ref-counted, auto-cleanup when no refs remain.
+
+```cpp
+class Synth {
+ public:
+ // Returns a handle that auto-releases on destruction
+ SpectrogramHandle register_spectrogram(const Spectrogram* spec);
+
+ void trigger_voice(const SpectrogramHandle& handle, float volume, float pan);
+};
+
+class SpectrogramHandle {
+ public:
+ ~SpectrogramHandle() { if (synth_) synth_->unregister(id_); }
+ SpectrogramHandle(const SpectrogramHandle&); // Ref count++
+ int id() const { return id_; }
+
+ private:
+ Synth* synth_;
+ int id_;
+};
+```
+
+**Pros:**
+- RAII: Automatic cleanup, no leaks
+- Safe against synth resets: Handles become invalid automatically
+- Modern C++ design: Follows std::shared_ptr pattern
+
+**Cons:**
+- Complex implementation: Ref counting overhead
+- Larger binary: More code for handle management
+- Doesn't solve init order (still need synth before tracker)
+
+**Implementation Steps:**
+1. Create `SpectrogramHandle` class with ref counting
+2. Update `synth_register_spectrogram()` to return handle
+3. Add `synth_unregister()` internal API
+4. Update tracker to store handles instead of IDs
+5. Test thoroughly (ref counting bugs are subtle)
+
+---
+
+## Recommended Approach: Option A (AudioEngine)
+
+**Why Option A?**
+- Most robust: Completely solves initialization order issues
+- Future-proof: Easy to extend with more audio subsystems
+- Testable: Single point of dependency injection
+- Size acceptable: ~500 bytes overhead (acceptable for robustness)
+
+**Migration Path (Incremental):**
+1. Create `AudioEngine` class alongside existing globals
+2. Update tests to use `AudioEngine` first (validate correctness)
+3. Add `audio_engine.cc` to build, mark old functions deprecated
+4. Migrate `main.cc` and production code
+5. Remove old `synth_init()` / `tracker_init()` in final cleanup
+
+**Estimated Effort:**
+- Week 1: Design + implement AudioEngine class
+- Week 2: Migrate tests + validate
+- Week 3: Migrate production code
+- Week 4: Cleanup + documentation
+
+---
+
+## Alternative: Keep Current Design with Better Documentation
+
+If refactor is too costly, improve current system:
+
+1. **Add Assertions:**
+ ```cpp
+ void tracker_init() {
+ assert(synth_is_initialized() && "Must call synth_init() first!");
+ // ... rest of init
+ }
+ ```
+
+2. **Add State Tracking:**
+ ```cpp
+ static bool g_synth_initialized = false;
+ void synth_init() {
+ // ... init code
+ g_synth_initialized = true;
+ }
+ ```
+
+3. **Document Init Order:**
+ - Update `CONTRIBUTING.md` with explicit init order requirements
+ - Add comments to every init function
+ - Create example code snippets
+
+**Pros:** Zero refactor cost
+**Cons:** Doesn't solve underlying fragility
+
+---
+
+## Decision Matrix
+
+| Criterion | Option A (Engine) | Option B (Handles) | Option C (RefCount) | Status Quo |
+|-----------|-------------------|-------------------|-------------------|------------|
+| Order Independence | ✅ Full | ⚠️ Partial | ❌ No | ❌ No |
+| Testability | ✅ Excellent | ✅ Good | ⚠️ Moderate | ❌ Poor |
+| Memory Safety | ✅ Full | ✅ Good | ✅ Excellent | ⚠️ Manual |
+| Refactor Effort | 🔴 High | 🟡 Medium | 🔴 High | 🟢 None |
+| Binary Size Impact | +500B | +200B | +1KB | 0B |
+| Future Extensibility | ✅ High | ⚠️ Medium | ⚠️ Medium | ❌ Low |
+
+**Recommendation:** Option A for production, Status Quo improvements for short-term
+
+---
+
+## Implementation Plan (Option A)
+
+### Phase 1: Design & Prototype (3-5 days)
+
+- [ ] Create `src/audio/audio_engine.h` with class interface
+- [ ] Implement `AudioEngine::init()` / `shutdown()` / `reset()`
+- [ ] Add delegation methods for synth/tracker APIs
+- [ ] Write unit tests for `AudioEngine` lifecycle
+
+### Phase 2: Test Migration (3-5 days)
+
+- [ ] Update `test_tracker.cc` to use `AudioEngine`
+- [ ] Update `test_tracker_timing.cc`
+- [ ] Update `test_variable_tempo.cc`
+- [ ] Update `test_wav_dump.cc`
+- [ ] Ensure 100% test pass rate
+
+### Phase 3: Production Integration (5-7 days)
+
+- [ ] Update `audio.cc` to use `AudioEngine` internally
+- [ ] Update `main.cc` demo loop
+- [ ] Update all effect classes that use audio
+- [ ] Add backwards compatibility shims (temporary)
+
+### Phase 4: Cleanup (2-3 days)
+
+- [ ] Remove old `synth_init()` / `tracker_init()` functions
+- [ ] Remove global synth/tracker state
+- [ ] Update documentation (HOWTO.md, CONTRIBUTING.md)
+- [ ] Remove compatibility shims
+
+### Phase 5: Optimization (Optional, 2-3 days)
+
+- [ ] Profile binary size impact
+- [ ] Optimize if >1KB overhead
+- [ ] Consider `#if !defined(STRIP_ALL)` for test-only features
+
+---
+
+## Success Criteria
+
+1. **No Init Order Failures:** Tests pass regardless of call order
+2. **Memory Clean:** No leaks detected by sanitizers
+3. **Size Budget:** <1KB binary size increase
+4. **API Simplicity:** Single `engine.init()` call for users
+5. **Test Coverage:** 100% coverage of lifecycle edge cases
+
+---
+
+## Open Questions
+
+1. Should `AudioEngine` be a singleton or instance-based?
+ - **Proposal:** Instance-based for testability, but provide global getter for convenience
+
+2. How to handle legacy code during migration?
+ - **Proposal:** Keep old functions as deprecated wrappers for 1-2 releases
+
+3. Should synth and tracker be members or pointers?
+ - **Proposal:** Direct members (smaller, faster, simpler ownership)
+
+4. What about backend abstraction (MiniaudioBackend, MockBackend)?
+ - **Proposal:** AudioEngine owns backend pointer, injected via `set_backend()`
+
+---
+
+## References
+
+- Current issue: Commit 7721f57 "fix(audio): Resolve tracker test failures..."
+- Related: `doc/TRACKER.md`, `doc/ASSET_SYSTEM.md`
+- Design patterns: Facade pattern, Dependency Injection