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.

Destination on completion: 01-rust-backend.md — replaces the player state-machine section; and 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. 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 cfgs 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

/// 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;
}

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