From 14b6a8609d81f8384e96bc4b71aec9dc1e92c6a4 Mon Sep 17 00:00:00 2001
From: Duncan Tourolle <duncan@tourolle.paris>
Date: Sat, 22 Aug 2026 21:17:29 +0200
Subject: [PATCH] fix(player): three defects native video exposed, and the logs
 to see them
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Each of these was invisible while Linux video played in the webview, and each
became reachable the moment mpv started rendering.

DR-238 — a transcoded seek re-negotiates the stream on every renderer, not
just the webview. `determine_video_seek_strategy` treated `is_hls` as a proxy
for "seekable in place", which held only because hls.js was always the HLS
renderer: it seeks within the VOD playlist it is handed and lets the server
catch up. mpv's HLS demuxer cannot make Jellyfin transcode from a new offset,
so with native video on, every transcoded seek became a backend seek that
silently did nothing. One cell of the truth table changes; all four webview
cells are byte-identical.

DR-239 — properties the mpv event loop handles are now observed. libmpv
delivers PropertyChange only for properties registered with
observe_property, so the `pause` arm was unreachable code that read as
implemented: StateChanged was never emitted and the play/pause control never
moved. UT-218 asserts the two lists agree, so the class cannot recur.

DR-240 — fullscreen moves whatever owns the pixels. requestFullscreen()
fullscreens the *document*, which sufficed while the <video> element lived
inside it and WebKit scaled it. A native surface is drawn behind the webview
at window size, so a document-only fullscreen expanded the page and left the
picture at its old size — on WebKitGTK, a maximised window with decorations
still holding a strip of the screen. Measured on a 3440x1440 panel: 1361 tall
before, 1440 after.

DR-241 — a seek issued before mpv has a file to seek in is honoured rather
than dropped. loadfile returns as soon as the command is queued, so
`time-pos` does not resolve yet and setting it fails. The two callers that
always hit that window are resume and a transcoded seek, both of which
re-open the stream and then ask for a position; the failed seek was discarded
and playback began at zero.

Also adds the instrumentation that made the diagnosis possible rather than
speculative: an entry log on player_stop, a render-size log that re-fires on
change instead of latching once, and decoded-vs-display video geometry on file
load. The last of those retired a wrong theory — a picture that does not fill
an ultrawide turned out to be a 16:9 source with its letterbox baked in, not a
rendering fault.
---
 src-tauri/src/commands/player/mod.rs          |  7 ++
 src-tauri/src/player/mpv_backend.rs           | 99 ++++++++++++++++++-
 src-tauri/src/player/mpv_backend_test.rs      | 46 +++++++++
 src-tauri/src/player/seek.rs                  | 69 ++++++++++---
 src-tauri/src/player/video_surface.rs         | 13 ++-
 src/lib/components/player/VideoPlayer.svelte  | 39 ++++++--
 .../player/fullscreenTarget.test.ts           | 15 +++
 src/lib/components/player/fullscreenTarget.ts | 35 +++++++
 8 files changed, 294 insertions(+), 29 deletions(-)
 create mode 100644 src/lib/components/player/fullscreenTarget.test.ts
 create mode 100644 src/lib/components/player/fullscreenTarget.ts

