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