avplan: paths an encoder can actually read, and a pipeline that runs

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
+The line is quoted for a shell, so `eval` runs it as written. A file that
-for it either.
+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.

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<String> {
+        // 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());

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<String> {
-    std::path::Path::new(path)
+    let trimmed = path.trim_end_matches(std::path::is_separator);
-        .extension()
+    let segment = trimmed.rsplit(std::path::is_separator).next().unwrap_or("");
-        .map(|extension| extension.to_string_lossy().to_lowercase())
+    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()),

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<String> {
             "-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()?;

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.

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"}

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"]}

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]