diff --git a/docs/requirements.md b/docs/requirements.md index d23fb9ba..8055f6ff 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -41,7 +41,7 @@ For a narrative overview of the system design, see | UR-028 | Navigate to artist/album by tapping names in now playing view | High | Done | | UR-029 | Toggle between grid and list view in library | Medium | Done | | UR-030 | Quick genre browsing and filtering | Medium | Done | -| UR-031 | Crossfade between audio tracks | Low | Done (Linux only) | +| UR-031 | Crossfade between audio tracks | Low | Not implemented (blocked — see DR-034) | | UR-032 | Gapless playback for seamless album listening | Medium | Done (Linux only) | | UR-033 | Volume normalization to prevent volume jumps between tracks | Low | Done (Linux only) | | UR-034 | Rich home screen with hero banners, carousels, and personalized sections | High | Done | @@ -193,7 +193,7 @@ Internal architecture, components, and application logic. | DR-031 | Clickable artist/album links in now playing view | UI | UR-028 | Done | | DR-032 | List view option for library browsing (albums, artists) | UI | UR-029 | Done | | DR-033 | Genre browsing screen with quick filters | UI | UR-030 | Done | -| DR-034 | Crossfade engine with configurable duration (0-12s) | Player | UR-031 | Done (Linux only) | +| DR-034 | Crossfade engine with configurable duration (0-12s) | Player | UR-031 | Not implemented (blocked on MPV: single-stream audio chain; `acrossfade` needs 2 inputs — see docs/specs/playback-backend-unification.md) | | DR-035 | Gapless playback between sequential tracks | Player | UR-032 | Done (Linux only) | | DR-036 | Volume normalization with preset levels (Loud/Normal/Quiet) | Player | UR-033 | Done (Linux only) | | DR-037 | Remote session browser and control UI | UI | UR-010 | Done | @@ -499,22 +499,36 @@ The `PlayerBackend` trait defines optional audio settings methods with default e | Basic playback | ✅ | ✅ | Parity | | Volume control | ✅ | ✅ | Parity | | Seek | ✅ | ✅ | Parity | -| Crossfade | ✅ | ❌ | Gap | -| Gapless playback | ✅ | ❌ | Gap | -| Volume normalization | ✅ | ❌ | Gap | +| Crossfade | ❌ | ❌ | Not implemented (blocked on MPV) | +| Gapless playback | ✅ | ⚠️ | Implemented, pending on-device verification | +| Volume normalization | ✅ | ⚠️ | Implemented (LoudnessEnhancer — gain stage, approximate vs MPV's dynaudnorm), pending on-device verification | +| Equalizer (10-band) | ✅ | ⚠️ | Implemented (resampled onto device bands), pending on-device verification | | Position updates | 250ms | On-demand | Inconsistent | -**Future Fix**: -1. Implement `set_audio_settings()` in `ExoPlayerBackend` -2. Add Kotlin-side ExoPlayer configuration for crossfade (using `ConcatenatingMediaSource` or `DefaultMediaSourceFactory`) -3. Implement gapless via ExoPlayer's built-in gapless support -4. Add volume normalization via ExoPlayer's `LoudnessEnhancer` or audio processor -5. Standardize position update frequency across platforms +**Status** (see docs/specs/android-audio-settings-parity.md): +1. ✅ `set_audio_settings()` implemented in `ExoPlayerBackend` (JSON over JNI) +2. ✅ Gapless via ExoPlayer's `pauseAtEndOfMediaItems` +3. ✅ Volume normalization via `LoudnessEnhancer` +4. ✅ Equalizer via `android.media.audiofx.Equalizer`, canonical 10 bands + resampled onto the device's band centres +5. ⬜ **Not yet verified on a physical device** — the EQ/normalization effects + depend on device-specific `AudioEffect` availability and band layouts +6. ⬜ Flip the trait's `set_audio_settings` default from `Ok(())` to + `Err(not_implemented())` so a backend that omits it fails loudly instead of + silently reporting success. Deferred until (5) confirms the Android path works +7. ⬜ Standardize position update frequency across platforms + +Crossfade is deliberately absent: it is unimplemented on every platform and +architecturally blocked on MPV, so building it on Android alone would invert the +parity gap. (The previously suggested `ConcatenatingMediaSource` is also +deprecated in current Media3.) **Impact**: - Medium - Android users lack audio enhancement features advertised in requirements - User experience differs between platforms -- UR-031 (Crossfade), UR-032 (Gapless), UR-033 (Normalization) only work on Linux +- UR-032 (Gapless), UR-033 (Normalization) and UR-027 (Equalizer) are now + implemented on Android as well as Linux, pending on-device verification +- UR-031 (Crossfade) works nowhere — see DR-034 **Traces To**: IR-004, UR-031, UR-032, UR-033, DR-034, DR-035, DR-036 diff --git a/docs/specs/android-audio-settings-parity.md b/docs/specs/android-audio-settings-parity.md new file mode 100644 index 00000000..a7136ff3 --- /dev/null +++ b/docs/specs/android-audio-settings-parity.md @@ -0,0 +1,209 @@ +# Spec: Android audio settings parity (EQ, normalization, gapless) + +**Status:** Proposed +**Requirements:** UR-031, UR-032, UR-033, UR-027 → DR-034, DR-035, DR-036, DR-030; IR-004 +**UX spec:** n/a — no UI change; Settings › Audio already renders these controls +**Supersedes / revises:** closes the audio half of the parity gap recorded in [playback-backend-unification.md](playback-backend-unification.md) + +## Summary + +Implement `set_audio_settings` / `audio_settings` on `ExoPlayerBackend` so the +equalizer, volume normalization, and gapless playback settings actually take +effect on Android. Today the Settings › Audio panel renders these controls on +Android and they silently do nothing — `ExoPlayerBackend` is the only backend +that does not override the trait's no-op defaults. + +Crossfade is explicitly **not** included; see Out of scope. + +## Motivation + +`PlayerBackend` declares `set_audio_settings` with a default `Ok(())` body. +`MpvBackend`, `NullBackend`, and `WebviewAudioBackend` all override it; +`ExoPlayerBackend` does not. The settings are persisted, pushed to the backend on +every track load, and displayed in the UI — and then dropped on the floor. + +This is the single most user-visible platform divergence in the app: a user who +sets a "Rock" EQ preset on Android sees the sliders move and hears no change. + +The backend-unification investigation ruled out fixing this by swapping engines +(video cannot be unified; see the sibling spec), so the fix is to implement the +trait methods where they are missing. + +## Layer assignment + +| Logic / responsibility | Layer | Why it belongs there | +|------------------------|-------|----------------------| +| Band count, centre frequencies, gain range, preset→curve map | Rust (existing) | Already domain-owned in `settings.rs` per [audio-equalizer.md](audio-equalizer.md). Android must consume the same `AudioSettings`, not define its own bands. Duplicating the band layout in Kotlin would be a taxonomy leak of exactly the kind `check:boundary` guards against. | +| Mapping `AudioSettings` → Android audio-effect parameters | Rust → JNI boundary | Platform playback detail, the direct analogue of `build_af_filter` in `mpv_backend.rs`. Belongs with the other `set_audio_settings` code. | +| Attaching/detaching `Equalizer` and `LoudnessEnhancer` to the ExoPlayer audio session | Kotlin (`JellyTauPlayer.kt`) | Android platform API mechanics; needs the live `audioSessionId`, which only the Kotlin layer holds. | +| Normalization preset (Loud/Normal/Quiet) → target gain | Rust (existing) | `VolumeLevel` is domain vocabulary; the same preset must mean the same loudness on every platform. | +| Rendering sliders / preset chips | Frontend (existing) | Pure presentation; unchanged by this spec. | + +Borderline row: attaching the effects could arguably be driven entirely from +Rust via JNI property calls. It goes to Kotlin because `AudioEffect` construction +requires the audio session id and must be re-attached when ExoPlayer rebuilds its +audio sink — lifecycle state that lives in `JellyTauPlayer.kt`. Rust still owns +*what* the values are; Kotlin owns *when* the effect objects exist. + +## Design + +### Rust — `ExoPlayerBackend` (`src-tauri/src/player/android/mod.rs`) + +Override the two defaulted methods, mirroring the shape of the existing +`set_audio_track` JNI call: + +```rust +fn set_audio_settings(&mut self, settings: &AudioSettings) -> Result<(), PlayerError> { + let s = settings.clone().with_crossfade_clamped().with_equalizer_normalised(); + // Serialize as JSON — the same pattern load() already uses for subtitles, + // avoiding a 6-arg JNI signature that has to change every time a field lands. + let json = serde_json::to_string(&s).map_err(|e| PlayerError { message: e.to_string() })?; + // Kotlin: fun setAudioSettings(json: String) + self.call_player_method_string("setAudioSettings", &json)?; + self.shared_state.lock_safe().audio_settings = s; + Ok(()) +} + +fn audio_settings(&self) -> AudioSettings { + self.shared_state.lock_safe().audio_settings.clone() +} +``` + +`ExoPlayerState` gains an `audio_settings: AudioSettings` field. Note +`ExoPlayerBackend` currently holds no such state — `position`/`state`/`volume` are +all pushed in by JNI callbacks — so this is the first *pull*-side field. That is +correct: audio settings are commanded downward, never reported upward. + +### Kotlin — `JellyTauPlayer.kt` + +```kotlin +fun setAudioSettings(json: String) { + val s = JSONObject(json) + applyEqualizer(s.getBoolean("equalizerEnabled"), s.getJSONArray("equalizerBands")) + applyNormalization(s.getBoolean("normalizeVolume"), s.getString("volumeLevel")) + exoPlayer.pauseAtEndOfMediaItems = !s.getBoolean("gaplessPlayback") +} +``` + +Three independent mechanisms: + +- **Gapless** — nearly free. ExoPlayer is gapless by default for compatible + formats; honouring the setting means *disabling* it when the user turns it off, + via `pauseAtEndOfMediaItems`. Note this only applies within a loaded playlist; + our queue loads one item at a time, so verify behaviour before claiming DR-035 + on Android (see Testing). +- **Equalizer** — `android.media.audiofx.Equalizer` bound to + `exoPlayer.audioSessionId`. Android's EQ exposes a device-dependent band count + (commonly 5) at fixed centre frequencies, which will **not** match our 10-band + ISO layout. Rust owns the canonical 10 bands; Kotlin resamples them onto the + device's bands by nearest-centre-frequency interpolation. Gains are in + millibels (`setBandLevel` takes mB, we store dB → ×100), clamped to the + device's reported `getBandLevelRange()`. +- **Normalization** — `android.media.audiofx.LoudnessEnhancer`, also bound to the + audio session, `setTargetGain(mB)` derived from `VolumeLevel`. This is a gain + booster, not a true EBU R128 normalizer like MPV's `dynaudnorm`; parity is + approximate and should be documented as such rather than overclaimed. + +Lifecycle: build the effects lazily on first use, release them in `release()`, +and re-attach on `onAudioSessionIdChanged` — ExoPlayer can rebuild its audio sink +(e.g. on a format change), which invalidates effects bound to the old session. + +### Make the silent-failure mode impossible + +The trait's default is the root cause of this whole class of bug: + +```rust +// backend.rs:85 — reports success while doing nothing +fn set_audio_settings(&mut self, _settings: &AudioSettings) -> Result<(), PlayerError> { + Ok(()) +} +``` + +Android inherits this, so every EQ/normalization change on Android returns `Ok` +and silently does nothing — the UI ships and has no effect, with no error anywhere. + +Once `ExoPlayerBackend` implements the methods, **change the trait default to +`Err(PlayerError::not_implemented())`**, matching how `set_audio_track` / +`set_subtitle_track` already behave. Any future backend that forgets to implement +audio settings then fails loudly instead of lying. + +Check the call sites before flipping it: `NullBackend` overrides both methods, so +the graceful-degradation path is unaffected, but confirm nothing treats a +`set_audio_settings` error as fatal to playback. + +### Re-application on track load + +`PlayerController` already re-pushes `AudioSettings` per track on the platforms +that implement it; the Android path inherits that for free once the trait methods +exist. No controller change. + +### 🔴 Threading note + +`setAudioSettings` is invoked from Rust on whatever thread the command lands on. +`AudioEffect` construction must not happen on the ExoPlayer application thread +from inside a player callback — that is the re-entrancy hazard CLAUDE.md warns +about, and the same shape as the `AutoplayDecision` deadlock. Post the work to +the player's handler rather than doing it inline in a listener. + +## Out of scope + +- **Crossfade (UR-031 / DR-034).** Not implemented on *any* platform today, and + architecturally blocked on MPV (single-stream audio chain; `acrossfade` needs + two inputs). Implementing it on Android alone would invert the parity gap. It + needs its own spec and probably two player instances. +- True EBU R128 normalization. `LoudnessEnhancer` is a gain stage; matching + `dynaudnorm` exactly is out of reach without a custom `AudioProcessor`. +- Windows audio settings — see [windows-native-audio-backend.md](windows-native-audio-backend.md). + +## Acceptance criteria + +- [ ] `ExoPlayerBackend` overrides `set_audio_settings` and `audio_settings`. +- [ ] EQ preset change on Android audibly changes playback; setting persists across track changes and app restart. +- [ ] Normalization toggle audibly changes level; the three presets are ordered Loud > Normal > Quiet. +- [ ] Disabling gapless produces a gap between consecutive tracks; enabling it does not. +- [ ] Effects are released on `release()` and survive an audio-session rebuild. +- [ ] `requirements.md` parity matrix updated: EQ and normalization ✅ Android. +- [ ] `bun run check` and `bun run test` pass. +- [ ] `cargo fmt` clean, `cargo clippy` clean, `bun run test:rust` passes. +- [ ] `bun run check:boundary` passes. +- [ ] New requirement-implementing code carries `// TRACES:` comments. +- [ ] `bindings.ts` regenerated if Rust types changed. + +## Testing + +**Rust** (`cargo test`): `set_audio_settings` stores the sanitized settings and +`audio_settings()` returns them — assert clamping/normalisation is applied +(crossfade clamped to 12s, band vector normalised to `EQ_BANDS.len()`). The JNI +call itself is not unit-testable; extract the JSON serialization into a pure +function and test that its shape matches what the Kotlin parser expects. That +serialization contract is the part most likely to silently break. + +**Kotlin**: the band-resampling function (10 canonical bands → N device bands) is +pure arithmetic — extract it and unit-test it, including the degenerate cases of +a 5-band device and a device reporting 10 bands. + +**Manual, on device** (these are the ones that actually prove it): +1. Set Bass Boost, play a track, confirm audible change. +2. Toggle normalization mid-track; confirm level change without a playback stall. +3. Queue two gapless-encoded tracks, toggle the setting, confirm the gap appears/disappears. +4. Force a format change (44.1kHz → 48kHz track) and confirm the EQ still applies afterwards — this exercises the session-rebuild re-attach. + +## TRACES + +- `ExoPlayerBackend::set_audio_settings` → `// TRACES: UR-027, UR-032, UR-033 | DR-030, DR-035, DR-036` +- Kotlin `setAudioSettings` / `applyEqualizer` / `applyNormalization` → same IDs +- Band-resampling helper + its tests → `DR-030 | UT-xxx` + +## Notes for the implementer + +- Read [audio-equalizer.md](audio-equalizer.md) first — it defines the canonical + band layout and the preset→curve rule this spec consumes. Do not redefine bands + in Kotlin. +- Android source edits go in `src-tauri/android/src` (canonical tree), then run + `scripts/sync-android-sources.sh`. Never edit the `gen/` tree. +- There is a **stale duplicate** `JellyTauPlayer.kt` (285 lines) at + `src-tauri/android/app/src/main/java/com/dtourolle/jellytau/player/` alongside + the real 1103-line file at `src-tauri/android/src/main/java/...`. Edit the + latter. Consider deleting the former as a separate change. +- A parallel Claude session may be active — `git diff` before "repairing" + unexpected changes. diff --git a/docs/specs/android-native-video-spike.md b/docs/specs/android-native-video-spike.md new file mode 100644 index 00000000..40095a7d --- /dev/null +++ b/docs/specs/android-native-video-spike.md @@ -0,0 +1,196 @@ +# Spec: Android native video — transparent-webview spike + +**Status:** Proposed (spike — timeboxed, may conclude "not viable") +**Requirements:** IR-004, UR-003, UR-004 → DR-001, DR-023, DR-024 +**UX spec:** n/a — no intended visual change; the video surface must land exactly where the `