diff --git a/src-tauri/src/commands/player/mod.rs b/src-tauri/src/commands/player/mod.rs
index 532f12ba..c043e4fd 100644
--- a/src-tauri/src/commands/player/mod.rs
+++ b/src-tauri/src/commands/player/mod.rs
@@ -1181,6 +1181,13 @@ pub async fn player_stop(
     // Check if we're in remote mode
     let mode = playback_mode.0.get_mode();
 
+    // Stopping is a state transition worth seeing in a log. Native video is
+    // what made its absence matter: the webview <video> stopped implicitly when
+    // the component unmounted, so nothing ever had to call this — and "never
+    // called" and "called but the backend kept playing" look identical from
+    // outside without it.
+    info!("[player_stop] called (mode: {:?})", mode);
+
     if let crate::playback_mode::PlaybackMode::Remote { session_id } = mode {
         // Send stop command to remote session - clone client before await
         let client = {
diff --git a/src-tauri/src/player/mpv_backend.rs b/src-tauri/src/player/mpv_backend.rs
index 1d1529b1..d05bc590 100644
--- a/src-tauri/src/player/mpv_backend.rs
+++ b/src-tauri/src/player/mpv_backend.rs
@@ -34,6 +34,19 @@ pub struct MpvBackend {
     /// through reported 0.0 / unknown exactly when end-of-file handling needed to
     /// know where playback reached. See [`ObservedTime`].
     observed: Arc<Mutex<ObservedTime>>,
+    /// A seek that arrived before MPV had a file to seek in.
+    ///
+    /// `loadfile` is asynchronous: it returns as soon as the command is queued,
+    /// so `time-pos` is not yet a resolvable property and setting it fails. A
+    /// seek issued in that window used to be dropped on the floor, and the two
+    /// callers that do exactly this are the ones a viewer notices — resume, and
+    /// a transcoded seek, both of which re-open the stream and then ask for a
+    /// position. The stream reloaded and played from zero.
+    ///
+    /// Held here and applied by the `FileLoaded` arm.
+    ///
+    /// TRACES: UR-040, UR-005 | DR-241
+    pending_seek: Arc<Mutex<Option<f64>>>,
 }
 
 struct InternalState {
@@ -237,6 +250,7 @@ impl MpvBackend {
             playback_reporter,
             position_throttler,
             last_seek_time: Arc::new(AtomicU64::new(0)),
+            pending_seek: Arc::new(Mutex::new(None)),
             observed: Arc::new(Mutex::new(ObservedTime::default())),
         };
 
@@ -254,6 +268,7 @@ impl MpvBackend {
         let state = self.state.clone();
         let reporter = self.playback_reporter.clone();
         let throttler = self.position_throttler.clone();
+        let pending_seek_for_events = self.pending_seek.clone();
 
         std::thread::spawn(move || {
             info!("[MpvBackend] Event loop started");
@@ -263,6 +278,30 @@ impl MpvBackend {
                 error!("[MpvBackend] Failed to disable deprecated events: {:?}", e);
             });
 
+            // libmpv delivers PropertyChange only for properties registered
+            // here. Every name matched in the loop below needs a line in this
+            // block or its handler is unreachable — an omission that reads as
+            // working code, because the handler is sitting right there.
+            // UT-218 holds the two lists together.
+            //
+            // `pause` drives the play/pause control: the UI consumes
+            // StateChanged rather than tracking playback itself, per the
+            // one-directional state rule. Unobserved, the event never came and
+            // the button never moved. Invisible until native video shipped,
+            // because the webview <video> element's own DOM events drove that
+            // control on Linux.
+            //
+            // TRACES: UR-005 | DR-239
+            ev_ctx
+                .observe_property("pause", libmpv::Format::Flag, 0)
+                .unwrap_or_else(|e| {
+                    error!(
+                        "[MpvBackend] Failed to observe 'pause': {:?} — the play/pause \
+                         control will not follow the player",
+                        e
+                    );
+                });
+
             loop {
                 match ev_ctx.wait_event(1.0) {
                     Some(Ok(event)) => match event {
@@ -272,6 +311,43 @@ impl MpvBackend {
                         libmpv::events::Event::FileLoaded => {
                             info!("[MpvBackend] File loaded");
 
+                            // Apply a seek that arrived while there was nothing
+                            // to seek in. TRACES: UR-040, UR-005 | DR-241
+                            {
+                                let target = pending_seek_for_events.lock_safe().take();
+                                if let Some(position) = target {
+                                    match mpv.set_property("time-pos", position) {
+                                        Ok(()) => info!(
+                                            "[MpvBackend] applied deferred seek to {position}"
+                                        ),
+                                        Err(e) => warn!(
+                                            "[MpvBackend] deferred seek to {position} failed: {:?}",
+                                            e
+                                        ),
+                                    }
+                                }
+                            }
+
+                            // Geometry, so "the picture does not fill the screen"
+                            // can be attributed rather than guessed at. `width`/
+                            // `height` are the decoded frame; `dwidth`/`dheight`
+                            // are what mpv will *display* after aspect
+                            // correction. A file that carries its letterbox
+                            // baked into the picture reports a 16:9 dwidth and
+                            // is then pillarboxed on a wider panel — which looks
+                            // identical to a rendering bug from outside.
+                            {
+                                let n = |k: &str| mpv.get_property::<i64>(k).unwrap_or(-1);
+                                info!(
+                                    "[MpvBackend] video geometry: {}x{} decoded, {}x{} display, aspect {:?}",
+                                    n("width"),
+                                    n("height"),
+                                    n("dwidth"),
+                                    n("dheight"),
+                                    mpv.get_property::<f64>("video-params/aspect").ok(),
+                                );
+                            }
+
                             // Get duration
                             if let Ok(duration) = mpv.get_property::<f64>("duration") {
                                 if let Some(emitter) = &event_emitter {
@@ -574,11 +650,24 @@ impl PlayerBackend for MpvBackend {
             .as_millis() as u64;
         self.last_seek_time.store(now, Ordering::Relaxed);
 
-        self.mpv
-            .set_property("time-pos", position)
-            .map_err(|e| PlayerError {
-                message: format!("Failed to seek: {:?}", e),
-            })?;
+        // `time-pos` only resolves while a file is loaded. `loadfile` is
+        // asynchronous, so a seek issued straight after a reload — resume, or a
+        // transcoded seek — lands in a window where this fails, and dropping it
+        // there is what makes the stream play from zero instead of the position
+        // that was asked for. Hold it and let `FileLoaded` apply it.
+        // TRACES: UR-040, UR-005 | DR-241
+        if let Err(e) = self.mpv.set_property("time-pos", position) {
+            debug!(
+                "[MpvBackend] seek to {position} deferred until the file loads ({:?})",
+                e
+            );
+            *self.pending_seek.lock_safe() = Some(position);
+            self.observed.lock_safe().record_position(position);
+            return Ok(());
+        }
+
+        // A seek that lands clears any earlier deferred one: the newer intent wins.
+        *self.pending_seek.lock_safe() = None;
 
         // The poll thread suppresses updates for 150ms after a seek, so without
         // this a file ending inside that window would report the pre-seek time.
diff --git a/src-tauri/src/player/mpv_backend_test.rs b/src-tauri/src/player/mpv_backend_test.rs
index 6ea3ff9c..cbdd0812 100644
--- a/src-tauri/src/player/mpv_backend_test.rs
+++ b/src-tauri/src/player/mpv_backend_test.rs
@@ -13,6 +13,52 @@ mod tests {
     use std::sync::{Arc, Mutex};
     use tokio::sync::Mutex as TokioMutex;
 
+    /// Every property the event loop *handles* must also be *observed*.
+    ///
+    /// libmpv only delivers `PropertyChange` for properties registered with
+    /// `mpv_observe_property`. A `match` arm for an unobserved property is
+    /// unreachable code that looks exactly like working code: the handler is
+    /// right there, so the behaviour reads as implemented.
+    ///
+    /// This cost a real bug. `pause` was handled and never observed, so
+    /// `StateChanged` was never emitted on pause or resume. It stayed invisible
+    /// while Linux video played in the webview, because the `<video>` element's
+    /// own DOM events drove the play/pause control; turning native video on made
+    /// the UI depend on the event that never came, and the button stopped
+    /// responding.
+    ///
+    /// Asserted against the source because there is no way to observe the
+    /// registration at runtime without a live mpv instance.
+    ///
+    /// TRACES: UR-005 | DR-239 | UT-218
+    #[test]
+    fn test_every_handled_property_is_observed() {
+        let src = include_str!("mpv_backend.rs");
+
+        let handled: Vec<&str> = src
+            .match_indices("PropertyChange { name: \"")
+            .filter_map(|(i, m)| {
+                let rest = &src[i + m.len()..];
+                rest.find('"').map(|end| &rest[..end])
+            })
+            .collect();
+
+        assert!(
+            !handled.is_empty(),
+            "no PropertyChange arms found - has the event loop been restructured?"
+        );
+
+        for name in handled {
+            let observed = format!("observe_property(\"{name}\"");
+            assert!(
+                src.contains(&observed),
+                "mpv_backend.rs handles PropertyChange for {name:?} but never calls \
+                 observe_property({name:?}, ..). libmpv will never deliver that event, \
+                 so the handler is dead code."
+            );
+        }
+    }
+
     /// Test that simulates the position update thread spawning async tasks
     /// without a Tokio runtime (the bug we just fixed)
     #[test]
diff --git a/src-tauri/src/player/seek.rs b/src-tauri/src/player/seek.rs
index a32f31c4..40d5b52b 100644
--- a/src-tauri/src/player/seek.rs
+++ b/src-tauri/src/player/seek.rs
@@ -39,23 +39,38 @@ pub fn determine_video_seek_strategy(
         return VideoSeekStrategy::LocalNativeSeek;
     }
 
-    // HLS streams and direct play (non-transcoded) support native seeking
-    if is_hls || !needs_transcoding {
-        if use_html5 {
-            // HTML5 backend - frontend handles seeking via videoElement.currentTime
-            // We don't call backend.seek() because video is in HTML5 element, not in MPV
-            VideoSeekStrategy::Html5NativeSeek
-        } else {
-            // Native backend (MPV) - backend handles seeking
-            VideoSeekStrategy::BackendNativeSeek
-        }
-    } else {
-        // Transcoded non-HLS streams need server-side seek (reload from new position)
-        if use_html5 {
-            VideoSeekStrategy::Html5ReloadStream
+    // A server-side transcode is produced *from* `StartTimeTicks`, so where the
+    // seek lands is a property of the request, not of the stream in hand.
+    //
+    // hls.js is the exception: handed a VOD playlist it seeks within it and lets
+    // the server catch up segment by segment. mpv's HLS demuxer cannot make
+    // Jellyfin transcode from a new offset, so for the native backend a
+    // transcoded seek must re-negotiate the stream regardless of container.
+    //
+    // Before native video shipped, `use_html5` was always true for HLS and the
+    // native+HLS+transcode cell was unreachable, which is why `is_hls` alone
+    // used to be a safe proxy for "seekable in place". It no longer is: turning
+    // native video on routed every transcoded seek into a backend seek that
+    // silently does nothing, and presents as "resume does not work".
+    if needs_transcoding {
+        return if use_html5 {
+            if is_hls {
+                VideoSeekStrategy::Html5NativeSeek
+            } else {
+                VideoSeekStrategy::Html5ReloadStream
+            }
         } else {
             VideoSeekStrategy::BackendReloadStream
-        }
+        };
+    }
+
+    // Direct play and direct stream are seekable where they sit.
+    if use_html5 {
+        // The frontend seeks via videoElement.currentTime; calling backend.seek()
+        // would move a player that is not the one rendering.
+        VideoSeekStrategy::Html5NativeSeek
+    } else {
+        VideoSeekStrategy::BackendNativeSeek
     }
 }
 
@@ -240,6 +255,30 @@ mod tests {
         );
     }
 
+    /// A server-side transcode cannot be seeked by the native backend.
+    ///
+    /// Jellyfin produces a transcode from `StartTimeTicks`; hls.js can seek
+    /// within the VOD playlist it is handed, but mpv's HLS demuxer cannot make
+    /// the server transcode from a new offset, so the stream has to be
+    /// re-negotiated. Before native video existed, `use_html5` was always true
+    /// for HLS and this case was unreachable — turning native video on routed
+    /// every transcoded seek into a native seek that silently does nothing,
+    /// which presents as "resume does not work".
+    ///
+    /// TRACES: UR-040 | DR-238 | UT-217
+    #[test]
+    fn test_seek_strategy_transcoded_hls_native_backend() {
+        assert_eq!(
+            determine_video_seek_strategy(false, true, true, false),
+            VideoSeekStrategy::BackendReloadStream
+        );
+        // The HTML5 side of the same case is unchanged: hls.js seeks in-playlist.
+        assert_eq!(
+            determine_video_seek_strategy(false, true, true, true),
+            VideoSeekStrategy::Html5NativeSeek
+        );
+    }
+
     /// Test video seek strategy for direct play (non-transcoded) streams
     #[test]
     fn test_seek_strategy_direct_play() {
diff --git a/src-tauri/src/player/video_surface.rs b/src-tauri/src/player/video_surface.rs
index bbbe9db4..60595fc2 100644
--- a/src-tauri/src/player/video_surface.rs
+++ b/src-tauri/src/player/video_surface.rs
@@ -58,6 +58,8 @@ struct SurfaceState {
     // One-shot diagnostic latches; see `draw`.
     logged_first_draw: bool,
     logged_first_frame: bool,
+    /// Last size we logged, so a size change re-reports rather than staying silent.
+    logged_size: (i32, i32),
     logged_no_gl: bool,
     logged_no_window: bool,
     logged_no_size: bool,
@@ -158,6 +160,7 @@ pub fn attach(vbox: &gtk::Box, mpv: *mut libmpv_sys::mpv_handle) -> bool {
         callback_ctx: std::ptr::null_mut(),
         logged_first_draw: false,
         logged_first_frame: false,
+        logged_size: (0, 0),
         logged_no_gl: false,
         logged_no_window: false,
         logged_no_size: false,
@@ -366,9 +369,15 @@ fn draw(widget: &gtk::Box, cr: &gtk::cairo::Context, state: &Rc<RefCell<SurfaceS
         once(f, "mpv render produced no texture");
         return;
     };
-    if !s.logged_first_frame {
+    // Log the first frame, and again whenever the target size changes. Latching
+    // this once per session hid the case that matters: a second file, rendered
+    // at a different size, in a window that never moved. "The picture is a small
+    // box in the middle" and "the picture fills the widget" are indistinguishable
+    // from outside without it.
+    if !s.logged_first_frame || s.logged_size != (width, height) {
         s.logged_first_frame = true;
-        info!("[VideoSurface] first frame rendered ({width}x{height}, texture {texture})");
+        s.logged_size = (width, height);
+        info!("[VideoSurface] rendering {width}x{height} (texture {texture})");
     }
 
     unsafe {
diff --git a/src/lib/components/player/VideoPlayer.svelte b/src/lib/components/player/VideoPlayer.svelte
index 7e59016d..d0a1c6e4 100644
--- a/src/lib/components/player/VideoPlayer.svelte
+++ b/src/lib/components/player/VideoPlayer.svelte
@@ -1,6 +1,7 @@
 <!-- TRACES: UR-003, UR-005, UR-020, UR-021, UR-026, UR-040, UR-061 | DR-010, DR-023, DR-024, DR-051, DR-052, DR-092, DR-098, DR-099 -->
 <script lang="ts">
   import { onMount, onDestroy, tick, untrack } from "svelte";
+  import { planFullscreen } from "./fullscreenTarget";
   import { get } from "svelte/store";
   import { goto } from "$app/navigation";
   import { commands } from "$lib/api/bindings";
@@ -2085,23 +2086,47 @@
   // Activity, so on its own it left the status and navigation bars painted over
   // the video. The native bridge is what actually makes fullscreen full screen;
   // requestFullscreen() still does the work everywhere else. (UR-066, DR-157)
-  function toggleFullscreen() {
+  async function toggleFullscreen() {
+    // A native surface draws the picture *behind* the webview at window size, so
+    // fullscreening the document alone leaves the video at its old size while
+    // the page around it expands. See fullscreenTarget.ts. (DR-240)
+    const plan = planFullscreen(!useHtml5Element);
+
     if (!document.fullscreenElement) {
-      document.documentElement.requestFullscreen().catch((err) => {
-        // WebKitGTK rejects when the gesture isn't recognised as user-activated;
-        // the immersive call below is what matters on Android, so don't let a
-        // rejection here abort it.
-        log.warn("requestFullscreen rejected:", err);
-      });
+      if (plan.document) {
+        document.documentElement.requestFullscreen().catch((err) => {
+          // WebKitGTK rejects when the gesture isn't recognised as user-activated;
+          // the immersive call below is what matters on Android, so don't let a
+          // rejection here abort it.
+          log.warn("requestFullscreen rejected:", err);
+        });
+      }
+      if (plan.osWindow) {
+        await setOsWindowFullscreen(true);
+      }
       enterImmersive();
       isFullscreen = true;
     } else {
       document.exitFullscreen();
+      if (plan.osWindow) {
+        await setOsWindowFullscreen(false);
+      }
       exitImmersive();
       isFullscreen = false;
     }
   }
 
+  /// Resize the OS window itself. Best-effort: a platform without a window to
+  /// resize (Android) must not break the rest of the toggle.
+  async function setOsWindowFullscreen(on: boolean) {
+    try {
+      const { getCurrentWindow } = await import("@tauri-apps/api/window");
+      await getCurrentWindow().setFullscreen(on);
+    } catch (err) {
+      log.warn("setFullscreen on the OS window failed:", err);
+    }
+  }
+
   function formatTime(seconds: number): string {
     const mins = Math.floor(seconds / 60);
     const secs = Math.floor(seconds % 60);
diff --git a/src/lib/components/player/fullscreenTarget.test.ts b/src/lib/components/player/fullscreenTarget.test.ts
new file mode 100644
index 00000000..9087c106
--- /dev/null
+++ b/src/lib/components/player/fullscreenTarget.test.ts
@@ -0,0 +1,15 @@
+import { describe, it, expect } from "vitest";
+import { planFullscreen } from "./fullscreenTarget";
+
+describe("planFullscreen", () => {
+  it("fullscreens only the document when an in-document <video> renders", () => {
+    // Unchanged behaviour: WebKit scales the element, the window need not move.
+    expect(planFullscreen(false)).toEqual({ document: true, osWindow: false });
+  });
+
+  it("also fullscreens the OS window when a native surface renders", () => {
+    // The picture is drawn behind the webview at window size, so a
+    // document-only fullscreen leaves it at the old size.
+    expect(planFullscreen(true)).toEqual({ document: true, osWindow: true });
+  });
+});
diff --git a/src/lib/components/player/fullscreenTarget.ts b/src/lib/components/player/fullscreenTarget.ts
new file mode 100644
index 00000000..b884f0c1
--- /dev/null
+++ b/src/lib/components/player/fullscreenTarget.ts
@@ -0,0 +1,35 @@
+/**
+ * Which surfaces a fullscreen toggle has to move.
+ *
+ * `requestFullscreen()` only ever fullscreens the *document*. That was
+ * sufficient while every renderer lived inside it: the HTML5 `<video>` element
+ * is part of the document, so WebKit scaled it to the screen and the OS
+ * window's real size never mattered.
+ *
+ * A native video surface is drawn *behind* the webview at **window** size, so a
+ * document-only fullscreen leaves the picture exactly where it was while the
+ * page around it goes fullscreen. On WebKitGTK the observed result is a
+ * maximised window with decorations still taking a strip of the screen — the
+ * video renders correctly, at the wrong size, which reads as "fullscreen is
+ * broken" rather than as a windowing problem.
+ *
+ * Android already needed its own answer here for the system bars (DR-157); this
+ * is the desktop equivalent of the same rule: whoever actually owns the pixels
+ * has to be the thing that goes fullscreen.
+ *
+ * TRACES: UR-066 | DR-240 | UT-219
+ */
+export interface FullscreenPlan {
+  /** Ask the document to go fullscreen (harmless everywhere, needed for CSS). */
+  document: boolean;
+  /** Resize the OS window itself. Required when a native surface owns the picture. */
+  osWindow: boolean;
+}
+
+/**
+ * @param rendersNatively true when a native surface (mpv/ExoPlayer) draws the
+ *        picture rather than an in-document `<video>` element.
+ */
+export function planFullscreen(rendersNatively: boolean): FullscreenPlan {
+  return { document: true, osWindow: rendersNatively };
+}