Add warning for using an old version of ffmpeg, be accomodating for l…

…og quirks of older versions (#8005)

### What

* part of #7607

Most replacing randomness with warnings. I haven't deep dived on which
version will work but 5.1 went fine with my test data minus a bit of log
spam which I fixed here.

I considered adding a check for coded availability, but this seems a bit
cumbersome given that any off-the-shelf ffmpeg install comes with h264:
the problem is that while we get a full string of compile options that
ffmpeg was built with, we can't really infer the decoders from this
since any given library may fulfill various roles. Instead we'd need to
parse the output of `ffmpeg -codecs`. We should eventually! But doesn't
seem urgent right now.

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/8005?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/8005?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!
* [x] If have noted any breaking changes to the log API in
`CHANGELOG.md` and the migration guide

- [PR Build Summary](https://build.rerun.io/pr/8005)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.

crates/store/re_video/src/decode/ffmpeg_h264/ffmpeg.rs

@@ -25,6 +25,11 @@ use crate::{
     PixelFormat, Time,
 };
 
+// FFmpeg 5.1 "Riemann" is from 2022-07-22.
+// It's simply the oldest I tested manually as of writing. We might be able to go lower.
+const FFMPEG_MINIMUM_VERSION_MAJOR: u32 = 5;
+const FFMPEG_MINIMUM_VERSION_MINOR: u32 = 1;
+
 #[derive(thiserror::Error, Debug)]
 pub enum Error {
     #[error("Couldn't find an installation of the FFmpeg executable.")]
@@ -200,7 +205,8 @@ impl FfmpegProcessAndListener {
         };
 
         let mut ffmpeg = FfmpegCommand::new()
-            .hide_banner()
+            // Keep banner enabled so we can check on the version more easily.
+            //.hide_banner()
             // "Reduce the latency introduced by buffering during initial input streams analysis."
             //.arg("-fflags nobuffer")
             //
@@ -382,46 +388,6 @@ fn read_ffmpeg_output(
     pixel_format: &PixelFormat,
     on_output: &Mutex<Option<Arc<OutputCallback>>>,
 ) -> Option<()> {
-    /// Ignore some common output from ffmpeg:
-    fn should_ignore_log_msg(msg: &str) -> bool {
-        let patterns = [
-            "Duration: N/A, bitrate: N/A",
-            "frame=    0 fps=0.0 q=0.0 size=       0kB time=N/A bitrate=N/A speed=N/A",
-            "encoder         : ", // Describes the encoder that was used to encode a video.
-            "Metadata:",
-            "Stream mapping:",
-             // It likes to say this a lot, almost no matter the format.
-            // Some sources say this is more about internal formats, i.e. specific decoders using the wrong values, rather than the cli passed formats.
-            "deprecated pixel format used, make sure you did set range correctly",
-            // Not entirely sure why it tells us this sometimes:
-            // Nowhere in the pipeline do we ask for this conversion, so it must be a transitional format?
-            // This is supported by experimentation yielding that it shows only up when using the `-colorspace` parameter.
-            // (color range and yuvj formats are fine though!)
-            "No accelerated colorspace conversion found from yuv420p to bgr24",
-            // We actually don't even want it to estimate a framerate!
-            "not enough frames to estimate rate",
-            // Similar: we don't want it to be able to estimate any of these things and we set those values explicitly, see invocation.
-            // Observed on Windows FFmpeg 7.1, but not with the same version on Mac with the same video.
-            "Consider increasing the value for the 'analyzeduration' (0) and 'probesize' (32) options",
-            // Size etc. *is* specified in SPS & PPS, unclear why it's missing that.
-            // Observed on Windows FFmpeg 7.1, but not with the same version on Mac with the same video.
-            "Could not find codec parameters for stream 0 (Video: h264, none): unspecified size",
-        ];
-
-        // Why would we get an empty message? Observed on Windows FFmpeg 7.1.
-        if msg.is_empty() {
-            return true;
-        }
-
-        for pattern in patterns {
-            if msg.contains(pattern) {
-                return true;
-            }
-        }
-
-        false
-    }
-
     // Pending frames, sorted by their presentation timestamp.
     let mut pending_frame_infos = BTreeMap::new();
     let mut highest_dts = Time::MIN; // Highest dts encountered so far.
@@ -435,17 +401,12 @@ fn read_ffmpeg_output(
                 }
             }
 
-            FfmpegEvent::Log(LogLevel::Warning, mut msg) => {
+            FfmpegEvent::Log(LogLevel::Warning, msg) => {
                 if !should_ignore_log_msg(&msg) {
-                    // Make warn_once work on `[swscaler @ 0x148db8000]` style warnings even if the address is different every time.
-                    if let Some(pos) = msg.find("[swscaler @ 0x") {
-                        msg = [
-                            &msg[..pos],
-                            &msg[(pos + "[swscaler @ 0x148db8000]".len())..],
-                        ]
-                        .join("[swscaler]");
-                    };
-                    re_log::warn_once!("{debug_name} decoder: {msg}");
+                    re_log::warn_once!(
+                        "{debug_name} decoder: {}",
+                        sanitize_ffmpeg_log_message(&msg)
+                    );
                 }
             }
 
@@ -464,7 +425,11 @@ fn read_ffmpeg_output(
                     return None;
                 }
                 if !should_ignore_log_msg(&msg) {
-                    re_log::warn_once!("{debug_name} decoder: {msg}");
+                    // Note that older ffmpeg versions don't flag their warnings as such and may end up here.
+                    re_log::warn_once!(
+                        "{debug_name} decoder: {}",
+                        sanitize_ffmpeg_log_message(&msg)
+                    );
                 }
             }
 
@@ -620,7 +585,47 @@ fn read_ffmpeg_output(
             }
 
             FfmpegEvent::ParsedVersion(ffmpeg_version) => {
-                re_log::debug_once!("FFmpeg version is: {}", ffmpeg_version.version);
+                re_log::debug_once!("FFmpeg version is {}", ffmpeg_version.version);
+
+                fn download_advice() -> String {
+                    if let Ok(download_url) = ffmpeg_sidecar::download::ffmpeg_download_url() {
+                        format!("\nYou can download an up to date version for your system at {download_url}.")
+                    } else {
+                        String::new()
+                    }
+                }
+
+                // Version strings can get pretty wild!
+                // E.g. choco installed ffmpeg on Windows gives me "7.1-essentials_build-www.gyan.dev".
+                let mut version_parts = ffmpeg_version.version.split('.');
+                let major = version_parts
+                    .next()
+                    .and_then(|part| part.parse::<u32>().ok());
+                let minor = version_parts.next().and_then(|part| {
+                    part.split('-')
+                        .next()
+                        .and_then(|part| part.parse::<u32>().ok())
+                });
+
+                if let (Some(major), Some(minor)) = (major, minor) {
+                    re_log::debug_once!("Parsed FFmpeg version as {}.{}", major, minor);
+
+                    if major < FFMPEG_MINIMUM_VERSION_MAJOR
+                        || (major == FFMPEG_MINIMUM_VERSION_MAJOR
+                            && minor < FFMPEG_MINIMUM_VERSION_MINOR)
+                    {
+                        re_log::warn_once!(
+                            "FFmpeg version is {}. Only versions >= {FFMPEG_MINIMUM_VERSION_MAJOR}.{FFMPEG_MINIMUM_VERSION_MINOR} are officially supported.{}",
+                            ffmpeg_version.version, download_advice()
+                        );
+                    }
+                } else {
+                    re_log::warn_once!(
+                        "Failed to parse FFmpeg version: {}{}",
+                        ffmpeg_version.version,
+                        download_advice()
+                    );
+                }
             }
 
             FfmpegEvent::ParsedConfiguration(ffmpeg_configuration) => {
@@ -801,12 +806,13 @@ fn write_avc_chunk_to_nalu_stream(
             return Err(Error::BadVideoData("Not enough bytes to".to_owned()));
         }
 
-        let nal_header = NalHeader(chunk.data[data_start]);
-        re_log::trace!(
-            "nal_header: {:?}, {}",
-            nal_header.unit_type(),
-            nal_header.ref_idc()
-        );
+        // Can be useful for finding issues, but naturally very spammy.
+        // let nal_header = NalHeader(chunk.data[data_start]);
+        // re_log::trace!(
+        //     "nal_header: {:?}, {}",
+        //     nal_header.unit_type(),
+        //     nal_header.ref_idc()
+        // );
 
         let data = &chunk.data[data_start..data_end];
 
@@ -837,3 +843,110 @@ fn write_avc_chunk_to_nalu_stream(
 
     Ok(())
 }
+
+/// Ignore some common output from ffmpeg.
+fn should_ignore_log_msg(msg: &str) -> bool {
+    let patterns = [
+        "Duration: N/A, bitrate: N/A",
+        "frame=    0 fps=0.0 q=0.0 size=       0kB time=N/A bitrate=N/A speed=N/A",
+        "encoder         : ", // Describes the encoder that was used to encode a video.
+        "Metadata:",
+        "Stream mapping:",
+        // It likes to say this a lot, almost no matter the format.
+        // Some sources say this is more about internal formats, i.e. specific decoders using the wrong values, rather than the cli passed formats.
+        "deprecated pixel format used, make sure you did set range correctly",
+        // Not entirely sure why it tells us this sometimes:
+        // Nowhere in the pipeline do we ask for this conversion, so it must be a transitional format?
+        // This is supported by experimentation yielding that it shows only up when using the `-colorspace` parameter.
+        // (color range and yuvj formats are fine though!)
+        "No accelerated colorspace conversion found from yuv420p to bgr24",
+        // We actually don't even want it to estimate a framerate!
+        "not enough frames to estimate rate",
+        // Similar: we don't want it to be able to estimate any of these things and we set those values explicitly, see invocation.
+        // Observed on Windows FFmpeg 7.1, but not with the same version on Mac with the same video.
+        "Consider increasing the value for the 'analyzeduration' (0) and 'probesize' (32) options",
+        // Size etc. *is* specified in SPS & PPS, unclear why it's missing that.
+        // Observed on Windows FFmpeg 7.1, but not with the same version on Mac with the same video.
+        "Could not find codec parameters for stream 0 (Video: h264, none): unspecified size",
+    ];
+
+    // Why would we get an empty message? Observed on Windows FFmpeg 7.1.
+    if msg.is_empty() {
+        return true;
+    }
+
+    for pattern in patterns {
+        if msg.contains(pattern) {
+            return true;
+        }
+    }
+
+    false
+}
+
+/// Strips out buffer addresses from `FFmpeg` log messages so that we can use it with the log-once family of methods.
+fn sanitize_ffmpeg_log_message(msg: &str) -> String {
+    // Make warn_once work on `[swscaler @ 0x148db8000]` style warnings even if the address is different every time.
+    // In older versions of FFmpeg this may happen several times in the same message (happens in 5.1, did not happen in 7.1).
+    let mut msg = msg.to_owned();
+    while let Some(start_pos) = msg.find("[swscaler @ 0x") {
+        if let Some(end_offset) = msg[start_pos..].find(']') {
+            if start_pos + end_offset + 1 > msg.len() {
+                break;
+            }
+
+            msg = [&msg[..start_pos], &msg[start_pos + end_offset + 1..]].join("[swscaler]");
+        } else {
+            // Huh, strange. Ignore it :shrug:
+            break;
+        }
+    }
+
+    msg
+}
+
+#[cfg(test)]
+mod tests {
+    use super::sanitize_ffmpeg_log_message;
+
+    #[test]
+    fn test_sanitize_ffmpeg_log_message() {
+        assert_eq!(
+            sanitize_ffmpeg_log_message("[swscaler @ 0x148db8000]"),
+            "[swscaler]"
+        );
+
+        assert_eq!(
+            sanitize_ffmpeg_log_message(
+                "Some text prior [swscaler @ 0x148db8000] Warning: invalid pixel format specified"
+            ),
+            "Some text prior [swscaler] Warning: invalid pixel format specified"
+        );
+
+        assert_eq!(
+            sanitize_ffmpeg_log_message(
+                "Some text prior [swscaler @ 0x148db8000 other stuff we don't care about I guess] Warning: invalid pixel format specified"
+            ),
+            "Some text prior [swscaler] Warning: invalid pixel format specified"
+        );
+
+        assert_eq!(
+            sanitize_ffmpeg_log_message("[swscaler @ 0x148db8100] Warning: invalid poxel format specified [swscaler @ 0x148db8200]"),
+            "[swscaler] Warning: invalid poxel format specified [swscaler]"
+        );
+
+        assert_eq!(
+            sanitize_ffmpeg_log_message("[swscaler @ 0x248db8000] Warning: invalid päxel format specified [swscaler @ 0x198db8000] [swscaler @ 0x148db8030]"),
+            "[swscaler] Warning: invalid päxel format specified [swscaler] [swscaler]"
+        );
+
+        assert_eq!(
+            sanitize_ffmpeg_log_message("[swscaler @ 0x148db8000 something is wrong here"),
+            "[swscaler @ 0x148db8000 something is wrong here"
+        );
+        assert_eq!(
+            sanitize_ffmpeg_log_message("swscaler @ 0x148db8000] something is wrong here"),
+            "swscaler @ 0x148db8000] something is wrong here"
+        );
+    }
+}

crates/store/re_video/src/decode/ffmpeg_h264/nalu.rs

@@ -86,6 +86,7 @@ impl NalHeader {
     }
 
     /// Ref idc is a value from 0-3 that tells us how "important" the frame/sample is.
+    #[allow(dead_code)]
     pub fn ref_idc(self) -> u8 {
         (self.0 >> 5) & 0b11
     }