diff options
| -rw-r--r-- | doc/AUDIO_LIFECYCLE_REFACTOR.md | 265 |
1 files changed, 260 insertions, 5 deletions
diff --git a/doc/AUDIO_LIFECYCLE_REFACTOR.md b/doc/AUDIO_LIFECYCLE_REFACTOR.md index 28de5a2..adf0045 100644 --- a/doc/AUDIO_LIFECYCLE_REFACTOR.md +++ b/doc/AUDIO_LIFECYCLE_REFACTOR.md @@ -175,7 +175,7 @@ class SpectrogramHandle { --- -## Recommended Approach: Option A (AudioEngine) +## Recommended Approach: Option A (AudioEngine) + Resource Manager **Why Option A?** - Most robust: Completely solves initialization order issues @@ -183,6 +183,45 @@ class SpectrogramHandle { - Testable: Single point of dependency injection - Size acceptable: ~500 bytes overhead (acceptable for robustness) +### Component Separation: Addressing Asset/Procedural Spectrograms + +**Current Problem:** +Tracker currently handles 3 distinct responsibilities: +1. Loading assets via `GetAsset()` (AssetManager dependency) +2. Generating procedural spectrograms (owns allocated memory) +3. Pattern sequencing (triggers events at correct times) + +**Proposed Architecture:** + +``` + ┌─────────────────┐ + │ AudioEngine │ + │ (Facade) │ + └────────┬────────┘ + │ + ┌─────────────────┼─────────────────┐ + │ │ │ + ┌──────▼──────┐ ┌─────▼──────┐ ┌─────▼────────┐ + │ Synth │ │ Tracker │ │ Resource │ + │ │ │ │ │ Manager │ + │ (Playback) │ │ (Sequence) │ │ (Loading) │ + └─────────────┘ └────────────┘ └──────┬───────┘ + │ + ┌──────────┼──────────┐ + │ │ + ┌──────▼──────┐ ┌──────▼──────┐ + │ AssetManager│ │ Procedural │ + │ (Assets) │ │ Generator │ + └─────────────┘ └─────────────┘ +``` + +**SpectrogramResourceManager Responsibilities:** +- Load asset spectrograms from AssetManager +- Generate procedural spectrograms from NoteParams +- Own all allocated memory (consistent ownership) +- Provide unified interface for both types +- Cache resources to avoid re-generation + **Migration Path (Incremental):** 1. Create `AudioEngine` class alongside existing globals 2. Update tests to use `AudioEngine` first (validate correctness) @@ -244,14 +283,166 @@ If refactor is too costly, improve current system: --- +## Detailed Design: SpectrogramResourceManager + +### Interface + +```cpp +class SpectrogramResourceManager { + public: + // Lifecycle + void init(); + void shutdown(); // Frees all owned memory + + // Loading API + int load_asset_spectrogram(AssetId asset_id); + int generate_procedural_spectrogram(const NoteParams& params); + + // Query API + const Spectrogram* get_spectrogram(int resource_id) const; + bool is_valid(int resource_id) const; + + // Cache management + void clear_cache(); + int get_cache_size() const; + + private: + struct Resource { + Spectrogram spec; + float* owned_data; // nullptr if asset (not owned) + AssetId asset_id; // ASSET_LAST_ID if procedural + bool is_procedural; + }; + + Resource resources_[MAX_SPECTROGRAMS]; + int next_slot_ = 0; +}; +``` + +### Usage in AudioEngine + +```cpp +class AudioEngine { + public: + void init() { + synth_.init(); + resource_mgr_.init(); + tracker_.init(&synth_, &resource_mgr_); + } + + void load_music_data(const TrackerScore* score, + const NoteParams* samples, + const AssetId* sample_assets, + uint32_t sample_count) { + // Load all resources ONCE at init + for (uint32_t i = 0; i < sample_count; ++i) { + int resource_id; + if (sample_assets[i] != AssetId::ASSET_LAST_ID) { + resource_id = resource_mgr_.load_asset_spectrogram(sample_assets[i]); + } else { + resource_id = resource_mgr_.generate_procedural_spectrogram(samples[i]); + } + + // Register with synth + const Spectrogram* spec = resource_mgr_.get_spectrogram(resource_id); + int synth_id = synth_.register_spectrogram(spec); + + // Store mapping for tracker + sample_to_synth_id_[i] = synth_id; + } + + tracker_.load_score(score, sample_to_synth_id_); + } + + private: + Synth synth_; + Tracker tracker_; + SpectrogramResourceManager resource_mgr_; + int sample_to_synth_id_[256]; +}; +``` + +### Memory Ownership Rules + +**Clear Ownership:** +1. **Assets:** AssetManager owns, ResourceManager borrows (pointer only) +2. **Procedurals:** ResourceManager owns (allocated with `new[]`, freed in `shutdown()`) +3. **Synth:** Only stores pointers, never owns data + +**Lifetime Guarantees:** +- Assets live until program exit (static data) +- Procedurals live until `ResourceManager::shutdown()` +- Synth references valid as long as ResourceManager exists + +### Benefits Over Current Design + +| Aspect | Current (Tracker-owned) | Proposed (ResourceManager) | +|--------|------------------------|---------------------------| +| **Ownership** | Split (assets borrowed, procedurals owned) | Centralized (clear rules) | +| **Coupling** | Tracker → AssetManager + gen | ResourceManager → AssetManager + gen | +| **Testability** | Hard (global GetAsset) | Easy (inject mock manager) | +| **Reusability** | Tracker-specific | Usable by any audio component | +| **Memory Safety** | Manual (delete[] in init) | RAII in shutdown() | + +### Integration with AssetManager + +**Option 1: Direct Dependency (Simple)** +```cpp +int SpectrogramResourceManager::load_asset_spectrogram(AssetId id) { + size_t size; + const uint8_t* data = GetAsset(id, &size); // Direct call + // ... parse and store +} +``` + +**Option 2: Injected Interface (Testable)** +```cpp +class IAssetProvider { + public: + virtual const uint8_t* get_asset(AssetId id, size_t* size) = 0; +}; + +class SpectrogramResourceManager { + public: + void set_asset_provider(IAssetProvider* provider) { + asset_provider_ = provider; + } + + int load_asset_spectrogram(AssetId id) { + const uint8_t* data = asset_provider_->get_asset(id, &size); + // ... parse and store + } + + private: + IAssetProvider* asset_provider_ = nullptr; +}; + +// Production: Adapter for AssetManager +class AssetManagerAdapter : public IAssetProvider { + public: + const uint8_t* get_asset(AssetId id, size_t* size) override { + return GetAsset(id, size); // Delegate to global + } +}; +``` + +**Recommendation:** Option 1 for now (simpler), Option 2 if we refactor AssetManager too + +--- + ## Implementation Plan (Option A) -### Phase 1: Design & Prototype (3-5 days) +### Phase 1: Design & Prototype (5-7 days) -- [ ] Create `src/audio/audio_engine.h` with class interface +- [ ] Create `src/audio/spectrogram_resource_manager.h/cc` + - [ ] Implement resource loading (assets + procedurals) + - [ ] Add memory ownership tracking + - [ ] Write unit tests for resource lifecycle +- [ ] Create `src/audio/audio_engine.h/cc` with class interface - [ ] Implement `AudioEngine::init()` / `shutdown()` / `reset()` - [ ] Add delegation methods for synth/tracker APIs -- [ ] Write unit tests for `AudioEngine` lifecycle +- [ ] Integrate ResourceManager into AudioEngine +- [ ] Write unit tests for `AudioEngine` lifecycle with resources ### Phase 2: Test Migration (3-5 days) @@ -309,8 +500,72 @@ If refactor is too costly, improve current system: --- +## FAQ + +### Q: How does this handle procedural vs asset spectrograms? + +**A:** Via a dedicated `SpectrogramResourceManager`: + +- **Assets:** Loaded from AssetManager, ResourceManager stores pointer only (doesn't own) +- **Procedurals:** Generated via `generate_note_spectrogram()`, ResourceManager owns memory +- **Unified Interface:** Both types returned as `const Spectrogram*`, caller doesn't care about source + +**Key Insight:** Separating resource loading from pattern sequencing allows: +- Tracker focuses on timing/triggering (single responsibility) +- Resources loaded once at init (no per-frame overhead) +- Easy to add new spectrogram sources (streaming, compressed, etc.) + +### Q: Does AudioEngine depend on AssetManager? + +**A:** Indirectly, via ResourceManager: + +``` +AudioEngine → ResourceManager → AssetManager (via GetAsset) + ↘ Tracker (no asset dependency) + ↘ Synth (no asset dependency) +``` + +**Benefits:** +- Only one component knows about assets (ResourceManager) +- Easy to mock for testing (inject fake ResourceManager) +- If AssetManager is refactored, only ResourceManager needs updating + +### Q: What happens to the current caching mechanism? + +**A:** Improved and centralized: + +**Current (Tracker-owned):** +```cpp +static int g_sample_synth_cache[256]; // Maps sample_id → synth_id +``` + +**Proposed (AudioEngine-owned):** +```cpp +class AudioEngine { + private: + int sample_to_synth_id_[256]; // Clearer ownership +}; +``` + +**Why better?** +- Cache lifetime matches engine lifetime (no stale data) +- Cleared automatically on `engine.reset()` +- Testable (can verify cache contents) + +### Q: How does this affect binary size? + +**Estimated overhead:** +- `SpectrogramResourceManager`: ~300 bytes (struct + logic) +- `AudioEngine` wrapper: ~200 bytes (vtable + members) +- Total: **~500 bytes** (0.8% of 64KB budget) + +**Savings from removing global state:** ~100 bytes +**Net impact:** **~400 bytes** (acceptable for robustness) + +--- + ## 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 +- Design patterns: Facade pattern, Dependency Injection, Resource Manager pattern |