Both the compression worker and the HTML export ran the same invocation:
ffmpeg -i <src> -vframes 1 -ss 00:00:01 -vf scale=… -y <out>
`-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:
- the worker wrote `thumbnail_path` and logged "thumbnail generated" for a file that
was never created, so GET /upload/{id}/thumbnail 404s in the live feed;
- the export listed media/<id>_thumb.jpg in data.json while the ZIP writer skipped the
unopenable file, so the keepsake drew a broken image tile.
Any clip at or under a second, which phones produce constantly — mis-taps, Live
Photos, boomerangs. Not data loss; the .mp4 is in both archives and plays. Every
server-side signal stayed green.
New `services/video.rs` owns the extraction for both callers, mirroring the imaging.rs
precedent (created for the same duplication, and it paid off when the max_alloc fix
landed in both workers at once). Three changes in it:
- `-ss` before `-i`, an input-side seek. NOT sufficient alone: verified against the
production image, seeking to 1s in a 1.000s clip is still past the last frame and
still exits 0 with no file. The 0s fallback is what actually fixes this, and 1s is
tried first only because an opening frame makes a poor poster.
- Verify the artifact, not the exit status. This is the check both sites were missing.
- Carry compression.rs's 120s timeout. export.rs had NONE — a hung ffmpeg there would
strand the job at `running` and the keepsake would never complete.
The worker's call used `?`. Tightening the check without also making a missing poster
non-fatal would have been far worse than the bug: every sub-second clip would fail
compression, exhaust its retries and be soft-deleted. It now logs a warning and leaves
`thumbnail_path` NULL, which FeedListCard, VirtualFeed and LightboxModal already
handle.
The export now sets `thumb: ""` and skips the manifest entry when there is no poster —
and does the same for the IMAGE branch, whose decode failure left the identical
dangling reference. No viewer change was needed: +page.svelte already guards
`{#if post.media.thumb}` and falls back to a video tile with a play glyph. The comment
claiming "viewer handles missing thumbs gracefully" was true about the viewer and false
about what the backend sent — the guard never fired because the string was never empty.
e2e/specs/06-export/export-video.spec.ts had DOCUMENTED this as intended behaviour
("the fixture clip is <1s, so ffmpeg extracts no thumbnail frame — but exits 0 …
that's the intended shape here"). sample.mp4 is exactly 1.000s, so every video test in
the suite ran at that boundary and none ever fetched the poster. That comment is now
corrected to say what it actually was.
Tests: 2 unit; a new sample-5s.mp4 fixture so the ordinary first-seek path is covered
at all; video-playback now FETCHES the poster rather than asserting the attribute (the
one extra request that nine rounds of green never made); a new spec covering both
fixtures plus the mirror that a posterless video still uploads and plays; and a
keepsake spec asserting every <img> in the opened viewer resolves — naturalWidth === 0
is exactly the broken-tile case, whatever produced it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
141 lines
5.8 KiB
Rust
141 lines
5.8 KiB
Rust
//! 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 <src> -vframes 1 -ss 00:00:01 -vf scale=… -y <out>
|
|
//! ```
|
|
//!
|
|
//! `-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<bool> {
|
|
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 mut 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(),
|
|
])
|
|
.stdout(std::process::Stdio::piped())
|
|
.stderr(std::process::Stdio::piped())
|
|
.kill_on_drop(true)
|
|
.spawn()
|
|
.context("failed to spawn ffmpeg")?;
|
|
|
|
match tokio::time::timeout(FFMPEG_TIMEOUT, child.wait()).await {
|
|
Ok(res) => {
|
|
res.context("ffmpeg wait failed")?;
|
|
}
|
|
Err(_) => {
|
|
let _ = child.kill().await;
|
|
anyhow::bail!("ffmpeg timed out after {}s", FFMPEG_TIMEOUT.as_secs());
|
|
}
|
|
}
|
|
Ok(())
|
|
}
|
|
|
|
#[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);
|
|
}
|
|
}
|