Merge branch 'master' into worktree-mosaic-library
Renumbers the mosaic's requirement IDs out of the way of the download work that landed on master in parallel: it had already claimed DR-163/DR-164 and UT-162, so the mosaic layout is now DR-172, the library favourites scope DR-173, and its composition test UT-167. Note for the download branch: its UT-162..UT-165 rows trace to DR-163..DR-166, none of which are defined in requirements.md — that branch defined DR-167..171 instead. Those references are orphaned and want a look; nothing here touches them.
This commit is contained in:
@@ -631,8 +631,6 @@ pub async fn resume_queued_downloads(
|
||||
) -> Result<ResumeQueuedResult, String> {
|
||||
use crate::repository::MediaRepository;
|
||||
|
||||
use crate::repository::HybridRepository;
|
||||
|
||||
let repo = repository.0.get(&handle).ok_or("Repository not found")?;
|
||||
|
||||
// The pump needs a target_dir; use the same storage root the other download
|
||||
@@ -683,12 +681,13 @@ pub async fn resume_queued_downloads(
|
||||
async move {
|
||||
if media_type == "video" {
|
||||
Some(
|
||||
<HybridRepository as MediaRepository>::get_video_download_url(
|
||||
crate::repository::resolve_video_download_url(
|
||||
repo.as_ref(),
|
||||
&item_id,
|
||||
&quality,
|
||||
None,
|
||||
),
|
||||
)
|
||||
.await,
|
||||
)
|
||||
} else {
|
||||
match repo.get_audio_stream_url(&item_id).await {
|
||||
|
||||
@@ -845,7 +845,19 @@ pub async fn get_downloads(
|
||||
Ok(DownloadsResponse { downloads, stats })
|
||||
}
|
||||
|
||||
/// Pause a download
|
||||
/// Pause a download.
|
||||
///
|
||||
/// Writing `status = 'paused'` is only half of it, and used to be all of it: the
|
||||
/// streaming task knew nothing about the row and kept running, then overwrote it
|
||||
/// with `completed`/`failed` when it finished. The row flicked to "paused" and
|
||||
/// undid itself — the reported "pause does not work". Signalling the worker is
|
||||
/// what actually stops the bytes; it leaves the `.part` file in place so
|
||||
/// [`resume_download`] can continue from it.
|
||||
///
|
||||
/// A queued (not yet started) download has no worker to signal, and the status
|
||||
/// write alone is enough — the pump skips anything that is not `pending`.
|
||||
///
|
||||
/// TRACES: UR-055 | DR-168
|
||||
#[tauri::command]
|
||||
#[specta::specta]
|
||||
pub async fn pause_download(
|
||||
@@ -858,19 +870,34 @@ pub async fn pause_download(
|
||||
};
|
||||
|
||||
let query = Query::with_params(
|
||||
"UPDATE downloads SET status = 'paused' WHERE id = ? AND status = 'downloading'",
|
||||
"UPDATE downloads SET status = 'paused' WHERE id = ? AND status IN ('downloading', 'pending')",
|
||||
vec![QueryParam::Int64(download_id)],
|
||||
);
|
||||
|
||||
db_service.execute(query).await.map_err(|e| e.to_string())?;
|
||||
|
||||
let was_running = crate::download::stop::signal(download_id);
|
||||
info!(
|
||||
"[pause] Download {} paused (in flight: {})",
|
||||
download_id, was_running
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Resume a paused download
|
||||
/// Resume a paused download.
|
||||
///
|
||||
/// Flipping the row back to `pending` is likewise not enough on its own: the
|
||||
/// pump is not a poller, it runs when something calls it, so a resumed download
|
||||
/// sat untouched until some unrelated event happened to pump the queue. That is
|
||||
/// the other half of "resume does not work".
|
||||
///
|
||||
/// TRACES: UR-055 | DR-168
|
||||
#[tauri::command]
|
||||
#[specta::specta]
|
||||
pub async fn resume_download(
|
||||
app: tauri::AppHandle,
|
||||
db: State<'_, DatabaseWrapper>,
|
||||
download_manager: State<'_, DownloadManagerWrapper>,
|
||||
download_id: i64,
|
||||
) -> Result<(), String> {
|
||||
let db_service = {
|
||||
@@ -879,11 +906,22 @@ pub async fn resume_download(
|
||||
};
|
||||
|
||||
let query = Query::with_params(
|
||||
"UPDATE downloads SET status = 'pending' WHERE id = ? AND status = 'paused'",
|
||||
"UPDATE downloads SET status = 'pending', error_message = NULL WHERE id = ? AND status IN ('paused', 'failed')",
|
||||
vec![QueryParam::Int64(download_id)],
|
||||
);
|
||||
|
||||
db_service.execute(query).await.map_err(|e| e.to_string())?;
|
||||
|
||||
// Drop any stale stop flag before the pump can start this id again, or the
|
||||
// resumed run would read the pause that stopped it and halt immediately.
|
||||
crate::download::stop::clear(download_id);
|
||||
|
||||
let active_downloads = {
|
||||
let manager = download_manager.0.lock().map_err(|e| e.to_string())?;
|
||||
manager.get_active_downloads()
|
||||
};
|
||||
pump_download_queue(app, db_service, active_downloads).await;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -923,6 +961,13 @@ pub async fn cancel_download(
|
||||
.await
|
||||
.map_err(|e| e.to_string())?;
|
||||
|
||||
// Stop the worker if this download is actually running. Without this the
|
||||
// task keeps streaming into a `.part` file whose `downloads` row has just
|
||||
// been deleted — bytes with nothing pointing at them, and the file below is
|
||||
// removed while still being written to. (DR-168)
|
||||
crate::download::stop::signal(download_id);
|
||||
crate::download::stop::clear(download_id);
|
||||
|
||||
// Unregister from download manager (in case it was active)
|
||||
{
|
||||
let manager = download_manager.0.lock().map_err(|e| e.to_string())?;
|
||||
@@ -934,10 +979,12 @@ pub async fn cancel_download(
|
||||
);
|
||||
}
|
||||
|
||||
// Delete partial file if exists
|
||||
// Delete the partial file, and any completed file, if present. Both go
|
||||
// through `partial_path` so this cannot drift from what the worker writes —
|
||||
// it did, and every cancelled download leaked its partial. (DR-169)
|
||||
if let Some(path) = file_path {
|
||||
let partial_path = format!("{}.part", path);
|
||||
let _ = std::fs::remove_file(&partial_path); // Ignore errors
|
||||
let target = std::path::PathBuf::from(&path);
|
||||
let _ = std::fs::remove_file(crate::download::worker::partial_path(&target));
|
||||
}
|
||||
|
||||
Ok(())
|
||||
@@ -1244,8 +1291,6 @@ pub async fn enqueue_video_downloads(
|
||||
download_ids: Vec<i64>,
|
||||
target_dir: String,
|
||||
) -> Result<(), String> {
|
||||
use crate::repository::MediaRepository;
|
||||
|
||||
let repo = repository.0.get(&handle).ok_or("Repository not found")?;
|
||||
|
||||
let db_service = {
|
||||
@@ -1270,10 +1315,12 @@ pub async fn enqueue_video_downloads(
|
||||
}
|
||||
};
|
||||
|
||||
// Build the transcode URL (pure URL builder, no server round-trip).
|
||||
let stream_url = repo
|
||||
.as_ref()
|
||||
.get_video_download_url(&item_id, &quality, None);
|
||||
// Build the download URL, resolving the source's audio codec first so a
|
||||
// track this device cannot decode is re-encoded on the way down rather
|
||||
// than saved as a silent file (DR-167).
|
||||
let stream_url =
|
||||
crate::repository::resolve_video_download_url(repo.as_ref(), &item_id, &quality, None)
|
||||
.await;
|
||||
|
||||
let update_query = Query::with_params(
|
||||
"UPDATE downloads SET status = 'pending', stream_url = ?, target_dir = ? WHERE id = ?",
|
||||
@@ -1530,7 +1577,11 @@ fn spawn_download_worker(
|
||||
let _ = progress_app.emit("download-event", event);
|
||||
};
|
||||
|
||||
let result = worker.download(&task, on_progress).await;
|
||||
// Registering returns a fresh flag, so a download resumed after a pause
|
||||
// does not inherit the stop that ended its previous run. (DR-168)
|
||||
let stop_flag = crate::download::stop::register(download_id);
|
||||
let result = worker.download(&task, &stop_flag, on_progress).await;
|
||||
crate::download::stop::clear(download_id);
|
||||
|
||||
// Free the slot before pumping so the next download can take it.
|
||||
if let Ok(mut active) = active_downloads.lock() {
|
||||
@@ -1601,6 +1652,17 @@ fn spawn_download_worker(
|
||||
Err(e) => error!(" Completed event emit failed: {:?}", e),
|
||||
}
|
||||
}
|
||||
// A pause or cancel is not a failure. The row already says `paused`
|
||||
// (or the row is gone, for a cancel), and overwriting that with
|
||||
// `failed` is what made a pause look like an error and stranded the
|
||||
// download outside the resumable set. The `.part` file is deliberately
|
||||
// left alone — it is what the resume continues from. (DR-168)
|
||||
Err(e) if e.is_stopped() => {
|
||||
info!(
|
||||
"[pump] Download {} stopped by request; partial file kept for resume",
|
||||
download_id
|
||||
);
|
||||
}
|
||||
Err(e) => {
|
||||
error!("Download failed: {:?}", e);
|
||||
|
||||
@@ -1848,17 +1910,26 @@ pub async fn clear_stale_downloads(
|
||||
Arc::new(database.service())
|
||||
};
|
||||
|
||||
// Get file paths for stale downloads (pending/paused/failed)
|
||||
// Ids as well as paths: a stale row may still have a worker attached (a
|
||||
// 'downloading' row that was paused mid-flight is 'paused' here), and
|
||||
// deleting the row without stopping the task leaves it writing to a file we
|
||||
// are about to remove. (DR-168)
|
||||
let file_query = Query::with_params(
|
||||
"SELECT file_path FROM downloads WHERE user_id = ? AND status IN ('pending', 'paused', 'failed')",
|
||||
"SELECT id, file_path FROM downloads WHERE user_id = ? AND status IN ('pending', 'paused', 'failed')",
|
||||
vec![QueryParam::String(user_id.clone())],
|
||||
);
|
||||
|
||||
let file_paths: Vec<String> = db_service
|
||||
.query_many(file_query, |row| row.get(0))
|
||||
let stale: Vec<(i64, String)> = db_service
|
||||
.query_many(file_query, |row| Ok((row.get(0)?, row.get(1)?)))
|
||||
.await
|
||||
.map_err(|e| e.to_string())?;
|
||||
|
||||
for (id, _) in &stale {
|
||||
crate::download::stop::signal(*id);
|
||||
crate::download::stop::clear(*id);
|
||||
}
|
||||
let file_paths: Vec<String> = stale.into_iter().map(|(_, path)| path).collect();
|
||||
|
||||
// Delete all pending, paused, and failed downloads (but keep completed ones)
|
||||
let delete_query = Query::with_params(
|
||||
"DELETE FROM downloads WHERE user_id = ? AND status IN ('pending', 'paused', 'failed')",
|
||||
@@ -1870,10 +1941,12 @@ pub async fn clear_stale_downloads(
|
||||
.await
|
||||
.map_err(|e| e.to_string())?;
|
||||
|
||||
// Delete any partial files
|
||||
// Delete any partial files, via the shared helper so this cannot drift from
|
||||
// what the worker actually writes. (DR-169)
|
||||
for path in file_paths {
|
||||
let _ = std::fs::remove_file(&path);
|
||||
let _ = std::fs::remove_file(format!("{}.part", path));
|
||||
let target = std::path::PathBuf::from(&path);
|
||||
let _ = std::fs::remove_file(&target);
|
||||
let _ = std::fs::remove_file(crate::download::worker::partial_path(&target));
|
||||
}
|
||||
|
||||
Ok(deleted_count as i64)
|
||||
|
||||
@@ -804,7 +804,7 @@ pub fn repository_get_subtitle_url(
|
||||
#[tauri::command]
|
||||
#[specta::specta]
|
||||
#[allow(dead_code)]
|
||||
pub fn repository_get_video_download_url(
|
||||
pub async fn repository_get_video_download_url(
|
||||
manager: State<'_, RepositoryManagerWrapper>,
|
||||
handle: String,
|
||||
item_id: String,
|
||||
@@ -812,9 +812,16 @@ pub fn repository_get_video_download_url(
|
||||
media_source_id: Option<String>,
|
||||
) -> Result<String, String> {
|
||||
let repo = manager.0.get(&handle).ok_or("Repository not found")?;
|
||||
Ok(repo
|
||||
.as_ref()
|
||||
.get_video_download_url(&item_id, &quality, media_source_id.as_deref()))
|
||||
// Async because the audio-codec policy has to know what the source's audio
|
||||
// is before it can decide whether the file may be copied verbatim (DR-171).
|
||||
// The frontend calls this exactly as before — the decision stays in Rust.
|
||||
Ok(crate::repository::resolve_video_download_url(
|
||||
repo.as_ref(),
|
||||
&item_id,
|
||||
&quality,
|
||||
media_source_id.as_deref(),
|
||||
)
|
||||
.await)
|
||||
}
|
||||
|
||||
/// Mark an item as favorite
|
||||
|
||||
@@ -9,6 +9,7 @@
|
||||
pub mod cache;
|
||||
pub mod events;
|
||||
pub mod network;
|
||||
pub mod stop;
|
||||
pub mod worker;
|
||||
|
||||
use crate::utils::lock::MutexSafe;
|
||||
|
||||
@@ -0,0 +1,150 @@
|
||||
//! Stop signalling for in-flight downloads.
|
||||
//!
|
||||
//! TRACES: UR-055 | DR-168
|
||||
//!
|
||||
//! Pausing and cancelling used to be database-only: `pause_download` wrote
|
||||
//! `status = 'paused'` and nothing else. No cancellation existed anywhere in the
|
||||
//! download stack — no token, no flag, no abort — so the streaming task kept
|
||||
//! running, kept writing bytes, and on finishing overwrote the row with
|
||||
//! `completed` or `failed`. The row flicked to "paused" and then undid itself,
|
||||
//! which is precisely the reported "pause does not work".
|
||||
//!
|
||||
//! This is the missing half: a flag per in-flight download that the worker reads
|
||||
//! between chunks. Setting it makes the worker return [`Stopped`] promptly and
|
||||
//! leave the `.part` file **intact**, which is what lets a resume pick up from
|
||||
//! where it stopped via the existing HTTP Range request.
|
||||
//!
|
||||
//! Kept as a module-level registry rather than on `DownloadManager` because the
|
||||
//! two sides never meet: the command handler holds the manager's lock, while the
|
||||
//! worker runs detached inside `tauri::async_runtime::spawn` with no access to
|
||||
//! Tauri state. A registry both can reach is the smallest thing that works.
|
||||
//!
|
||||
//! [`Stopped`]: crate::download::worker::DownloadError::Stopped
|
||||
|
||||
use std::collections::HashMap;
|
||||
use std::sync::atomic::{AtomicBool, Ordering};
|
||||
use std::sync::{Arc, Mutex, OnceLock};
|
||||
|
||||
use crate::utils::lock::MutexSafe;
|
||||
|
||||
/// download id → its stop flag, for downloads currently in flight.
|
||||
fn registry() -> &'static Mutex<HashMap<i64, Arc<AtomicBool>>> {
|
||||
static REGISTRY: OnceLock<Mutex<HashMap<i64, Arc<AtomicBool>>>> = OnceLock::new();
|
||||
REGISTRY.get_or_init(|| Mutex::new(HashMap::new()))
|
||||
}
|
||||
|
||||
/// Register `download_id` as in-flight and hand back its stop flag.
|
||||
///
|
||||
/// Called by the worker as it starts. A previous flag for the same id is
|
||||
/// replaced, so a download that is paused and later resumed does not inherit the
|
||||
/// set flag from its last run and stop immediately.
|
||||
pub fn register(download_id: i64) -> Arc<AtomicBool> {
|
||||
let flag = Arc::new(AtomicBool::new(false));
|
||||
registry().lock_safe().insert(download_id, flag.clone());
|
||||
flag
|
||||
}
|
||||
|
||||
/// Ask an in-flight download to stop.
|
||||
///
|
||||
/// Returns whether one was actually in flight — the caller uses this to tell a
|
||||
/// running download (which will stop shortly) from a merely queued one (which
|
||||
/// the database update alone has already handled).
|
||||
pub fn signal(download_id: i64) -> bool {
|
||||
match registry().lock_safe().get(&download_id) {
|
||||
Some(flag) => {
|
||||
flag.store(true, Ordering::SeqCst);
|
||||
true
|
||||
}
|
||||
None => false,
|
||||
}
|
||||
}
|
||||
|
||||
/// Forget a download's flag. Called when its task finishes, however it ended.
|
||||
pub fn clear(download_id: i64) {
|
||||
registry().lock_safe().remove(&download_id);
|
||||
}
|
||||
|
||||
/// Whether a stop has been requested for `download_id`.
|
||||
///
|
||||
/// The worker reads its own `Arc<AtomicBool>` directly rather than looking the id
|
||||
/// up, so this exists for the tests that assert the registry's behaviour.
|
||||
#[cfg(test)]
|
||||
pub fn is_stopping(download_id: i64) -> bool {
|
||||
registry()
|
||||
.lock_safe()
|
||||
.get(&download_id)
|
||||
.map(|f| f.load(Ordering::SeqCst))
|
||||
.unwrap_or(false)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
/// Ids are per-test so the shared registry cannot leak between them.
|
||||
fn unique_id(seed: i64) -> i64 {
|
||||
900_000 + seed
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_a_registered_download_starts_unflagged() {
|
||||
let id = unique_id(1);
|
||||
let flag = register(id);
|
||||
assert!(!flag.load(Ordering::SeqCst));
|
||||
assert!(!is_stopping(id));
|
||||
clear(id);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_signal_sets_the_flag_the_worker_reads() {
|
||||
let id = unique_id(2);
|
||||
let flag = register(id);
|
||||
|
||||
assert!(signal(id), "a registered download reports as in flight");
|
||||
assert!(
|
||||
flag.load(Ordering::SeqCst),
|
||||
"the worker's own handle sees it"
|
||||
);
|
||||
assert!(is_stopping(id));
|
||||
|
||||
clear(id);
|
||||
}
|
||||
|
||||
/// The pump only needs to abort a task that exists; a queued row is handled
|
||||
/// by its database status alone.
|
||||
#[test]
|
||||
fn test_signalling_an_unregistered_download_reports_not_in_flight() {
|
||||
assert!(!signal(unique_id(3)));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_clear_forgets_the_download() {
|
||||
let id = unique_id(4);
|
||||
register(id);
|
||||
signal(id);
|
||||
clear(id);
|
||||
|
||||
assert!(!is_stopping(id));
|
||||
assert!(!signal(id), "a cleared download is no longer in flight");
|
||||
}
|
||||
|
||||
/// The bug this guards: pause sets the flag, and resume re-runs the same
|
||||
/// download id. If registering reused the old flag, the resumed run would see
|
||||
/// a set flag and stop instantly — a download that could never be resumed.
|
||||
#[test]
|
||||
fn test_reregistering_clears_a_previous_stop() {
|
||||
let id = unique_id(5);
|
||||
register(id);
|
||||
signal(id);
|
||||
assert!(is_stopping(id));
|
||||
|
||||
let fresh = register(id);
|
||||
assert!(!fresh.load(Ordering::SeqCst));
|
||||
assert!(
|
||||
!is_stopping(id),
|
||||
"a resumed download must not inherit the pause"
|
||||
);
|
||||
|
||||
clear(id);
|
||||
}
|
||||
}
|
||||
@@ -1,6 +1,7 @@
|
||||
//! Download worker for HTTP streaming with progress tracking and retry logic
|
||||
|
||||
use log::warn;
|
||||
use std::sync::atomic::{AtomicBool, Ordering};
|
||||
use std::time::Duration;
|
||||
|
||||
use futures_util::StreamExt;
|
||||
@@ -31,10 +32,18 @@ impl DownloadWorker {
|
||||
}
|
||||
}
|
||||
|
||||
/// Download a file with retry logic and progress tracking
|
||||
/// Download a file with retry logic and progress tracking.
|
||||
///
|
||||
/// `stop` is the pause/cancel flag (see [`crate::download::stop`]). It is
|
||||
/// checked between chunks and again between retries, so a paused download
|
||||
/// stops promptly rather than after its next backoff — up to 45 seconds
|
||||
/// away, which reads as the pause having done nothing.
|
||||
///
|
||||
/// TRACES: UR-055 | DR-168
|
||||
pub async fn download<F>(
|
||||
&self,
|
||||
task: &DownloadTask,
|
||||
stop: &AtomicBool,
|
||||
on_progress: F,
|
||||
) -> Result<DownloadResult, DownloadError>
|
||||
where
|
||||
@@ -43,7 +52,10 @@ impl DownloadWorker {
|
||||
let mut retries = 0;
|
||||
|
||||
loop {
|
||||
match self.try_download(task, &on_progress).await {
|
||||
if stop.load(Ordering::SeqCst) {
|
||||
return Err(DownloadError::Stopped);
|
||||
}
|
||||
match self.try_download(task, stop, &on_progress).await {
|
||||
Ok(result) => return Ok(result),
|
||||
Err(e) if retries < self.max_retries && e.is_retryable() => {
|
||||
retries += 1;
|
||||
@@ -63,6 +75,7 @@ impl DownloadWorker {
|
||||
async fn try_download<F>(
|
||||
&self,
|
||||
task: &DownloadTask,
|
||||
stop: &AtomicBool,
|
||||
on_progress: &F,
|
||||
) -> Result<DownloadResult, DownloadError>
|
||||
where
|
||||
@@ -76,7 +89,7 @@ impl DownloadWorker {
|
||||
}
|
||||
|
||||
// Check for partial download
|
||||
let temp_path = task.target_path.with_extension("part");
|
||||
let temp_path = partial_path(&task.target_path);
|
||||
let existing_bytes = if temp_path.exists() {
|
||||
fs::metadata(&temp_path).await.map(|m| m.len()).unwrap_or(0)
|
||||
} else {
|
||||
@@ -100,22 +113,32 @@ impl DownloadWorker {
|
||||
return Err(DownloadError::Http(response.status().as_u16()));
|
||||
}
|
||||
|
||||
// Get content length
|
||||
// Did the server actually honour the Range? A transcode does not, and
|
||||
// answers 200 with the whole stream — appending that would duplicate what
|
||||
// we already hold. (DR-170)
|
||||
let resume_from = resume_offset(existing_bytes, response.status().as_u16());
|
||||
if existing_bytes > 0 && resume_from == 0 {
|
||||
warn!(
|
||||
"Server ignored the Range request (HTTP {}) — restarting {} from the beginning \
|
||||
instead of appending to {} existing bytes",
|
||||
response.status().as_u16(),
|
||||
task.target_path.display(),
|
||||
existing_bytes
|
||||
);
|
||||
}
|
||||
|
||||
// Get content length. Absent on a chunked transcode, which is why progress
|
||||
// for a non-`original` preset has no percentage to show.
|
||||
let _total_bytes = response
|
||||
.headers()
|
||||
.get(reqwest::header::CONTENT_LENGTH)
|
||||
.and_then(|v| v.to_str().ok())
|
||||
.and_then(|v| v.parse::<u64>().ok())
|
||||
.map(|len| {
|
||||
if existing_bytes > 0 {
|
||||
len + existing_bytes
|
||||
} else {
|
||||
len
|
||||
}
|
||||
});
|
||||
.map(|len| len + resume_from);
|
||||
|
||||
// Open file for appending
|
||||
let mut file = if existing_bytes > 0 {
|
||||
// Append only when resuming a range the server agreed to; otherwise
|
||||
// create/truncate so the restarted stream replaces the stale bytes.
|
||||
let mut file = if resume_from > 0 {
|
||||
fs::OpenOptions::new().append(true).open(&temp_path).await
|
||||
} else {
|
||||
fs::File::create(&temp_path).await
|
||||
@@ -123,11 +146,22 @@ impl DownloadWorker {
|
||||
.map_err(|e| DownloadError::FileSystem(e.to_string()))?;
|
||||
|
||||
// Stream download with progress tracking
|
||||
let mut downloaded = existing_bytes;
|
||||
let mut downloaded = resume_from;
|
||||
let mut stream = response.bytes_stream();
|
||||
let mut last_progress_emit = std::time::Instant::now();
|
||||
|
||||
while let Some(chunk) = stream.next().await {
|
||||
// Checked before writing, so a paused download stops on a byte
|
||||
// boundary the `.part` file already accounts for — the Range request
|
||||
// on resume then asks for exactly what is missing. Flushing what we
|
||||
// have and leaving the file in place is the whole mechanism behind
|
||||
// "resume", so this must never delete it. (DR-168)
|
||||
if stop.load(Ordering::SeqCst) {
|
||||
let _ = file.flush().await;
|
||||
let _ = file.sync_all().await;
|
||||
return Err(DownloadError::Stopped);
|
||||
}
|
||||
|
||||
let chunk = chunk.map_err(|e| DownloadError::Network(e.to_string()))?;
|
||||
|
||||
file.write_all(&chunk)
|
||||
@@ -138,7 +172,7 @@ impl DownloadWorker {
|
||||
|
||||
// Emit progress every 500ms or every MB
|
||||
if last_progress_emit.elapsed() > Duration::from_millis(500)
|
||||
|| downloaded % (1024 * 1024) == 0
|
||||
|| downloaded.is_multiple_of(1024 * 1024)
|
||||
{
|
||||
last_progress_emit = std::time::Instant::now();
|
||||
on_progress(downloaded, _total_bytes);
|
||||
@@ -167,6 +201,56 @@ impl DownloadWorker {
|
||||
}
|
||||
}
|
||||
|
||||
/// Where to resume writing a partial download, given how the server answered.
|
||||
///
|
||||
/// A byte offset of 0 means "start the file again"; anything else means "append
|
||||
/// from here".
|
||||
///
|
||||
/// This is what makes non-`original` downloads survive. Those presets ask
|
||||
/// Jellyfin to **transcode**, and a live transcode is chunked with no
|
||||
/// `Content-Length` and cannot be byte-seeked: the server ignores `Range` and
|
||||
/// answers `200` with the whole stream from the beginning, not `206` with the
|
||||
/// requested tail. The worker sent the header and appended the body regardless,
|
||||
/// so every retry — and every resume — concatenated a fresh copy of the whole
|
||||
/// transcode onto the bytes already on disk. The file grew past its real size
|
||||
/// and would not play. Only a `206` actually promises the tail; a `200` means we
|
||||
/// must discard what we have and take the stream from the top.
|
||||
///
|
||||
/// TRACES: UR-071 | DR-170
|
||||
pub fn resume_offset(existing_bytes: u64, status: u16) -> u64 {
|
||||
if existing_bytes == 0 {
|
||||
return 0;
|
||||
}
|
||||
// 206 Partial Content is the only answer that honours the Range request.
|
||||
if status == 206 {
|
||||
existing_bytes
|
||||
} else {
|
||||
0
|
||||
}
|
||||
}
|
||||
|
||||
/// The partial-download sidecar for `target`.
|
||||
///
|
||||
/// **Appends** `.part` rather than replacing the extension. The worker used
|
||||
/// `Path::with_extension("part")`, which replaces: `movie.mp4` became
|
||||
/// `movie.part`. Every cleanup path meanwhile deleted `"{file_path}.part"` —
|
||||
/// `movie.mp4.part` — so nothing ever matched and the partial file of every
|
||||
/// cancelled or failed download was left on disk forever, invisible to the
|
||||
/// disk-usage totals because no `downloads` row pointed at it. That is the
|
||||
/// reported "failure is not cleaned".
|
||||
///
|
||||
/// Appending also removes a collision the old form had: `movie.mp4` and
|
||||
/// `movie.mkv` both mapped to `movie.part` and would have fought over one file.
|
||||
///
|
||||
/// One function so the writer and the cleaners cannot disagree again.
|
||||
///
|
||||
/// TRACES: UR-055 | DR-169
|
||||
pub fn partial_path(target: &std::path::Path) -> std::path::PathBuf {
|
||||
let mut s = target.as_os_str().to_os_string();
|
||||
s.push(".part");
|
||||
std::path::PathBuf::from(s)
|
||||
}
|
||||
|
||||
/// Result of a successful download
|
||||
#[derive(Debug)]
|
||||
pub struct DownloadResult {
|
||||
@@ -179,6 +263,10 @@ pub enum DownloadError {
|
||||
Network(String),
|
||||
Http(u16),
|
||||
FileSystem(String),
|
||||
/// The download was asked to stop (paused or cancelled). Not a failure: the
|
||||
/// row's status already says what happened, and the partial file is kept so a
|
||||
/// resume can continue from it.
|
||||
Stopped,
|
||||
}
|
||||
|
||||
impl DownloadError {
|
||||
@@ -188,8 +276,16 @@ impl DownloadError {
|
||||
DownloadError::Network(_) => true,
|
||||
DownloadError::Http(status) => *status >= 500, // Retry server errors
|
||||
DownloadError::FileSystem(_) => false,
|
||||
// Retrying would restart the very download the user just paused.
|
||||
DownloadError::Stopped => false,
|
||||
}
|
||||
}
|
||||
|
||||
/// Whether this outcome means "the user stopped it", rather than a failure to
|
||||
/// record and report.
|
||||
pub fn is_stopped(&self) -> bool {
|
||||
matches!(self, DownloadError::Stopped)
|
||||
}
|
||||
}
|
||||
|
||||
impl std::fmt::Display for DownloadError {
|
||||
@@ -198,6 +294,7 @@ impl std::fmt::Display for DownloadError {
|
||||
DownloadError::Network(msg) => write!(f, "Network error: {}", msg),
|
||||
DownloadError::Http(status) => write!(f, "HTTP error {}", status),
|
||||
DownloadError::FileSystem(msg) => write!(f, "File system error: {}", msg),
|
||||
DownloadError::Stopped => write!(f, "Download stopped by request"),
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -208,6 +305,64 @@ impl std::error::Error for DownloadError {}
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
/// The bitrate-download corruption: a transcode ignores `Range` and answers
|
||||
/// `200` with the whole stream. Appending that to the bytes already on disk
|
||||
/// duplicated them, so every retry grew the file past its real size and left
|
||||
/// it unplayable. Only `206` promises the requested tail.
|
||||
///
|
||||
/// TRACES: UR-071 | DR-170 | UT-164
|
||||
#[test]
|
||||
fn test_resume_offset_only_appends_when_the_server_honoured_the_range() {
|
||||
// Nothing on disk: start at the beginning either way.
|
||||
assert_eq!(resume_offset(0, 200), 0);
|
||||
assert_eq!(resume_offset(0, 206), 0);
|
||||
|
||||
// The server agreed to the range — append to what we have.
|
||||
assert_eq!(resume_offset(5_000, 206), 5_000);
|
||||
|
||||
// The server ignored it and is sending the whole file (a transcode).
|
||||
// Restart, or the bytes are duplicated.
|
||||
assert_eq!(
|
||||
resume_offset(5_000, 200),
|
||||
0,
|
||||
"a 200 carries the whole stream; appending it corrupts the file"
|
||||
);
|
||||
}
|
||||
|
||||
/// The regression: `with_extension` replaced the extension, so the worker
|
||||
/// wrote `movie.part` while every cleanup path deleted `movie.mp4.part`.
|
||||
/// Nothing matched, and partial files accumulated forever.
|
||||
///
|
||||
/// TRACES: UR-055 | DR-169 | UT-163
|
||||
#[test]
|
||||
fn test_partial_path_appends_rather_than_replacing_the_extension() {
|
||||
use std::path::Path;
|
||||
|
||||
assert_eq!(
|
||||
partial_path(Path::new("/media/movie.mp4")),
|
||||
Path::new("/media/movie.mp4.part"),
|
||||
"the cleanup paths delete \"{{file_path}}.part\"; this must produce it"
|
||||
);
|
||||
|
||||
// Two sources for one title must not fight over a single partial file.
|
||||
assert_ne!(
|
||||
partial_path(Path::new("/media/movie.mp4")),
|
||||
partial_path(Path::new("/media/movie.mkv")),
|
||||
);
|
||||
|
||||
// Extension-less targets still get a sidecar rather than being clobbered.
|
||||
assert_eq!(
|
||||
partial_path(Path::new("/media/track")),
|
||||
Path::new("/media/track.part"),
|
||||
);
|
||||
|
||||
// A dotted name keeps every part of its own name.
|
||||
assert_eq!(
|
||||
partial_path(Path::new("/media/S01.E02.episode.mkv")),
|
||||
Path::new("/media/S01.E02.episode.mkv.part"),
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_exponential_backoff() {
|
||||
assert_eq!(
|
||||
@@ -231,5 +386,9 @@ mod tests {
|
||||
assert!(DownloadError::Http(503).is_retryable());
|
||||
assert!(!DownloadError::Http(404).is_retryable());
|
||||
assert!(!DownloadError::FileSystem("disk full".to_string()).is_retryable());
|
||||
// Retrying a paused download would restart what the user just stopped.
|
||||
assert!(!DownloadError::Stopped.is_retryable());
|
||||
assert!(DownloadError::Stopped.is_stopped());
|
||||
assert!(!DownloadError::Network("timeout".to_string()).is_stopped());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -3269,7 +3269,13 @@ mod tests {
|
||||
fn get_subtitle_url(&self, _: &str, _: &str, _: i32, _: &str) -> String {
|
||||
unimplemented!()
|
||||
}
|
||||
fn get_video_download_url(&self, _: &str, _: &str, _: Option<&str>) -> String {
|
||||
fn get_video_download_url(
|
||||
&self,
|
||||
_: &str,
|
||||
_: &str,
|
||||
_: Option<&str>,
|
||||
_: Option<&str>,
|
||||
) -> String {
|
||||
unimplemented!()
|
||||
}
|
||||
async fn mark_favorite(&self, _: &str) -> Result<(), repo_types::RepoError> {
|
||||
|
||||
@@ -120,22 +120,34 @@ pub fn webview_can_decode_audio(codec: &str) -> bool {
|
||||
/// delegate this decision; it knows what its own renderer can decode and must
|
||||
/// apply that itself.
|
||||
///
|
||||
/// The track that matters is the one the server will actually serve: the
|
||||
/// default, or the first when none is marked. An unknown codec is left alone —
|
||||
/// forcing a transcode on a guess would burn server CPU for files that play.
|
||||
/// The track that matters is the one the server will actually serve (see
|
||||
/// [`served_audio_codec`]). An unknown codec is left alone — forcing a transcode
|
||||
/// on a guess would burn server CPU for files that play.
|
||||
///
|
||||
/// TRACES: UR-004 | DR-149 | UT-148
|
||||
pub fn audio_forces_transcode(streams: &[(Option<&str>, bool)]) -> bool {
|
||||
let served = streams
|
||||
match served_audio_codec(streams) {
|
||||
Some(codec) => !webview_can_decode_audio(codec),
|
||||
// No audio at all, or a codec the server did not name: leave it alone.
|
||||
None => false,
|
||||
}
|
||||
}
|
||||
|
||||
/// The codec of the audio track the server will actually serve, given the
|
||||
/// source's audio streams as `(codec, is_default)` in source order: the default,
|
||||
/// or the first when none is marked.
|
||||
///
|
||||
/// `None` means "nothing to judge" — no audio streams, or the server named no
|
||||
/// codec for the one it would serve. Both callers of this rule treat that as
|
||||
/// leave-well-alone, never as a licence to assume compatibility.
|
||||
///
|
||||
/// TRACES: UR-004, UR-071 | DR-149, DR-171 | UT-148, UT-166
|
||||
pub fn served_audio_codec<'a>(streams: &[(Option<&'a str>, bool)]) -> Option<&'a str> {
|
||||
streams
|
||||
.iter()
|
||||
.find(|(_, is_default)| *is_default)
|
||||
.or_else(|| streams.first());
|
||||
|
||||
match served {
|
||||
Some((Some(codec), _)) => !webview_can_decode_audio(codec),
|
||||
// No audio at all, or a codec the server did not name: leave it alone.
|
||||
Some((None, _)) | None => false,
|
||||
}
|
||||
.or_else(|| streams.first())
|
||||
.and_then(|(codec, _)| *codec)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
@@ -193,6 +205,25 @@ mod tests {
|
||||
assert!(!audio_forces_transcode(&[(None, true)]));
|
||||
}
|
||||
|
||||
/// The download path needs the codec itself, not just the verdict, so it can
|
||||
/// tell the server what to re-encode. It picks the same track the streaming
|
||||
/// verdict is formed from — one rule, one place.
|
||||
///
|
||||
/// TRACES: UR-071 | DR-171 | UT-166
|
||||
#[test]
|
||||
fn the_served_codec_is_the_one_the_verdict_is_formed_from() {
|
||||
assert_eq!(
|
||||
served_audio_codec(&[(Some("aac"), false), (Some("eac3"), true)]),
|
||||
Some("eac3")
|
||||
);
|
||||
assert_eq!(
|
||||
served_audio_codec(&[(Some("eac3"), false), (Some("aac"), false)]),
|
||||
Some("eac3")
|
||||
);
|
||||
assert_eq!(served_audio_codec(&[]), None);
|
||||
assert_eq!(served_audio_codec(&[(None, true)]), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_dolby_device_does_not_advertise_dolby_for_video() {
|
||||
// The bug: a Motorola reporting c2.dolby.eac3.decoder direct-played
|
||||
|
||||
@@ -872,10 +872,11 @@ impl MediaRepository for HybridRepository {
|
||||
item_id: &str,
|
||||
quality: &str,
|
||||
media_source_id: Option<&str>,
|
||||
source_audio_codec: Option<&str>,
|
||||
) -> String {
|
||||
// Always use online URL for downloads
|
||||
self.online
|
||||
.get_video_download_url(item_id, quality, media_source_id)
|
||||
.get_video_download_url(item_id, quality, media_source_id, source_audio_codec)
|
||||
}
|
||||
|
||||
async fn mark_favorite(&self, item_id: &str) -> Result<(), RepoError> {
|
||||
@@ -1299,6 +1300,7 @@ mod tests {
|
||||
_item_id: &str,
|
||||
_quality: &str,
|
||||
_media_source_id: Option<&str>,
|
||||
_source_audio_codec: Option<&str>,
|
||||
) -> String {
|
||||
unimplemented!()
|
||||
}
|
||||
@@ -1573,6 +1575,7 @@ mod tests {
|
||||
_item_id: &str,
|
||||
_quality: &str,
|
||||
_media_source_id: Option<&str>,
|
||||
_source_audio_codec: Option<&str>,
|
||||
) -> String {
|
||||
unimplemented!()
|
||||
}
|
||||
|
||||
@@ -197,14 +197,24 @@ pub trait MediaRepository: Send + Sync {
|
||||
format: &str,
|
||||
) -> String;
|
||||
|
||||
/// Get video download URL (synchronous - just constructs URL)
|
||||
/// Called by frontend via Tauri invoke (getVideoDownloadUrl in VideoDownloadButton.svelte)
|
||||
/// Build the URL a video download is fetched from. Synchronous — it only
|
||||
/// constructs a URL, so it stays testable without a server. Reach it through
|
||||
/// [`resolve_video_download_url`] rather than calling it directly.
|
||||
///
|
||||
/// `source_audio_codec` is the codec of the audio track the server would
|
||||
/// serve (see [`served_audio_codec`]); `None` when it is not known. At
|
||||
/// `original` quality it decides whether the file can be copied byte-for-byte
|
||||
/// or has to have its audio re-encoded on the way down — a downloaded file is
|
||||
/// played back with no server in reach, so it has to be decodable *here*.
|
||||
///
|
||||
/// TRACES: UR-071 | DR-171
|
||||
#[allow(dead_code)]
|
||||
fn get_video_download_url(
|
||||
&self,
|
||||
item_id: &str,
|
||||
quality: &str,
|
||||
media_source_id: Option<&str>,
|
||||
source_audio_codec: Option<&str>,
|
||||
) -> String;
|
||||
|
||||
/// Mark item as favorite
|
||||
@@ -323,3 +333,44 @@ pub trait MediaRepository: Send + Sync {
|
||||
new_index: u32,
|
||||
) -> Result<(), RepoError>;
|
||||
}
|
||||
|
||||
/// The audio codec the server would serve for `item_id` — the default track, or
|
||||
/// the first when none is marked, matching the track Jellyfin picks.
|
||||
///
|
||||
/// `None` when the item has no audio, names no codec, or cannot be fetched. A
|
||||
/// caller must read that as "unknown", never as "fine": it is the input to a
|
||||
/// policy that only *adds* a transcode, so an unknown codec leaves behaviour
|
||||
/// exactly as it was.
|
||||
///
|
||||
/// TRACES: UR-071 | DR-171 | UT-166
|
||||
pub async fn served_audio_codec(repo: &dyn MediaRepository, item_id: &str) -> Option<String> {
|
||||
let item = repo.get_item(item_id).await.ok()?;
|
||||
let audio: Vec<(Option<&str>, bool)> = item
|
||||
.media_streams
|
||||
.as_deref()
|
||||
.unwrap_or_default()
|
||||
.iter()
|
||||
.filter(|s| s.stream_type == "Audio")
|
||||
.map(|s| (s.codec.as_deref(), s.is_default))
|
||||
.collect();
|
||||
|
||||
device_profile::served_audio_codec(&audio).map(str::to_string)
|
||||
}
|
||||
|
||||
/// Resolve the download URL for a video, applying the audio-codec policy that
|
||||
/// keeps the saved file playable offline (DR-171).
|
||||
///
|
||||
/// Every video download goes through here rather than calling the builder
|
||||
/// directly: the builder is pure and cannot look the codec up, and a caller that
|
||||
/// forgets to is exactly how the silent downloads shipped.
|
||||
///
|
||||
/// TRACES: UR-071 | DR-171
|
||||
pub async fn resolve_video_download_url(
|
||||
repo: &dyn MediaRepository,
|
||||
item_id: &str,
|
||||
quality: &str,
|
||||
media_source_id: Option<&str>,
|
||||
) -> String {
|
||||
let codec = served_audio_codec(repo, item_id).await;
|
||||
repo.get_video_download_url(item_id, quality, media_source_id, codec.as_deref())
|
||||
}
|
||||
|
||||
@@ -828,6 +828,32 @@ impl OfflineRepository {
|
||||
/// the synced-but-not-downloaded catalog branch deliberately excluded, so it
|
||||
/// is authoritative regardless of the process-wide catalog-browse flag.
|
||||
///
|
||||
/// Whether cached item `i` belongs to library `l`, decided by media kind.
|
||||
///
|
||||
/// The cache leaves `library_id`/`parent_id` NULL on every item
|
||||
/// ([[offline-libraries-never-cached]]), so there is no link to follow: a
|
||||
/// library's `collection_type` and an item's `item_type` are the only things
|
||||
/// that can associate them. This is Jellyfin taxonomy and therefore lives in
|
||||
/// Rust, never in the frontend.
|
||||
///
|
||||
/// It is a named constant because it is needed in two places that must agree
|
||||
/// — which library *appears* in the Downloaded list, and which items appear
|
||||
/// *inside* it. They disagreed: the listing query used this mapping while the
|
||||
/// browse query only checked that the requested library existed, so opening
|
||||
/// any library showed every downloaded top-level item on the server.
|
||||
///
|
||||
/// A library of some other (or unknown) type keeps everything, since there is
|
||||
/// no mapping to narrow it by and hiding its contents would be worse.
|
||||
///
|
||||
/// TRACES: UR-055 | DR-082, DR-167
|
||||
const LIBRARY_HOLDS_ITEM: &'static str = "(
|
||||
(l.collection_type = 'music' AND i.item_type IN ('MusicAlbum', 'MusicArtist', 'Audio'))
|
||||
OR (l.collection_type = 'movies' AND i.item_type = 'Movie')
|
||||
OR (l.collection_type = 'tvshows' AND i.item_type IN ('Series', 'Season', 'Episode'))
|
||||
OR l.collection_type IS NULL
|
||||
OR l.collection_type NOT IN ('music', 'movies', 'tvshows')
|
||||
)";
|
||||
|
||||
/// TRACES: UR-055 | DR-082, DR-083
|
||||
const DOWNLOADED_ITEMS_CTE: &'static str = "
|
||||
WITH downloaded_items AS (
|
||||
@@ -908,6 +934,7 @@ impl OfflineRepository {
|
||||
EXISTS (
|
||||
SELECT 1 FROM libraries l
|
||||
WHERE l.id = ? AND l.server_id = i.server_id
|
||||
AND {membership}
|
||||
)
|
||||
-- Top-level only: hide leaves whose container is downloaded.
|
||||
AND NOT EXISTS (
|
||||
@@ -922,6 +949,7 @@ impl OfflineRepository {
|
||||
ORDER BY i.sort_name ASC, i.name ASC
|
||||
LIMIT {limit} OFFSET {start_index}",
|
||||
cte = Self::DOWNLOADED_ITEMS_CTE,
|
||||
membership = Self::LIBRARY_HOLDS_ITEM,
|
||||
);
|
||||
|
||||
let query = Query::with_params(
|
||||
@@ -966,7 +994,7 @@ impl OfflineRepository {
|
||||
// We match a library by collection_type ↔ item_type instead: any
|
||||
// completed download of a given media kind qualifies that library.
|
||||
let query = Query::with_params(
|
||||
&format!(
|
||||
format!(
|
||||
"{cte}
|
||||
SELECT l.id, l.name, l.collection_type, l.image_tag
|
||||
FROM libraries l
|
||||
@@ -975,15 +1003,11 @@ impl OfflineRepository {
|
||||
SELECT 1 FROM items i
|
||||
INNER JOIN downloaded_items di ON i.id = di.id
|
||||
WHERE i.server_id = l.server_id
|
||||
AND (
|
||||
(l.collection_type = 'music' AND i.item_type IN ('MusicAlbum', 'MusicArtist', 'Audio'))
|
||||
OR (l.collection_type = 'movies' AND i.item_type = 'Movie')
|
||||
OR (l.collection_type = 'tvshows' AND i.item_type IN ('Series', 'Season', 'Episode'))
|
||||
OR (l.collection_type NOT IN ('music', 'movies', 'tvshows'))
|
||||
)
|
||||
AND {membership}
|
||||
)
|
||||
ORDER BY l.sort_order ASC, l.name ASC",
|
||||
cte = Self::DOWNLOADED_ITEMS_CTE,
|
||||
membership = Self::LIBRARY_HOLDS_ITEM,
|
||||
),
|
||||
vec![QueryParam::String(self.server_id.clone())],
|
||||
);
|
||||
@@ -1957,6 +1981,7 @@ impl MediaRepository for OfflineRepository {
|
||||
_item_id: &str,
|
||||
_quality: &str,
|
||||
_media_source_id: Option<&str>,
|
||||
_source_audio_codec: Option<&str>,
|
||||
) -> String {
|
||||
// Cannot download while offline
|
||||
String::new()
|
||||
@@ -3713,6 +3738,82 @@ mod tests {
|
||||
assert_eq!(track_ids, vec!["track-1", "track-2"]);
|
||||
}
|
||||
|
||||
/// Regression: each downloaded library shows **only its own media**.
|
||||
///
|
||||
/// Cached items carry no link back to their library (`library_id`/`parent_id`
|
||||
/// are NULL — [[offline-libraries-never-cached]]), and the library branch of
|
||||
/// the query only asserted that the requested library *exists*, never that
|
||||
/// the item belongs to it. So opening any downloaded library listed every
|
||||
/// downloaded top-level item on the server: films in the music library,
|
||||
/// albums under TV. The library's `collection_type` decides which item types
|
||||
/// belong to it, the same mapping `get_downloaded_libraries` already uses.
|
||||
///
|
||||
/// TRACES: UR-055 | DR-167 | UT-162
|
||||
#[tokio::test]
|
||||
async fn test_get_downloaded_items_library_does_not_mix_media_types() {
|
||||
let db = create_test_db();
|
||||
seed_library(&db, "music-lib", "music").await;
|
||||
seed_library(&db, "movie-lib", "movies").await;
|
||||
seed_library(&db, "tv-lib", "tvshows").await;
|
||||
|
||||
insert_item(&db, "album-1", "MusicAlbum", None, None, None).await;
|
||||
insert_item(&db, "track-1", "Audio", Some("album-1"), None, None).await;
|
||||
insert_item(&db, "movie-1", "Movie", None, None, None).await;
|
||||
insert_item(&db, "series-1", "Series", None, None, None).await;
|
||||
insert_item(&db, "episode-1", "Episode", None, Some("series-1"), None).await;
|
||||
|
||||
seed_completed_download(&db, "track-1", 1000).await;
|
||||
seed_completed_download(&db, "movie-1", 2000).await;
|
||||
seed_completed_download(&db, "episode-1", 3000).await;
|
||||
|
||||
let repo = make_repo(&db);
|
||||
|
||||
let music: Vec<String> = repo
|
||||
.get_downloaded_items("music-lib", None)
|
||||
.await
|
||||
.unwrap()
|
||||
.items
|
||||
.iter()
|
||||
.map(|i| i.id.clone())
|
||||
.collect();
|
||||
assert_eq!(
|
||||
music,
|
||||
vec!["album-1"],
|
||||
"the music library must not list films or series; got {:?}",
|
||||
music
|
||||
);
|
||||
|
||||
let movies: Vec<String> = repo
|
||||
.get_downloaded_items("movie-lib", None)
|
||||
.await
|
||||
.unwrap()
|
||||
.items
|
||||
.iter()
|
||||
.map(|i| i.id.clone())
|
||||
.collect();
|
||||
assert_eq!(
|
||||
movies,
|
||||
vec!["movie-1"],
|
||||
"the movie library must not list albums or series; got {:?}",
|
||||
movies
|
||||
);
|
||||
|
||||
let tv: Vec<String> = repo
|
||||
.get_downloaded_items("tv-lib", None)
|
||||
.await
|
||||
.unwrap()
|
||||
.items
|
||||
.iter()
|
||||
.map(|i| i.id.clone())
|
||||
.collect();
|
||||
assert_eq!(
|
||||
tv,
|
||||
vec!["series-1"],
|
||||
"the TV library must not list albums or films; got {:?}",
|
||||
tv
|
||||
);
|
||||
}
|
||||
|
||||
/// Regression: a downloaded TV library lists the Series, not its Seasons or
|
||||
/// Episodes — the same "individual songs" bug seen for music, for TV. The
|
||||
/// season and episode are still reachable by drilling into the series.
|
||||
|
||||
@@ -1878,6 +1878,7 @@ impl MediaRepository for OnlineRepository {
|
||||
item_id: &str,
|
||||
quality: &str,
|
||||
media_source_id: Option<&str>,
|
||||
source_audio_codec: Option<&str>,
|
||||
) -> String {
|
||||
// NOTE: Jellyfin's `/Videos/{id}/download` endpoint is not universally
|
||||
// available (returns 404 on many server configs), which silently broke
|
||||
@@ -1928,10 +1929,39 @@ impl MediaRepository for OnlineRepository {
|
||||
params.push("audioCodec=aac".to_string());
|
||||
params.push("allowVideoStreamCopy=false".to_string());
|
||||
}
|
||||
// "original" (and any unknown value) → direct, resumable copy.
|
||||
_ => {
|
||||
params.push("Static=true".to_string());
|
||||
}
|
||||
// "original" (and any unknown value) → direct, resumable copy —
|
||||
// unless the audio in that copy is undecodable where the file will
|
||||
// be played back. A download is watched with no server in reach, so
|
||||
// it has to satisfy the same constraint DR-149 applies to streams:
|
||||
// the webview `<video>` element renders video on both platforms and
|
||||
// decodes none of AC-3/E-AC-3/DTS/TrueHD. Copying those bytes to
|
||||
// disk is what made a downloaded film play offline as picture with
|
||||
// no sound while the same film had sound when streamed.
|
||||
//
|
||||
// Only the *audio* is re-encoded. `allowVideoStreamCopy` keeps an
|
||||
// h264 source's picture byte-for-byte, so "original" still means
|
||||
// original quality, and no bitrate or resolution cap is added. A
|
||||
// source the webview could not have rendered anyway (HEVC) is
|
||||
// re-encoded to h264 as a side effect, which is the only form of it
|
||||
// that would have played.
|
||||
//
|
||||
// The cost of the transcode is that the response is no longer
|
||||
// range-resumable, which is exactly why this is decided per item
|
||||
// rather than applied to every `original` download.
|
||||
//
|
||||
// TRACES: UR-071, UR-004 | DR-171 | UT-166
|
||||
_ => match source_audio_codec {
|
||||
Some(codec) if !super::device_profile::webview_can_decode_audio(codec) => {
|
||||
params.push("videoCodec=h264".to_string());
|
||||
params.push("allowVideoStreamCopy=true".to_string());
|
||||
params.push("audioCodec=aac".to_string());
|
||||
params.push("audioBitRate=384000".to_string());
|
||||
}
|
||||
// Decodable, or unknown: an unknown codec must not provoke a
|
||||
// transcode — that would burn server CPU on a guess for files
|
||||
// that play perfectly well.
|
||||
_ => params.push("Static=true".to_string()),
|
||||
},
|
||||
}
|
||||
|
||||
// Add media source ID if provided
|
||||
@@ -2739,7 +2769,7 @@ mod tests {
|
||||
#[test]
|
||||
fn test_video_download_url_uses_stream_not_download_endpoint() {
|
||||
let repo = create_test_repository();
|
||||
let url = repo.get_video_download_url("item123", "original", None);
|
||||
let url = repo.get_video_download_url("item123", "original", None, None);
|
||||
|
||||
// Must NOT use the /download endpoint (404 on real servers).
|
||||
assert!(
|
||||
@@ -2757,7 +2787,7 @@ mod tests {
|
||||
#[test]
|
||||
fn test_video_download_url_original_is_static_direct_copy() {
|
||||
let repo = create_test_repository();
|
||||
let url = repo.get_video_download_url("item123", "original", None);
|
||||
let url = repo.get_video_download_url("item123", "original", None, None);
|
||||
|
||||
// "original" must request a direct static copy (byte-range resumable),
|
||||
// with no transcode params.
|
||||
@@ -2777,7 +2807,7 @@ mod tests {
|
||||
let repo = create_test_repository();
|
||||
|
||||
for (quality, height) in [("high", "1080"), ("medium", "720"), ("low", "480")] {
|
||||
let url = repo.get_video_download_url("item123", quality, None);
|
||||
let url = repo.get_video_download_url("item123", quality, None, None);
|
||||
assert!(
|
||||
url.contains("/Videos/item123/stream.mp4"),
|
||||
"{quality} must use stream.mp4: {url}"
|
||||
@@ -2810,7 +2840,7 @@ mod tests {
|
||||
let repo = create_test_repository();
|
||||
|
||||
for quality in ["high", "medium", "low"] {
|
||||
let url = repo.get_video_download_url("item123", quality, None);
|
||||
let url = repo.get_video_download_url("item123", quality, None, None);
|
||||
|
||||
assert!(
|
||||
url.contains("videoBitRate="),
|
||||
@@ -2844,7 +2874,7 @@ mod tests {
|
||||
let repo = create_test_repository();
|
||||
|
||||
for quality in ["high", "medium", "low"] {
|
||||
let url = repo.get_video_download_url("item123", quality, None);
|
||||
let url = repo.get_video_download_url("item123", quality, None, None);
|
||||
assert!(
|
||||
url.contains("allowVideoStreamCopy=false"),
|
||||
"{quality} must forbid video stream copy: {url}"
|
||||
@@ -2852,17 +2882,99 @@ mod tests {
|
||||
}
|
||||
|
||||
// "original" is a deliberate direct copy — it must NOT disable copying.
|
||||
let original = repo.get_video_download_url("item123", "original", None);
|
||||
let original = repo.get_video_download_url("item123", "original", None, None);
|
||||
assert!(
|
||||
!original.contains("allowVideoStreamCopy=false"),
|
||||
"original must remain a direct copy: {original}"
|
||||
);
|
||||
}
|
||||
|
||||
/// A downloaded file is played with no server in reach, so `original`
|
||||
/// quality cannot mean "copy whatever the source holds" when the source
|
||||
/// holds audio this device cannot decode.
|
||||
///
|
||||
/// `Static=true` hands back the source bytes untouched, E-AC-3/AC-3/DTS
|
||||
/// track included, and video plays through the webview `<video>` element on
|
||||
/// both platforms — which decodes none of them. Streaming already knows this
|
||||
/// (DR-149 forces a transcode over the server's own direct-play offer); the
|
||||
/// download path did not, so a downloaded film played offline as picture with
|
||||
/// no sound while the very same film had sound when streamed.
|
||||
///
|
||||
/// TRACES: UR-071, UR-004 | DR-171 | UT-166
|
||||
#[test]
|
||||
fn test_video_download_url_original_transcodes_undecodable_audio() {
|
||||
let repo = create_test_repository();
|
||||
|
||||
for codec in ["eac3", "ac3", "dts", "truehd", "EAC3"] {
|
||||
let url = repo.get_video_download_url("item123", "original", None, Some(codec));
|
||||
assert!(
|
||||
!url.contains("Static=true"),
|
||||
"{codec} cannot be decoded here, so the source must not be copied verbatim: {url}"
|
||||
);
|
||||
assert!(
|
||||
url.contains("audioCodec=aac"),
|
||||
"{codec} must be re-encoded to aac on the way down: {url}"
|
||||
);
|
||||
// "Original" still has to mean original picture: the video stream is
|
||||
// copied when it can be, so no bitrate or resolution cap appears.
|
||||
assert!(
|
||||
url.contains("allowVideoStreamCopy=true"),
|
||||
"the video stream must still be copied where possible: {url}"
|
||||
);
|
||||
assert!(
|
||||
!url.contains("videoBitRate") && !url.contains("maxHeight"),
|
||||
"original must not degrade the picture to fix the audio: {url}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// The converse, and the reason the policy is per-item rather than blanket:
|
||||
/// audio that plays here keeps the byte-exact, range-resumable copy that the
|
||||
/// download worker's resume depends on.
|
||||
///
|
||||
/// TRACES: UR-071 | DR-171 | UT-166
|
||||
#[test]
|
||||
fn test_video_download_url_original_keeps_static_copy_for_playable_audio() {
|
||||
let repo = create_test_repository();
|
||||
|
||||
for codec in ["aac", "mp3", "opus", "vorbis", "flac", "AAC"] {
|
||||
let url = repo.get_video_download_url("item123", "original", None, Some(codec));
|
||||
assert!(
|
||||
url.contains("Static=true"),
|
||||
"{codec} plays here — the download must stay a direct copy: {url}"
|
||||
);
|
||||
assert!(
|
||||
!url.contains("audioCodec="),
|
||||
"{codec} needs no transcode: {url}"
|
||||
);
|
||||
}
|
||||
|
||||
// Unknown codec: the policy only ever *adds* a transcode, so an item we
|
||||
// could not look up behaves exactly as it did before.
|
||||
let unknown = repo.get_video_download_url("item123", "original", None, None);
|
||||
assert!(unknown.contains("Static=true"), "url: {unknown}");
|
||||
}
|
||||
|
||||
/// The explicit quality presets already transcode audio to AAC, so the
|
||||
/// policy has nothing to add — and must not start overriding a chosen cap.
|
||||
///
|
||||
/// TRACES: UR-071 | DR-171 | UT-166
|
||||
#[test]
|
||||
fn test_video_download_url_presets_ignore_the_audio_policy() {
|
||||
let repo = create_test_repository();
|
||||
|
||||
for quality in ["high", "medium", "low"] {
|
||||
let with = repo.get_video_download_url("item123", quality, None, Some("eac3"));
|
||||
let without = repo.get_video_download_url("item123", quality, None, None);
|
||||
assert_eq!(with, without, "{quality} must not vary with source audio");
|
||||
assert!(with.contains("audioCodec=aac"), "url: {with}");
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_video_download_url_passes_media_source_id() {
|
||||
let repo = create_test_repository();
|
||||
let url = repo.get_video_download_url("item123", "original", Some("src-42"));
|
||||
let url = repo.get_video_download_url("item123", "original", Some("src-42"), None);
|
||||
assert!(url.contains("mediaSourceId=src-42"), "url: {url}");
|
||||
}
|
||||
|
||||
|
||||
@@ -44,7 +44,7 @@ pub struct Library {
|
||||
/// collection-type → category table any more than an item-type one. See
|
||||
/// `SearchScope::for_collection_type`.
|
||||
///
|
||||
/// TRACES: UR-075 | DR-164
|
||||
/// TRACES: UR-075 | DR-173
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub favorites_scope: Option<SearchScope>,
|
||||
}
|
||||
@@ -389,7 +389,7 @@ impl SearchScope {
|
||||
/// `All` is never returned: it is the *absence* of a category, offered
|
||||
/// alongside the libraries rather than derived from one.
|
||||
///
|
||||
/// TRACES: UR-075 | DR-164 | UT-161
|
||||
/// TRACES: UR-075 | DR-173 | UT-161
|
||||
pub fn for_collection_type(collection_type: &str) -> Option<SearchScope> {
|
||||
match collection_type {
|
||||
"movies" => Some(SearchScope::Movies),
|
||||
@@ -707,7 +707,7 @@ mod search_scope_tests {
|
||||
assert!(matches!(all.scope, Some(SearchScope::All)));
|
||||
}
|
||||
|
||||
/// TRACES: DR-164 | UT-161
|
||||
/// TRACES: DR-173 | UT-161
|
||||
#[test]
|
||||
fn test_collection_type_maps_to_its_favorites_scope() {
|
||||
assert_eq!(
|
||||
@@ -728,7 +728,7 @@ mod search_scope_tests {
|
||||
/// than one that opens an unfiltered list. `All` is never derived from a
|
||||
/// library — it is the cross-library entry offered beside them.
|
||||
///
|
||||
/// TRACES: DR-164 | UT-161
|
||||
/// TRACES: DR-173 | UT-161
|
||||
#[test]
|
||||
fn test_uncategorised_collection_types_have_no_favorites_scope() {
|
||||
for collection_type in ["livetv", "channels", "boxsets", "books", "unknown", ""] {
|
||||
@@ -740,7 +740,7 @@ mod search_scope_tests {
|
||||
}
|
||||
}
|
||||
|
||||
/// TRACES: DR-164 | UT-161
|
||||
/// TRACES: DR-173 | UT-161
|
||||
#[test]
|
||||
fn test_library_carries_its_favorites_scope_to_the_frontend() {
|
||||
let music = Library::new("1".into(), "Music".into(), "music".into(), None);
|
||||
|
||||
Reference in New Issue
Block a user