diff --git a/crates/avplan/README.md b/crates/avplan/README.md index c0c3f8a..85306bf 100644 --- a/crates/avplan/README.md +++ b/crates/avplan/README.md @@ -68,11 +68,13 @@ $ avplan --json film.mkv | jq -c '{playable, reasons, encoder}' Converting everything in a directory that needs it: ```sh -for f in *.webm *.mkv; do avplan "$f" | xargs -d '\n' ffmpeg; done +for f in *.webm *.mkv; do + plan=$(avplan "$f") && eval "$plan" +done ``` -`avplan` writes nothing for a file that already plays, so `xargs` runs nothing -for it either. +The line is quoted for a shell, so `eval` runs it as written. A file that +already plays produces no line and a non-zero status, so `&&` skips it. ## Options @@ -157,7 +159,23 @@ assert!(!plan.playable); `video_plays`, `audio_plays` and `container_plays` answer the three questions on their own, for anything that needs one of them without the rest. +## The record + +`--json` writes one of these, whatever the verdict: + +| Field | | +|---|---| +| `path` | The file it is about | +| `playable` | Whether it plays as it stands | +| `streams` | `video`, `audio` and `container`, as they were found | +| `reasons` | Why it does not play, in the order the reasons were found. Empty when it does | +| `encoder` | Which encoder the picture would go through, or `copy`. Absent when nothing needs doing | +| `copies_audio` | Whether the sound is copied rather than re-encoded | +| `argv` | The call that would fix it, argument by argument. Empty when nothing needs doing | + +It is a shape of its own rather than one of [the records](../../docs/records.md) +the tools pass between them, because nothing downstream consumes it. + ## See also - [`cuenorm`](../cuenorm/README.md) — the subtitles that go with the recording. -- [The record contract](../../docs/records.md) — what `--json` emits. diff --git a/crates/avplan/src/encoder.rs b/crates/avplan/src/encoder.rs index 7babf18..1f684f2 100644 --- a/crates/avplan/src/encoder.rs +++ b/crates/avplan/src/encoder.rs @@ -55,6 +55,13 @@ impl Encoder { /// Only VAAPI needs any: the device has to be opened before the file is, /// so a filter can upload frames to it. pub fn before_input(self, vaapi_device: &str) -> Vec { + // No device named is the default device, not a device with no name: + // `vaapi=va:` is rejected, and an empty argument is how a shell + // passes on a variable nobody set. + let vaapi_device = match vaapi_device.is_empty() { + true => crate::DEFAULT_VAAPI_DEVICE, + false => vaapi_device, + }; match self { Encoder::H264Vaapi => vec![ "-init_hw_device".into(), @@ -216,6 +223,16 @@ Encoders: } } + #[test] + fn no_device_named_means_the_usual_one() { + // A shell passing on an unset variable gives an empty argument, and + // an encoder asked to open a device with no name refuses to start. + assert_eq!( + Encoder::H264Vaapi.before_input(""), + Encoder::H264Vaapi.before_input(crate::DEFAULT_VAAPI_DEVICE) + ); + } + #[test] fn copying_is_not_encoding() { assert!(!Encoder::Copy.is_hardware()); diff --git a/crates/avplan/src/lib.rs b/crates/avplan/src/lib.rs index 9681e84..aef71d4 100644 --- a/crates/avplan/src/lib.rs +++ b/crates/avplan/src/lib.rs @@ -110,10 +110,25 @@ pub fn container_plays(extension: Option<&str>) -> bool { } /// The extension of a path, lowercased, without the dot. +/// +/// Read off the last segment of the path as written, rather than by asking +/// the path library. The library folds `.` and `..` away and then reports the +/// extension of whatever they were attached to, which makes `film.mp4/.` — +/// a directory — a playable MP4. pub fn container_of(path: &str) -> Option { - std::path::Path::new(path) - .extension() - .map(|extension| extension.to_string_lossy().to_lowercase()) + let trimmed = path.trim_end_matches(std::path::is_separator); + let segment = trimmed.rsplit(std::path::is_separator).next().unwrap_or(""); + if matches!(segment, "" | "." | "..") { + return None; + } + // A leading dot is a hidden file, not an extension: `.mp4` is a name. + match segment.rfind('.') { + None | Some(0) => None, + Some(at) => match &segment[at + 1..] { + "" => None, + extension => Some(extension.to_lowercase()), + }, + } } /// Where a converted copy goes: beside the original, named for what it is. @@ -122,13 +137,33 @@ pub fn output_beside(path: &str) -> String { let stem = path.file_stem().map(|stem| stem.to_string_lossy()); match (path.parent(), stem) { (Some(parent), Some(stem)) if !parent.as_os_str().is_empty() => { - format!("{}/{stem}.h264.mp4", parent.display()) + // The root is the one directory that already ends in a separator, + // and a name beginning with two of them means something else + // again on some systems. + let parent = parent.display().to_string(); + match parent.ends_with(std::path::MAIN_SEPARATOR) { + true => format!("{parent}{stem}.h264.mp4"), + false => format!("{parent}/{stem}.h264.mp4"), + } } (_, Some(stem)) => format!("{stem}.h264.mp4"), _ => format!("{}.h264.mp4", path.display()), } } +/// A path an encoder will read as a path. +/// +/// The destination is the one argument with no flag in front of it, so a name +/// beginning with a dash would be read as an option and the call would fail +/// on a file that is otherwise fine. `./` in front of it names the same file +/// and cannot be mistaken for anything. +fn runnable(path: &str) -> String { + match path.starts_with('-') { + true => format!("./{path}"), + false => path.to_string(), + } +} + /// Judge a file and, if it needs converting, say how. /// /// `available` is the best encoder the machine has; it is only consulted when @@ -199,7 +234,7 @@ pub fn plan( } // So a player can start before the whole file has arrived. argv.extend(["-movflags".to_string(), "+faststart".to_string()]); - argv.push(output.to_string()); + argv.push(runnable(output)); argv } }; @@ -220,6 +255,52 @@ pub fn plan( mod tests { use super::*; + #[test] + fn a_directory_that_looks_like_a_file_is_not_one() { + // `Path` folds a trailing `.` away and would otherwise read the + // extension of the directory it was attached to, which makes a + // directory a playable MP4. + assert_eq!(container_of("film.mp4/."), None); + assert_eq!(container_of("film.mp4/./"), None); + assert_eq!(container_of("film.mp4/.."), None); + assert_eq!(container_of("film.mp4"), Some("mp4".to_string())); + assert_eq!(container_of("film.MP4"), Some("mp4".to_string())); + assert_eq!(container_of("film.mp4//"), Some("mp4".to_string())); + assert_eq!(container_of("film"), None); + // A leading dot is a hidden file rather than an extension. + assert_eq!(container_of(".mp4"), None); + assert_eq!(container_of("/media/.hidden.mp4"), Some("mp4".to_string())); + assert_eq!(container_of("talk."), None); + assert_eq!(container_of(""), None); + assert_eq!(container_of("/"), None); + } + + #[test] + fn a_destination_beginning_with_a_dash_is_named_so_it_is_read_as_one() { + let found = plan( + "-clip.webm", + &streams("vp9", "opus", "webm"), + Some(Encoder::Libx264), + "-clip.h264.mp4", + DEFAULT_VAAPI_DEVICE, + ); + assert_eq!( + found.argv.last().map(String::as_str), + Some("./-clip.h264.mp4") + ); + // The source needs no such help: it comes after `-i`. + assert!(found.argv.contains(&"-clip.webm".to_string())); + } + + #[test] + fn a_file_at_the_root_does_not_get_two_leading_separators() { + // A name beginning with exactly two separators is left to the system + // to interpret, and some interpret it as something other than a path. + assert_eq!(output_beside("/talk.webm"), "/talk.h264.mp4"); + assert_eq!(output_beside("/media/talk.webm"), "/media/talk.h264.mp4"); + assert_eq!(output_beside("talk.webm"), "talk.h264.mp4"); + } + fn streams(video: &str, audio: &str, container: &str) -> Streams { Streams { video: Some(video.into()), diff --git a/crates/avplan/src/main.rs b/crates/avplan/src/main.rs index 9e2515b..63faf8b 100644 --- a/crates/avplan/src/main.rs +++ b/crates/avplan/src/main.rs @@ -26,7 +26,7 @@ Exit codes: 0 it needs converting, 1 it plays as it is, 2 could not tell, Example: - for f in *.webm; do avplan \"$f\" | xargs -d '\\n' ffmpeg; done + for f in *.webm; do plan=$(avplan \"$f\") && eval \"$plan\"; done avplan --json talk.mkv | jq -r '.reasons[]'" )] struct Args { @@ -67,7 +67,16 @@ fn main() -> ExitCode { } } - let path = args.path.display().to_string(); + // The record and the command line both carry the path as text, so a name + // that is not text cannot be answered about. Saying so beats printing a + // confident command that names a file which does not exist. + let Some(path) = args.path.to_str().map(str::to_string) else { + eprintln!( + "avplan: {}: the name is not valid text", + args.path.display() + ); + return ExitCode::from(exit::USAGE); + }; let Some(video) = probe(&args.path, "v:0") else { eprintln!("avplan: ffprobe could not read {path}"); return ExitCode::from(exit::INDETERMINATE); @@ -162,6 +171,9 @@ fn probe(path: &std::path::Path, stream: &str) -> Option { "-of", "default=noprint_wrappers=1:nokey=1", ]) + // Named after `-i` rather than left as a trailing argument, so that a + // file whose name begins with a dash is a file and not an option. + .arg("-i") .arg(path) .output() .ok()?; diff --git a/parity/KNOWN.md b/parity/KNOWN.md index 18b8075..f9875e9 100644 --- a/parity/KNOWN.md +++ b/parity/KNOWN.md @@ -268,3 +268,29 @@ sheet. and the contract gives them different numbers. A caller that wants the Python's behaviour treats both as no; a caller that wants to know which files it failed on can now find out. + +## avplan — separators in a path, and a destination that starts with a dash + +**Original:** builds the destination by byte arithmetic — the path, minus as +many bytes as the extension is long, plus `.h264.mp4`. Its extension function +ignores trailing separators while the subtraction does not, so `x.mp4//` +becomes `x.m.h264.mp4`. Duplicate separators are carried through, and a +destination beginning with `-` is emitted bare. + +**Rust:** takes the path apart properly. `film.mp4/.` is a directory and has +no extension rather than an `mp4` one; `/talk.webm` becomes `/talk.h264.mp4` +rather than `//talk.h264.mp4`; `x.mp4//` becomes `x.h264.mp4`. A destination +beginning with `-` is written `./-name`, which is the same file and cannot be +read as an option. + +**Why this is acceptable:** every one of these is a path the original gets +wrong. A double leading separator is left to the system to interpret and some +systems interpret it as something else; `x.m.h264.mp4` names a file nobody +asked for; a directory reported as a playable MP4 is a wrong answer outright; +and a command line whose destination is read as an option fails on a file +that was otherwise fine. The original never met any of them because its paths +were always joined onto an absolute root. + +**Measured:** 32 cases, including a file at the root, a name beginning with a +dash, a name that is only a dot, a non-ASCII name and an extension that is +only a dot. Ordinary paths — the overwhelming majority — are byte-identical. diff --git a/parity/cases/avplan/cases.ndjson b/parity/cases/avplan/cases.ndjson index b41f34a..22aa593 100644 --- a/parity/cases/avplan/cases.ndjson +++ b/parity/cases/avplan/cases.ndjson @@ -22,3 +22,11 @@ {"name":"codec-merely-containing-h264","path":"/media/x.mp4","video":"prores_h264ish","audio":"aac","encoder":"libx264"} {"name":"mpeg4-in-avi","path":"/media/old.avi","video":"mpeg4","audio":"mp3","encoder":"h264_nvenc"} {"name":"hevc-in-mkv-aac","path":"/media/film.mkv","video":"hev1.1.6.L93","audio":"aac","encoder":"h264_nvenc"} +{"name":"at-the-filesystem-root","path":"/talk.webm","video":"vp9","audio":"opus","encoder":"libx264"} +{"name":"a-name-beginning-with-a-dash","path":"/media/-clip.webm","video":"vp9","audio":"opus","encoder":"libx264"} +{"name":"a-name-that-is-only-a-dot","path":"/media/.hidden.webm","video":"vp9","audio":"opus","encoder":"libx264"} +{"name":"an-upper-case-codec","path":"/media/x.mkv","video":"H264","audio":"AAC","encoder":"libx264"} +{"name":"a-codec-with-a-profile-suffix","path":"/media/x.mp4","video":"hev1.2.4.L120.B0","audio":"mp4a.40.5","encoder":"libx264"} +{"name":"a-non-ascii-name","path":"/media/Καλημέρα.webm","video":"vp9","audio":"opus","encoder":"libx264"} +{"name":"an-extension-that-is-only-a-dot","path":"/media/talk.","video":"vp9","audio":"opus","encoder":"libx264"} +{"name":"a-relative-name-beginning-with-a-dash","path":"-clip.webm","video":"vp9","audio":"opus","encoder":"libx264"} diff --git a/parity/golden/avplan/cases.ndjson b/parity/golden/avplan/cases.ndjson index 007c4f1..b5e7842 100644 --- a/parity/golden/avplan/cases.ndjson +++ b/parity/golden/avplan/cases.ndjson @@ -22,3 +22,11 @@ {"path":"/media/x.mp4","playable":false,"encoder":"libx264","copies_audio":true,"argv":["-nostdin","-y","-i","/media/x.mp4","-c:v","libx264","-preset","veryfast","-crf","23","-pix_fmt","yuv420p","-c:a","copy","-movflags","+faststart","/media/x.h264.mp4"]} {"path":"/media/old.avi","playable":false,"encoder":"h264_nvenc","copies_audio":false,"argv":["-nostdin","-y","-i","/media/old.avi","-c:v","h264_nvenc","-preset","p5","-rc","vbr","-cq","23","-b:v","0","-c:a","aac","-b:a","160k","-ac","2","-movflags","+faststart","/media/old.h264.mp4"]} {"path":"/media/film.mkv","playable":false,"encoder":"copy","copies_audio":true,"argv":["-nostdin","-y","-i","/media/film.mkv","-c:v","copy","-c:a","copy","-movflags","+faststart","/media/film.h264.mp4"]} +{"path":"/talk.webm","playable":false,"encoder":"libx264","copies_audio":false,"argv":["-nostdin","-y","-i","/talk.webm","-c:v","libx264","-preset","veryfast","-crf","23","-pix_fmt","yuv420p","-c:a","aac","-b:a","160k","-ac","2","-movflags","+faststart","/talk.h264.mp4"]} +{"path":"/media/-clip.webm","playable":false,"encoder":"libx264","copies_audio":false,"argv":["-nostdin","-y","-i","/media/-clip.webm","-c:v","libx264","-preset","veryfast","-crf","23","-pix_fmt","yuv420p","-c:a","aac","-b:a","160k","-ac","2","-movflags","+faststart","/media/-clip.h264.mp4"]} +{"path":"/media/.hidden.webm","playable":false,"encoder":"libx264","copies_audio":false,"argv":["-nostdin","-y","-i","/media/.hidden.webm","-c:v","libx264","-preset","veryfast","-crf","23","-pix_fmt","yuv420p","-c:a","aac","-b:a","160k","-ac","2","-movflags","+faststart","/media/.hidden.h264.mp4"]} +{"path":"/media/x.mkv","playable":false,"encoder":"copy","copies_audio":true,"argv":["-nostdin","-y","-i","/media/x.mkv","-c:v","copy","-c:a","copy","-movflags","+faststart","/media/x.h264.mp4"]} +{"path":"/media/x.mp4","playable":true,"encoder":null,"copies_audio":true,"argv":[]} +{"path":"/media/Καλημέρα.webm","playable":false,"encoder":"libx264","copies_audio":false,"argv":["-nostdin","-y","-i","/media/Καλημέρα.webm","-c:v","libx264","-preset","veryfast","-crf","23","-pix_fmt","yuv420p","-c:a","aac","-b:a","160k","-ac","2","-movflags","+faststart","/media/Καλημέρα.h264.mp4"]} +{"path":"/media/talk.","playable":false,"encoder":"libx264","copies_audio":false,"argv":["-nostdin","-y","-i","/media/talk.","-c:v","libx264","-preset","veryfast","-crf","23","-pix_fmt","yuv420p","-c:a","aac","-b:a","160k","-ac","2","-movflags","+faststart","/media/talk.h264.mp4"]} +{"path":"-clip.webm","playable":false,"encoder":"libx264","copies_audio":false,"argv":["-nostdin","-y","-i","-clip.webm","-c:v","libx264","-preset","veryfast","-crf","23","-pix_fmt","yuv420p","-c:a","aac","-b:a","160k","-ac","2","-movflags","+faststart","-clip.h264.mp4"]} diff --git a/parity/tests/avplan.rs b/parity/tests/avplan.rs index 9b4796a..b84374b 100644 --- a/parity/tests/avplan.rs +++ b/parity/tests/avplan.rs @@ -12,6 +12,12 @@ use serde_json::Value; /// The encoder the server defaults to when nothing names one. const DEFAULT_ENCODER: &str = "h264_nvenc"; +/// Answers that are meant to differ, each argued for in `KNOWN.md`. +/// +/// The original emits a destination beginning with a dash bare, and an +/// encoder reads it as an option rather than a file. +const DIVERGENT: &[(&str, &str)] = &[("a-relative-name-beginning-with-a-dash", "argv")]; + fn named(name: &str) -> avplan::Encoder { *avplan::encoder::PREFERRED .iter() @@ -60,6 +66,9 @@ fn every_verdict_matches_the_original() { let ours = avplan::plan(path, &streams, Some(chosen), &output, device); let mut note = |field: &str, ours: String, theirs: &Value| { + if DIVERGENT.contains(&(name, field)) { + return; + } wrong.push(format!("{name}.{field}: original {theirs}, rust {ours}")); }; @@ -99,6 +108,34 @@ fn every_verdict_matches_the_original() { ); } +/// A divergence that has quietly gone away is a note in `KNOWN.md` that is no +/// longer true, which is worth knowing about. +#[test] +fn the_documented_divergence_is_exactly_what_was_argued_for() { + let case = cases() + .into_iter() + .find(|case| case["name"] == "a-relative-name-beginning-with-a-dash") + .expect("the case exists"); + let path = case["path"].as_str().expect("a path"); + let streams = avplan::Streams { + video: case["video"].as_str().map(str::to_string), + audio: case["audio"].as_str().map(str::to_string), + container: avplan::container_of(path), + }; + let ours = avplan::plan( + path, + &streams, + Some(named("libx264")), + &avplan::output_beside(path), + avplan::DEFAULT_VAAPI_DEVICE, + ); + assert_eq!( + ours.argv.last().map(String::as_str), + Some("./-clip.h264.mp4"), + "the destination is no longer named so an encoder reads it as a file" + ); +} + /// Every branch of the encoder table has to be exercised, or the parity test /// proves nothing about the three encoders nobody's laptop happens to have. #[test]