dtourolle
954546434a
DR-242 … DR-247 are in. The spec now says so rather than reading as a proposal for work that already exists. One deviation is recorded rather than quietly absorbed: DR-246 called for the engines to own seek strategy outright, and they cannot — re-negotiating a stream needs the repository, which sits above them. The engine declares the ability and the caller acts on it. `determine_video_seek_strategy` therefore survives, correctly typed over a declared capability instead of over a guess, because the defect was its input rather than its existence.
284 lines
15 KiB
Markdown
284 lines
15 KiB
Markdown
# Spec: MediaPlayer — one controller API, three interchangeable engines
|
||
|
||
**Status:** **Partially implemented.** DR-242 … DR-247 have shipped: the
|
||
contract, `FakePlayer` and the conformance suite, `MpvPlayer`, the standalone
|
||
runner, `LegacyPlayer`, the controller port, the capability-driven seek
|
||
strategy, and ExoPlayer conformance on a device. What is left is DR-248 (the
|
||
webview as an engine) and DR-249 (deleting `PlayerBackend` and the frontend
|
||
playback-state flags).
|
||
**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.
|
||
|
||
**Shipped with a deviation.** The engine cannot own this outright:
|
||
re-negotiating a stream needs the repository, which sits *above* the engine.
|
||
So the engine *declares* `seeks_transcoded_in_place` and the caller acts on
|
||
it. That removes the defect — nobody guesses on another component's behalf,
|
||
and adding an engine no longer means editing a shared table — without
|
||
pretending an engine can reach upward. `determine_video_seek_strategy`
|
||
survives as a correctly-typed decision over declared abilities rather than
|
||
being deleted; the defect was its *input*, not its existence.
|
||
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.
|