From f1afa7d7c1419e12dc1d08819a2c46fe0875fb33 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Sun, 15 Dec 2024 08:38:18 +0100 Subject: [PATCH 1/8] ffmpeg: cache error too --- Cargo.lock | 1 + crates/utils/re_video/Cargo.toml | 1 + .../src/decode/ffmpeg_h264/version.rs | 36 +++++++++++-------- 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fef05a09e3c4..533c32008c24 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6512,6 +6512,7 @@ dependencies = [ "js-sys", "once_cell", "parking_lot", + "poll-promise", "re_build_info", "re_build_tools", "re_log", diff --git a/crates/utils/re_video/Cargo.toml b/crates/utils/re_video/Cargo.toml index 2188bdb102ed..890c7441faf9 100644 --- a/crates/utils/re_video/Cargo.toml +++ b/crates/utils/re_video/Cargo.toml @@ -53,6 +53,7 @@ econtext.workspace = true itertools.workspace = true once_cell.workspace = true parking_lot.workspace = true +poll-promise.workspace = true re_mp4.workspace = true thiserror.workspace = true diff --git a/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs b/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs index a23feaaa14e2..fb8d587f5166 100644 --- a/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs +++ b/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs @@ -111,21 +111,7 @@ impl FFmpegVersion { } } - // Run FFmpeg (or whatever was passed to us) to get the version. - let raw_version = if let Some(path) = path { - ffmpeg_sidecar::version::ffmpeg_version_with_path(path) - } else { - ffmpeg_sidecar::version::ffmpeg_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(), - }) - }; + let version = ffmpeg_version(path); cache.insert( cache_key.to_path_buf(), @@ -143,6 +129,26 @@ impl FFmpegVersion { } } +fn ffmpeg_version( + path: Option<&std::path::Path>, +) -> Result { + let raw_version = if let Some(path) = path { + re_tracing::profile_scope!("ffmpeg_version_with_path"); + ffmpeg_sidecar::version::ffmpeg_version_with_path(path) + } else { + re_tracing::profile_scope!("ffmpeg_version"); + ffmpeg_sidecar::version::ffmpeg_version() + } + .map_err(|err| FFmpegVersionParseError::RunFFmpeg(err.to_string()))?; + if let Some(version) = FFmpegVersion::parse(&raw_version) { + Ok(version) + } else { + Err(FFmpegVersionParseError::ParseVersion { + raw_version: raw_version.clone(), + }) + } +} + #[cfg(test)] mod tests { use crate::decode::ffmpeg_h264::FFmpegVersion; From ee4a2c3b2276fd60870bef1f4a7f9dd19fa6bb1a Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Sun, 15 Dec 2024 08:52:15 +0100 Subject: [PATCH 2/8] Extract cache into a type --- .../src/decode/ffmpeg_h264/version.rs | 47 ++++++++++++------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs b/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs index fb8d587f5166..72de3c73e1af 100644 --- a/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs +++ b/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs @@ -79,14 +79,7 @@ impl FFmpegVersion { /// /// 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>) -> Result { - type VersionMap = HashMap< - PathBuf, - ( - Option, - Result, - ), - >; - static CACHE: Lazy> = Lazy::new(|| Mutex::new(HashMap::new())); + static CACHE: Lazy> = Lazy::new(|| Mutex::new(VersionMap::default())); re_tracing::profile_function!(); @@ -103,7 +96,36 @@ impl FFmpegVersion { }; // Check first if we already have the version cached. - let mut cache = CACHE.lock(); + CACHE.lock().version(path, modification_time) + } + + /// Returns true if this version is compatible with Rerun's minimum requirements. + pub fn is_compatible(&self) -> bool { + self.major > FFMPEG_MINIMUM_VERSION_MAJOR + || (self.major == FFMPEG_MINIMUM_VERSION_MAJOR + && self.minor >= FFMPEG_MINIMUM_VERSION_MINOR) + } +} + +#[derive(Default)] +struct VersionMap( + HashMap< + PathBuf, + ( + Option, + Result, + ), + >, +); + +impl VersionMap { + fn version( + &mut self, + path: Option<&std::path::Path>, + modification_time: Option, + ) -> Result { + let Self(cache) = self; + let cache_key = path.unwrap_or(std::path::Path::new("ffmpeg")); if let Some(cached) = cache.get(cache_key) { if modification_time == cached.0 { @@ -120,13 +142,6 @@ impl FFmpegVersion { version } - - /// Returns true if this version is compatible with Rerun's minimum requirements. - pub fn is_compatible(&self) -> bool { - self.major > FFMPEG_MINIMUM_VERSION_MAJOR - || (self.major == FFMPEG_MINIMUM_VERSION_MAJOR - && self.minor >= FFMPEG_MINIMUM_VERSION_MINOR) - } } fn ffmpeg_version( From 6c67d5da4a416f89dcc02f3bc0056212a43ca751 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Sun, 15 Dec 2024 08:59:57 +0100 Subject: [PATCH 3/8] Use a type alias and HashMap.entry --- .../src/decode/ffmpeg_h264/version.rs | 51 ++++++++----------- 1 file changed, 21 insertions(+), 30 deletions(-) diff --git a/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs b/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs index 72de3c73e1af..079dfac42dc5 100644 --- a/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs +++ b/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs @@ -1,7 +1,7 @@ -use std::{collections::HashMap, path::PathBuf}; +use std::{collections::HashMap, path::PathBuf, sync::Arc}; use once_cell::sync::Lazy; -use parking_lot::Mutex; +use parking_lot::Mutex;` // 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. @@ -9,6 +9,9 @@ use parking_lot::Mutex; pub const FFMPEG_MINIMUM_VERSION_MAJOR: u32 = 5; pub const FFMPEG_MINIMUM_VERSION_MINOR: u32 = 1; + +pub type FfmpegVersionResult = Result; + /// A successfully parsed `FFmpeg` version. #[derive(Clone, Debug, PartialEq, Eq)] pub struct FFmpegVersion { @@ -78,8 +81,9 @@ impl FFmpegVersion { /// 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>) -> Result { - static CACHE: Lazy> = Lazy::new(|| Mutex::new(VersionMap::default())); + pub fn for_executable(path: Option<&std::path::Path>) -> FfmpegVersionResult { + static CACHE: Lazy>> = + Lazy::new(|| Arc::new(Mutex::new(VersionMap::default()))); re_tracing::profile_function!(); @@ -96,7 +100,7 @@ impl FFmpegVersion { }; // Check first if we already have the version cached. - CACHE.lock().version(path, modification_time) + CACHE.lock().version(path, modification_time).clone() } /// Returns true if this version is compatible with Rerun's minimum requirements. @@ -108,53 +112,40 @@ impl FFmpegVersion { } #[derive(Default)] -struct VersionMap( - HashMap< - PathBuf, - ( - Option, - Result, - ), - >, -); +struct VersionMap(HashMap, FfmpegVersionResult)>); impl VersionMap { fn version( &mut self, path: Option<&std::path::Path>, modification_time: Option, - ) -> Result { + ) -> &FfmpegVersionResult { let Self(cache) = self; - 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 cached.1.clone(); + let cache_key = path.unwrap_or(std::path::Path::new("ffmpeg")).to_path_buf(); + + match cache.entry(cache_key) { + std::collections::hash_map::Entry::Occupied(entry) => &entry.into_mut().1, + std::collections::hash_map::Entry::Vacant(entry) => { + let version = ffmpeg_version(path); + &entry.insert((modification_time, version)).1 } } - - let version = ffmpeg_version(path); - - cache.insert( - cache_key.to_path_buf(), - (modification_time, version.clone()), - ); - - version } } fn ffmpeg_version( path: Option<&std::path::Path>, ) -> Result { + re_tracing::profile_function!("ffmpeg_version_with_path"); + let raw_version = if let Some(path) = path { - re_tracing::profile_scope!("ffmpeg_version_with_path"); ffmpeg_sidecar::version::ffmpeg_version_with_path(path) } else { - re_tracing::profile_scope!("ffmpeg_version"); ffmpeg_sidecar::version::ffmpeg_version() } .map_err(|err| FFmpegVersionParseError::RunFFmpeg(err.to_string()))?; + if let Some(version) = FFmpegVersion::parse(&raw_version) { Ok(version) } else { From bc7c6c84e459b42cb5f06c29f981ffa710c60f6d Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Sun, 15 Dec 2024 09:04:49 +0100 Subject: [PATCH 4/8] Remove unnecessary Arc --- crates/utils/re_video/src/decode/ffmpeg_h264/version.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs b/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs index 079dfac42dc5..f2d7022e0e94 100644 --- a/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs +++ b/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs @@ -82,8 +82,8 @@ impl FFmpegVersion { /// /// 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>) -> FfmpegVersionResult { - static CACHE: Lazy>> = - Lazy::new(|| Arc::new(Mutex::new(VersionMap::default()))); + static CACHE: Lazy> = + Lazy::new(|| Mutex::new(VersionMap::default())); re_tracing::profile_function!(); From 9ee04f0a37d661ad898340088f39c4a09c841ebf Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Sun, 15 Dec 2024 09:07:25 +0100 Subject: [PATCH 5/8] Cleaner --- .../src/decode/ffmpeg_h264/version.rs | 42 ++++++++++--------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs b/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs index f2d7022e0e94..920940f865fa 100644 --- a/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs +++ b/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs @@ -82,25 +82,10 @@ impl FFmpegVersion { /// /// 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>) -> FfmpegVersionResult { - static CACHE: Lazy> = - Lazy::new(|| Mutex::new(VersionMap::default())); - re_tracing::profile_function!(); - // Retrieve file modification time first. - let modification_time = if let Some(path) = path { - path.metadata() - .map_err(|err| { - FFmpegVersionParseError::RetrieveFileModificationTime(err.to_string()) - })? - .modified() - .ok() - } else { - None - }; - - // Check first if we already have the version cached. - CACHE.lock().version(path, modification_time).clone() + let modification_time = file_modification_time(path)?; + VersionCache::global(|cache| cache.version(path, modification_time).clone()) } /// Returns true if this version is compatible with Rerun's minimum requirements. @@ -111,10 +96,29 @@ impl FFmpegVersion { } } +fn file_modification_time(path: Option<&std::path::Path>) -> Result, FFmpegVersionParseError> { + Ok(if let Some(path) = path { + path.metadata() + .map_err(|err| { + FFmpegVersionParseError::RetrieveFileModificationTime(err.to_string()) + })? + .modified() + .ok() + } else { + None + }) +} + #[derive(Default)] -struct VersionMap(HashMap, FfmpegVersionResult)>); +struct VersionCache(HashMap, FfmpegVersionResult)>); + +impl VersionCache { + fn global(f: impl FnOnce(&mut Self) -> R) -> R { + static CACHE: Lazy> = Lazy::new(|| Mutex::new(VersionCache::default())); + f(&mut CACHE.lock()) + } + -impl VersionMap { fn version( &mut self, path: Option<&std::path::Path>, From 1a702a7389b339f88e85caf012e45880b0c81618 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Sun, 15 Dec 2024 09:15:24 +0100 Subject: [PATCH 6/8] Use poll_promise for ffmpeg executable version, to speed up option panel --- .../re_video/src/decode/ffmpeg_h264/ffmpeg.rs | 2 +- .../src/decode/ffmpeg_h264/version.rs | 51 ++++++++++++++----- .../re_viewer/src/ui/settings_screen.rs | 13 +++-- 3 files changed, 47 insertions(+), 19 deletions(-) diff --git a/crates/utils/re_video/src/decode/ffmpeg_h264/ffmpeg.rs b/crates/utils/re_video/src/decode/ffmpeg_h264/ffmpeg.rs index 3f8775ab8005..d48f647aa979 100644 --- a/crates/utils/re_video/src/decode/ffmpeg_h264/ffmpeg.rs +++ b/crates/utils/re_video/src/decode/ffmpeg_h264/ffmpeg.rs @@ -811,7 +811,7 @@ impl FFmpegCliH264Decoder { // Check the version once ahead of running FFmpeg. // The error is still handled if it happens while running FFmpeg, but it's a bit unclear if we can get it to start in the first place then. - match FFmpegVersion::for_executable(ffmpeg_path.as_deref()) { + match FFmpegVersion::for_executable_blocking(ffmpeg_path.as_deref()) { Ok(version) => { if !version.is_compatible() { return Err(Error::UnsupportedFFmpegVersion { diff --git a/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs b/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs index 920940f865fa..f84777659ff5 100644 --- a/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs +++ b/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs @@ -1,7 +1,8 @@ -use std::{collections::HashMap, path::PathBuf, sync::Arc}; +use std::{collections::HashMap, path::PathBuf, task::Poll}; use once_cell::sync::Lazy; -use parking_lot::Mutex;` +use parking_lot::Mutex; +use poll_promise::Promise; // 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. @@ -9,7 +10,6 @@ use parking_lot::Mutex;` pub const FFMPEG_MINIMUM_VERSION_MAJOR: u32 = 5; pub const FFMPEG_MINIMUM_VERSION_MINOR: u32 = 1; - pub type FfmpegVersionResult = Result; /// A successfully parsed `FFmpeg` version. @@ -81,11 +81,31 @@ impl FFmpegVersion { /// 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>) -> FfmpegVersionResult { + pub fn for_executable_poll(path: Option<&std::path::Path>) -> Poll { + re_tracing::profile_function!(); + + let modification_time = file_modification_time(path)?; + VersionCache::global(|cache| { + cache + .version(path, modification_time) + .poll() + .map(|r| r.clone()) + }) + } + + /// Like [`for_executable_polling`], but blocks until the version is ready. + /// + /// WARNING: this blocks for half a second on Mac, maybe more on other platforms. + pub fn for_executable_blocking(path: Option<&std::path::Path>) -> FfmpegVersionResult { re_tracing::profile_function!(); let modification_time = file_modification_time(path)?; - VersionCache::global(|cache| cache.version(path, modification_time).clone()) + VersionCache::global(|cache| { + cache + .version(path, modification_time) + .block_until_ready() + .clone() + }) } /// Returns true if this version is compatible with Rerun's minimum requirements. @@ -96,12 +116,12 @@ impl FFmpegVersion { } } -fn file_modification_time(path: Option<&std::path::Path>) -> Result, FFmpegVersionParseError> { +fn file_modification_time( + path: Option<&std::path::Path>, +) -> Result, FFmpegVersionParseError> { Ok(if let Some(path) = path { path.metadata() - .map_err(|err| { - FFmpegVersionParseError::RetrieveFileModificationTime(err.to_string()) - })? + .map_err(|err| FFmpegVersionParseError::RetrieveFileModificationTime(err.to_string()))? .modified() .ok() } else { @@ -110,7 +130,9 @@ fn file_modification_time(path: Option<&std::path::Path>) -> Result, FfmpegVersionResult)>); +struct VersionCache( + HashMap, Promise)>, +); impl VersionCache { fn global(f: impl FnOnce(&mut Self) -> R) -> R { @@ -118,12 +140,11 @@ impl VersionCache { f(&mut CACHE.lock()) } - fn version( &mut self, path: Option<&std::path::Path>, modification_time: Option, - ) -> &FfmpegVersionResult { + ) -> &Promise { let Self(cache) = self; let cache_key = path.unwrap_or(std::path::Path::new("ffmpeg")).to_path_buf(); @@ -131,7 +152,9 @@ impl VersionCache { match cache.entry(cache_key) { std::collections::hash_map::Entry::Occupied(entry) => &entry.into_mut().1, std::collections::hash_map::Entry::Vacant(entry) => { - let version = ffmpeg_version(path); + let path = path.map(|path| path.to_path_buf()); + let version = + Promise::spawn_thread("ffmpeg_version", move || ffmpeg_version(path.as_ref())); &entry.insert((modification_time, version)).1 } } @@ -139,7 +162,7 @@ impl VersionCache { } fn ffmpeg_version( - path: Option<&std::path::Path>, + path: Option<&std::path::PathBuf>, ) -> Result { re_tracing::profile_function!("ffmpeg_version_with_path"); diff --git a/crates/viewer/re_viewer/src/ui/settings_screen.rs b/crates/viewer/re_viewer/src/ui/settings_screen.rs index 2a0aa8b88cf1..21d57618d148 100644 --- a/crates/viewer/re_viewer/src/ui/settings_screen.rs +++ b/crates/viewer/re_viewer/src/ui/settings_screen.rs @@ -203,6 +203,7 @@ fn video_section_ui(ui: &mut Ui, app_options: &mut AppOptions) { #[cfg(not(target_arch = "wasm32"))] fn ffmpeg_path_status_ui(ui: &mut Ui, app_options: &AppOptions) { use re_video::decode::{FFmpegVersion, FFmpegVersionParseError}; + use std::task::Poll; let path = app_options .video_decoder_override_ffmpeg_path @@ -211,17 +212,21 @@ fn ffmpeg_path_status_ui(ui: &mut Ui, app_options: &AppOptions) { if path.is_some_and(|path| !path.is_file()) { ui.error_label("The specified FFmpeg binary path does not exist or is not a file."); } else { - let res = FFmpegVersion::for_executable(path); + let res = FFmpegVersion::for_executable_poll(path); match res { - Ok(version) => { + Poll::Pending => { + ui.spinner(); + } + + Poll::Ready(Ok(version)) => { if version.is_compatible() { ui.success_label(format!("FFmpeg found (version {version})")); } else { ui.error_label(format!("Incompatible FFmpeg version: {version}")); } } - Err(FFmpegVersionParseError::ParseVersion { raw_version }) => { + Poll::Ready(Err(FFmpegVersionParseError::ParseVersion { raw_version })) => { // We make this one a warning instead of an error because version parsing is flaky, and // it might end up still working. ui.warning_label(format!( @@ -229,7 +234,7 @@ fn ffmpeg_path_status_ui(ui: &mut Ui, app_options: &AppOptions) { )); } - Err(err) => { + Poll::Ready(Err(err)) => { ui.error_label(format!("Unable to check FFmpeg version: {err}")); } } From b7abf0f2661581d09f9b7702e39d84f3190560d5 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Sun, 15 Dec 2024 10:10:24 +0100 Subject: [PATCH 7/8] Fix doclink --- crates/utils/re_video/src/decode/ffmpeg_h264/version.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs b/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs index f84777659ff5..5add364ffca4 100644 --- a/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs +++ b/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs @@ -93,7 +93,7 @@ impl FFmpegVersion { }) } - /// Like [`for_executable_polling`], but blocks until the version is ready. + /// Like [`Self::for_executable_poll`], but blocks until the version is ready. /// /// WARNING: this blocks for half a second on Mac, maybe more on other platforms. pub fn for_executable_blocking(path: Option<&std::path::Path>) -> FfmpegVersionResult { From a2846529d3a04b86f8f8db4db5bc5408fb14c84f Mon Sep 17 00:00:00 2001 From: Clement Rey Date: Mon, 16 Dec 2024 14:37:46 +0100 Subject: [PATCH 8/8] Update crates/utils/re_video/src/decode/ffmpeg_h264/version.rs Co-authored-by: Andreas Reich --- crates/utils/re_video/src/decode/ffmpeg_h264/version.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs b/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs index 5add364ffca4..5842108d2fac 100644 --- a/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs +++ b/crates/utils/re_video/src/decode/ffmpeg_h264/version.rs @@ -95,7 +95,7 @@ impl FFmpegVersion { /// Like [`Self::for_executable_poll`], but blocks until the version is ready. /// - /// WARNING: this blocks for half a second on Mac, maybe more on other platforms. + /// WARNING: this blocks for half a second on Mac the first time this is called with a given path, maybe more on other platforms. pub fn for_executable_blocking(path: Option<&std::path::Path>) -> FfmpegVersionResult { re_tracing::profile_function!();