diff --git a/docs/specs/media-player-controller.md b/docs/specs/media-player-controller.md new file mode 100644 index 00000000..67f78487 --- /dev/null +++ b/docs/specs/media-player-controller.md @@ -0,0 +1,269 @@ +# Spec: MediaPlayer — one controller API, three interchangeable engines + +**Status:** Proposed +**Requirements:** UR-081 (new) → DR-242 … DR-249 (new); IR-034. Re-check +`requirements.md` before allocating — ids moved several times while this was +written. +**UX spec:** n/a — no user-visible change is intended. That is the point. +**Supersedes / revises:** absorbs `determine_video_seek_strategy` +(`player/seek.rs`, DR-238) into the engines. Revises the backend half of +[playback-backend-unification.md](playback-backend-unification.md). + +**Destination on completion:** +[01-rust-backend.md](../architecture/01-rust-backend.md) — replaces the player +state-machine section; and +[05-platform-backends.md](../architecture/05-platform-backends.md) — the engines +become implementations of a stated contract rather than three separate designs. + +## Summary + +Replace the `PlayerBackend` trait with a `MediaPlayer` contract that expresses +**intent** ("present this item, starting here") rather than **device operations** +("load", then "seek"). MPV, ExoPlayer and the webview element implement it; a +`FakePlayer` implements it for tests; and one conformance suite runs against +every implementation so a backend is either correct or visibly failing. + +No user-visible behaviour changes. What changes is that playback logic stops +being written three times in the command layer. + +## Motivation + +A day of debugging Linux native video produced four defects (DR-238 … DR-241). +Every one of them traces to the same missing seam, not to mpv: + +| Defect | What it looked like | What it was | +|---|---|---| +| DR-241 | "Resume is broken", "I cannot skip" | `loadfile` is async, so a seek issued straight after a load fails and was discarded. The trait has no way to say *open at a position*, so every caller does load-then-seek and each races independently. | +| DR-238 | Transcoded seeks silently did nothing | `use_html5` was doing double duty as "who renders" **and** "how do I seek", decided in the command layer by a truth table. | +| DR-239 | Play/pause control never moved | `PropertyChange { name: "pause" }` was handled but never observed. Nothing in the contract required an engine to report its own state. | +| DR-240 | Fullscreen left the picture at window size | `requestFullscreen()` moves the document; whoever owns the pixels has to be told separately. | + +The shape is consistent: **the same intent implemented in several places, each +with its own timing and its own idea of the rules.** Resume worked through the +adapter (which seeks after `File loaded`) and failed through the command (which +seeks immediately). Two callers, one intent, two behaviours. + +Supporting evidence for the diagnosis: + +- `commands/player/mod.rs` is **3,561 lines** and is where "stop → rebuild URL → + update queue → load → seek" lives. That is playback orchestration in the IPC + layer. +- `player_play_item` needed a `#[cfg(not(target_os = "linux"))]` guard, i.e. a + platform decision in a command handler. +- The frontend carries `didStartNativePlayback`, `didStopBackendEarly`, + `hasPerformedInitialSeek`, `lastAppliedInitialPosition` — playback state in the + UI, which contradicts the one-directional rule in CLAUDE.md. + +### Why an abstraction, and not more fixes + +Each defect above was individually cheap to patch, and patching them is what +produced a regression: routing transcoded seeks to a reload path turned "seek +does nothing" into "seek jumps to zero", because the reload path's own seek was +broken in the same way. **Symptom fixes in this area compound.** + +## Layer assignment + +| Logic / responsibility | Layer | Why it belongs there | +|---|---|---| +| Presenting an item at a position, in one operation | **Engine** (`MediaPlayer`) | Only the engine knows when its pipeline can accept a position. Expressing it as caller-sequenced load-then-seek exports a race the engine is the only one able to close. | +| Whether *this* stream can be seeked in place, or must be re-opened | **Engine** | A property of the engine × transport pair: hls.js seeks a VOD playlist, mpv's HLS demuxer cannot make Jellyfin transcode from a new offset. Today this is a truth table in a command handler that has to guess for engines it does not own. | +| Reporting position, phase, duration, active tracks | **Engine** | The player is the authoritative source of playback state (CLAUDE.md). An engine that does not report is not implementing the contract — DR-239 was exactly this. | +| Choosing *which* stream to open (direct play vs transcode, ceiling, transport) | **Rust, above the engine** | Domain: depends on Jellyfin's `PlaybackInfo`, codec support, quality ceiling. See [backend-owned-stream-selection.md](backend-owned-stream-selection.md). The engine is handed a `StreamSelection`; it never negotiates one. | +| Queue, autoplay, session, playback reporting | **`PlayerController`** | Policy across items. Unchanged — but it talks to one contract instead of branching per platform. | +| Which engine this platform uses | **Rust, at construction** | Already correct today; stays a single `cfg` at the composition root rather than `cfg`s scattered through command handlers. | +| Rendering surfaces, controls, fullscreen chrome | **Frontend / platform** | Presentation. The engine reports *what* is playing; it does not own the window. | + +Borderline row and its tie-breaker: "should a transcoded seek re-open the +stream?" reads like domain policy. It is **engine** capability — the *decision* +is "seek to T", and how to achieve it is the engine's business. If it were +policy, every new engine would require editing a shared truth table, which is +precisely the coupling DR-238 came from. + +## Design + +### The contract + +```rust +/// Anything that can present media: MpvPlayer, ExoPlayer, WebviewPlayer, FakePlayer. +pub trait MediaPlayer: Send { + /// Present `req.selection`, beginning at `req.start`. + /// + /// One operation, deliberately. `open` is where a start position is + /// *expressible*, so no caller has to sequence load-then-seek and no caller + /// can race the engine's own load. An engine that cannot start at an offset + /// natively must absorb that internally (defer until loaded, or re-open) — + /// it is the only layer that knows when it is able to. + fn open(&mut self, req: OpenRequest) -> Result<(), PlayerError>; + + fn play(&mut self) -> Result<(), PlayerError>; + fn pause(&mut self) -> Result<(), PlayerError>; + + /// Stop and release the current item. Must be idempotent, and must leave the + /// engine producing no audio — DR-2xx exists because "stopped" and "silent" + /// were not the same thing. + fn close(&mut self) -> Result<(), PlayerError>; + + /// Seek to an absolute position on the item's timeline. + /// + /// The engine decides in-place vs re-open. Callers never choose. + fn seek(&mut self, to: Duration) -> Result<(), PlayerError>; + + fn set_volume(&mut self, volume: Volume) -> Result<(), PlayerError>; + fn set_rate(&mut self, rate: f64) -> Result<(), PlayerError>; + fn select_audio_track(&mut self, index: Option) -> Result<(), PlayerError>; + fn select_subtitle_track(&mut self, index: Option) -> Result<(), PlayerError>; + + /// One coherent read of everything the UI consumes. + fn snapshot(&self) -> PlaybackSnapshot; + + /// Engine capabilities, so callers can adapt without naming engines. + fn capabilities(&self) -> Capabilities; +} +``` + +```rust +pub struct OpenRequest { + pub media: MediaItem, + pub selection: StreamSelection, // url + transport + playback kind + pub start: Duration, // Duration::ZERO for "from the beginning" + pub audio_track: Option, + pub subtitle_track: Option, + pub autoplay: bool, +} + +pub struct PlaybackSnapshot { + pub phase: Phase, + pub position: Duration, + pub duration: Option, + pub seekable: bool, + pub volume: Volume, + pub rate: f64, + pub audio_track: Option, + pub subtitle_track: Option, +} + +/// `Opening` is the state today's code cannot express, and the direct cause of +/// DR-241: a seek arriving with nothing loaded had no phase to be rejected or +/// queued against, so it was simply lost. +pub enum Phase { Idle, Opening, Ready, Playing, Paused, Ended, Failed(String) } +``` + +Engines emit `PlayerEvent` for phase, position, track and error changes. Emitting +is part of the contract, and the conformance suite asserts it — an engine that +stays silent fails, which is what would have caught DR-239 the day it landed. + +### What this deletes + +- `determine_video_seek_strategy` and `VideoSeekStrategy` — replaced by + `seek()` + `capabilities()`. The command layer stops deciding how engines seek. +- The reload orchestration in `player_seek_video` — moves inside the engines that + need it. +- `#[cfg(target_os = "linux")]` branches in command handlers. +- Frontend playback-state flags, which become reads of `snapshot()`. + +### IPC + +No new commands. Existing ones keep their names and shapes; they become thin +delegations. `PlayerStatus` gains nothing the frontend does not already receive. +Regenerate `bindings.ts` only if `PlaybackSnapshot` is exposed directly — prefer +mapping it onto the existing `PlayerStatus` so this stays invisible at the wire. + +## Testing + +This is the half that makes the abstraction worth having, and it is the reason to +do it rather than keep patching. + +### 1. A conformance suite, run against every engine + +One set of tests, parameterised over implementations. Any `MediaPlayer` must pass +it; a new engine is "done" when it does. + +``` +conformance::run(&mut engine, fixture) covering: + open(start = ZERO) -> phase Ready|Playing, position ~0 + open(start = 10min) -> position within tolerance of 10min, NEVER 0 [DR-241] + seek while Opening -> honoured once Ready, not discarded [DR-241] + seek on a transcoded stream -> position lands, by whatever means [DR-238] + pause / play -> phase changes AND an event is emitted [DR-239] + close -> phase Idle, silent, idempotent + close during Opening -> no playback ever starts [audio-on-exit] + volume / rate / track select -> reflected in snapshot() +``` + +The `open(start = 10min)` and `seek while Opening` cases are the ones that fail +on today's code. They are written first, and they are the acceptance criterion. + +### 2. `FakePlayer` + +A deterministic in-memory implementation with a controllable clock. Lets +`PlayerController`, autoplay, queue, sleep-timer and session logic be tested with +no mpv, no device, no network — most of which is currently only reachable through +a real engine. + +### 3. Per-engine runs + +| Engine | Where | Note | +|---|---|---| +| `FakePlayer` | `cargo test` | Always. | +| `MpvPlayer` | `cargo test`, Linux | libmpv is already in the builder image (the Linux build links it), so **no CI toolchain install** — see CLAUDE.md. Needs a tiny local fixture file; generate it in-test rather than committing media. | +| `ExoPlayer` | instrumented, on device | Not in the standard CI job. Run via `scripts/` on a connected device; record results in the PR. | +| `WebviewPlayer` | vitest | Against a stubbed element, as `html5Adapter` is tested today. | + +An engine that cannot run in CI still has the same suite; it is just run by hand. +That is the point of writing it once. + +## Migration + +Strangler, not a rewrite. Each step ships independently and leaves the app working. + +1. **DR-242** Define `MediaPlayer`, `OpenRequest`, `PlaybackSnapshot`, `Phase`, + `Capabilities`. No implementations. Compiles alongside `PlayerBackend`. +2. **DR-243** `FakePlayer` + the conformance suite. The suite fails against + nothing yet — it is the specification. +3. **DR-244** `MpvPlayer` implementing `MediaPlayer`, wrapping today's + `MpvBackend` internals. Make conformance pass, including `open(start)`. +4. **DR-245** `PlayerController` talks to `MediaPlayer`. `PlayerBackend` retained + behind an adapter so the other engines keep working. +5. **DR-246** Move seek strategy and reload orchestration out of + `commands/player/mod.rs` into the engines; delete `seek.rs`'s truth table. +6. **DR-247** `ExoPlayerPlayer`; conformance on device. +7. **DR-248** `WebviewPlayer`; retire the adapter shim. +8. **DR-249** Delete `PlayerBackend` and the frontend playback-state flags. + +Steps 1–3 are pure addition and risk nothing. Step 5 is where today's defect +classes actually die. + +## Out of scope + +- Stream selection (which URL, which quality) — that is + [backend-owned-stream-selection.md](backend-owned-stream-selection.md), and + this spec consumes its `StreamSelection` rather than duplicating it. +- Rendering surfaces and compositing. +- Any user-visible behaviour change. If one appears, it is a bug in the migration. +- Replacing hls.js or changing the transcode path. + +## Acceptance criteria + +- [ ] The conformance suite exists and `open(start = 10min)` fails against the + pre-migration mpv path — proving it reproduces DR-241 — then passes. +- [ ] `FakePlayer` lets at least one controller-level test run with no engine. +- [ ] `determine_video_seek_strategy` is deleted, not merely bypassed. +- [ ] No `cfg(target_os = ...)` remains in `commands/player/`. +- [ ] `bun run check`, `bun run test`, `bun run format:check`, `bun run lint` pass. +- [ ] `cargo fmt`, `cargo clippy -D warnings`, `bun run test:rust` pass. +- [ ] `bun run check:boundary` passes. +- [ ] `// TRACES:` on new code; `bun run traces:validate` passes; coverage stays + at or above the CI ratchet. +- [ ] Manual: resume, skip on a transcoded item, pause/play, and exit-while-playing + verified on Linux **and** Android before `PlayerBackend` is deleted. + +## Notes for the implementer + +- **Write the conformance suite before the second engine**, or it will encode + whatever the first engine happens to do. +- `close()` must mean *silent*. The bug that motivated this spec had `stop` being + called, reported, and audible afterwards. +- Do not let `Capabilities` grow into engine sniffing. If a caller branches on + the engine's identity, the contract is missing something — add it there. +- A parallel Claude session may be active in this repo — `git diff` before + "repairing" unexpected changes.