make parse error structured, make ffmpeg casing more consistent

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

@@ -26,29 +26,27 @@ use crate::{
     PixelFormat, Time,
 };
 
+use super::version::FFmpegVersionParseError;
+
 #[derive(thiserror::Error, Debug)]
 pub enum Error {
     #[error("Couldn't find an installation of the FFmpeg executable.")]
-    FfmpegNotInstalled,
+    FFmpegNotInstalled,
 
     #[error("Failed to start FFmpeg: {0}")]
     FailedToStartFfmpeg(std::io::Error),
 
     #[error("FFmpeg version is {actual_version}. Only versions >= {minimum_version_major}.{minimum_version_minor} are officially supported.")]
     UnsupportedFFmpegVersion {
         actual_version: FFmpegVersion,
-
-        /// Copy of [`FFMPEG_MINIMUM_VERSION_MAJOR`].
         minimum_version_major: u32,
-
-        /// Copy of [`FFMPEG_MINIMUM_VERSION_MINOR`].
         minimum_version_minor: u32,
     },
 
     // TODO(andreas): This error can have a variety of reasons and is as such redundant to some of the others.
     // It works with an inner error because some of the error sources are behind an anyhow::Error inside of ffmpeg-sidecar.
-    #[error("Failed to determine FFmpeg version: {0}")]
-    FailedToDetermineFFmpegVersion(anyhow::Error),
+    #[error(transparent)]
+    FailedToDetermineFFmpegVersion(FFmpegVersionParseError),
 
     #[error("Failed to get stdin handle")]
     NoStdin,
@@ -95,7 +93,7 @@ impl From<Error> for crate::decode::Error {
 
 /// ffmpeg does not tell us the timestamp/duration of a given frame, so we need to remember it.
 #[derive(Clone)]
-struct FfmpegFrameInfo {
+struct FFmpegFrameInfo {
     /// The start of a new [`crate::demux::GroupOfPictures`]?
     ///
     /// This probably means this is a _keyframe_, and that and entire frame
@@ -106,7 +104,7 @@ struct FfmpegFrameInfo {
     decode_timestamp: Time,
 }
 
-enum FfmpegFrameData {
+enum FFmpegFrameData {
     Chunk(Chunk),
     EndOfStream,
 }
@@ -141,14 +139,14 @@ impl std::io::Write for StdinWithShutdown {
     }
 }
 
-struct FfmpegProcessAndListener {
+struct FFmpegProcessAndListener {
     ffmpeg: FfmpegChild,
 
     /// For sending frame timestamps to the ffmpeg listener thread.
-    frame_info_tx: Sender<FfmpegFrameInfo>,
+    frame_info_tx: Sender<FFmpegFrameInfo>,
 
     /// For sending chunks to the ffmpeg write thread.
-    frame_data_tx: Sender<FfmpegFrameData>,
+    frame_data_tx: Sender<FFmpegFrameData>,
 
     listen_thread: Option<std::thread::JoinHandle<()>>,
     write_thread: Option<std::thread::JoinHandle<()>>,
@@ -160,7 +158,7 @@ struct FfmpegProcessAndListener {
     on_output: Arc<Mutex<Option<Arc<OutputCallback>>>>,
 }
 
-impl FfmpegProcessAndListener {
+impl FFmpegProcessAndListener {
     fn new(
         debug_name: &str,
         on_output: Arc<OutputCallback>,
@@ -301,7 +299,7 @@ impl FfmpegProcessAndListener {
     }
 }
 
-impl Drop for FfmpegProcessAndListener {
+impl Drop for FFmpegProcessAndListener {
     fn drop(&mut self) {
         re_tracing::profile_function!();
 
@@ -313,7 +311,7 @@ impl Drop for FfmpegProcessAndListener {
         }
 
         // Notify (potentially wake up) the stdin write thread to stop it (it might be sleeping).
-        self.frame_data_tx.send(FfmpegFrameData::EndOfStream).ok();
+        self.frame_data_tx.send(FFmpegFrameData::EndOfStream).ok();
         // Kill stdin for the write thread. This helps cancelling ongoing stream write operations.
         self.stdin_shutdown
             .store(true, std::sync::atomic::Ordering::Release);
@@ -361,16 +359,16 @@ impl Drop for FfmpegProcessAndListener {
 
 fn write_ffmpeg_input(
     ffmpeg_stdin: &mut dyn std::io::Write,
-    frame_data_rx: &Receiver<FfmpegFrameData>,
+    frame_data_rx: &Receiver<FFmpegFrameData>,
     on_output: &Mutex<Option<Arc<OutputCallback>>>,
     avcc: &re_mp4::Avc1Box,
 ) {
     let mut state = NaluStreamState::default();
 
     while let Ok(data) = frame_data_rx.recv() {
         let chunk = match data {
-            FfmpegFrameData::Chunk(chunk) => chunk,
-            FfmpegFrameData::EndOfStream => break,
+            FFmpegFrameData::Chunk(chunk) => chunk,
+            FFmpegFrameData::EndOfStream => break,
         };
 
         if let Err(err) = write_avc_chunk_to_nalu_stream(avcc, ffmpeg_stdin, &chunk, &mut state) {
@@ -396,7 +394,7 @@ fn write_ffmpeg_input(
 fn read_ffmpeg_output(
     debug_name: &str,
     ffmpeg_iterator: ffmpeg_sidecar::iter::FfmpegIterator,
-    frame_info_rx: &Receiver<FfmpegFrameInfo>,
+    frame_info_rx: &Receiver<FFmpegFrameInfo>,
     pixel_format: &PixelFormat,
     on_output: &Mutex<Option<Arc<OutputCallback>>>,
 ) -> Option<()> {
@@ -644,14 +642,14 @@ fn read_ffmpeg_output(
 }
 
 /// Decode H.264 video via ffmpeg over CLI
-pub struct FfmpegCliH264Decoder {
+pub struct FFmpegCliH264Decoder {
     debug_name: String,
-    ffmpeg: FfmpegProcessAndListener,
+    ffmpeg: FFmpegProcessAndListener,
     avcc: re_mp4::Avc1Box,
     on_output: Arc<OutputCallback>,
 }
 
-impl FfmpegCliH264Decoder {
+impl FFmpegCliH264Decoder {
     pub fn new(
         debug_name: String,
         avcc: re_mp4::Avc1Box,
@@ -677,13 +675,18 @@ impl FfmpegCliH264Decoder {
                     });
                 }
             }
+            Err(FFmpegVersionParseError::ParseVersion { raw_version }) => {
+                // This happens quite often, don't fail playing video over it!
+                re_log::warn_once!("Failed to parse FFmpeg version: {raw_version}");
+            }
+
             Err(err) => {
                 return Err(Error::FailedToDetermineFFmpegVersion(err));
             }
         }
 
         let on_output = Arc::new(on_output);
-        let ffmpeg = FfmpegProcessAndListener::new(&debug_name, on_output.clone(), avcc.clone())?;
+        let ffmpeg = FFmpegProcessAndListener::new(&debug_name, on_output.clone(), avcc.clone())?;
 
         Ok(Self {
             debug_name,
@@ -694,19 +697,19 @@ impl FfmpegCliH264Decoder {
     }
 }
 
-impl AsyncDecoder for FfmpegCliH264Decoder {
+impl AsyncDecoder for FFmpegCliH264Decoder {
     fn submit_chunk(&mut self, chunk: Chunk) -> crate::decode::Result<()> {
         re_tracing::profile_function!();
 
         // We send the information about this chunk first.
         // Chunks are defined to always yield a single frame.
-        let frame_info = FfmpegFrameInfo {
+        let frame_info = FFmpegFrameInfo {
             is_sync: chunk.is_sync,
             presentation_timestamp: chunk.presentation_timestamp,
             decode_timestamp: chunk.decode_timestamp,
             duration: chunk.duration,
         };
-        let chunk = FfmpegFrameData::Chunk(chunk);
+        let chunk = FFmpegFrameData::Chunk(chunk);
 
         if self.ffmpeg.frame_info_tx.send(frame_info).is_err()
             || self.ffmpeg.frame_data_tx.send(chunk).is_err()
@@ -729,7 +732,7 @@ impl AsyncDecoder for FfmpegCliH264Decoder {
 
     fn reset(&mut self) -> crate::decode::Result<()> {
         re_log::debug!("Resetting ffmpeg decoder {}", self.debug_name);
-        self.ffmpeg = FfmpegProcessAndListener::new(
+        self.ffmpeg = FFmpegProcessAndListener::new(
             &self.debug_name,
             self.on_output.clone(),
             self.avcc.clone(),

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

@@ -3,8 +3,11 @@ mod nalu;
 mod sps;
 mod version;
 
-pub use ffmpeg::{Error, FfmpegCliH264Decoder};
-pub use version::{FFmpegVersion, FFMPEG_MINIMUM_VERSION_MAJOR, FFMPEG_MINIMUM_VERSION_MINOR};
+pub use ffmpeg::{Error, FFmpegCliH264Decoder};
+pub use version::{
+    FFmpegVersion, FFmpegVersionParseError, FFMPEG_MINIMUM_VERSION_MAJOR,
+    FFMPEG_MINIMUM_VERSION_MINOR,
+};
 
 /// Download URL for the latest version of `FFmpeg` on the current platform.
 /// None if the platform is not supported.

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

@@ -23,6 +23,18 @@ impl std::fmt::Display for FFmpegVersion {
     }
 }
 
+#[derive(thiserror::Error, Debug, Clone)]
+pub enum FFmpegVersionParseError {
+    #[error("Failed to retrieve file modification time of FFmpeg executable: {0}")]
+    RetrieveFileModificationTime(String),
+
+    #[error("Failed to determine FFmpeg version: {0}")]
+    RunFFmpeg(String),
+
+    #[error("Failed to parse FFmpeg version: {raw_version}")]
+    ParseVersion { raw_version: String },
+}
+
 impl FFmpegVersion {
     pub fn parse(raw_version: &str) -> Option<Self> {
         // Version strings can get pretty wild!
@@ -58,17 +70,28 @@ impl FFmpegVersion {
     ///
     /// If none is passed for the path, it uses `ffmpeg` from PATH.
     ///
+    /// Error indicates issues running `FFmpeg`. Ok(None) indicates that we weren't able to parse the
+    /// version string. Since version strings can get pretty wild, we don't want to fail in this case.
+    ///
     /// Internally caches the result per path together with its modification time to re-run/parse the version only if the file has changed.
-    pub fn for_executable(path: Option<&std::path::Path>) -> anyhow::Result<Self> {
-        type VersionMap = HashMap<PathBuf, (Option<std::time::SystemTime>, FFmpegVersion)>;
+    pub fn for_executable(path: Option<&std::path::Path>) -> Result<Self, FFmpegVersionParseError> {
+        type VersionMap = HashMap<
+            PathBuf,
+            (
+                Option<std::time::SystemTime>,
+                Result<FFmpegVersion, FFmpegVersionParseError>,
+            ),
+        >;
         static CACHE: Lazy<Mutex<VersionMap>> = Lazy::new(|| Mutex::new(HashMap::new()));
 
         re_tracing::profile_function!();
 
         // Retrieve file modification time first.
         let modification_time = if let Some(path) = path {
             path.metadata()
-                .map_err(|err| anyhow::anyhow!("Failed to read file: {err}"))?
+                .map_err(|err| {
+                    FFmpegVersionParseError::RetrieveFileModificationTime(err.to_string())
+                })?
                 .modified()
                 .ok()
         } else {
@@ -80,7 +103,7 @@ impl FFmpegVersion {
         let cache_key = path.unwrap_or(std::path::Path::new("ffmpeg"));
         if let Some(cached) = cache.get(cache_key) {
             if modification_time == cached.0 {
-                return Ok(cached.1.clone());
+                return cached.1.clone();
             }
         }
 
@@ -89,15 +112,23 @@ impl FFmpegVersion {
             ffmpeg_sidecar::version::ffmpeg_version_with_path(path)
         } else {
             ffmpeg_sidecar::version::ffmpeg_version()
-        }?;
-        let version = Self::parse(&raw_version)
-            .ok_or_else(|| anyhow::anyhow!("Failed to parse FFmpeg version: {raw_version}"))?;
+        }
+        .map_err(|err| FFmpegVersionParseError::RunFFmpeg(err.to_string()))?;
+
+        let version = if let Some(version) = Self::parse(&raw_version) {
+            Ok(version)
+        } else {
+            Err(FFmpegVersionParseError::ParseVersion {
+                raw_version: raw_version.clone(),
+            })
+        };
+
         cache.insert(
             cache_key.to_path_buf(),
             (modification_time, version.clone()),
         );
 
-        Ok(version)
+        version
     }
 
     /// Returns true if this version is compatible with Rerun's minimum requirements.

crates/store/re_video/src/decode/mod.rs

@@ -86,7 +86,7 @@ mod av1;
 mod ffmpeg_h264;
 
 #[cfg(with_ffmpeg)]
-pub use ffmpeg_h264::{ffmpeg_download_url, Error as FfmpegError};
+pub use ffmpeg_h264::{ffmpeg_download_url, Error as FFmpegError, FFmpegVersionParseError};
 
 #[cfg(target_arch = "wasm32")]
 mod webcodecs;
@@ -120,7 +120,7 @@ pub enum Error {
 
     #[cfg(with_ffmpeg)]
     #[error(transparent)]
-    Ffmpeg(std::sync::Arc<FfmpegError>),
+    Ffmpeg(std::sync::Arc<FFmpegError>),
 
     #[error("Unsupported bits per component: {0}")]
     BadBitsPerComponent(usize),
@@ -193,7 +193,7 @@ pub fn new_decoder(
         #[cfg(with_ffmpeg)]
         re_mp4::StsdBoxContent::Avc1(avc1_box) => {
             re_log::trace!("Decoding H.264…");
-            Ok(Box::new(ffmpeg_h264::FfmpegCliH264Decoder::new(
+            Ok(Box::new(ffmpeg_h264::FFmpegCliH264Decoder::new(
                 debug_name.to_owned(),
                 avc1_box.clone(),
                 on_output,

crates/viewer/re_data_ui/src/video.rs

@@ -209,9 +209,9 @@ pub fn show_decoded_frame_info(
             ) = &err
             {
                 match err.as_ref() {
-                    re_video::decode::FfmpegError::UnsupportedFFmpegVersion { .. }
-                    | re_video::decode::FfmpegError::FailedToDetermineFFmpegVersion(_)
-                    | re_video::decode::FfmpegError::FfmpegNotInstalled => {
+                    re_video::decode::FFmpegError::UnsupportedFFmpegVersion { .. }
+                    | re_video::decode::FFmpegError::FailedToDetermineFFmpegVersion(_)
+                    | re_video::decode::FFmpegError::FfmpegNotInstalled => {
                         if let Some(download_url) = re_video::decode::ffmpeg_download_url() {
                             ui.markdown_ui(&format!("You can download a build of `FFmpeg` [here]({download_url}). For Rerun to be able to use it, its binaries need to be reachable from `PATH`."));
                         }