docs(player): backend unification findings + correct false parity claims
Investigation into unifying the playback backends (Linux/MPV, Android/ExoPlayer, Windows/webview) onto one engine with hardware acceleration. Conclusion: video cannot be unified onto a native engine; audio can. The blocker is not mpv-specific. WebKitGTK, WebView2 and Android WebView each draw into their own compositor surface, so a native video surface sits either entirely above or entirely below the webview and cannot interleave with HTML. GStreamer and libVLC fail identically. mpv would additionally regress streaming: it has no adaptive bitrate, while the current hls.js path does. Six specs added: - playback-backend-unification: the analysis and decision, with evidence - android-audio-settings-parity: set_audio_settings on ExoPlayerBackend - android-native-video-spike: timeboxed test of SurfaceView compositing - windows-native-audio-backend: replace the webview <audio> shim with libmpv - libmpv2-migration: dead libmpv git pin -> libmpv2, plus a LICENSE file - playback-docs-corrections: the requirement-status fixes applied here Corrections to requirements.md, all verified against source: - UR-031/DR-034 claimed crossfade was "Done (Linux only)". It is implemented nowhere (mpv_backend.rs has a bare TODO) and is architecturally blocked on mpv, whose single-stream audio chain cannot feed acrossfade's two inputs. - Parity matrix listed crossfade as a Linux/Android gap; it is neither. - The matrix omitted the equalizer, which has the same Linux-only shape. - The suggested ConcatenatingMediaSource is deprecated in current Media3. nativeAdapter.ts cited tauri#10152 as an upstream blocker for native Android video. That issue is a stale feature request, dead since 2024-07-01; the capability shipped in tauri 27d01834 (2024-09-02), and the related black-screen bug was fixed in wry 0.39.4 (we ship 0.55.x). What is genuinely unproven is SurfaceView-behind-WebView compositing, which the spike now tracks.
This commit is contained in:
@@ -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.
|
||||
Reference in New Issue
Block a user