fix(player): a junk duration from an engine must not panic the backend

DR-252, and a regression I introduced in DR-245.

`Duration::from_secs_f64` panics on a negative or non-finite value. The old
PlayerBackend contract passed durations around as a bare Option<f64> and never
promised otherwise, so junk flowed through harmlessly. LegacyPlayer converts
that value to a Duration on the way into the MediaPlayer contract, which turned
it into a hard panic.

ExoPlayer reports C.TIME_UNSET — Long::MIN_VALUE, about -9.2e15 seconds — for
any stream whose length it does not know. That is every background-audio
handoff: /Audio/{id}/universal is a chunked, length-less transcode. So the
panic fired exactly when the handoff started, killed the Rust backend
mid-swap, and left a black screen with no controls.

Caught on the device, in the user's own repro sequence: enable background
audio, background the app, come back. Not by any suite — the conformance cases
run against engines that report sane numbers, and nothing was asking what
happens when one does not.

One guard on the contract now, used by every engine crossing into it, rather
than each adapter deciding for itself. mpv had the same unguarded conversion
for its duration property and would have hit it the moment libmpv reported
something odd.

UT-222 pins the values: TIME_UNSET as seconds, negatives, zero, NaN and both
infinities yield no duration; a real runtime survives.

791 Rust tests, mpv conformance still 9/9, clippy clean both ways.
This commit is contained in:
2026-08-23 10:13:43 +02:00
parent e5b7003489
commit 7e23da46e2
4 changed files with 57 additions and 5 deletions
+2
View File
@@ -445,6 +445,7 @@ Internal architecture, components, and application logic.
| 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-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-250 | Stopping means nothing is playing, from any renderer — not "whatever we believe owns playback has been asked to stop". A background-audio handoff swaps which renderer that is, and the swap is bookkeeping that can be mid-flight: `exit_background_audio` marks the webview element the player again the moment it is called, while the element has not reloaded. The teardown's stop was gated on flags describing what the component started, so after a handoff it described a player that was no longer making sound and the stop was skipped — the audio stream kept running and the mini player adopted it, which is why a movie reappeared as an audio track. The stop is now unconditional (it is idempotent) and clears the handoff base and flag, so a later position read cannot be interpreted against a handoff that no longer exists | Player | UR-040, UR-005 | Done | | DR-250 | Stopping means nothing is playing, from any renderer — not "whatever we believe owns playback has been asked to stop". A background-audio handoff swaps which renderer that is, and the swap is bookkeeping that can be mid-flight: `exit_background_audio` marks the webview element the player again the moment it is called, while the element has not reloaded. The teardown's stop was gated on flags describing what the component started, so after a handoff it described a player that was no longer making sound and the stop was skipped — the audio stream kept running and the mini player adopted it, which is why a movie reappeared as an audio track. The stop is now unconditional (it is idempotent) and clears the handoff base and flag, so a later position read cannot be interpreted against a handoff that no longer exists | Player | UR-040, UR-005 | Done |
| DR-251 | A duration of zero is treated as "the engine does not know yet", and falls back to the runtime the item already carries. ExoPlayer reports `C.TIME_UNSET` until it resolves one and `JellyTauPlayer.getDuration()` maps that to `0.0`, so the engine answered `Some(0.0)` rather than `None` — which satisfied every "unknown duration" fallback and left the seek bar with no scale. It presented as scrubbing being broken rather than as a duration that never arrived, and the catalog had the runtime the whole time | Player | UR-005, UR-040 | Done | | DR-251 | A duration of zero is treated as "the engine does not know yet", and falls back to the runtime the item already carries. ExoPlayer reports `C.TIME_UNSET` until it resolves one and `JellyTauPlayer.getDuration()` maps that to `0.0`, so the engine answered `Some(0.0)` rather than `None` — which satisfied every "unknown duration" fallback and left the seek bar with no scale. It presented as scrubbing being broken rather than as a duration that never arrived, and the catalog had the runtime the whole time | Player | UR-005, UR-040 | Done |
| DR-252 | Seconds reported by an engine are converted to a `Duration` only when finite and positive. `Duration::from_secs_f64` panics on a negative or non-finite value and no engine promises otherwise: ExoPlayer reports `C.TIME_UNSET` (`Long::MIN_VALUE`, about -9.2e15) for a stream whose length it does not know, which is every background-audio handoff — `/Audio/{id}/universal` is a chunked, length-less transcode. Held as a float that junk was harmless; converted to a `Duration` by the `MediaPlayer` adapter it became a panic that killed the backend mid-handoff and left a black screen with no controls. One guard on the contract, used by every engine crossing into it | Player | 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 | | 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 |
--- ---
@@ -758,6 +759,7 @@ Internal architecture, components, and application logic.
| UT-219 | A fullscreen toggle moves the document only when an in-document `<video>` renders, and moves the OS window as well when a native surface does | DR-240 | Done | | UT-219 | A fullscreen toggle moves the document only when an in-document `<video>` renders, and moves the OS window as well when a native surface does | DR-240 | Done |
| UT-220 | The conformance suite: opening at a position starts there and never at zero, a seek issued while opening is honoured and overrides the start it overtook, pause and play are observable, close is silent and idempotent, and an open cancelled by close never begins playing | DR-242, DR-243 | In Progress | | UT-220 | The conformance suite: opening at a position starts there and never at zero, a seek issued while opening is honoured and overrides the start it overtook, pause and play are observable, close is silent and idempotent, and an open cancelled by close never begins playing | DR-242, DR-243 | In Progress |
| UT-221 | An engine that cannot report a duration does not erase the one the item carries: with the queue holding a 1800s item and the engine answering nothing usable, the controller still reports 1800s | DR-251 | Done | | UT-221 | An engine that cannot report a duration does not erase the one the item carries: with the queue holding a 1800s item and the engine answering nothing usable, the controller still reports 1800s | DR-251 | Done |
| UT-222 | The values that killed the backend are rejected rather than converted: `C.TIME_UNSET` as seconds, negatives, zero, NaN and both infinities all yield no duration, while a real runtime survives | DR-252 | Done |
### Integration Tests ### Integration Tests
+5 -3
View File
@@ -21,7 +21,9 @@
use std::time::Duration; use std::time::Duration;
use super::backend::{PlayerBackend, PlayerError}; use super::backend::{PlayerBackend, PlayerError};
use super::media_player::{Capabilities, MediaPlayer, OpenRequest, Phase, PlaybackSnapshot}; use super::media_player::{
duration_from_secs, Capabilities, MediaPlayer, OpenRequest, Phase, PlaybackSnapshot,
};
use super::state::PlayerState; use super::state::PlayerState;
pub struct LegacyPlayer<B: PlayerBackend> { pub struct LegacyPlayer<B: PlayerBackend> {
@@ -124,8 +126,8 @@ impl<B: PlayerBackend + Send> MediaPlayer for LegacyPlayer<B> {
}; };
PlaybackSnapshot { PlaybackSnapshot {
phase, phase,
position: Duration::from_secs_f64(self.inner.position().max(0.0)), position: duration_from_secs(self.inner.position()).unwrap_or(Duration::ZERO),
duration: self.inner.duration().map(Duration::from_secs_f64), duration: self.inner.duration().and_then(duration_from_secs),
seekable: true, seekable: true,
volume: self.inner.volume(), volume: self.inner.volume(),
muted: false, muted: false,
+46
View File
@@ -34,6 +34,23 @@ use super::media::MediaItem;
use crate::repository::stream_selection::StreamSelection; use crate::repository::stream_selection::StreamSelection;
use crate::settings::AudioSettings; use crate::settings::AudioSettings;
/// Seconds reported by an engine, as a `Duration`, without trusting the number.
///
/// `Duration::from_secs_f64` **panics** on a negative or non-finite value, and
/// no engine promises otherwise. ExoPlayer reports `C.TIME_UNSET` —
/// `Long::MIN_VALUE`, about -9.2e15 — for a stream whose length it does not
/// know, which is every background-audio handoff: `/Audio/{id}/universal` is a
/// chunked, length-less transcode.
///
/// Held as a float that junk was harmless. Converted to a `Duration` it became
/// a panic that killed the backend mid-handoff and left a black screen with no
/// controls. Every engine crossing into this contract goes through here.
///
/// TRACES: UR-005 | DR-252
pub fn duration_from_secs(seconds: f64) -> Option<Duration> {
(seconds.is_finite() && seconds > 0.0).then(|| Duration::from_secs_f64(seconds))
}
/// What an engine is doing right now. /// What an engine is doing right now.
/// ///
/// `Opening` is the state the previous design could not express, and is the /// `Opening` is the state the previous design could not express, and is the
@@ -280,3 +297,32 @@ pub trait MediaPlayer: Send {
AudioSettings::default() AudioSettings::default()
} }
} }
#[cfg(test)]
mod tests {
use super::*;
/// The value that killed the backend: `C.TIME_UNSET` as seconds.
///
/// ExoPlayer reports it for any stream whose length it does not know, and
/// `Duration::from_secs_f64` panics on it. A player must not be the place
/// anyone discovers a float was strange.
///
/// TRACES: UR-005 | DR-252 | UT-222
#[test]
fn test_junk_durations_do_not_panic() {
// Long::MIN_VALUE milliseconds, as ExoPlayer hands it over.
assert_eq!(duration_from_secs(-9_223_372_036_854_776.0), None);
assert_eq!(duration_from_secs(-1.0), None);
assert_eq!(duration_from_secs(0.0), None, "zero is not a duration");
assert_eq!(duration_from_secs(f64::NAN), None);
assert_eq!(duration_from_secs(f64::INFINITY), None);
assert_eq!(duration_from_secs(f64::NEG_INFINITY), None);
// A real one still survives.
assert_eq!(
duration_from_secs(6997.024),
Some(Duration::from_secs_f64(6997.024))
);
}
}
+4 -2
View File
@@ -21,7 +21,9 @@ use libmpv::Mpv;
use log::{debug, info, warn}; use log::{debug, info, warn};
use super::backend::PlayerError; use super::backend::PlayerError;
use super::media_player::{Capabilities, MediaPlayer, OpenRequest, Phase, PlaybackSnapshot}; use super::media_player::{
duration_from_secs, Capabilities, MediaPlayer, OpenRequest, Phase, PlaybackSnapshot,
};
use crate::utils::lock::MutexSafe; use crate::utils::lock::MutexSafe;
/// State the event thread writes and the caller reads. /// State the event thread writes and the caller reads.
@@ -145,7 +147,7 @@ impl MpvPlayer {
s.duration = mpv s.duration = mpv
.get_property::<f64>("duration") .get_property::<f64>("duration")
.ok() .ok()
.map(Duration::from_secs_f64); .and_then(duration_from_secs);
s.seekable = mpv.get_property::<bool>("seekable").unwrap_or(true); s.seekable = mpv.get_property::<bool>("seekable").unwrap_or(true);
s.phase = Phase::Playing; s.phase = Phase::Playing;
s.deferred_seek.take() s.deferred_seek.take()