From 137727c4e12a4ad2350d1ce22d45e03fec6e510a Mon Sep 17 00:00:00 2001 From: skal Date: Thu, 5 Feb 2026 18:44:31 +0100 Subject: 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 --- TODO.md | 12 ++ doc/AUDIO_LIFECYCLE_REFACTOR.md | 316 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 328 insertions(+) create mode 100644 doc/AUDIO_LIFECYCLE_REFACTOR.md 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 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 -- cgit v1.2.3