Compare commits

..
Author SHA1 Message Date
dtourolleandClaude Opus 5 adc460f35d fix(downloads): honor the selected bitrate (videoBitRate, capital R)
Downloading at a specific quality silently returned the full-size
original. The download URL builder spelled the transcode params
`videoBitrate`/`audioBitrate`, but Jellyfin binds `videoBitRate`/
`audioBitRate` — with a capital R.

Query-key binding is case-insensitive, so this is not a casing
preference: the lowercase-r form is a different token that fails to
bind. The server discards it without error and then stream-copies the
source, so picking "480p" produced an original-quality file with no
failure surfaced anywhere. `maxHeight`/`videoCodec` were unaffected
(case-insensitive binding covers them), which is why the height cap
applied while the bitrate cap vanished.

Also set `allowVideoStreamCopy=false` on the transcode presets to force
a real re-encode. Video stream-copy is gated by `allowVideoStreamCopy`,
not `enableAutoStreamCopy` — the latter governs audio only.

`original` is unchanged: it stays a deliberate direct static copy, now
pinned by a test.

The pre-existing unit tests asserted the broken lowercase-r spellings,
so they passed against broken code; corrected. Verified red -> green by
extracting the pre-fix and post-fix builder bodies into an isolated
harness: 15 assertion failures before, 0 after.

