//! Poster-frame extraction, shared by the compression worker and the HTML export. //! //! Both used to spawn `ffmpeg` themselves with the same broken invocation: //! //! ```text //! ffmpeg -i -vframes 1 -ss 00:00:01 -vf scale=… -y //! ``` //! //! `-ss` AFTER `-i` is an output-side seek. Against a clip of a second or less ffmpeg exits **0 and //! writes nothing** — and both call sites gated on the exit status, so neither noticed. The worker //! then wrote `thumbnail_path` for a file that was never created (404 in the live feed) and the //! export listed the entry in `data.json` while the ZIP writer skipped it (a broken image tile in //! the keepsake). Every server-side signal stayed green. Phones produce such clips constantly: //! mis-taps, Live Photos, boomerangs. //! //! This module exists for the same reason `imaging.rs` does — that one was created when compression //! and export duplicated decode logic, and it paid off immediately when the `max_alloc` fix landed //! in both workers at once. Same duplication, same fix. use std::path::Path; use std::time::Duration; use anyhow::{Context, Result}; /// A malformed video can hang `ffmpeg` indefinitely. In the compression worker that never releases /// the semaphore permit and the pool eventually deadlocks; in the export worker it strands the job /// at `running` so the keepsake never completes. `export.rs` had NO timeout at all before this /// module — sharing the spawn fixes that too. const FFMPEG_TIMEOUT: Duration = Duration::from_secs(120); /// Seek positions to try, in order. /// /// One second first: the opening frame of a real video is often black, a fade-in, or motion-blurred /// as the camera settles, so it makes a poor poster. Zero second as the fallback, which is what /// makes short clips work — and it is genuinely required, not defensive. Moving `-ss` before `-i` /// (an input-side seek) is necessary but NOT sufficient: seeking to 1 s in a 1.000 s clip is still /// past the last frame, and ffmpeg still exits 0 having written nothing. Verified against the real /// production image. const SEEK_POSITIONS: &[&str] = &["00:00:01", "0"]; /// Extract one poster frame from `src` into `dest`, scaled to `width` px wide. /// /// `Ok(false)` means the video yielded no frame — a normal outcome for a very short or unusual /// clip, NOT an error. Callers must degrade (no poster) rather than fail the upload: treating this /// as an error would soft-delete every sub-second video, turning a cosmetic defect into data loss. /// /// `Err` is reserved for something genuinely wrong — a hang we had to kill, or a failure to spawn. pub async fn extract_poster_frame(src: &Path, dest: &Path, width: u32) -> Result { for seek in SEEK_POSITIONS { // A stale file from a previous attempt would be indistinguishable from a fresh success. let _ = tokio::fs::remove_file(dest).await; run_ffmpeg(src, dest, width, seek).await?; // THE CHECK BOTH CALL SITES WERE MISSING: ask the filesystem, not the exit status. // Non-empty, because a zero-byte file is not a poster either. if tokio::fs::metadata(dest) .await .map(|m| m.is_file() && m.len() > 0) .unwrap_or(false) { return Ok(true); } } // Leave nothing behind for a caller to mistake for a result. let _ = tokio::fs::remove_file(dest).await; Ok(false) } /// Run one ffmpeg attempt. A non-zero exit is NOT an error here — the artifact check above is the /// authority, and a corrupt input that fails at 1 s may still yield a frame at 0. async fn run_ffmpeg(src: &Path, dest: &Path, width: u32, seek: &str) -> Result<()> { let child = tokio::process::Command::new("ffmpeg") .args([ // BEFORE -i: an input-side seek. See SEEK_POSITIONS. "-ss", seek, "-i", src.to_str().unwrap_or_default(), "-vframes", "1", "-vf", &format!("scale={width}:-1"), "-y", dest.to_str().unwrap_or_default(), ]) // ffmpeg writes the poster to `dest` itself; nothing here ever reads stdout, so // giving it a pipe only created something that could fill. .stdout(std::process::Stdio::null()) // stderr IS piped — it is the only diagnostic when a clip yields no frame — but it // must be DRAINED, which is the whole point of `wait_with_output` below. .stderr(std::process::Stdio::piped()) .kill_on_drop(true) .spawn() .context("failed to spawn ffmpeg")?; // `wait_with_output`, NOT `wait`. ffmpeg is verbose on stderr (banner, stream info, // per-frame progress) and `wait()` reads neither pipe — so once the ~64 KiB pipe buffer // filled, ffmpeg blocked writing, `wait()` never returned, and the call burned the full // timeout. That is not merely slow: the timeout is an `Err`, so after 2 seek positions x // 3 compression attempts the caller soft-deletes a perfectly playable video for a // poster-frame failure. `wait_with_output` polls the pipe and the exit status together. // // It also CONSUMES the child, so the explicit `child.kill()` that used to sit on the // timeout arm cannot exist here — and is not needed: `kill_on_drop(true)` is set above, // and dropping the future on timeout drops the child with it. let out = match tokio::time::timeout(FFMPEG_TIMEOUT, child.wait_with_output()).await { Ok(res) => res.context("ffmpeg wait failed")?, Err(_) => anyhow::bail!("ffmpeg timed out after {}s", FFMPEG_TIMEOUT.as_secs()), }; // A non-zero exit is not an error (see the doc comment) — the artifact check in // `extract_poster_frame` is the authority. Log the tail so a systematically failing // format is diagnosable without turning it into data loss. if !out.status.success() { tracing::debug!( seek, status = ?out.status, stderr = %tail_lines(&out.stderr, 10), "ffmpeg exited non-zero; the artifact check decides" ); } Ok(()) } /// Last `n` lines of a child's stderr, lossily decoded. /// /// Bounded on purpose: ffmpeg's stderr is unbounded, and the reason we now drain it is that /// unbounded output used to be a hazard. Emitting all of it into a log line — into container /// logs that are themselves size-capped — would just move the problem. fn tail_lines(bytes: &[u8], n: usize) -> String { let text = String::from_utf8_lossy(bytes); let lines: Vec<&str> = text.lines().filter(|l| !l.trim().is_empty()).collect(); lines[lines.len().saturating_sub(n)..].join(" | ") } #[cfg(test)] mod tests { use super::*; /// The order is the whole fix. `-ss` must precede `-i`, and 0 must be tried after 1 s. #[test] fn the_fallback_seek_exists_and_comes_last() { assert_eq!( SEEK_POSITIONS, &["00:00:01", "0"], "1s first for a better poster, 0 as the fallback that makes short clips work" ); } /// A missing input yields no frame rather than an error: the caller must degrade to "no /// poster", never fail the upload. `Err` is reserved for a hang or a spawn failure. #[tokio::test] async fn a_missing_source_yields_no_frame_rather_than_an_error() { let dir = std::env::temp_dir().join(format!("es-video-{}", std::process::id())); std::fs::create_dir_all(&dir).unwrap(); let dest = dir.join("out.jpg"); let got = extract_poster_frame(Path::new("/nonexistent/clip.mp4"), &dest, 400).await; match got { Ok(false) => {} other => panic!("expected Ok(false) for a missing input, got {other:?}"), } assert!( !dest.exists(), "a failed extraction must leave nothing a caller could mistake for a poster" ); let _ = std::fs::remove_dir_all(&dir); } #[test] fn the_stderr_tail_is_bounded_and_survives_invalid_utf8() { let noisy: Vec = (0..500) .map(|i| format!("line {i}\n")) .collect::() .into_bytes(); let got = tail_lines(&noisy, 3); assert_eq!(got, "line 497 | line 498 | line 499"); // ffmpeg emits filenames verbatim, so its stderr is not guaranteed to be UTF-8. assert_eq!(tail_lines(&[b'o', b'k', 0xff], 5), "ok\u{fffd}"); assert_eq!(tail_lines(b"", 5), ""); } /// A real extraction must finish in a small fraction of `FFMPEG_TIMEOUT`. /// /// Wall-clock is the ONLY observable of the bug this guards: piping stderr and then /// calling `wait()` (which drains nothing) blocks ffmpeg on a full pipe buffer until the /// timeout fires, and the timeout is an `Err`, so the upload is soft-deleted. The /// assertion is deliberately on elapsed time, not on the exit status. /// /// Honest limitation: our fixture is quiet enough not to fill a 64 KiB pipe on its own, /// so this catches a regression to `wait()` only in combination with a verbose input. It /// is still worth pinning — a reverted drain plus any chatty clip is data loss. #[tokio::test] async fn a_real_clip_yields_a_poster_well_inside_the_timeout() { if tokio::process::Command::new("ffmpeg") .arg("-version") .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::null()) .status() .await .is_err() { eprintln!("skipping: ffmpeg not on PATH"); return; } let src = Path::new("../e2e/fixtures/media/sample.mp4"); if !src.exists() { eprintln!("skipping: {} missing", src.display()); return; } let dir = std::env::temp_dir().join(format!("es-video-ok-{}", std::process::id())); std::fs::create_dir_all(&dir).unwrap(); let dest = dir.join("poster.jpg"); let started = std::time::Instant::now(); let got = extract_poster_frame(src, &dest, 400).await; let elapsed = started.elapsed(); assert!(matches!(got, Ok(true)), "expected a poster, got {got:?}"); assert!(dest.metadata().unwrap().len() > 0); assert!( elapsed < FFMPEG_TIMEOUT / 4, "extraction took {elapsed:?}; a drained stderr finishes in well under \ {FFMPEG_TIMEOUT:?} — this is the pipe-deadlock regression guard" ); let _ = std::fs::remove_dir_all(&dir); } }