Skip to content

Commit 69fbaa5

Browse files
emilkteh-cmcWumpf
authored
Remove wait-time when opening settings panel (#8464)
### Related * Closes #8263 ### What I couldn't stand the half-second delay when opening the options screen. Rerun needs to feel snappy! Gettings the ffmpeg version is now done on a background thread, showing s spinner until it is done. Unfortunately we still have to wait for ffmpeg when starting up an H.264 video on native. We could fix that by pre-warming the cache though 🤔 --------- Co-authored-by: Clement Rey <[email protected]> Co-authored-by: Andreas Reich <[email protected]>
1 parent 40338bd commit 69fbaa5

File tree

5 files changed

+103
-57
lines changed

5 files changed

+103
-57
lines changed

Cargo.lock

+1
Original file line numberDiff line numberDiff line change
@@ -6511,6 +6511,7 @@ dependencies = [
65116511
"js-sys",
65126512
"once_cell",
65136513
"parking_lot",
6514+
"poll-promise",
65146515
"re_build_info",
65156516
"re_build_tools",
65166517
"re_log",

crates/utils/re_video/Cargo.toml

+1
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ econtext.workspace = true
5353
itertools.workspace = true
5454
once_cell.workspace = true
5555
parking_lot.workspace = true
56+
poll-promise.workspace = true
5657
re_mp4.workspace = true
5758
thiserror.workspace = true
5859

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -811,7 +811,7 @@ impl FFmpegCliH264Decoder {
811811

812812
// Check the version once ahead of running FFmpeg.
813813
// 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.
814-
match FFmpegVersion::for_executable(ffmpeg_path.as_deref()) {
814+
match FFmpegVersion::for_executable_blocking(ffmpeg_path.as_deref()) {
815815
Ok(version) => {
816816
if !version.is_compatible() {
817817
return Err(Error::UnsupportedFFmpegVersion {

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

+91-52
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,17 @@
1-
use std::{collections::HashMap, path::PathBuf};
1+
use std::{collections::HashMap, path::PathBuf, task::Poll};
22

33
use once_cell::sync::Lazy;
44
use parking_lot::Mutex;
5+
use poll_promise::Promise;
56

67
// FFmpeg 5.1 "Riemann" is from 2022-07-22.
78
// It's simply the oldest I tested manually as of writing. We might be able to go lower.
89
// However, we also know that FFmpeg 4.4 is already no longer working.
910
pub const FFMPEG_MINIMUM_VERSION_MAJOR: u32 = 5;
1011
pub const FFMPEG_MINIMUM_VERSION_MINOR: u32 = 1;
1112

13+
pub type FfmpegVersionResult = Result<FFmpegVersion, FFmpegVersionParseError>;
14+
1215
/// A successfully parsed `FFmpeg` version.
1316
#[derive(Clone, Debug, PartialEq, Eq)]
1417
pub struct FFmpegVersion {
@@ -78,61 +81,31 @@ impl FFmpegVersion {
7881
/// version string. Since version strings can get pretty wild, we don't want to fail in this case.
7982
///
8083
/// Internally caches the result per path together with its modification time to re-run/parse the version only if the file has changed.
81-
pub fn for_executable(path: Option<&std::path::Path>) -> Result<Self, FFmpegVersionParseError> {
82-
type VersionMap = HashMap<
83-
PathBuf,
84-
(
85-
Option<std::time::SystemTime>,
86-
Result<FFmpegVersion, FFmpegVersionParseError>,
87-
),
88-
>;
89-
static CACHE: Lazy<Mutex<VersionMap>> = Lazy::new(|| Mutex::new(HashMap::new()));
90-
84+
pub fn for_executable_poll(path: Option<&std::path::Path>) -> Poll<FfmpegVersionResult> {
9185
re_tracing::profile_function!();
9286

93-
// Retrieve file modification time first.
94-
let modification_time = if let Some(path) = path {
95-
path.metadata()
96-
.map_err(|err| {
97-
FFmpegVersionParseError::RetrieveFileModificationTime(err.to_string())
98-
})?
99-
.modified()
100-
.ok()
101-
} else {
102-
None
103-
};
104-
105-
// Check first if we already have the version cached.
106-
let mut cache = CACHE.lock();
107-
let cache_key = path.unwrap_or(std::path::Path::new("ffmpeg"));
108-
if let Some(cached) = cache.get(cache_key) {
109-
if modification_time == cached.0 {
110-
return cached.1.clone();
111-
}
112-
}
113-
114-
// Run FFmpeg (or whatever was passed to us) to get the version.
115-
let raw_version = if let Some(path) = path {
116-
ffmpeg_sidecar::version::ffmpeg_version_with_path(path)
117-
} else {
118-
ffmpeg_sidecar::version::ffmpeg_version()
119-
}
120-
.map_err(|err| FFmpegVersionParseError::RunFFmpeg(err.to_string()))?;
121-
122-
let version = if let Some(version) = Self::parse(&raw_version) {
123-
Ok(version)
124-
} else {
125-
Err(FFmpegVersionParseError::ParseVersion {
126-
raw_version: raw_version.clone(),
127-
})
128-
};
87+
let modification_time = file_modification_time(path)?;
88+
VersionCache::global(|cache| {
89+
cache
90+
.version(path, modification_time)
91+
.poll()
92+
.map(|r| r.clone())
93+
})
94+
}
12995

130-
cache.insert(
131-
cache_key.to_path_buf(),
132-
(modification_time, version.clone()),
133-
);
96+
/// Like [`Self::for_executable_poll`], but blocks until the version is ready.
97+
///
98+
/// WARNING: this blocks for half a second on Mac the first time this is called with a given path, maybe more on other platforms.
99+
pub fn for_executable_blocking(path: Option<&std::path::Path>) -> FfmpegVersionResult {
100+
re_tracing::profile_function!();
134101

135-
version
102+
let modification_time = file_modification_time(path)?;
103+
VersionCache::global(|cache| {
104+
cache
105+
.version(path, modification_time)
106+
.block_until_ready()
107+
.clone()
108+
})
136109
}
137110

138111
/// Returns true if this version is compatible with Rerun's minimum requirements.
@@ -143,6 +116,72 @@ impl FFmpegVersion {
143116
}
144117
}
145118

119+
fn file_modification_time(
120+
path: Option<&std::path::Path>,
121+
) -> Result<Option<std::time::SystemTime>, FFmpegVersionParseError> {
122+
Ok(if let Some(path) = path {
123+
path.metadata()
124+
.map_err(|err| FFmpegVersionParseError::RetrieveFileModificationTime(err.to_string()))?
125+
.modified()
126+
.ok()
127+
} else {
128+
None
129+
})
130+
}
131+
132+
#[derive(Default)]
133+
struct VersionCache(
134+
HashMap<PathBuf, (Option<std::time::SystemTime>, Promise<FfmpegVersionResult>)>,
135+
);
136+
137+
impl VersionCache {
138+
fn global<R>(f: impl FnOnce(&mut Self) -> R) -> R {
139+
static CACHE: Lazy<Mutex<VersionCache>> = Lazy::new(|| Mutex::new(VersionCache::default()));
140+
f(&mut CACHE.lock())
141+
}
142+
143+
fn version(
144+
&mut self,
145+
path: Option<&std::path::Path>,
146+
modification_time: Option<std::time::SystemTime>,
147+
) -> &Promise<FfmpegVersionResult> {
148+
let Self(cache) = self;
149+
150+
let cache_key = path.unwrap_or(std::path::Path::new("ffmpeg")).to_path_buf();
151+
152+
match cache.entry(cache_key) {
153+
std::collections::hash_map::Entry::Occupied(entry) => &entry.into_mut().1,
154+
std::collections::hash_map::Entry::Vacant(entry) => {
155+
let path = path.map(|path| path.to_path_buf());
156+
let version =
157+
Promise::spawn_thread("ffmpeg_version", move || ffmpeg_version(path.as_ref()));
158+
&entry.insert((modification_time, version)).1
159+
}
160+
}
161+
}
162+
}
163+
164+
fn ffmpeg_version(
165+
path: Option<&std::path::PathBuf>,
166+
) -> Result<FFmpegVersion, FFmpegVersionParseError> {
167+
re_tracing::profile_function!("ffmpeg_version_with_path");
168+
169+
let raw_version = if let Some(path) = path {
170+
ffmpeg_sidecar::version::ffmpeg_version_with_path(path)
171+
} else {
172+
ffmpeg_sidecar::version::ffmpeg_version()
173+
}
174+
.map_err(|err| FFmpegVersionParseError::RunFFmpeg(err.to_string()))?;
175+
176+
if let Some(version) = FFmpegVersion::parse(&raw_version) {
177+
Ok(version)
178+
} else {
179+
Err(FFmpegVersionParseError::ParseVersion {
180+
raw_version: raw_version.clone(),
181+
})
182+
}
183+
}
184+
146185
#[cfg(test)]
147186
mod tests {
148187
use crate::decode::ffmpeg_h264::FFmpegVersion;

crates/viewer/re_viewer/src/ui/settings_screen.rs

+9-4
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,7 @@ fn video_section_ui(ui: &mut Ui, app_options: &mut AppOptions) {
203203
#[cfg(not(target_arch = "wasm32"))]
204204
fn ffmpeg_path_status_ui(ui: &mut Ui, app_options: &AppOptions) {
205205
use re_video::decode::{FFmpegVersion, FFmpegVersionParseError};
206+
use std::task::Poll;
206207

207208
let path = app_options
208209
.video_decoder_override_ffmpeg_path
@@ -211,25 +212,29 @@ fn ffmpeg_path_status_ui(ui: &mut Ui, app_options: &AppOptions) {
211212
if path.is_some_and(|path| !path.is_file()) {
212213
ui.error_label("The specified FFmpeg binary path does not exist or is not a file.");
213214
} else {
214-
let res = FFmpegVersion::for_executable(path);
215+
let res = FFmpegVersion::for_executable_poll(path);
215216

216217
match res {
217-
Ok(version) => {
218+
Poll::Pending => {
219+
ui.spinner();
220+
}
221+
222+
Poll::Ready(Ok(version)) => {
218223
if version.is_compatible() {
219224
ui.success_label(format!("FFmpeg found (version {version})"));
220225
} else {
221226
ui.error_label(format!("Incompatible FFmpeg version: {version}"));
222227
}
223228
}
224-
Err(FFmpegVersionParseError::ParseVersion { raw_version }) => {
229+
Poll::Ready(Err(FFmpegVersionParseError::ParseVersion { raw_version })) => {
225230
// We make this one a warning instead of an error because version parsing is flaky, and
226231
// it might end up still working.
227232
ui.warning_label(format!(
228233
"FFmpeg binary found but unable to parse version: {raw_version}"
229234
));
230235
}
231236

232-
Err(err) => {
237+
Poll::Ready(Err(err)) => {
233238
ui.error_label(format!("Unable to check FFmpeg version: {err}"));
234239
}
235240
}

0 commit comments

Comments
 (0)