From 5fcf58fa784e328c5940a0a532855086a4500dba Mon Sep 17 00:00:00 2001
From: Duncan Tourolle <duncan@tourolle.paris>
Date: Sat, 22 Aug 2026 22:12:51 +0200
Subject: [PATCH] feat(player): the controller talks to one contract
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

DR-245. PlayerController now holds a MediaPlayer instead of a PlayerBackend,
and every engine reaches it through that contract.

Deliberately a seam swap, not four rewrites: the existing backends are carried
across by LegacyPlayer, so MPV keeps its EQ and normalisation, ExoPlayer keeps
its media session, and nothing loses a feature to the migration. MpvPlayer
stays available for conformance until it grows the audio-settings half.

The substantive change is at the load site. Where the controller used to call
load() and then play(), it now issues one open() carrying the item and where
to begin — so the window a start position could be lost in is gone from the
controller as well as from the engines.

`state()` maps the engine's Phase back onto PlayerState using the queue, which
is what knows the item. External behaviour is unchanged.

Supporting pieces:

  - The contract gains set_audio_settings/audio_settings as *provided*
    methods. Engines that cannot honour them say so through Capabilities and
    inherit a no-op, rather than every implementation carrying an Ok(()) it
    does not mean.
  - PlayerBackend is implemented for Box<dyn PlayerBackend>, without which the
    boxed engine built at the composition root cannot be handed to anything
    generic over the trait.
  - StreamSelection::for_queued_item rebuilds a selection for an item already
    in the queue, without re-negotiating. The transport falls back rather than
    being sniffed out of the URL — that substring check is what DR-230 removed
    — and needs_transcoding is an exact stand-in because every transcode this
    app requests is HLS (DR-140).
  - default-run = "jellytau". The conformance binary made a bare `cargo run`
    ambiguous, which broke `tauri dev` outright. Caught by running the app
    rather than by any suite, which is the argument for doing both.

789 tests, clippy -D warnings clean with and without the feature.
---
 docs/requirements.md                         |  2 +-
 src-tauri/Cargo.toml                         |  4 ++
 src-tauri/src/lib.rs                         |  6 +-
 src-tauri/src/player/backend.rs              | 50 ++++++++++++++
 src-tauri/src/player/legacy_player.rs        | 17 +++--
 src-tauri/src/player/media.rs                | 13 ++++
 src-tauri/src/player/media_player.rs         | 14 ++++
 src-tauri/src/player/mod.rs                  | 68 ++++++++++++++------
 src-tauri/src/repository/stream_selection.rs | 39 +++++++++++
 9 files changed, 187 insertions(+), 26 deletions(-)

diff --git a/docs/requirements.md b/docs/requirements.md
index 35737f06..70801534 100644
--- a/docs/requirements.md
+++ b/docs/requirements.md
@@ -440,7 +440,7 @@ Internal architecture, components, and application logic.
 | DR-242 | The player contract expresses intent, not device operations. `MediaPlayer::open` carries the start position, so no caller sequences load-then-seek and none can race an engine's asynchronous load; `seek` states a destination and leaves in-place-vs-re-open to the engine, which is the only layer that knows its own transport; `snapshot` is one coherent read; and `Phase::Opening` names the window a seek used to be lost in. Replaces `PlayerBackend`, which abstracted a device and required each of the three engines to re-derive the same rules | Player | UR-081 | In Progress |
 | DR-243 | Every engine passes one conformance suite, and a `FakePlayer` implements the contract deterministically. The suite is written before the second engine so it cannot encode whatever the first happened to do, and it drives readiness through a harness rather than sleeping. `FakePlayer` models the one behaviour that matters — opening is not instantaneous — so the load/seek race can be expressed on purpose, and lets the controller, queue, autoplay and session logic be tested with no engine at all | Player | UR-081 | In Progress |
 | DR-244 | `MpvPlayer` implements `MediaPlayer` over libmpv, applying the start position at load time via mpv's own `start` option rather than seeking after an asynchronous `loadfile`, and holding a seek that arrives during `Opening` until the file loads. A standalone `player-conformance` binary runs the suite against it with audio and video routed to null, so a wrapper is verifiable without building or launching the app | Player | UR-081, UR-040 | Done |
