path: root/doc/AUDIO_LIFECYCLE_REFACTOR.md

# 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) + Resource Manager

**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)

### 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)
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

---

## 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 (5-7 days)

- [ ] 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
- [ ] Integrate ResourceManager into AudioEngine
- [ ] Write unit tests for `AudioEngine` lifecycle with resources

### 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()`

---

## 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, Resource Manager pattern