From 1279cb8430431132cb662cf12597125f173347ce Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Tue, 13 Oct 2026 10:46:46 -0400 Subject: [PATCH 1/6] fix(video): stop stream-copying oversized sources into HLS HLS generation stream-copied any h264 source with no rotation and a GOP within one segment, which keeps the source's resolution and bitrate. Mirrorless footage (4K at 100-160 Mbps) came through as-is, giving ~50 MB 3-second segments; playback start and seeks stalled on anything slower than wired gigabit. On prod, 16 playlists run above 50 Mbps, versus ~10 Mbps typical for phone clips. The stream probe now also reads width, height and bit_rate (stream level, falling back to format level; all header fields, so the probe stays header-only). Copy additionally requires the short side to be <= 1080 and the bitrate <= HLS_COPY_MAX_BITRATE (default 20 Mbps). Unknown size or bitrate transcodes, matching the GOP probe's fallback. The transcode scale filter capped width at 1080, so landscape sources came out at 1080x608. It now caps the short side instead: 3840x2160 -> 1920x1080, 2160x3840 -> 1080x1920, smaller sources unchanged (verified with ffmpeg). Existing oversized playlists need regenerating after deploy. Co-Authored-By: Claude Opus 5.5 --- src/video/actors.rs | 127 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 119 insertions(+), 8 deletions(-) diff --git a/src/video/actors.rs b/src/video/actors.rs index 6f1520a..009aa4a 100644 --- a/src/video/actors.rs +++ b/src/video/actors.rs @@ -161,6 +161,34 @@ pub struct VideoStreamMeta { /// `r_frame_rate`, which lies on variable-framerate sources). `None` /// when ffprobe couldn't parse either field — caller picks a fallback. pub frame_rate: Option, + /// Coded frame size. `None` when ffprobe didn't report it. + pub width: Option, + pub height: Option, + /// Bits per second — the video stream's own rate when the container + /// reports it, else the overall format rate. `None` when unknown. + pub bit_rate: Option, +} + +/// Longest the short side of an HLS rendition may be. Sources above this +/// are transcoded down; the transcode scale filter enforces the same cap. +const HLS_MAX_SHORT_SIDE: u32 = 1080; + +/// Default ceiling for stream-copying a source into HLS, in bits/s. +/// Overridable via `HLS_COPY_MAX_BITRATE`. +const HLS_COPY_MAX_BITRATE_DEFAULT: u64 = 20_000_000; + +/// Whether a source is small enough to stream-copy into HLS as-is. +/// +/// Copy keeps the source's resolution and bitrate, so a 4K/100+ Mbps +/// mirrorless clip would become ~50 MB 3-second segments that stall +/// playback start and seeks on anything slower than wired gigabit. +/// Unknown size or bitrate counts as too big — transcoding is the safe +/// fallback, same as the GOP probe. +fn fits_hls_copy_limits(meta: &VideoStreamMeta, max_bit_rate: u64) -> bool { + let (Some(w), Some(h), Some(br)) = (meta.width, meta.height, meta.bit_rate) else { + return false; + }; + w.min(h) <= HLS_MAX_SHORT_SIDE && br <= max_bit_rate } /// Parse ffprobe's rational frame-rate strings (`"30000/1001"`, @@ -196,9 +224,10 @@ pub async fn probe_video_stream_meta(video_path: &str) -> VideoStreamMeta { // network. For non-faststart phone clips on an SMB mount that turned a // metadata probe into a full-file read (tens of seconds per open). The // Display Matrix rotation we need is present at stream level, so this - // keeps codec/fps/rotation while reading only the header. + // keeps codec/fps/rotation while reading only the header. Size and + // bit_rate (stream and format level) are header fields too. .arg("-show_entries") - .arg("stream=codec_name,r_frame_rate,avg_frame_rate:stream_tags=rotate:stream_side_data_list") + .arg("stream=codec_name,r_frame_rate,avg_frame_rate,width,height,bit_rate:stream_tags=rotate:stream_side_data_list:format=bit_rate") .arg(video_path) .output() .await; @@ -262,15 +291,38 @@ pub async fn probe_video_stream_meta(video_path: &str) -> VideoStreamMeta { .and_then(parse_ffprobe_rational) }); + let dimension = |key: &str| { + stream + .get(key) + .and_then(|v| v.as_u64()) + .and_then(|v| u32::try_from(v).ok()) + .filter(|v| *v > 0) + }; + let width = dimension("width"); + let height = dimension("height"); + + // ffprobe reports bit_rate as a decimal string. Prefer the video + // stream's own rate; fall back to the container's overall rate. + let parse_bit_rate = |v: &serde_json::Value| { + v.get("bit_rate") + .and_then(|b| b.as_str()) + .and_then(|s| s.parse::().ok()) + .filter(|b| *b > 0) + }; + let bit_rate = parse_bit_rate(stream).or_else(|| parse_bit_rate(&json["format"])); + debug!( - "Probed {}: codec_h264={}, rotation={}°, fps={:?}", - video_path, is_h264, rotation, frame_rate + "Probed {}: codec_h264={}, rotation={}°, fps={:?}, size={:?}x{:?}, bit_rate={:?}", + video_path, is_h264, rotation, frame_rate, width, height, bit_rate ); VideoStreamMeta { is_h264, rotation, frame_rate, + width, + height, + bit_rate, } } @@ -507,12 +559,20 @@ impl Handler for PlaylistGenerator { let rotation = stream_meta.rotation; let has_rotation = rotation != 0; + // Stream-copy keeps the source's resolution and bitrate, so only + // copy sources that are already a sensible streaming size. + let max_copy_bit_rate = std::env::var("HLS_COPY_MAX_BITRATE") + .ok() + .and_then(|v| v.parse::().ok()) + .unwrap_or(HLS_COPY_MAX_BITRATE_DEFAULT); + let size_ok = fits_hls_copy_limits(&stream_meta, max_copy_bit_rate); + // Stream-copy is only safe when the source GOP fits inside a // single HLS segment. Otherwise ffmpeg has to extend segments // past hls_time to land on a keyframe, producing uneven // segments and seeking glitches. const HLS_SEGMENT_SECONDS: f64 = 3.0; - let gop_ok = if is_h264 && !has_rotation { + let gop_ok = if is_h264 && !has_rotation && size_ok { match get_max_gop_seconds(&video_file).await { Some(g) if g > HLS_SEGMENT_SECONDS => { info!( @@ -536,7 +596,7 @@ impl Handler for PlaylistGenerator { false }; - let use_copy = is_h264 && !has_rotation && gop_ok; + let use_copy = is_h264 && !has_rotation && size_ok && gop_ok; if has_rotation { info!( @@ -550,6 +610,12 @@ impl Handler for PlaylistGenerator { } else if use_copy { info!("Video {} is already h264, using stream copy", video_file); span.add_event("Using stream copy (h264 detected)", vec![]); + } else if is_h264 && !size_ok { + info!( + "Video {} is h264 but too large to copy ({:?}x{:?} @ {:?} bps), transcoding down", + video_file, stream_meta.width, stream_meta.height, stream_meta.bit_rate + ); + span.add_event("Transcoding to reduce size/bitrate", vec![]); } else if is_h264 { info!( "Video {} is h264 but needs transcoding for GOP alignment", @@ -591,7 +657,12 @@ impl Handler for PlaylistGenerator { cmd.arg("-crf").arg("21"); cmd.arg("-preset").arg("veryfast"); } - cmd.arg("-vf").arg("scale='min(1080,iw)':-2,setsar=1:1"); + // Cap the short side so landscape and portrait both land at + // real 1080p. iw/ih are post-autorotation display dims. + cmd.arg("-vf").arg(format!( + "scale='if(gt(iw,ih),-2,min({m},iw))':'if(gt(iw,ih),min({m},ih),-2)',setsar=1:1", + m = HLS_MAX_SHORT_SIDE + )); cmd.arg("-c:a").arg("aac"); // Force an IDR frame every hls_time seconds so each HLS // segment starts on a keyframe — accurate seeking without @@ -867,7 +938,47 @@ impl Handler for PreviewClipGenerator { #[cfg(test)] mod tests { - use super::parse_ffprobe_rational; + use super::{VideoStreamMeta, fits_hls_copy_limits, parse_ffprobe_rational}; + + const LIMIT: u64 = 20_000_000; + + fn meta(width: u32, height: u32, bit_rate: u64) -> VideoStreamMeta { + VideoStreamMeta { + width: Some(width), + height: Some(height), + bit_rate: Some(bit_rate), + ..Default::default() + } + } + + #[test] + fn copies_phone_sized_sources() { + assert!(fits_hls_copy_limits(&meta(1920, 1080, 16_000_000), LIMIT)); + // Portrait 1080p: short side is the width. + assert!(fits_hls_copy_limits(&meta(1080, 1920, 16_000_000), LIMIT)); + } + + #[test] + fn transcodes_4k_sources() { + assert!(!fits_hls_copy_limits(&meta(3840, 2160, 16_000_000), LIMIT)); + assert!(!fits_hls_copy_limits(&meta(2160, 3840, 16_000_000), LIMIT)); + } + + #[test] + fn transcodes_high_bitrate_1080p() { + // Mirrorless 1080p at 100 Mbps fits the size cap but not the bitrate. + assert!(!fits_hls_copy_limits(&meta(1920, 1080, 100_000_000), LIMIT)); + } + + #[test] + fn transcodes_when_size_or_bitrate_unknown() { + assert!(!fits_hls_copy_limits(&VideoStreamMeta::default(), LIMIT)); + let no_bit_rate = VideoStreamMeta { + bit_rate: None, + ..meta(1920, 1080, 0) + }; + assert!(!fits_hls_copy_limits(&no_bit_rate, LIMIT)); + } #[test] fn parses_common_rational_framerates() { -- 2.54.0 From 052609d60456d0e0bc6ecb8797e954d3dd6f420e Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Tue, 13 Oct 2026 10:58:12 -0400 Subject: [PATCH 2/6] docs: document HLS_COPY_MAX_BITRATE Co-Authored-By: Claude Opus 5.5 --- .env.example | 3 +++ CLAUDE.md | 1 + README.md | 1 + 3 files changed, 5 insertions(+) diff --git a/.env.example b/.env.example index 8e184f5..f730de5 100644 --- a/.env.example +++ b/.env.example @@ -29,6 +29,9 @@ EXCLUDED_DIRS= # ── Video / HLS ───────────────────────────────────────────────────────── HLS_CONCURRENCY=2 HLS_TIMEOUT_SECONDS=900 +# Sources above this bitrate (bits/s) or 1080p are transcoded instead of +# stream-copied, so HLS segments stay small enough to start and seek quickly. +HLS_COPY_MAX_BITRATE=20000000 PLAYLIST_CLEANUP_INTERVAL_SECONDS=86400 # ── Telemetry ──────────────────────────────────────────────────────────── diff --git a/CLAUDE.md b/CLAUDE.md index d77ea8f..7d9d3e0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -708,6 +708,7 @@ Optional: WATCH_QUICK_INTERVAL_SECONDS=60 # Quick scan interval WATCH_FULL_INTERVAL_SECONDS=3600 # Full scan interval DATE_BACKFILL_MAX_PER_TICK=500 # Cap on canonical-date drain per watcher tick +HLS_COPY_MAX_BITRATE=20000000 # Max source bits/s to stream-copy into HLS; above it (or >1080p) transcodes OTLP_OTLS_ENDPOINT=http://... # OTLP collector for span export (any build; unset = no export) # AI Insights Configuration diff --git a/README.md b/README.md index 47ad03a..62c3770 100644 --- a/README.md +++ b/README.md @@ -69,6 +69,7 @@ They should be defined where the binary is located or above it in an `.env` file skip them. Delete the `*.unsupported` files to force retries (for example after upgrading ffmpeg or adding libheif) - `VIDEO_PATH` is a path where HLS playlists and video parts should be stored +- `HLS_COPY_MAX_BITRATE` (optional) is the highest source bitrate, in bits/s, that is stream-copied into HLS as-is. Higher-bitrate sources, and any with a short side over 1080 px, are transcoded down to 1080p [default: `20000000`] - `GIFS_DIRECTORY` is a path where generated video GIF thumbnails should be stored - `BIND_URL` is the url and port to bind to (typically your own IP address) - `LOCAL_BIND_URL` (optional) is a second, loopback listener [default: `localhost:8088`]. Set it to an empty string to skip it, e.g. to run a second instance such as the `dev/e2e` harness alongside one that already owns 8088 -- 2.54.0 From 83b1923e257d30e69fcbe18bc51298741c6f8e9a Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Tue, 13 Oct 2026 10:58:13 -0400 Subject: [PATCH 3/6] fix(video): keep source frame timing when transcoding to HLS Variable-frame-rate sources (phone clips that change capture rate, screen recordings) report r_frame_rate as their 1/90000 timebase. The HLS transcode used the mpegts muxer's default CFR mode, so ffmpeg duplicated frames to fill 90000 fps: on prod, three 2.6-5 s phone clips became 3-second "segments" of up to 446 MB / 250k frames. Players download the whole thing before starting and can't seek within it. -fps_mode vfr passes source timestamps through. Verified on 20220208_164356_01.mp4: 233 frames over 2.578 s in, 233 frames over 2.578 s out, 5.9 MB vs 221 MB. The copy path already keeps timestamps and is unchanged. Requires ffmpeg >= 5.1 (prod runs 8.0.1). Co-Authored-By: Claude Opus 5.5 --- src/video/actors.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/video/actors.rs b/src/video/actors.rs index 009aa4a..7a22a5a 100644 --- a/src/video/actors.rs +++ b/src/video/actors.rs @@ -668,6 +668,12 @@ impl Handler for PlaylistGenerator { // segment starts on a keyframe — accurate seeking without // players having to decode from a prior segment. cmd.arg("-force_key_frames").arg("expr:gte(t,n_forced*3)"); + // Keep source frame timestamps. Variable-frame-rate phone + // clips and screen recordings report r_frame_rate as their + // timebase (90000/1); the mpegts muxer's default CFR mode + // then duplicates frames to fill it — a 2.6 s clip came out + // as 233k frames / 221 MB. vfr leaves timing untouched. + cmd.arg("-fps_mode").arg("vfr"); } // -f hls is required because the playlist is written to a .tmp -- 2.54.0 From 0b87320ef79a2e71e3daee9629d58f0305314054 Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Tue, 13 Oct 2026 11:03:18 -0400 Subject: [PATCH 4/6] fix(video): make NVENC honour forced HLS keyframes -force_key_frames asks for a keyframe every 3 s, but h264_nvenc inserts those as plain I-frames unless -forced-idr is set. The HLS muxer only splits on IDR frames, so NVENC transcodes fell back to the encoder's default 250-frame GOP: 8.34 s segments at 29.97 fps, ~10 s at 25 fps. On prod ~480 playlists have a target duration above 4 s. Long segments mean more data before playback starts and coarser seeks. Verified on a 10 s 29.97 fps clip: segments went from 8.34 + 1.67 s to 3.003 x 3 + 1.001 s, matching the libx264 path, which was already correct. Co-Authored-By: Claude Opus 5.5 --- src/video/actors.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/video/actors.rs b/src/video/actors.rs index 7a22a5a..e45c77e 100644 --- a/src/video/actors.rs +++ b/src/video/actors.rs @@ -652,6 +652,10 @@ impl Handler for PlaylistGenerator { cmd.arg("-rc").arg("vbr"); cmd.arg("-cq").arg("23"); cmd.arg("-pix_fmt").arg("yuv420p"); + // Without this NVENC turns -force_key_frames into plain + // I-frames, which the HLS muxer can't split on: segments + // fall back to NVENC's 250-frame GOP (8.34 s at 29.97). + cmd.arg("-forced-idr").arg("1"); } else { cmd.arg("-c:v").arg("h264"); cmd.arg("-crf").arg("21"); -- 2.54.0 From 985f366bc25f89c3ab675e0597a67bd7d4ea8349 Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Tue, 13 Oct 2026 11:07:05 -0400 Subject: [PATCH 5/6] chore(video): drop unused Ffmpeg::_generate_playlist, fix reels comment _generate_playlist was a private, never-called predecessor of the HLS path in video/actors.rs. Its leftover args (-hls_list_size 100, width 1080 scale, libx264 only) no longer described how playlists are built. Removing it also drops the now-unused std::process::Output import. The reels encoder comment said its NVENC settings matched the HLS transcode; reels use preset p4, HLS uses p1. Only the quality target (cq 23) is shared. Co-Authored-By: Claude Opus 5.5 --- src/reels/render.rs | 3 ++- src/video/ffmpeg.rs | 33 +-------------------------------- 2 files changed, 3 insertions(+), 33 deletions(-) diff --git a/src/reels/render.rs b/src/reels/render.rs index 221df5f..05e3915 100644 --- a/src/reels/render.rs +++ b/src/reels/render.rs @@ -155,7 +155,8 @@ pub fn beat_filtergraph(opts: &SegmentOpts, per_photo: &[f64]) -> String { fn video_encoder_args(nvenc: bool) -> Vec { if nvenc { - // p4 ≈ balanced; cq 23 ≈ libx264 crf 21. Matches the HLS transcode path. + // p4 ≈ balanced; cq 23 ≈ libx264 crf 21. Same quality target as the + // HLS transcode, which uses the faster p1 preset for throughput. [ "-c:v", "h264_nvenc", diff --git a/src/video/ffmpeg.rs b/src/video/ffmpeg.rs index 019bd86..5c88fcd 100644 --- a/src/video/ffmpeg.rs +++ b/src/video/ffmpeg.rs @@ -1,7 +1,7 @@ use futures::TryFutureExt; use log::{debug, error, info, warn}; use std::io::Result; -use std::process::{Output, Stdio}; +use std::process::Stdio; use std::sync::OnceLock; use std::time::Instant; use tokio::process::Command; @@ -47,37 +47,6 @@ pub enum GifType { } impl Ffmpeg { - async fn _generate_playlist(&self, input_file: &str, output_file: &str) -> Result { - let ffmpeg_result: Result = Command::new("ffmpeg") - .arg("-i") - .arg(input_file) - .arg("-c:v") - .arg("h264") - .arg("-crf") - .arg("21") - .arg("-preset") - .arg("veryfast") - .arg("-hls_time") - .arg("3") - .arg("-hls_list_size") - .arg("100") - .arg("-vf") - .arg("scale=1080:-2,setsar=1:1") - .arg(output_file) - .stdout(Stdio::null()) - .stderr(Stdio::piped()) - .output() - .inspect_err(|e| error!("Failed to run ffmpeg on child process: {}", e)) - .map_err(|e| std::io::Error::other(e.to_string())) - .await; - - if let Ok(ref res) = ffmpeg_result { - debug!("ffmpeg output: {:?}", res); - } - - ffmpeg_result.map(|_| output_file.to_string()) - } - async fn get_video_duration(&self, input_file: &str) -> Result { Command::new("ffprobe") .args(["-i", input_file]) -- 2.54.0 From 84843c1c254044fa5b2a764704e83ad067361917 Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Tue, 13 Oct 2026 11:09:14 -0400 Subject: [PATCH 6/6] docs: run tests with an in-memory DATABASE_URL The docs said tests need BASE_PATH in .env. The full suite (lib, bin and integration tests) passes with no .env and no BASE_PATH; the only thing it needs is DATABASE_URL, and ":memory:" works. Without it, DB-backed tests such as reels::tests::reel_capture_preserves_owner_defaults panic with "DATABASE_URL must be set". Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 10 +++++----- README.md | 7 +++++++ 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 7d9d3e0..72d25b6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -25,14 +25,14 @@ RUST_LOG=debug cargo run ### Testing ```bash -# Run all tests (requires BASE_PATH in .env) -cargo test +# Run all tests. DATABASE_URL is the only env var the suite needs; no .env required +DATABASE_URL=":memory:" cargo test # Run specific test -cargo test test_name +DATABASE_URL=":memory:" cargo test test_name # Run tests with output -cargo test -- --nocapture +DATABASE_URL=":memory:" cargo test -- --nocapture ``` ### Database Migrations @@ -664,7 +664,7 @@ Module map: ## Testing Patterns -Tests require `BASE_PATH` environment variable. Many integration tests create temporary directories and files. +Tests need `DATABASE_URL` set; `DATABASE_URL=":memory:"` is enough to run the whole suite without a `.env` (otherwise DB-backed tests such as `reels::tests::reel_capture_preserves_owner_defaults` panic with `DATABASE_URL must be set`). Many integration tests create temporary directories and files. When testing database code: - Use in-memory SQLite: `DATABASE_URL=":memory:"` diff --git a/README.md b/README.md index 62c3770..650d667 100644 --- a/README.md +++ b/README.md @@ -243,3 +243,10 @@ unset. populated retroactively) [default: `2000`]. Errors don't burn the cap; only successful hashes count. +## Testing +`DATABASE_URL` is the only environment variable the test suite needs, and an +in-memory SQLite database works, so no `.env` is required: + +```bash +DATABASE_URL=":memory:" cargo test +``` -- 2.54.0