-| DR-245 | `LegacyPlayer` drives the old `PlayerBackend` through the `MediaPlayer` contract, so engines not yet ported keep working during the migration and the two designs can be compared on one engine and one file. It reproduces the old load-then-play-then-seek sequence faithfully rather than a fixed-up version, because making it pass would defeat its purpose | Player | UR-081 | In Progress |
+| DR-245 | `PlayerController` holds a `MediaPlayer` rather than a `PlayerBackend`, and every engine reaches it through that one contract — `LegacyPlayer` carries the not-yet-ported ones across unchanged, so the port swaps a seam rather than four implementations. Loading an item is now a single `open` carrying its start position, and the controller maps the engine's `Phase` back onto `PlayerState` using the queue, so nothing outside changes. `LegacyPlayer` drives the old `PlayerBackend` through the `MediaPlayer` contract, so engines not yet ported keep working during the migration and the two designs can be compared on one engine and one file. It reproduces the old load-then-play-then-seek sequence faithfully rather than a fixed-up version, because making it pass would defeat its purpose | Player | UR-081 | Done |
 | DR-247 | ExoPlayer can be told where to start. `JellyTauPlayer.load(url, mediaId)` had no way to express a start position, so every caller loaded and then seeked; the position is now handed to ExoPlayer with the media item via `setMediaItem(item, startPositionMs)`, and the two-argument form delegates to it. Running the conformance cases on a device also settled which half of DR-241 was engine-specific: ExoPlayer already queues a seek issued before `prepare()` completes, so it never had the lost-seek defect mpv did — only the missing vocabulary for a start position | Player | UR-081, UR-005 | Done |
 | DR-198 | The webview runs under a real Content-Security-Policy, and the asset protocol is scoped to the one directory it still serves. `csp` was `null`, which disables CSP entirely: any script that reached the web layer — through a future `{@html}`, a dependency, or a devtools paste — would have inherited the whole IPC surface, and with it the user's session. `script-src 'self'` (Tauri injects a nonce for SvelteKit's inline bootstrap script at build time, so no `'unsafe-inline'` is needed) plus `object-src`/`frame-src 'none'` and `base-uri 'self'` is the part that is genuinely restrictive. `img-src`/`media-src`/`connect-src` cannot be: the Jellyfin origin is typed in by the user at run time and is commonly plain `http` on a LAN, so they allow `http:`/`https:` — a wide grant for *data*, but one that still bars `file:`, `filesystem:` and scripting schemes, and leaves `script-src` untouched. `style-src` keeps `'unsafe-inline'` because Svelte compiles `style="…"` attributes (including `app.html`'s `display: contents` wrapper) into markup; this is safe only while no `<style>` element survives into `index.html`, since a nonce there would make Tauri's injection outrank — and therefore void — `'unsafe-inline'`. `worker-src blob:` and `media-src blob:` are hls.js: it demuxes in a worker built from a blob and attaches MSE through `URL.createObjectURL`. `asset:` and `http://asset.localhost` are the same protocol under the two naming schemes `convertFileSrc` emits (custom scheme on Linux/macOS, `http` host on Windows/Android); `ipc:`/`http://ipc.localhost` is the invoke transport, which would otherwise be blocked by `connect-src`. A run-time CSP naming the server origin exactly was rejected: Tauri computes the header from immutable config when it serves the HTML, so it would mean rebuilding config and reloading the webview on every server change, for a policy the user can already point anywhere. The asset-protocol scope narrows from `$APPDATA/**` to `$APPDATA/thumbnails/**` — since DR-137 moved downloaded media to the loopback server, `imageCache` is the only `convertFileSrc` caller left, so the database and the encrypted-token fallback file no longer sit inside the grant | Security | UR-012, UR-071 | Done |
 
diff --git a/src-tauri/Cargo.toml b/src-tauri/Cargo.toml
index 9c60ae2c..9974724b 100644
--- a/src-tauri/Cargo.toml
+++ b/src-tauri/Cargo.toml
@@ -1,5 +1,9 @@
 [package]
 name = "jellytau"
+# The app. Named explicitly because the crate also builds
+# `player-conformance`, and a second binary makes a bare `cargo run` —
+# which `tauri dev` issues — ambiguous.
+default-run = "jellytau"
 version = "0.10.1"
 description = "A cross-platform Jellyfin client"
 authors = ["Duncan Tourolle <duncan@tourolle.paris>"]
diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs
index 216e704c..5e86b948 100644
--- a/src-tauri/src/lib.rs
+++ b/src-tauri/src/lib.rs
@@ -1409,8 +1409,12 @@ pub fn run() {
                 }
             }
 
