fix(player): three defects native video exposed, and the logs to see them
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.
This commit is contained in:
@@ -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 = {
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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]
|
||||
|
||||
@@ -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
|
||||
// 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 {
|
||||
// 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
|
||||
}
|
||||
} 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() {
|
||||
|
||||
@@ -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: >k::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: >k::Box, cr: >k::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 {
|
||||
|
||||
@@ -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) {
|
||||
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);
|
||||
|
||||
@@ -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 });
|
||||
});
|
||||
});
|
||||
@@ -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 };
|
||||
}
|
||||
Reference in New Issue
Block a user