collective
Browse / github.com/storytold/effectcraft / patch
export: tag BT.709 limited-range nclx; the ffmpeg oracles pick decoders by name and skip unbuildable fixtures
export: tag BT.709 limited-range nclx; the ffmpeg oracles pick decoders by name and skip unbuildable fixtures One of the standalone parts the v1 review asked #1508 to be split into (test runs and format metadata; builds straight on `main`, file-disjoint from the other Android parts). These are the changes of #1508's `ce32d715` and `c8c394b4`, plus the parts of its review commit `df1ceff7` that live in the same files, verbatim. **What it adds.** - `crates/export/src/{encode.rs,hevc_av1.rs}` — the exported HEVC/AV1 `nclx` says BT.709 (colour primaries 1, transfer 1, matrix 1) and limited range where the pipeline means it, matching what the players we round-trip against assume; the round-trip tests pin
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- Kaedo17 (imported from pull request #2362, open)
- Base
991904888a20b7da72b2a4880b21c7d912154736- Size
- +166 −17 in 7 files
- Review
- pending
- Shared
- 2026-10-28
- Id
7e2a1f673d7d4f881ef1a556
How it went
No other operator has reported using it yet.
Diff
raw · crates/export/src/encode.rs crates/export/src/hevc_av1.rs crates/export/tests/it/hevc_av1_roundtrip.rs crates/export/tests/it/roundtrip.rs crates/hevcenc/tests/roundtrip.rs crates/media/tests/it/footage.rs crates/media/tests/it/open_in_place.rs
@@ -7,7 +7,7 @@ use std::io::Write; use effectcraft_project::Comp; use effectcraft_project::render_queue::{AudioFormat, Channels, OutputFormat, ProResProfile}; use effectcraft_time::{FrameRate, Tick};-use filmcraft_isobmff::{Brand, FourCc, Mp4Writer, PcmConfig, SampleEntry, TrackConfig, WriteSample, WriterOptions};+use filmcraft_isobmff::{Brand, ColorInfo, FourCc, Mp4Writer, PcmConfig, SampleEntry, TrackConfig, WriteSample, WriterOptions}; use rayon::prelude::*; use crate::{Cx, ExportError, Report, Result, State, batch_size, wants_audio};@@ -19,6 +19,17 @@ fn mux_err(e: impl std::fmt::Display) -> ExportError { ExportError::Io(e.to_string()) } +/// Tag a video [`SampleEntry`] with the BT.709 limited-range `colr` (nclx) box every video+/// encoder here outputs (`rgba_to_yuv420`, `rgba_to_yuv_10` and `rgba_to_yuv420_bits` all use+/// the 0.2126/0.7152/0.0722 luma and a 16–235 luma range). Without it a re-import falls back to+/// the container's resolution-based `default_matrix` — BT.601 below 1024×577 — and shifts the+/// colours (pure red came back as [232,0,1]).+pub(crate) fn tag_bt709_limited(entry: &mut SampleEntry) {+ if let Some(v) = entry.video.as_mut() {+ v.color = Some(ColorInfo::Nclx { primaries: 1, transfer: 1, matrix: 1, full_range: false });+ }+}+ pub(crate) struct Packet { pub(crate) data: Vec<u8>, pub(crate) key: bool,@@ -70,7 +81,9 @@ impl VideoEncoder for H264 { let (sps, pps) = self.enc.sps_pps(); filmcraft_isobmff::AvcConfig::new(vec![sps], vec![pps], 4) });- SampleEntry::avc(cfg, self.w as u16, self.h as u16)+ let mut entry = SampleEntry::avc(cfg, self.w as u16, self.h as u16);+ tag_bt709_limited(&mut entry);+ entry } fn encode(&mut self, rgba: &[u8], index: u64) -> Result<Vec<Packet>> { rgba_to_yuv420(rgba, self.w as usize, self.h as usize, &mut self.y, &mut self.u, &mut self.v);@@ -92,7 +92,9 @@ impl Hevc { impl VideoEncoder for Hevc { fn sample_entry(&self) -> SampleEntry { let cfg = HevcConfig::parse(&self.enc.hvcc()).unwrap_or_default();- SampleEntry::hevc(cfg, self.w as u16, self.h as u16)+ let mut entry = SampleEntry::hevc(cfg, self.w as u16, self.h as u16);+ crate::encode::tag_bt709_limited(&mut entry);+ entry } fn encode(&mut self, rgba: &[u8], _index: u64) -> Result<Vec<Packet>> { let (w, h) = (self.w as usize, self.h as usize);@@ -148,7 +150,9 @@ impl Av1 { impl VideoEncoder for Av1 { fn sample_entry(&self) -> SampleEntry { let cfg = Av1Config::parse(&self.enc.av1c()).unwrap_or_default();- SampleEntry::video(FourCc(*b"av01"), CodecConfig::Av1(cfg), self.w as u16, self.h as u16)+ let mut entry = SampleEntry::video(FourCc(*b"av01"), CodecConfig::Av1(cfg), self.w as u16, self.h as u16);+ crate::encode::tag_bt709_limited(&mut entry);+ entry } fn encode(&mut self, rgba: &[u8], _index: u64) -> Result<Vec<Packet>> { let (w, h) = (self.w as usize, self.h as usize);@@ -123,7 +123,15 @@ fn ffprobe_video(path: &Path) -> Option<String> { .output() .expect("ffprobe"); assert!(out.status.success() && out.stderr.is_empty(), "ffprobe rejected {}: {}", path.display(), String::from_utf8_lossy(&out.stderr));- Some(String::from_utf8_lossy(&out.stdout).trim().to_string())+ let s = String::from_utf8_lossy(&out.stdout).trim().to_string();+ // An ffprobe whose build lacks this codec's decoder (distro "free" builds) only parses the+ // container: pix_fmt and the counted frames come back unknown/N/A, describing the build+ // rather than our output. Skip the oracle, as when ffprobe is absent.+ if s.contains("|unknown|") || s.ends_with("|N/A") {+ eprintln!("ffprobe cannot decode this codec here: skipping the oracle ({s})");+ return None;+ }+ Some(s) } /// Decode every frame with ffmpeg (no errors allowed) and check frames 0, 4, 5 and the last.@@ -140,7 +148,14 @@ fn ffmpeg_check(path: &Path, decoder: Option<&str>, tol: f32) { cmd.args(["-c:v", d]); } let out = cmd.arg("-i").arg(path).args(["-pix_fmt", "rgba"]).arg(dir.join("f%03d.png")).output().expect("ffmpeg");- assert!(out.status.success() && out.stderr.is_empty(), "ffmpeg ({decoder:?}) failed on {}: {}", path.display(), String::from_utf8_lossy(&out.stderr));+ let err = String::from_utf8_lossy(&out.stderr).to_string();+ // Same as ffprobe: an ffmpeg without this codec's decoder cannot judge our output, so the+ // oracle skips instead of failing (the in-repo decode above already proved the frames).+ if !out.status.success() && err.contains("no decoder found") {+ eprintln!("ffmpeg cannot decode this codec here: skipping the oracle decode ({err})");+ return;+ }+ assert!(out.status.success() && err.is_empty(), "ffmpeg ({decoder:?}) failed on {}: {}", path.display(), err); for k in [0, 4, 5, FRAMES - 1] { let img = image::open(dir.join(format!("f{:03}.png", k + 1))).expect("ffmpeg frame").to_rgba8(); check_frame(&Image::from_rgba8(img.width(), img.height(), img.as_raw()), k, tol);@@ -23,6 +23,24 @@ const H: u32 = 48; const BG: [f32; 3] = [0.1, 0.1, 0.4]; const FRAMES: u64 = 12; +/// An H.264 encoder this ffmpeg build actually ships, or `None` when it has none. Distro "free"+/// builds often omit `libx264` but ship an alternative (e.g. libopenh264), so pick the first+/// present by name rather than hard-coding one that may be absent. The oracle only needs *an*+/// H.264 encoder; the encoder identity is irrelevant to the assertion.+fn h264_encoder() -> Option<&'static str> {+ static ENC: std::sync::OnceLock<Option<&'static str>> = std::sync::OnceLock::new();+ *ENC.get_or_init(|| {+ let out = Command::new("ffmpeg").args(["-hide_banner", "-encoders"]).output().ok()?;+ if !out.status.success() {+ return None;+ }+ let text = String::from_utf8_lossy(&out.stdout);+ ["libx264", "libopenh264", "h264_nvenc", "h264_vaapi", "h264_qsv", "h264_v4l2m2m", "h264_amf"]+ .into_iter()+ .find(|name| text.lines().any(|l| l.split_whitespace().any(|tok| tok == *name)))+ })+}+ fn out_dir(name: &str) -> PathBuf { let d = Path::new(env!("CARGO_MANIFEST_DIR")).join("../../target/test-out/export").join(name); let _ = std::fs::remove_dir_all(&d);@@ -489,14 +507,18 @@ fn ffmpeg_audio(src: &Path) -> Vec<f32> { fn movies_carry_resampled_footage_audio() { let d = out_dir("audio-44k"); let clip = d.join("clip.mp4");+ let Some(enc) = h264_encoder() else {+ eprintln!("ffmpeg has no H.264 encoder: skipping");+ return;+ }; let made = Command::new("ffmpeg") .args(["-hide_banner", "-loglevel", "error", "-y", "-f", "lavfi", "-i", "testsrc2=size=64x48:rate=30000/1001:duration=3", "-f", "lavfi", "-i"])- .args(["aevalsrc=0.5*sin(2*PI*440*t)|0.5*sin(2*PI*(200+400*t/3)*t):s=44100:d=3", "-c:v", "libx264", "-pix_fmt", "yuv420p", "-c:a", "aac", "-shortest"])+ .args(["aevalsrc=0.5*sin(2*PI*440*t)|0.5*sin(2*PI*(200+400*t/3)*t):s=44100:d=3", "-c:v", enc, "-pix_fmt", "yuv420p", "-c:a", "aac", "-shortest"]) .arg(&clip) .status() .is_ok_and(|s| s.success()); if !made {- eprintln!("ffmpeg not found: skipping");+ eprintln!("ffmpeg could not make the H.264 fixture: skipping"); return; } let fps = FrameRate::new(30000, 1001);@@ -132,12 +132,36 @@ fn ffmpeg_available() -> bool { Command::new("ffmpeg").arg("-version").output().map(|o| o.status.success()).unwrap_or(false) } +/// `true` when this ffmpeg/ffprobe build actually ships an HEVC decoder. Distro "free" builds+/// (e.g. Fedora's ffmpeg-free) ship without one — the tools run but cannot decode our stream, so+/// the external oracle means nothing and the test skips rather than failing on a gap in the tool.+fn hevc_decoder_available() -> bool {+ Command::new("ffmpeg")+ .args(["-hide_banner", "-decoders"])+ .output()+ .map(|o| {+ String::from_utf8_lossy(&o.stdout).lines().any(|l| {+ // Decoder rows look like ` V....D hevc ...`; match the codec name, not the text.+ // Every row of `-decoders` is a decoder, so the flags column says nothing about+ // that — its trailing `D` is "supports direct rendering method 1", which a+ // build's hevc decoder may well lack (requiring it would skip a working oracle).+ let mut it = l.split_whitespace();+ it.next().is_some() && it.next() == Some("hevc")+ })+ })+ .unwrap_or(false)+}+ /// Decode the Annex-B stream with ffmpeg and require bit-exact equality and an empty stderr. fn check_ffmpeg(run: &Run, w: usize, h: usize, bd: u32, tag: &str) { if !ffmpeg_available() { eprintln!("ffmpeg not found; skipping the ffmpeg oracle"); return; }+ if !hevc_decoder_available() {+ eprintln!("ffmpeg has no HEVC decoder here; skipping the ffmpeg oracle");+ return;+ } let dir = std::env::temp_dir(); let uniq = format!("{}-{}-{}", tag, std::process::id(), std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_nanos()); let inp = dir.join(format!("hevcenc-{uniq}.hevc"));@@ -197,6 +221,10 @@ fn ffprobe_stream_info() { if !ffmpeg_available() { return; }+ if !hevc_decoder_available() {+ eprintln!("ffmpeg has no HEVC decoder here; ffprobe cannot fully describe our stream, skipping");+ return;+ } let (w, h) = (130, 72); let pics: Vec<Pic> = (0..2).map(|i| synth(w, h, 10, i, 0)).collect(); let mut c = cfg(w as u32, h as u32, 30, 30, Profile::Main10);@@ -24,9 +24,40 @@ fn have_ffmpeg() -> bool { *HAVE.get_or_init(|| Command::new("ffmpeg").arg("-version").output().is_ok_and(|o| o.status.success())) } +/// An H.264 encoder this ffmpeg build actually ships, or `None` when it has none. Distro "free"+/// builds often omit `libx264` (the one named historically here) but ship an alternative, so pick+/// the first present by name rather than hard-coding one that may be absent. These tests only+/// need *an* H.264 encoder to make an oracle fixture; the encoder identity is irrelevant.+fn h264_encoder() -> Option<&'static str> {+ static ENC: std::sync::OnceLock<Option<&'static str>> = std::sync::OnceLock::new();+ *ENC.get_or_init(|| {+ let out = Command::new("ffmpeg").args(["-hide_banner", "-encoders"]).output().ok()?;+ if !out.status.success() {+ return None;+ }+ let text = String::from_utf8_lossy(&out.stdout);+ ["libx264", "libopenh264", "h264_nvenc", "h264_vaapi", "h264_qsv", "h264_v4l2m2m", "h264_amf"]+ .into_iter()+ .find(|name| text.lines().any(|l| l.split_whitespace().any(|tok| tok == *name)))+ })+}+ /// Run ffmpeg to produce `name` (once; written to a temp name and renamed so concurrent test /// processes never see a partial file). `None` when ffmpeg is unavailable. fn ffmpeg_fixture(name: &str, args: &[&str]) -> Option<PathBuf> {+ ffmpeg_fixture_inner(name, args, true)+}++/// Like [`ffmpeg_fixture`] for a fixture this build of ffmpeg may fail to make: an encoder found+/// in the `-encoders` listing can still not run here (the hardware backends need their device,+/// Fedora's `libopenh264` is the `noopenh264` stub), and fixtures built from one, like+/// [`h264_mp4`], should skip with a note rather than fail on the packager's choice of ffmpeg —+/// the same way every test here skips when ffmpeg is missing altogether.+fn try_ffmpeg_fixture(name: &str, args: &[&str]) -> Option<PathBuf> {+ ffmpeg_fixture_inner(name, args, false)+}++fn ffmpeg_fixture_inner(name: &str, args: &[&str], must_succeed: bool) -> Option<PathBuf> { if !have_ffmpeg() { eprintln!("ffmpeg not found: skipping"); return None;@@ -39,7 +70,14 @@ fn ffmpeg_fixture(name: &str, args: &[&str]) -> Option<PathBuf> { let ext = out.extension().map(|e| e.to_string_lossy().to_string()).unwrap_or_default(); let tmp = out.with_file_name(format!("tmp-{}-{:?}.{ext}", std::process::id(), std::thread::current().id()).replace(['(', ')'], "")); let st = Command::new("ffmpeg").args(["-hide_banner", "-loglevel", "error", "-y"]).args(args).arg(&tmp).status().expect("run ffmpeg");- assert!(st.success(), "ffmpeg failed for {name}");+ if !st.success() {+ let _ = std::fs::remove_file(&tmp);+ if must_succeed {+ panic!("ffmpeg failed for {name}");+ }+ eprintln!("this ffmpeg build cannot make {name}: skipping");+ return None;+ } if std::fs::rename(&tmp, &out).is_err() { // Another test made it first; on Windows the rename fails while that test has it open. let _ = std::fs::remove_file(&tmp);@@ -49,7 +87,10 @@ fn ffmpeg_fixture(name: &str, args: &[&str]) -> Option<PathBuf> { } fn h264_mp4() -> Option<PathBuf> {- ffmpeg_fixture(+ // Made with whatever encoder this build has listed: a listed encoder can still fail at run+ // time (the hardware ones without their device), so this may skip rather than fail.+ let enc = h264_encoder()?;+ try_ffmpeg_fixture( "testsrc2_1080p.mp4", &[ "-f",@@ -61,7 +102,7 @@ fn h264_mp4() -> Option<PathBuf> { "-i", "sine=frequency=440:sample_rate=48000:duration=2", "-c:v",- "libx264",+ enc, "-pix_fmt", "yuv420p", "-preset",@@ -281,7 +322,10 @@ fn h264_audio_samples() { /// H.264 + AAC at 44.1 kHz in MP4 (ffmpeg writes an edit list that skips the AAC priming): /// 440 Hz left, a 200 → 1000 Hz sweep right, 29.97 fps. fn aac_44k_mp4() -> Option<PathBuf> {- ffmpeg_fixture(+ // Made with whatever encoder this build has listed: a listed encoder can still fail at run+ // time (the hardware ones without their device), so this may skip rather than fail.+ let enc = h264_encoder()?;+ try_ffmpeg_fixture( "aac_44k.mp4", &[ "-f",@@ -293,7 +337,7 @@ fn aac_44k_mp4() -> Option<PathBuf> { "-i", "aevalsrc=0.5*sin(2*PI*440*t)|0.5*sin(2*PI*(200+400*t/3)*t):s=44100:d=3", "-c:v",- "libx264",+ enc, "-pix_fmt", "yuv420p", "-c:a",@@ -1098,7 +1142,7 @@ fn a_called_off_read_does_not_decode() { /// footage reports it (not a black frame without a word). #[test] fn an_unsupported_stream_reports_why_it_shows_no_picture() {- let Some(p) = ffmpeg_fixture(+ let Some(p) = try_ffmpeg_fixture( "h264_high422_10bit.mp4", &["-f", "lavfi", "-i", "testsrc=size=320x180:rate=24:duration=1", "-c:v", "libx264", "-profile:v", "high422", "-pix_fmt", "yuv422p10le"], ) else {@@ -19,8 +19,26 @@ fn fixtures() -> PathBuf { Path::new(env!("CARGO_MANIFEST_DIR")).join("../../target/fixtures/media") } +/// An H.264 encoder this ffmpeg build actually ships, or `None` when it has none (see the same+/// helper in `footage.rs`: distro "free" builds often omit `libx264` and ship an alternative).+fn h264_encoder() -> Option<&'static str> {+ static ENC: std::sync::OnceLock<Option<&'static str>> = std::sync::OnceLock::new();+ *ENC.get_or_init(|| {+ let out = Command::new("ffmpeg").args(["-hide_banner", "-encoders"]).output().ok()?;+ if !out.status.success() {+ return None;+ }+ let text = String::from_utf8_lossy(&out.stdout);+ ["libx264", "libopenh264", "h264_nvenc", "h264_vaapi", "h264_qsv", "h264_v4l2m2m", "h264_amf"]+ .into_iter()+ .find(|name| text.lines().any(|l| l.split_whitespace().any(|tok| tok == *name)))+ })+}+ /// A 2 s 320×240 H.264 + AAC movie (written to a temp name and renamed, so concurrent test-/// processes never see a partial file). `None` without ffmpeg.+/// processes never see a partial file). `None` without ffmpeg, or when this build cannot encode+/// H.264 at all — the test only measures how a movie is read, so it skips rather than asserting+/// on the packager's choice of ffmpeg. fn small_mp4() -> Option<PathBuf> { let out = fixtures().join("in_place_small.mp4"); if out.exists() {@@ -29,16 +47,21 @@ fn small_mp4() -> Option<PathBuf> { if !Command::new("ffmpeg").arg("-version").output().is_ok_and(|o| o.status.success()) { return None; }+ let enc = h264_encoder()?; std::fs::create_dir_all(fixtures()).unwrap(); let tmp = fixtures().join(format!("in_place_tmp_{}.mp4", std::process::id())); let st = Command::new("ffmpeg") .args(["-hide_banner", "-loglevel", "error", "-y", "-f", "lavfi", "-i", "testsrc2=size=320x240:rate=30:duration=2"]) .args(["-f", "lavfi", "-i", "sine=frequency=440:sample_rate=48000:duration=2"])- .args(["-c:v", "libx264", "-pix_fmt", "yuv420p", "-c:a", "aac", "-shortest"])+ .args(["-c:v", enc, "-pix_fmt", "yuv420p", "-c:a", "aac", "-shortest"]) .arg(&tmp) .status() .unwrap();- assert!(st.success(), "ffmpeg failed");+ if !st.success() {+ let _ = std::fs::remove_file(&tmp);+ eprintln!("this ffmpeg build cannot make in_place_small.mp4: skipping");+ return None;+ } std::fs::rename(&tmp, &out).unwrap(); Some(out) }