fix(player): closing the player stops every renderer, not the believed one
DR-250. The user's diagnosis, and a better fix than modelling the handoff more carefully: a close that stops only what we believe is playing is fragile by construction. A close that stops everything is correct whatever the bookkeeping thinks. Two places it did not. The teardown's stop was gated on `didStartNativePlayback && !didStopBackendEarly` — flags describing what *this component* started. A background-audio handoff swaps the renderer underneath them, so after one they describe a player that is no longer making sound and the stop was skipped entirely. The audio stream kept running and the mini player adopted it, which is exactly why a movie reappeared as an audio track. It is unconditional now; `playerStop` is idempotent, so the cost of calling it when nothing plays is a no-op round trip, against the alternative of silently leaving audio running. And `PlayerController::stop` never cleared the handoff. Leaving the base offset and the active flag behind lets a later position read be interpreted against a handoff that no longer exists. Stopping now clears both. `didStartNativePlayback` had no remaining reader and is deleted rather than silenced — dead bookkeeping about which renderer was in charge is precisely the frontend playback state this contract is meant to remove, and the eslint ratchet caught it going one over. The underlying unconfirmed state swap is still there and still worth fixing — it is written up in media-player-controller.md. This makes the symptom impossible while that lands. 789 Rust tests, 1088 frontend, lint back at 158, clippy clean both ways.
This commit is contained in:
@@ -443,6 +443,7 @@ Internal architecture, components, and application logic.
|
||||
| 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-246 | The seek strategy turns on an ability the engine declares, not on the container the stream arrives in. `Capabilities::seeks_transcoded_in_place` is stated by each engine — true for hls.js, which seeks within the VOD playlist it was handed; false for mpv, which cannot make the server transcode from a new offset — and the command asks the engine currently rendering instead of inferring from `is_hls` and `use_html5`. The item's transport is no longer read at the seek site at all. Re-negotiating a stream needs the repository, which sits above the engine, so the engine states the capability and the caller acts on it rather than the engine owning the whole decision | Player | UR-040, 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-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-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 |
|
||||
|
||||
---
|
||||
|
||||
@@ -789,6 +789,28 @@ impl PlayerController {
|
||||
drop(backend);
|
||||
self.clear_reported_time();
|
||||
|
||||
// Stopping means *nothing is playing*, from any renderer — not "the
|
||||
// thing we currently 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 yet. A stop aimed at what the flags say
|
||||
// is playing therefore misses the audio stream that actually is, and it
|
||||
// resurfaces in the mini player as an audio track.
|
||||
//
|
||||
// Clearing the handoff here is the other half of that: a stop that
|
||||
// leaves the base offset and the active flag behind lets the next
|
||||
// position read be interpreted against a handoff that no longer exists.
|
||||
//
|
||||
// TRACES: UR-040, UR-005 | DR-250
|
||||
if self.is_background_audio_active() {
|
||||
debug!("[PlayerController] stop: clearing an active background-audio handoff");
|
||||
}
|
||||
*self.background_audio_active.lock_safe() = false;
|
||||
self.set_background_audio_base(0.0);
|
||||
*self.html5_playing.lock_safe() = None;
|
||||
|
||||
if let Some(jellyfin_id) = jellyfin_id {
|
||||
self.report_stopped_at(jellyfin_id, position);
|
||||
}
|
||||
|
||||
@@ -251,7 +251,6 @@
|
||||
function nativeSeekSettling(): boolean {
|
||||
return Date.now() - lastNativeSeekAt < NATIVE_SEEK_SETTLE_MS;
|
||||
}
|
||||
let didStartNativePlayback = $state(false); // Track if we started playback (to know if we should stop on unmount)
|
||||
let didStopBackendEarly = $state(false); // Track if we stopped backend early for non-transcoded content
|
||||
let swipeType = $state<"brightness" | null>(null);
|
||||
let hls: Hls | null = null; // HLS.js instance for streaming HLS content
|
||||
@@ -1032,7 +1031,6 @@
|
||||
"Using HTML5 for transcoded stream - keeping backend for seeking/transcoding decisions",
|
||||
);
|
||||
// Backend is kept running but should not play audio since HTML5 element handles playback
|
||||
didStartNativePlayback = true; // Track that we need to stop backend on unmount
|
||||
}
|
||||
|
||||
// Register the adapter with the facade so control intents (UI, or a
|
||||
@@ -1098,7 +1096,6 @@
|
||||
|
||||
if (!useHtml5Element) {
|
||||
// Using native backend, subscribe to player events
|
||||
didStartNativePlayback = true; // Track that we started native playback
|
||||
isPlaying = (response.state?.kind ?? response.state) === "playing";
|
||||
// Cleanup happens in the component's top-level onDestroy. Calling
|
||||
// onDestroy() here — after an await — throws lifecycle_outside_component,
|
||||
@@ -1139,7 +1136,6 @@
|
||||
}
|
||||
} else {
|
||||
// For transcoded content, keep backend for seeking
|
||||
didStartNativePlayback = true;
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1273,15 +1269,26 @@
|
||||
}
|
||||
|
||||
// Stop the player when component is destroyed
|
||||
// Skip if we already stopped the backend early (non-transcoded + HTML5)
|
||||
if (didStartNativePlayback && !didStopBackendEarly) {
|
||||
// Unconditional. Leaving the player means nothing should still be playing,
|
||||
// whichever renderer happened to own it.
|
||||
//
|
||||
// This used to be gated on `didStartNativePlayback && !didStopBackendEarly`
|
||||
// — flags describing what *this component* started. A background-audio
|
||||
// handoff swaps the renderer underneath them, so after one they describe a
|
||||
// player that is no longer the one making sound, and the stop was skipped
|
||||
// while the audio stream kept going. It then reappeared in the mini player
|
||||
// as an audio track.
|
||||
//
|
||||
// `playerStop` is idempotent, so calling it when nothing is playing costs a
|
||||
// no-op IPC round trip. That is a far cheaper failure than the alternative.
|
||||
//
|
||||
// TRACES: UR-040, UR-005 | DR-250
|
||||
try {
|
||||
log.debug("Stopping backend player on component unmount");
|
||||
await commands.playerStop();
|
||||
} catch (err) {
|
||||
log.error("Failed to stop backend player:", err);
|
||||
}
|
||||
}
|
||||
|
||||
// Report stop when component is destroyed (skip for live - no resume tracking)
|
||||
if (!isLive && onReportStop && currentTime > 0) {
|
||||
@@ -2039,7 +2046,6 @@
|
||||
transport: targetSelection.transport,
|
||||
subtitles: nativeSubtitleTracks(sentSubtitleTracks),
|
||||
});
|
||||
didStartNativePlayback = true;
|
||||
await playerAdapter?.load(targetSelection.url, {
|
||||
mediaId: media.id,
|
||||
selection: targetSelection,
|
||||
|
||||
Reference in New Issue
Block a user