dtourolle
11d9d760d8
mpv now decodes video on Linux, drawn into a framebuffer we own and blitted
into the default vbox's draw handler. Tauri's widget tree is untouched, so an
upgrade that assumes its own layout cannot invalidate this. Direct play means
the original file, hardware decoding, and no server transcode at all — where
previously every desktop video was re-encoded to h264 for the browser engine,
whatever the file actually was. Off by default: JELLYTAU_NATIVE_VIDEO=1.
That settles finding 2 of playback-backend-unification.md — "native video
cannot be composited with a Tauri webview" — by demonstration rather than
argument, on X11 and Wayland both.
Turning it on exposed nine defects, none of them mpv's. Each was the same
mistake in a different place: a capability written down as a compile-time fact
about the platform, or a state asserted instead of confirmed.
DR-238/246 a seek routed by the stream's container rather than by what the
engine could do with it - correct only while one player handled
those streams, silent the moment another did
DR-239 a property handled but never observed, so the play/pause button
waited for an event that could not arrive
DR-240 fullscreen expanding the document while the window stayed put
DR-241 a seek issued before the engine had a file, failed, and discarded
- which is why resume began at zero
DR-247 a Linux-only gate outliving the caller that made it Linux-only,
breaking the Android build outright
DR-250 a stop aimed at whichever renderer bookkeeping believed was in
charge, missing the one actually making sound
DR-251 a duration of zero believed, leaving the seek bar no scale
DR-252 a junk float converted to a Duration, panicking the backend the
instant a length-less stream appeared
So the MediaPlayer contract (DR-242 … DR-247): `open` carries a start position,
so no caller sequences load-then-seek and none can race an engine's load;
`seek` states a destination and leaves in-place-versus-re-open to the engine;
`snapshot` is one coherent read; and `Phase::Opening` names the window where
intent used to be lost. One conformance suite runs against every engine —
FakePlayer and mpv under cargo test, ExoPlayer instrumented on a device — so an
engine is either correct or visibly failing.
Two of the nine were introduced during this work and caught on hardware, not by
any suite: an over-broad capability that grouped ExoPlayer with mpv, and the
Duration panic. The suites test engines that behave. That is recorded in
docs/native-player-verification.md, which asks for the exact action sequences
that found them.
Verified: all automated gates, conformance (mpv 9/9, legacy 8/9 by design,
ExoPlayer 7/7 on device), and manual desktop and Android passes on real
hardware.
Known open and deliberately shipped: resume reads local progress and never the
server's; the background-audio handoff still declares a state swap it does not
confirm (the symptom is now impossible, the race is not); and `bun run
android:dev` builds an APK carrying the release application id, whose failure
message advises an uninstall that would destroy app data. Fix that last one
before anyone else builds for Android.
Squashed from worktree-linux-native-video, which keeps the per-defect history.
325 lines
17 KiB
Markdown
325 lines
17 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.**
|
||
|
||
## The background-audio handoff is an unconfirmed state swap
|
||
|
||
Diagnosed on a device, 2026-08-23, and the likeliest explanation for "audio
|
||
keeps playing after I leave the player" — the report this whole line of work
|
||
started from.
|
||
|
||
`enter_background_audio` and `exit_background_audio` in `PlayerController` are
|
||
pure bookkeeping: they flip a boolean and set or clear a base offset. Neither
|
||
confirms that the audio stream actually opened, nor that the webview `<video>`
|
||
actually came back. `exit_background_audio`'s own doc comment says the element
|
||
"becomes the player again once it reloads" — a future event nothing waits for,
|
||
while the flag declares the swap complete the moment it is called.
|
||
|
||
The sequence that exposes it:
|
||
|
||
1. Background audio is enabled.
|
||
2. The app is backgrounded — `enter_background_audio(pos)`, audio stream opens.
|
||
3. The app is foregrounded — `exit_background_audio()` sets the flag back, so
|
||
the controller believes the video element owns playback again.
|
||
4. The player is exited *before the element has reloaded*. The stop is aimed at
|
||
an element that does not exist yet; the audio stream is still running.
|
||
5. The mini player sees a live audio session and adopts it — which is why the
|
||
symptom is a **movie appearing as an audio track**, and why it is
|
||
intermittent rather than reliable.
|
||
|
||
Duration reporting `0.0` on Android widens the window: the reload is slower and
|
||
less certain to land at the right position.
|
||
|
||
**This is the same defect class as DR-238 … DR-241: state asserted rather than
|
||
confirmed.** It is what `Phase::Opening` and `MpvPlayer`'s open generation
|
||
exist for — a handoff *is* an open in flight, and a `close` during one has to
|
||
cancel it rather than race it. The handoff is not modelled as an open at all
|
||
today; it is two booleans and an offset.
|
||
|
||
The fix therefore belongs with this contract rather than beside it: route the
|
||
handoff through `open`/`close` so the swap has a phase, and so leaving the
|
||
player during one cancels the thing that is actually playing instead of the
|
||
thing the controller believes is playing. `close_during_open_never_plays`
|
||
already states the required behaviour and passes on all four engines — the gap
|
||
is that the handoff never reaches an engine as an open.
|
||
|
||
## 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.
|