+            // Every engine reaches the controller through the one contract.
+            // `LegacyPlayer` carries the not-yet-ported ones across unchanged,
+            // so this port swaps a seam rather than four implementations.
+            // TRACES: UR-081 | DR-245
             let player_controller = PlayerController::new(
-                backend,
+                Box::new(crate::player::LegacyPlayer::new(backend)),
                 playback_reporter.clone(),
                 position_throttler.clone(),
             );
diff --git a/src-tauri/src/player/backend.rs b/src-tauri/src/player/backend.rs
index f2967d27..602b4517 100644
--- a/src-tauri/src/player/backend.rs
+++ b/src-tauri/src/player/backend.rs
@@ -249,6 +249,56 @@ impl PlayerBackend for NullBackend {
 }
 
 // TRACES: UR-003, UR-004 | DR-004 | UT-026, UT-027, UT-028, UT-029, UT-030, UT-031, UT-032, UT-033
+/// Forward the trait through a box.
+///
+/// `Box<dyn PlayerBackend>` does not implement `PlayerBackend` on its own, so
+/// without this the boxed engine built at the composition root cannot be handed
+/// to anything generic over the trait — `LegacyPlayer` in particular.
+impl PlayerBackend for Box<dyn PlayerBackend> {
+    fn load(&mut self, media: &MediaItem) -> Result<(), PlayerError> {
+        (**self).load(media)
+    }
+    fn play(&mut self) -> Result<(), PlayerError> {
+        (**self).play()
+    }
+    fn pause(&mut self) -> Result<(), PlayerError> {
+        (**self).pause()
+    }
+    fn stop(&mut self) -> Result<(), PlayerError> {
+        (**self).stop()
+    }
+    fn seek(&mut self, position: f64) -> Result<(), PlayerError> {
+        (**self).seek(position)
+    }
+    fn set_volume(&mut self, volume: f32) -> Result<(), PlayerError> {
+        (**self).set_volume(volume)
+    }
+    fn position(&self) -> f64 {
+        (**self).position()
+    }
+    fn duration(&self) -> Option<f64> {
+        (**self).duration()
+    }
+    fn state(&self) -> PlayerState {
+        (**self).state()
+    }
+    fn volume(&self) -> f32 {
+        (**self).volume()
+    }
+    fn set_audio_settings(&mut self, settings: &AudioSettings) -> Result<(), PlayerError> {
+        (**self).set_audio_settings(settings)
+    }
+    fn audio_settings(&self) -> AudioSettings {
+        (**self).audio_settings()
+    }
+    fn set_audio_track(&mut self, stream_index: i32) -> Result<(), PlayerError> {
+        (**self).set_audio_track(stream_index)
+    }
+    fn set_subtitle_track(&mut self, stream_index: Option<i32>) -> Result<(), PlayerError> {
+        (**self).set_subtitle_track(stream_index)
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
diff --git a/src-tauri/src/player/legacy_player.rs b/src-tauri/src/player/legacy_player.rs
index 0e306deb..662179bc 100644
--- a/src-tauri/src/player/legacy_player.rs
+++ b/src-tauri/src/player/legacy_player.rs
@@ -18,8 +18,6 @@
 //!
 //! TRACES: UR-081 | DR-245
 
-#![allow(dead_code)] // Consumed when PlayerController is ported (DR-245).
-
 use std::time::Duration;
 
 use super::backend::{PlayerBackend, PlayerError};
@@ -41,10 +39,6 @@ impl<B: PlayerBackend> LegacyPlayer<B> {
             has_item: false,
         }
     }
-
-    pub fn inner_mut(&mut self) -> &mut B {
-        &mut self.inner
-    }
 }
 
 impl<B: PlayerBackend + Send> MediaPlayer for LegacyPlayer<B> {
@@ -135,6 +129,17 @@ impl<B: PlayerBackend + Send> MediaPlayer for LegacyPlayer<B> {
         }
     }
 