TRACES: UR-071 | DR-123

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-08-12 18:44:54 +02:00
4 changed files with 91 additions and 95 deletions
+85 -8
View File
@@ -1772,6 +1772,7 @@ impl MediaRepository for OnlineRepository {
)
}
/// TRACES: UR-071 | DR-123
fn get_video_download_url(
&self,
item_id: &str,
@@ -1789,27 +1790,43 @@ impl MediaRepository for OnlineRepository {
// Map the frontend quality preset to concrete transcode params. For
// "original" we request a direct static copy (no transcode) which is
// byte-range resumable; other presets ask the server to transcode.
//
// 🔴 It is `videoBitRate`/`audioBitRate` — **capital R**. Jellyfin binds
// query keys case-insensitively, so `maxHeight`/`videoCodec` casing is
// free, but `videoBitrate` (lowercase r) is a *different token*: it
// fails to bind, is silently dropped, and the requested cap vanishes
// with no error. That is why every "480p"/"720p" download came back at
// full original quality. See `Jellyfin.Api` BaseEncodingJobOptions.
//
// `allowVideoStreamCopy=false` forces a real re-encode. Without it the
// server may stream-copy the source when it already satisfies the cap —
// fine in itself, but it also means a mis-typed cap degrades silently.
// Note `enableAutoStreamCopy=false` alone does NOT stop a *video* copy;
// video copy is gated by `allowVideoStreamCopy`.
match quality {
"high" => {
params.push("videoBitrate=8000000".to_string());
params.push("videoBitRate=8000000".to_string());
params.push("maxHeight=1080".to_string());
params.push("audioBitrate=384000".to_string());
params.push("audioBitRate=384000".to_string());
params.push("videoCodec=h264".to_string());
params.push("audioCodec=aac".to_string());
params.push("allowVideoStreamCopy=false".to_string());
}
"medium" => {
params.push("videoBitrate=4000000".to_string());
params.push("videoBitRate=4000000".to_string());
params.push("maxHeight=720".to_string());
params.push("audioBitrate=256000".to_string());
params.push("audioBitRate=256000".to_string());
params.push("videoCodec=h264".to_string());
params.push("audioCodec=aac".to_string());
params.push("allowVideoStreamCopy=false".to_string());
}
"low" => {
params.push("videoBitrate=1500000".to_string());
params.push("videoBitRate=1500000".to_string());
params.push("maxHeight=480".to_string());
params.push("audioBitrate=128000".to_string());
params.push("audioBitRate=128000".to_string());
params.push("videoCodec=h264".to_string());
params.push("audioCodec=aac".to_string());
params.push("allowVideoStreamCopy=false".to_string());
}
// "original" (and any unknown value) → direct, resumable copy.
_ => {
@@ -2548,7 +2565,7 @@ mod tests {
// with no transcode params.
assert!(url.contains("Static=true"), "url: {url}");
assert!(
!url.contains("videoBitrate"),
!url.contains("videoBitRate"),
"original must not transcode: {url}"
);
assert!(
@@ -2568,7 +2585,7 @@ mod tests {
"{quality} must use stream.mp4: {url}"
);
assert!(
url.contains("videoBitrate="),
url.contains("videoBitRate="),
"{quality} must set bitrate: {url}"
);
assert!(
@@ -2584,6 +2601,66 @@ mod tests {
}
}
/// The bitrate params are spelled `videoBitRate`/`audioBitRate` — **capital
/// R**. Jellyfin binds query keys case-insensitively, so this is not a
/// casing preference: `videoBitrate` is a *different token* that fails to
/// bind and is silently discarded, taking the user's quality cap with it.
/// Nothing errors — the download just returns the full-size original, which
/// is exactly how this bug went unnoticed.
#[test]
fn test_video_download_url_bitrate_params_use_capital_r_spelling() {
let repo = create_test_repository();
for quality in ["high", "medium", "low"] {
let url = repo.get_video_download_url("item123", quality, None);
assert!(
url.contains("videoBitRate="),
"{quality} must spell it videoBitRate (capital R): {url}"
);
assert!(
url.contains("audioBitRate="),
"{quality} must spell it audioBitRate (capital R): {url}"
);
// The lowercase-r spellings never bind — they must not appear at
// all, or the cap is silently dropped by the server.
assert!(
!url.contains("videoBitrate="),
"{quality} emits the unbindable lowercase-r spelling: {url}"
);
assert!(
!url.contains("audioBitrate="),
"{quality} emits the unbindable lowercase-r spelling: {url}"
);
}
}
/// A correctly-spelled cap is still only *conditionally* honored: the server
/// may stream-copy the source when it already satisfies the cap. Video copy
/// is gated by `allowVideoStreamCopy` (NOT `enableAutoStreamCopy`, which
/// only governs audio), so the transcode presets must disable it to
/// guarantee a real re-encode at the requested bitrate.
#[test]
fn test_video_download_url_transcode_presets_forbid_video_stream_copy() {
let repo = create_test_repository();
for quality in ["high", "medium", "low"] {
let url = repo.get_video_download_url("item123", quality, None);
assert!(
url.contains("allowVideoStreamCopy=false"),
"{quality} must forbid video stream copy: {url}"
);
}
// "original" is a deliberate direct copy — it must NOT disable copying.
let original = repo.get_video_download_url("item123", "original", None);
assert!(
!original.contains("allowVideoStreamCopy=false"),
"original must remain a direct copy: {original}"
);
}
#[test]
fn test_video_download_url_passes_media_source_id() {
let repo = create_test_repository();
+4 -68
View File
@@ -97,14 +97,9 @@ fn belongs_to_series(item: &MediaItem, series_id: &str) -> bool {
/// working through.
/// 2. **The server's Next Up** for this series — it accounts for watch history
/// we do not cache locally.
/// 3. **The episode after the furthest-watched one**, falling back to the first
/// unwatched episode when nothing has been watched or the series is finished.
/// This is the offline path: `OfflineRepository::get_next_up_episodes`
/// returns an empty vec, so without this rung the whole feature would be
/// online-only. It deliberately does *not* return the first unwatched
/// episode outright — an unwatched episode behind the viewer's furthest
/// point was skipped on purpose, and sending them back to it is the bug
/// DR-101 was reopened for.
/// 3. **The first unwatched episode** in series order. This is the offline path:
/// `OfflineRepository::get_next_up_episodes` returns an empty vec, so without
/// this rung the whole feature would be online-only.
/// 4. **The first episode**, so a never-watched series opens on its premiere
/// rather than on nothing.
///
@@ -141,18 +136,7 @@ pub fn pick_current_episode(
return Some(matched.unwrap_or(found).clone());
}
// 3. The episode after the furthest-watched one. Not simply the first
// unwatched: a viewer who skipped the pilot but is deep into season 3
// must not be dragged back to S1E1. An earlier gap is a deliberate skip;
// where they stopped is the *last* thing they watched.
if let Some(furthest) = episodes.iter().rposition(is_played) {
if let Some(found) = episodes.get(furthest + 1) {
return Some(found.clone());
}
}
// Nothing watched yet (or the furthest-watched episode is the finale):
// the first unwatched episode in series order.
// 3. First unwatched in series order.
if let Some(found) = episodes.iter().find(|e| !is_played(e)) {
return Some(found.clone());
}
@@ -368,54 +352,6 @@ mod tests {
assert_eq!(current.id, "s2e2");
}
/// A viewer deep in season 3 who never watched the pilot must not be sent
/// back to it: the gap was a skip, not the place they stopped.
#[test]
fn resumes_after_the_furthest_watched_episode_not_the_first_gap() {
let mut eps = [season(1, 4), season(2, 4), season(3, 4)].concat();
for ep in eps.iter_mut() {
// Everything through S3E3 watched, except the never-watched pilot.
let watched_through = ep.parent_index_number < Some(3) || ep.index_number <= Some(3);
if watched_through && ep.id != "s1e1" {
*ep = watched(ep.clone());
}
}
let current = pick_current_episode(SERIES, &eps, &[], &[]).unwrap();
assert_eq!(current.id, "s3e4");
}
/// The furthest-watched episode being a finale must still roll into the
/// next season rather than stopping the series.
#[test]
fn resumes_into_the_next_season_after_a_skipped_earlier_episode() {
let mut eps = [season(1, 3), season(2, 3)].concat();
for ep in eps.iter_mut() {
if ep.parent_index_number == Some(1) && ep.id != "s1e1" {
*ep = watched(ep.clone());
}
}
let current = pick_current_episode(SERIES, &eps, &[], &[]).unwrap();
assert_eq!(current.id, "s2e1");
}
/// Specials sort last, so watching one must not mark the series finished
/// while numbered episodes remain.
#[test]
fn a_watched_special_does_not_end_the_series() {
let mut eps = [season(1, 3), vec![episode("s0e1", 0, 1)]].concat();
sort_series_order(&mut eps);
for ep in eps.iter_mut() {
if ep.id == "s1e1" || ep.id == "s0e1" {
*ep = watched(ep.clone());
}
}
let current = pick_current_episode(SERIES, &eps, &[], &[]).unwrap();
assert_eq!(current.id, "s1e2");
}
#[test]
fn crosses_a_season_boundary_when_a_season_is_finished() {
let mut eps = [season(1, 3), season(2, 3)].concat();
+1 -15
View File
@@ -45,16 +45,9 @@
* TRACES: UR-068 | DR-119
*/
showFavorite?: boolean;
/**
* Force the artwork box to a fixed aspect ratio instead of deriving one from
* the item. Use on rows that mix item kinds (e.g. the home "Your Libraries"
* strip, where square music art next to 16:9 video art would otherwise give
* the cards different heights). Artwork still fills the box via object-cover.
*/
aspect?: "square" | "video" | "poster";
}
let { item, size = "medium", showProgress = false, showDownloadStatus = true, sizeLabel, downloadedBadge, onRemove, onclick, onLongPress, showFavorite = true, aspect }: Props = $props();
let { item, size = "medium", showProgress = false, showDownloadStatus = true, sizeLabel, downloadedBadge, onRemove, onclick, onLongPress, showFavorite = true }: Props = $props();
// Long-press detection. We arm a timer on pointerdown; if it fires before the
// pointer is released (or moves too far), we treat it as a long press and set a
@@ -186,14 +179,7 @@
"kind" in item && (item.kind === "track" || item.kind === "album" || item.kind === "artist" || item.kind === "playlist")
);
const FIXED_ASPECT = {
square: "aspect-square",
video: "aspect-video",
poster: "aspect-[2/3]",
} as const;
const aspectRatio = $derived(() => {
if (aspect) return FIXED_ASPECT[aspect];
if ("kind" in item) {
return isMusicType ? "aspect-square" : "aspect-[2/3]";
}
+1 -4
View File
@@ -159,15 +159,12 @@
{#if shortcutLibraries.length > 0}
<div>
<h2 class="text-xl font-bold text-white mb-4 px-4">Your Libraries</h2>
<div class="flex gap-4 overflow-x-auto px-4 pb-2 items-start">
<div class="flex gap-4 overflow-x-auto px-4 pb-2">
{#each shortcutLibraries as lib (lib.id)}
<div class="flex-shrink-0">
<!-- Uniform 16:9 artwork so music (square) and video libraries
line up at the same height in this mixed row. -->
<MediaCard
item={lib}
size="medium"
aspect="video"
onclick={() => handleLibraryClick(lib)}
/>
</div>