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>
This commit is contained in:
@@ -1772,6 +1772,7 @@ impl MediaRepository for OnlineRepository {
|
|||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// TRACES: UR-071 | DR-123
|
||||||
fn get_video_download_url(
|
fn get_video_download_url(
|
||||||
&self,
|
&self,
|
||||||
item_id: &str,
|
item_id: &str,
|
||||||
@@ -1789,27 +1790,43 @@ impl MediaRepository for OnlineRepository {
|
|||||||
// Map the frontend quality preset to concrete transcode params. For
|
// Map the frontend quality preset to concrete transcode params. For
|
||||||
// "original" we request a direct static copy (no transcode) which is
|
// "original" we request a direct static copy (no transcode) which is
|
||||||
// byte-range resumable; other presets ask the server to transcode.
|
// 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 {
|
match quality {
|
||||||
"high" => {
|
"high" => {
|
||||||
params.push("videoBitrate=8000000".to_string());
|
params.push("videoBitRate=8000000".to_string());
|
||||||
params.push("maxHeight=1080".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("videoCodec=h264".to_string());
|
||||||
params.push("audioCodec=aac".to_string());
|
params.push("audioCodec=aac".to_string());
|
||||||
|
params.push("allowVideoStreamCopy=false".to_string());
|
||||||
}
|
}
|
||||||
"medium" => {
|
"medium" => {
|
||||||
params.push("videoBitrate=4000000".to_string());
|
params.push("videoBitRate=4000000".to_string());
|
||||||
params.push("maxHeight=720".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("videoCodec=h264".to_string());
|
||||||
params.push("audioCodec=aac".to_string());
|
params.push("audioCodec=aac".to_string());
|
||||||
|
params.push("allowVideoStreamCopy=false".to_string());
|
||||||
}
|
}
|
||||||
"low" => {
|
"low" => {
|
||||||
params.push("videoBitrate=1500000".to_string());
|
params.push("videoBitRate=1500000".to_string());
|
||||||
params.push("maxHeight=480".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("videoCodec=h264".to_string());
|
||||||
params.push("audioCodec=aac".to_string());
|
params.push("audioCodec=aac".to_string());
|
||||||
|
params.push("allowVideoStreamCopy=false".to_string());
|
||||||
}
|
}
|
||||||
// "original" (and any unknown value) → direct, resumable copy.
|
// "original" (and any unknown value) → direct, resumable copy.
|
||||||
_ => {
|
_ => {
|
||||||
@@ -2548,7 +2565,7 @@ mod tests {
|
|||||||
// with no transcode params.
|
// with no transcode params.
|
||||||
assert!(url.contains("Static=true"), "url: {url}");
|
assert!(url.contains("Static=true"), "url: {url}");
|
||||||
assert!(
|
assert!(
|
||||||
!url.contains("videoBitrate"),
|
!url.contains("videoBitRate"),
|
||||||
"original must not transcode: {url}"
|
"original must not transcode: {url}"
|
||||||
);
|
);
|
||||||
assert!(
|
assert!(
|
||||||
@@ -2568,7 +2585,7 @@ mod tests {
|
|||||||
"{quality} must use stream.mp4: {url}"
|
"{quality} must use stream.mp4: {url}"
|
||||||
);
|
);
|
||||||
assert!(
|
assert!(
|
||||||
url.contains("videoBitrate="),
|
url.contains("videoBitRate="),
|
||||||
"{quality} must set bitrate: {url}"
|
"{quality} must set bitrate: {url}"
|
||||||
);
|
);
|
||||||
assert!(
|
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]
|
#[test]
|
||||||
fn test_video_download_url_passes_media_source_id() {
|
fn test_video_download_url_passes_media_source_id() {
|
||||||
let repo = create_test_repository();
|
let repo = create_test_repository();
|
||||||
|
|||||||
Reference in New Issue
Block a user