+    fn set_audio_settings(
+        &mut self,
+        settings: &crate::settings::AudioSettings,
+    ) -> Result<(), PlayerError> {
+        self.inner.set_audio_settings(settings)
+    }
+
+    fn audio_settings(&self) -> crate::settings::AudioSettings {
+        self.inner.audio_settings()
+    }
+
     fn capabilities(&self) -> Capabilities {
         Capabilities {
             video: false,
diff --git a/src-tauri/src/player/media.rs b/src-tauri/src/player/media.rs
index 781cb073..b4b0d583 100644
--- a/src-tauri/src/player/media.rs
+++ b/src-tauri/src/player/media.rs
@@ -171,6 +171,19 @@ pub enum MediaSource {
     DirectUrl { url: String },
 }
 
+impl MediaItem {
+    /// The URL or path an engine should open.
+    ///
+    /// TRACES: UR-081 | DR-245
+    pub fn playable_url(&self) -> String {
+        match &self.source {
+            MediaSource::Remote { stream_url, .. } => stream_url.clone(),
+            MediaSource::Local { file_path, .. } => file_path.to_string_lossy().into_owned(),
+            MediaSource::DirectUrl { url } => url.clone(),
+        }
+    }
+}
+
 impl MediaItem {
     /// Get the Jellyfin item ID if available
     pub fn jellyfin_id(&self) -> Option<&str> {
diff --git a/src-tauri/src/player/media_player.rs b/src-tauri/src/player/media_player.rs
index 767955d3..c2d6cf24 100644
--- a/src-tauri/src/player/media_player.rs
+++ b/src-tauri/src/player/media_player.rs
@@ -32,6 +32,7 @@ use std::time::Duration;
 use super::backend::PlayerError;
 use super::media::MediaItem;
 use crate::repository::stream_selection::StreamSelection;
+use crate::settings::AudioSettings;
 
 /// What an engine is doing right now.
 ///
@@ -203,4 +204,17 @@ pub trait MediaPlayer: Send {
     fn snapshot(&self) -> PlaybackSnapshot;
 
     fn capabilities(&self) -> Capabilities;
+
+    /// Apply EQ, normalisation and gapless settings.
+    ///
+    /// Provided rather than required: engines that cannot honour them say so
+    /// through [`Capabilities::audio_settings`] and inherit this no-op, instead
+    /// of every implementation carrying an `Ok(())` it does not mean.
+    fn set_audio_settings(&mut self, _settings: &AudioSettings) -> Result<(), PlayerError> {
+        Ok(())
+    }
+
+    fn audio_settings(&self) -> AudioSettings {
+        AudioSettings::default()
+    }
 }
diff --git a/src-tauri/src/player/mod.rs b/src-tauri/src/player/mod.rs
index f2b687aa..66e1e090 100644
--- a/src-tauri/src/player/mod.rs
+++ b/src-tauri/src/player/mod.rs
@@ -12,7 +12,6 @@ pub mod events;
 pub mod fake_player;
 #[cfg(test)]
 mod fake_player_conformance;
-#[cfg(any(test, feature = "conformance"))]
 pub mod legacy_player;
 pub mod media;
 pub mod media_player;
@@ -60,10 +59,13 @@ pub mod video_surface;
 pub mod webview_audio_backend;
 
 // Re-export commonly used types
+use crate::repository::stream_selection::StreamSelection;
 pub use autoplay::{AutoplayDecision, AutoplaySettings};
 pub use backend::{NullBackend, PlayerBackend, PlayerError};
 pub use events::{PlayerEventEmitter, PlayerStatusEvent, TauriEventEmitter};
+pub use legacy_player::LegacyPlayer;
 pub use media::{MediaItem, MediaSource, MediaType, QueueContext, SubtitleTrack};
+pub use media_player::{MediaPlayer, OpenRequest, Phase};
 pub use queue::{QueueManager, RepeatMode};
 pub use seek::{determine_video_seek_strategy, VideoSeekStrategy};
 pub use session::{MediaSessionManager, MediaSessionType};
@@ -238,7 +240,9 @@ use crate::utils::conversions::seconds_to_ticks;
 
 /// Central player controller that coordinates playback
 pub struct PlayerController {
-    backend: Arc<Mutex<Box<dyn PlayerBackend>>>,
+    /// The engine. One contract, so the controller stops branching on which
+    /// platform it is running on — see docs/specs/media-player-controller.md.
+    backend: Arc<Mutex<Box<dyn MediaPlayer>>>,
     queue: Arc<Mutex<QueueManager>>,
     jellyfin_client: Arc<Mutex<Option<JellyfinClient>>>,
     muted: bool,
@@ -337,7 +341,7 @@ pub struct PlayerController {
 
 impl PlayerController {
     pub fn new(
-        backend: Box<dyn PlayerBackend>,
+        backend: Box<dyn MediaPlayer>,
         playback_reporter: Arc<TokioMutex<Option<PlaybackReporter>>>,
         position_throttler: Arc<EventThrottler>,
     ) -> Self {
@@ -568,8 +572,16 @@ impl PlayerController {
         *self.html5_playing.lock_safe() = None;
 
         let mut backend = self.backend.lock_safe();
-        backend.load(item)?;
-        backend.play()?;
+        // One operation: the engine is handed the item and where to begin, so
+        // there is no window between them for a position to be lost in.
+        backend.open(OpenRequest::new(
+            item.clone(),
+            StreamSelection::for_queued_item(
+                item.playable_url(),
+                item.transport,
+                item.needs_transcoding,
+            ),
+        ))?;
         drop(backend);
 
         // A different item is loading; the last one's reported position must not
@@ -743,7 +755,7 @@ impl PlayerController {
             return Ok(());
         }
         let mut backend = self.backend.lock_safe();
-        if backend.state().is_playing() {
+        if backend.snapshot().phase.is_active() {
             backend.pause()
         } else {
             backend.play()
@@ -768,7 +780,7 @@ impl PlayerController {
         let position = self.absolute_position();
 
         let mut backend = self.backend.lock_safe();
-        backend.stop()?;
+        backend.close()?;
         drop(backend);
         self.clear_reported_time();
 
@@ -849,7 +861,7 @@ impl PlayerController {
         // If we're more than 3 seconds in, restart current track
         {
             let backend = self.backend.lock_safe();
-            if backend.position() > 3.0 {
+            if backend.snapshot().position.as_secs_f64() > 3.0 {
                 debug!("[PlayerController] previous: restarting current track (position > 3s)");
                 drop(backend);
                 return self.seek(0.0);
@@ -881,7 +893,7 @@ impl PlayerController {
     /// timeline and is what every caller outside the player itself means.
     pub fn seek(&self, position: f64) -> Result<(), PlayerError> {
         let mut backend = self.backend.lock_safe();
-        backend.seek(position)
+        backend.seek(Duration::from_secs_f64(position.max(0.0)))
     }
 
     /// Seek to an **absolute** position on the item's own timeline.
@@ -932,23 +944,41 @@ impl PlayerController {
     /// Set the active audio track by stream index
     pub fn set_audio_track(&self, stream_index: i32) -> Result<(), PlayerError> {
         let mut backend = self.backend.lock_safe();
-        backend.set_audio_track(stream_index)
+        backend.select_audio_track(Some(stream_index))
     }
 
     /// Set the active subtitle track by stream index (None to disable subtitles)
     pub fn set_subtitle_track(&self, stream_index: Option<i32>) -> Result<(), PlayerError> {
         let mut backend = self.backend.lock_safe();
-        backend.set_subtitle_track(stream_index)
+        backend.select_subtitle_track(stream_index)
     }
 
     /// Get current state
     pub fn state(&self) -> PlayerState {
-        self.backend.lock_safe().state()
+        let phase = self.backend.lock_safe().snapshot().phase;
+        let media = self.queue.lock_safe().current().cloned();
+        match (phase, media) {
+            (Phase::Playing, Some(media)) => PlayerState::Playing {
+                media,
+                position: self.position(),
+                duration: self.duration().unwrap_or(0.0),
+            },
+            (Phase::Paused, Some(media)) => PlayerState::Paused {
+                media,
+                position: self.position(),
+                duration: self.duration().unwrap_or(0.0),
+            },
+            (Phase::Opening, Some(media)) => PlayerState::Loading { media },
+            (Phase::Failed(error), media) => PlayerState::Error { media, error },
+            // Ready without an item, or anything terminal, reads as idle: the
+            // queue is what says whether there is something to resume.
+            _ => PlayerState::Idle,
+        }
     }
 
     /// Get current position
     pub fn position(&self) -> f64 {
-        self.backend.lock_safe().position()
+        self.backend.lock_safe().snapshot().position.as_secs_f64()
     }
 
     /// The position on the **item's own timeline**, whatever is rendering it.
@@ -974,7 +1004,7 @@ impl PlayerController {
     ///
     /// TRACES: UR-040, UR-005, UR-025 | DR-178 | UT-176, UT-177
     pub fn absolute_position(&self) -> f64 {
-        let native = self.backend.lock_safe().position().max(0.0);
+        let native = self.backend.lock_safe().snapshot().position.as_secs_f64();
         let reported = self.reported_time.lock_safe().last_position();
         let base = if self.is_background_audio_active() {
             *self.background_audio_base.lock_safe()
@@ -1040,7 +1070,9 @@ impl PlayerController {
     pub fn duration(&self) -> Option<f64> {
         self.backend
             .lock_safe()
-            .duration()
+            .snapshot()
+            .duration
+            .map(|d| d.as_secs_f64())
             .or_else(|| self.observed_duration())
     }
 
@@ -1096,7 +1128,7 @@ impl PlayerController {
 
     /// Get current volume (0.0 - 1.0)
     pub fn volume(&self) -> f32 {
-        self.backend.lock_safe().volume()
+        self.backend.lock_safe().snapshot().volume
     }
 
     /// Check if muted
@@ -1197,7 +1229,7 @@ impl PlayerController {
                         drop(timer);
 
                         // Stop the backend
-                        if let Err(e) = backend.lock_safe().stop() {
+                        if let Err(e) = backend.lock_safe().close() {
                             error!("[SleepTimer] Failed to stop playback: {}", e);
                         }
                         continue;
@@ -2212,7 +2244,7 @@ impl Default for PlayerController {
         let playback_reporter = Arc::new(TokioMutex::new(None));
         let position_throttler = Arc::new(EventThrottler::new());
         Self::new(
-            Box::new(NullBackend::new()),
+            Box::new(LegacyPlayer::new(NullBackend::new())),
             playback_reporter,
             position_throttler,
         )
diff --git a/src-tauri/src/repository/stream_selection.rs b/src-tauri/src/repository/stream_selection.rs
index 9569ebe9..aec7c8b9 100644
--- a/src-tauri/src/repository/stream_selection.rs
+++ b/src-tauri/src/repository/stream_selection.rs
@@ -184,6 +184,45 @@ impl StreamSelection {
             needs_transcoding: false,
         }
     }
+
+    /// A selection for an item already sitting in the queue.
+    ///
+    /// The queue predates `StreamSelection`: its items carry a URL, an optional
+    /// transport and the older `needs_transcoding` flag. This rebuilds a
+    /// selection from those without re-negotiating with the server, so the
+    /// controller can hand an engine an `OpenRequest` for an item it already
+    /// holds.
+    ///
+    /// The transport falls back rather than being sniffed from the URL — the
+    /// substring check is exactly what DR-230 removed. `needs_transcoding` is an
+    /// exact stand-in because every transcode this app requests is HLS (DR-140).
+    ///
+    /// TRACES: UR-079, UR-081 | DR-225, DR-245
+    pub fn for_queued_item(
+        url: impl Into<String>,
+        transport: Option<Transport>,
+        needs_transcoding: bool,
+    ) -> Self {
+        let transport = transport.unwrap_or(if needs_transcoding {
+            Transport::Hls
+        } else {
+            Transport::Progressive
+        });
+        Self {
+            url: url.into(),
+            transport,
+            playback_kind: if needs_transcoding {
+                PlaybackKind::Transcode
+            } else {
+                PlaybackKind::DirectPlay
+            },
+            rendition: None,
+            available: Vec::new(),
+            media_source_id: None,
+            play_session_id: None,
+            needs_transcoding,
+        }
+    }
 }
 
 /// Build the quality ladder as it applies to a source of a known bitrate.