From 5476ed8ac4c61c4393d179dc9d91da18583244d9 Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Sat, 9 May 2026 23:08:16 -0400 Subject: [PATCH] video: handle unknown/short durations in thumb + preview gen `get_duration_seconds` now returns `Option` and falls back from `format=duration` to `stream=duration`. Empty stdout no longer parse-panics with "cannot parse float from empty string", which was poisoning the preview-clip row with status=failed and re-queueing every full scan (notably for GoPro LRV files). `generate_preview_clip` handles the unknown-duration case by transcoding the whole file (capped at 10s). `generate_video_thumbnail` seeks to ~50% of the probed duration instead of a hardcoded `-ss 3`, with a first-frame fallback when the probe returns nothing. Fixes the loop where short Snapchat clips (<3s) got "missing thumbnail" logged on every scan because ffmpeg exited 0 without writing a frame, and never wrote the .unsupported sentinel either. Adds unit tests for `parse_ffprobe_duration` covering the empty-output, N/A, multi-line, non-positive, and non-finite cases. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/video/actors.rs | 33 +++++-- src/video/ffmpeg.rs | 214 +++++++++++++++++++++++++++++++++++--------- 2 files changed, 201 insertions(+), 46 deletions(-) diff --git a/src/video/actors.rs b/src/video/actors.rs index cfe2aa7..808824b 100644 --- a/src/video/actors.rs +++ b/src/video/actors.rs @@ -2,7 +2,7 @@ use crate::database::PreviewDao; use crate::is_video; use crate::libraries::Library; use crate::otel::global_tracer; -use crate::video::ffmpeg::generate_preview_clip; +use crate::video::ffmpeg::{generate_preview_clip, get_duration_seconds_blocking}; use actix::prelude::*; use log::{debug, error, info, trace, warn}; use opentelemetry::KeyValue; @@ -108,6 +108,13 @@ pub async fn create_playlist(video_path: &str, playlist_file: &str) -> Result std::io::Result<()> { + // Probe duration up front and seek to ~50% — gives a more + // representative frame than a fixed offset (skipping title cards on + // long videos, landing inside the clip on 1–2s Snapchat MP4s) and + // sidesteps the seek-past-EOF class of bug entirely. When duration + // probing fails (LRV files, fragmented MP4s, ffprobe missing) fall + // back to the first frame: ugly but reliable. + // // -vf scale + -c:v mjpeg mirrors `generate_image_thumbnail_ffmpeg`. The // filter chain matters as much as the scale does: without it, ffmpeg // hands the decoded frame straight to the mjpeg encoder, which rejects @@ -115,10 +122,14 @@ pub fn generate_video_thumbnail(path: &Path, destination: &Path) -> std::io::Res // filter chain lets ffmpeg auto-insert the pix_fmt converter the // encoder needs, which is how the image-thumbnail path already handles // the same class of source. - let output = Command::new("ffmpeg") - .arg("-y") - .arg("-ss") - .arg("3") + let seek = get_duration_seconds_blocking(path).map(|d| format!("{:.3}", d / 2.0)); + + let mut cmd = Command::new("ffmpeg"); + cmd.arg("-y"); + if let Some(s) = &seek { + cmd.arg("-ss").arg(s); + } + let output = cmd .arg("-i") .arg(path) .arg("-vframes") @@ -139,6 +150,18 @@ pub fn generate_video_thumbnail(path: &Path, destination: &Path) -> std::io::Res String::from_utf8_lossy(&output.stderr).trim() ))); } + // ffmpeg can exit 0 without writing a frame for malformed files where + // the probe duration lies. Confirm a non-empty file actually landed — + // returning Err makes the caller write the `.unsupported` sentinel so + // we stop re-detecting on every scan. + let wrote = std::fs::metadata(destination) + .map(|m| m.len() > 0) + .unwrap_or(false); + if !wrote { + return Err(std::io::Error::other( + "ffmpeg exited successfully but produced no thumbnail output", + )); + } Ok(()) } diff --git a/src/video/ffmpeg.rs b/src/video/ffmpeg.rs index a31fd0c..d385cac 100644 --- a/src/video/ffmpeg.rs +++ b/src/video/ffmpeg.rs @@ -223,20 +223,83 @@ impl Ffmpeg { } /// Get video duration in seconds as f64 for precise interval calculation. -async fn get_duration_seconds(input_file: &str) -> Result { - Command::new("ffprobe") - .args(["-i", input_file]) - .args(["-show_entries", "format=duration"]) +/// +/// Returns `Ok(None)` when ffprobe runs successfully but the container has no +/// readable duration (notably GoPro `LRV` low-res preview files, some +/// fragmented MP4s, and short Snapchat clips with stripped headers). Callers +/// can fall back to a duration-agnostic encode rather than treating this as +/// a hard failure — previously the `parse::` on empty stdout produced +/// "cannot parse float from empty string" and poisoned the preview-clip row +/// with status=failed, which the watcher would re-queue every full scan. +async fn get_duration_seconds(input_file: &str) -> Result> { + if let Some(d) = probe_duration(input_file, "format=duration").await? { + return Ok(Some(d)); + } + // Fall back to the per-stream duration — populated for some MP4s where + // the format-level duration tag is missing. + probe_duration(input_file, "stream=duration").await +} + +/// Synchronous cousin of `get_duration_seconds`, for callers running on +/// blocking thread pools (Rayon). Same fallback strategy: tries +/// `format=duration`, then `stream=duration`. Returns `None` for any +/// failure — ffprobe missing, container without a duration tag, parse +/// error — so callers can pick a duration-agnostic default. +pub fn get_duration_seconds_blocking(input_file: &std::path::Path) -> Option { + if let Some(d) = probe_duration_blocking(input_file, "format=duration") { + return Some(d); + } + probe_duration_blocking(input_file, "stream=duration") +} + +fn probe_duration_blocking(input_file: &std::path::Path, show_entries: &str) -> Option { + let out = std::process::Command::new("ffprobe") .args(["-v", "quiet"]) + .args(["-show_entries", show_entries]) .args(["-of", "csv=p=0"]) + .arg("-i") + .arg(input_file) .output() - .await - .map(|out| String::from_utf8_lossy(&out.stdout).trim().to_string()) - .and_then(|duration_str| { - duration_str - .parse::() - .map_err(|e| std::io::Error::other(e.to_string())) - }) + .ok()?; + let raw = String::from_utf8_lossy(&out.stdout); + parse_ffprobe_duration(&raw) +} + +async fn probe_duration(input_file: &str, show_entries: &str) -> Result> { + let out = Command::new("ffprobe") + .args(["-v", "quiet"]) + .args(["-show_entries", show_entries]) + .args(["-of", "csv=p=0"]) + .args(["-i", input_file]) + .output() + .await?; + let raw = String::from_utf8_lossy(&out.stdout); + Ok(parse_ffprobe_duration(&raw)) +} + +/// Parse ffprobe's `csv=p=0` duration output. Returns the first valid +/// positive finite duration, or `None` when there isn't one. +/// +/// Stream-level queries (`-show_entries stream=duration`) emit one value per +/// stream, one per line; format-level queries emit a single line. The shape +/// also varies — `N/A` for streams without a known duration, empty string +/// for containers without the tag at all, and (rarely) `0`/`-1` for +/// fragmented MP4s. All of those have to map to `None` so the caller can +/// fall back to a duration-agnostic encode. +fn parse_ffprobe_duration(stdout: &str) -> Option { + for line in stdout.lines() { + let trimmed = line.trim(); + if trimmed.is_empty() || trimmed == "N/A" { + continue; + } + if let Ok(d) = trimmed.parse::() + && d.is_finite() + && d > 0.0 + { + return Some(d); + } + } + None } /// Generate a preview clip from a video file. @@ -268,28 +331,39 @@ pub async fn generate_preview_clip(input_file: &str, output_file: &str) -> Resul cmd.arg("-i").arg(input_file); - if duration < 1.0 { - // Very short video (<1s): transcode the whole thing to 480p MP4 - // format=yuv420p ensures 10-bit sources are converted to 8-bit for h264_nvenc - cmd.args(["-vf", "scale=-2:480,format=yuv420p"]); - } else { - let segment_count = if duration < 10.0 { - duration.floor() as u32 - } else { - 10 - }; - let interval = duration / segment_count as f64; - - // format=yuv420p ensures 10-bit sources are converted to 8-bit for h264_nvenc - let vf = format!( - "select='lt(mod(t,{:.4}),1)',setpts=N/FRAME_RATE/TB,fps=30,scale=-2:480,format=yuv420p", - interval - ); - let af = format!("aselect='lt(mod(t,{:.4}),1)',asetpts=N/SR/TB", interval); - - cmd.args(["-vf", &vf]); - cmd.args(["-af", &af]); - } + // Branch on duration. `None` means ffprobe couldn't tell us — we treat + // it like the <1s case and just transcode the whole file. The selected + // clip-duration we report back is computed alongside, so callers don't + // need to re-probe. + let clip_duration = match duration { + None => { + warn!( + "Unknown duration for '{}', transcoding whole file as preview", + input_file + ); + cmd.args(["-vf", "scale=-2:480,format=yuv420p"]); + // Cap the encode at 10s so a long video with stripped duration + // metadata doesn't spend forever generating a "preview". + cmd.args(["-t", "10"]); + 10.0 + } + Some(d) if d < 1.0 => { + cmd.args(["-vf", "scale=-2:480,format=yuv420p"]); + d + } + Some(d) => { + let segment_count = if d < 10.0 { d.floor() as u32 } else { 10 }; + let interval = d / segment_count as f64; + let vf = format!( + "select='lt(mod(t,{:.4}),1)',setpts=N/FRAME_RATE/TB,fps=30,scale=-2:480,format=yuv420p", + interval + ); + let af = format!("aselect='lt(mod(t,{:.4}),1)',asetpts=N/SR/TB", interval); + cmd.args(["-vf", &vf]); + cmd.args(["-af", &af]); + if d < 10.0 { d.floor() } else { 10.0 } + } + }; // Force 30fps output so high-framerate sources (60fps) don't play back // at double speed due to select/setpts timestamp mismatches. @@ -320,14 +394,6 @@ pub async fn generate_preview_clip(input_file: &str, output_file: &str) -> Resul let metadata = std::fs::metadata(output_file)?; let file_size = metadata.len(); - let clip_duration = if duration < 1.0 { - duration - } else if duration < 10.0 { - duration.floor() - } else { - 10.0 - }; - info!( "Generated preview clip '{}' ({:.1}s, {} bytes) in {:?}", output_file, @@ -338,3 +404,69 @@ pub async fn generate_preview_clip(input_file: &str, output_file: &str) -> Resul Ok((clip_duration, file_size)) } + +#[cfg(test)] +mod tests { + use super::parse_ffprobe_duration; + + #[test] + fn empty_output_returns_none() { + // The original bug: ffprobe -show_entries format=duration returned + // "" for some GoPro LRV files, and `parse::` panicked with + // "cannot parse float from empty string". + assert_eq!(parse_ffprobe_duration(""), None); + assert_eq!(parse_ffprobe_duration("\n"), None); + assert_eq!(parse_ffprobe_duration(" \n \n"), None); + } + + #[test] + fn na_returns_none() { + // ffprobe emits "N/A" for streams without a known duration. + assert_eq!(parse_ffprobe_duration("N/A"), None); + assert_eq!(parse_ffprobe_duration("N/A\nN/A\n"), None); + } + + #[test] + fn parses_simple_duration() { + assert_eq!(parse_ffprobe_duration("12.345"), Some(12.345)); + assert_eq!(parse_ffprobe_duration("12.345\n"), Some(12.345)); + assert_eq!(parse_ffprobe_duration("0.5"), Some(0.5)); + } + + #[test] + fn rejects_non_positive_durations() { + // Fragmented MP4s and broken containers occasionally report 0 or a + // negative duration. Treat as "unknown" so the caller falls back to + // whole-file transcoding rather than dividing by zero downstream. + assert_eq!(parse_ffprobe_duration("0"), None); + assert_eq!(parse_ffprobe_duration("0.0"), None); + assert_eq!(parse_ffprobe_duration("-1.5"), None); + } + + #[test] + fn rejects_non_finite_durations() { + assert_eq!(parse_ffprobe_duration("inf"), None); + assert_eq!(parse_ffprobe_duration("nan"), None); + } + + #[test] + fn first_valid_line_wins_for_stream_query() { + // `-show_entries stream=duration` emits one value per stream. For a + // video file the video stream is first; we accept it and ignore + // any audio-stream values that follow. + assert_eq!(parse_ffprobe_duration("12.5\n8.3\n"), Some(12.5)); + } + + #[test] + fn skips_leading_na_and_blank_lines() { + // Stream queries can put N/A first (e.g. data stream before the + // video stream); the parser should keep scanning. + assert_eq!(parse_ffprobe_duration("N/A\n\n7.25\n"), Some(7.25)); + } + + #[test] + fn rejects_garbage() { + assert_eq!(parse_ffprobe_duration("not a number"), None); + assert_eq!(parse_ffprobe_duration("12.5abc"), None); + } +}