jellytau/docs/specs/media-player-controller.md

# 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<i32>) -> Result<(), PlayerError>;
    fn select_subtitle_track(&mut self, index: Option<i32>) -> 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<i32>,
    pub subtitle_track: Option<i32>,
    pub autoplay: bool,
}

pub struct PlaybackSnapshot {
    pub phase: Phase,
    pub position: Duration,
    pub duration: Option<Duration>,
    pub seekable: bool,
    pub volume: Volume,
    pub rate: f64,
    pub audio_track: Option<i32>,
    pub subtitle_track: Option<i32>,
}

/// `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.