From 2d67b0e4f5026b4e41876fd0d69662cbc24adb5c Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sun, 16 Aug 2026 09:47:27 +0200 Subject: [PATCH] fix(player): give every transcode its own play session, and stop the one it replaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Switching bitrate mid-film stalled playback. The server served the new playlist and then rejected its segments: 400 on hls1/main/0.ts, six times over 25 seconds, never recovering, while the UI logged "Streaming quality changed" as if nothing were wrong. Jellyfin keys a transcode job by device and play session. Every stream URL this app built carried the same hardcoded DeviceId and no PlaySessionId at all, so the second stream for an item was indistinguishable from the first and nothing ever stopped the old ffmpeg. Re-opening a stream is not rare — a quality switch, a transcoded seek and an audio-track switch all do it. Replayed against the server, a second stream opened for a live job's item alternates per attempt between serving bytes and 400ing, which is why it read as flaky rather than broken. begin_video_play_session mints a session id per open and reports the one it supersedes; the URL builder stops that job (DELETE /Videos/ActiveEncodings, un-retried — a slow stop must not delay playback) before returning. Putting it in the builder rather than in each caller covers every re-open path by construction. adopt_video_play_session takes ownership of the job the server starts itself when PlaybackInfo answers with a TranscodingUrl: without it the first switch on a stream has nothing to stop and collides with what is playing. Two client faults made the same incident worse and go with it: - The fatal-HLS-error handler added the transcode seek offset to a position that already included it. Past roughly the halfway mark of a film the doubled value cleared the "near end" threshold, so any transient network error was reported as end-of-stream and autoplay skipped to the next item — precisely when a quality switch had just made the offset large. The decision now lives in hlsRecovery.ts, against the absolute position. - The HTML5 reload primitive resolved on its own canplay timeout, so a reload the server never served reported success. The picker showed a quality that was not playing and the caller had nothing to revert. TRACES: UR-074, UR-004 | DR-177 | UT-173, UT-174, UT-175 --- docs/requirements.md | 21 +- docs/traceability.md | 9557 ++++++++--------- scripts/extract-traces.test.ts | 4 +- src-tauri/src/repository/device_profile.rs | 15 - src-tauri/src/repository/online.rs | 43 +- src-tauri/src/repository/types.rs | 12 - src/lib/api/bindings.ts | 15 +- src/lib/components/player/VideoPlayer.svelte | 47 +- src/lib/components/player/hlsRecovery.test.ts | 64 + src/lib/components/player/hlsRecovery.ts | 55 + .../components/player/subtitleTracks.test.ts | 62 - src/lib/components/player/subtitleTracks.ts | 37 +- src/lib/player/adapters/html5Adapter.test.ts | 23 + src/lib/player/adapters/html5Adapter.ts | 35 +- 14 files changed, 4566 insertions(+), 5424 deletions(-) create mode 100644 src/lib/components/player/hlsRecovery.test.ts create mode 100644 src/lib/components/player/hlsRecovery.ts diff --git a/docs/requirements.md b/docs/requirements.md index 3edef1ff..b7538a39 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -334,11 +334,8 @@ Internal architecture, components, and application logic. | DR-162 | Video streams are opened against a **bandwidth ceiling the user chose**, instead of a fixed allowance nobody could change. Every video URL carried `MaxStreamingBitrate=20000000`/`VideoBitrate=18000000`, `PlaybackInfo` negotiated at 20 Mbps, and the device profile advertised `999999999` — so on a metered or slow connection the only lever was not watching. `StreamingQuality` is a ladder of ceilings (Original, 20/10/8/4/2/1 Mbps, 720 kbps) in which a step is not a label but a bundle of transcode parameters: the total ceiling, the audio share of it, and the resolution that budget can carry. It lives in Rust because those numbers are Jellyfin encoding vocabulary — the frontend names a variant and reads labels back over `player_get_streaming_qualities`, the same arrangement as the EQ preset curves. The video bitrate is the total *minus* the audio share, so the two together honour the cap rather than overshooting it by the size of the audio track, and `MaxHeight` falls with the ladder so a small budget is not spent on pixels it cannot afford. The cap has to reach the **negotiation**, not only the transcode URL: `max_static_bitrate` in the device profile is what makes the server refuse to direct-play a source fatter than the ceiling, and without it a 30 Mbps remux is handed over untouched and every URL parameter downstream is moot — which is why it is applied at all four places that decide bandwidth (the HLS builder, `PlaybackInfo`, `open_live_stream`, and the background-audio handoff, which takes the lower of the cap and its own 384 kbps). The ceiling is process-wide rather than a field on `OnlineRepository`, mirroring `INCLUDE_CATALOG_BROWSE`: it is a preference about *this device's connection*, it must survive a repository rebuilt on re-login, and every builder plus the negotiation have to agree on it or the cap leaks. Settings owns the durable default and is the only writer to `app_settings` — persisted unlike the rest of `VideoSettings`, because a limit set for a metered connection that silently reverts to uncapped on the next launch spends the user's data with no changed setting to show for it — and it is restored at startup from the async runtime, defaulting to uncapped if the read fails so a database problem degrades to the old behaviour rather than to an arbitrary limit. The in-player menu is the per-video override: a cap is a property of the stream the server is producing, so it cannot be applied to one already in flight — `player_set_stream_quality` re-opens the stream at the new quality and resumes at the current position, reloading a native backend itself and handing HTML5 a URL for the same `reloadSource` primitive the audio-track switch uses, so no strategy branch lives in the UI. It deliberately does not persist. This gives UR-070 its resume-at-the-same-point mechanism; the server-offered per-item rendition list that requirement also asks for remains proposed | Playback | UR-074, UR-070 | Done | | DR-174 | Tiles of mixed shapes are laid out **justified** rather than gridded. A CSS grid gives every cell one box, so on a page holding square music covers, 16:9 library backdrops and 2:3 posters at once, everything that is not the chosen shape is cropped to it — the home shortcut strip was explicitly forcing `aspect="video"` on music libraries for exactly this reason, which lined the row up by cutting the covers down. `layoutMosaic` packs tiles into rows of a **shared height** and gives each its own width from its own aspect ratio: it adds tiles to a row until the height needed to fill the container has fallen to the target, closes the row there (so rows land at or below the target, never above), and justifies the row to the container width by absorbing the rounding remainder into its widest tile, where a pixel is least visible. The last row is deliberately *not* justified — with one tile left over, filling the width would inflate it to a banner — so it sits at the target height, left-aligned. Ratios are clamped to a band, which costs a crop on genuine outliers and stops one panorama owning a row or one very tall image shrinking to a sliver. It is a pure module with no DOM: the component supplies only the two things the DOM knows — the measured container width, and the artwork's *decoded* aspect ratio, reported by `CachedImage` so the layout uses the shape an image actually has rather than the one its item type implies. Those measurements are committed in one debounced batch rather than per image, because artwork arrives over several hundred milliseconds and re-packing on each arrival would shuffle the grid under the pointer repeatedly. Labels are drawn *over* the bottom of each tile rather than beneath it: a caption below sits outside the computed box, and one that wraps to two lines would break the row alignment the layout exists to provide | UI | UR-075 | Done | | DR-175 | A library knows which favourites category it belongs to, and the frontend does not work it out. The mosaic offers a favourites tile per category beside its library, which needs a collection-type → category answer; deriving it in Svelte would have re-created the exact leak `SearchScope::item_types` was extracted to close (docs/specs/scoped-search-boundary.md) — one table of Jellyfin vocabulary, differing only in which vocabulary. `SearchScope::for_collection_type` maps `movies`/`tvshows`/`music` and returns `None` for everything else, so a Live TV or books library gets no tile at all rather than one opening an unfiltered list; `All` is never derived from a library, being the cross-library entry offered beside them rather than a property of one. `Library::new` stamps the result onto every library at construction — a constructor rather than a struct literal precisely so a derived field cannot be forgotten at one of the four sites — and it rides to the frontend as an optional `favoritesScope`, absent rather than null when there is none. The UI's remaining share is presentation only: what to call the tile, where to put it, and showing a category's tile **once** however many libraries share it, since two movie libraries have one favourites list between them | UI | UR-075, UR-067 | Done | -| DR-176 | The server is never asked to burn a subtitle into the picture. `PlaybackInfo` omitted `SubtitleStreamIndex`, which does not mean "none" — the server then honours the source's default/forced flag and picks a track itself. On a source whose default subtitle is image-based (PGS/DVD/DVB) that track cannot go out as a sidecar, so the server falls back to `SubtitleMethod=Encode` and composites it into the video. The cost lands on the *video*, not the subtitle: burn-in rules out remuxing, so an HEVC stream the device could have taken untouched is re-encoded frame by frame. Observed on an HEVC + E-AC-3 + PGSSUB episode, where only the audio actually needed transcoding: the server could not sustain the re-encode in real time, the buffer never grew past a single segment, and playback stalled every few seconds — taking seeking with it, since each seek restarted the encoder and cost seconds before the first frame. The fix is to request `SubtitleStreamIndex=-1` explicitly and to advertise every *text* format we can render (`srt`/`subrip`/`ass`/`ssa`/`vtt`) as `External`, so a subtitle can only ever arrive as a sidecar. Nothing is lost, because the app already fetches subtitle tracks itself and draws them over the video (UR-020) — the server's composited copy was always redundant. Image-based tracks are consequently not offered, which is honest rather than a regression: the renderer cannot composite a bitmap, and the previous behaviour paid for them by making the whole stream unwatchable. Both halves of that hold at the layer that can enforce them. The sentinel travels on the stream URL as well as in the negotiation, because the negotiation is not what opens most streams — a quality switch, a transcoded seek and an audio-track switch each rebuild the URL on their own, and an omitted index there lets the server pick the default track back up out of whatever session state it still holds. And "not offered" is enforced where the offer is made: each subtitle stream crosses the boundary carrying the backend's verdict on whether it can arrive as a sidecar, so the picker lists only tracks the app can draw instead of showing an entry that ticks and displays nothing. Only an explicit "no" hides a track, so a stream carrying no verdict behaves as before | Playback | UR-020, UR-004 | Done | +| DR-176 | The server is never asked to burn a subtitle into the picture. `PlaybackInfo` omitted `SubtitleStreamIndex`, which does not mean "none" — the server then honours the source's default/forced flag and picks a track itself. On a source whose default subtitle is image-based (PGS/DVD/DVB) that track cannot go out as a sidecar, so the server falls back to `SubtitleMethod=Encode` and composites it into the video. The cost lands on the *video*, not the subtitle: burn-in rules out remuxing, so an HEVC stream the device could have taken untouched is re-encoded frame by frame. Observed on an HEVC + E-AC-3 + PGSSUB episode, where only the audio actually needed transcoding: the server could not sustain the re-encode in real time, the buffer never grew past a single segment, and playback stalled every few seconds — taking seeking with it, since each seek restarted the encoder and cost seconds before the first frame. The fix is to request `SubtitleStreamIndex=-1` explicitly and to advertise every *text* format we can render (`srt`/`subrip`/`ass`/`ssa`/`vtt`) as `External`, so a subtitle can only ever arrive as a sidecar. Nothing is lost, because the app already fetches subtitle tracks itself and draws them over the video (UR-020) — the server's composited copy was always redundant. Image-based tracks are consequently not offered, which is honest rather than a regression: the renderer cannot composite a bitmap, and the previous behaviour paid for them by making the whole stream unwatchable | Playback | UR-020, UR-004 | Done | | DR-177 | Each video transcode this device opens is its own server-side job, and the one it replaces is stopped. Jellyfin keys a transcode job by device **and** play session, and every stream URL the app built carried the same hardcoded `DeviceId` with no `PlaySessionId` at all — so the second stream for an item was indistinguishable from the first. Re-opening a stream is not rare: a mid-playback quality switch (UR-074), a transcoded seek and an audio-track switch all do it, each leaving the previous ffmpeg running. Observed on-device when switching bitrate mid-film: the server served the new playlist, then rejected the new job's segments with `400 hls1/main/0.ts` while the two jobs contended for one transcode path, and playback stalled — reproducible against the server, where a second stream for a live job's item alternates between serving bytes and 400ing per attempt, which is what made it read as flaky rather than broken. `begin_video_play_session` mints a session id per open and reports the one it supersedes; the URL builder stops that job (`DELETE /Videos/ActiveEncodings`, un-retried and best-effort — a slow stop must not delay playback, and the new stream no longer collides either way) before returning. Placing it in the URL builder rather than in each caller means every re-open path is covered by construction. Two client faults made the same incident worse and are fixed with it: the fatal-HLS-error handler added the transcode seek offset to a position that already included it, so past roughly the halfway mark of a film any transient network error cleared the "near end" threshold and was reported as end-of-stream — turning a recoverable stall into a skip to the next item, exactly when a quality switch had just made the offset large; and the HTML5 reload primitive resolved on its own `canplay` timeout, so a reload the server never served reported success, leaving the picker showing a quality that was not playing and the caller with nothing to revert | Playback | UR-074, UR-004 | Done | -| DR-178 | Every position that leaves the app is read from the controller, not from a backend that may not be playing anything. `PlayerController::position()` forwards to the native backend, which is authoritative for exactly one of the three ways this app renders media. On the **webview** path — the shipping default for video on both platforms — nothing is loaded into that backend at all: the `