diff --git a/crates/store/re_video/src/decode/ffmpeg_h264/ffmpeg.rs b/crates/store/re_video/src/decode/ffmpeg_h264/ffmpeg.rs index ec47d600f87b..f5db3a5e2581 100644 --- a/crates/store/re_video/src/decode/ffmpeg_h264/ffmpeg.rs +++ b/crates/store/re_video/src/decode/ffmpeg_h264/ffmpeg.rs @@ -666,15 +666,17 @@ impl FFmpegCliH264Decoder { ) -> Result { re_tracing::profile_function!(); - // TODO(ab): Pass executable path here. - if !ffmpeg_sidecar::command::ffmpeg_is_installed() { + if let Some(ffmpeg_path) = &ffmpeg_path { + if !ffmpeg_path.is_file() { + return Err(Error::FFmpegNotInstalled); + } + } else if !ffmpeg_sidecar::command::ffmpeg_is_installed() { return Err(Error::FFmpegNotInstalled); } // 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. - // TODO(ab): Pass executable path here. - match FFmpegVersion::for_executable(None) { + match FFmpegVersion::for_executable(ffmpeg_path.as_deref()) { Ok(version) => { if !version.is_compatible() { return Err(Error::UnsupportedFFmpegVersion { diff --git a/crates/store/re_video/src/decode/mod.rs b/crates/store/re_video/src/decode/mod.rs index 37d042da63df..855b7ceddebb 100644 --- a/crates/store/re_video/src/decode/mod.rs +++ b/crates/store/re_video/src/decode/mod.rs @@ -86,7 +86,9 @@ mod av1; mod ffmpeg_h264; #[cfg(with_ffmpeg)] -pub use ffmpeg_h264::{ffmpeg_download_url, Error as FFmpegError, FFmpegVersionParseError}; +pub use ffmpeg_h264::{ + ffmpeg_download_url, Error as FFmpegError, FFmpegVersion, FFmpegVersionParseError, +}; #[cfg(target_arch = "wasm32")] mod webcodecs; diff --git a/crates/viewer/re_ui/src/context_ext.rs b/crates/viewer/re_ui/src/context_ext.rs index d20e89caffa5..1132bd7e585d 100644 --- a/crates/viewer/re_ui/src/context_ext.rs +++ b/crates/viewer/re_ui/src/context_ext.rs @@ -1,5 +1,6 @@ use egui::{emath::Float, pos2, Align2, Color32, Mesh, Rect, Shape, Vec2}; +use crate::toasts::SUCCESS_COLOR; use crate::{DesignTokens, TopBarStyle}; /// Extension trait for [`egui::Context`]. @@ -58,6 +59,11 @@ pub trait ContextExt { // egui::Stroke::new(stroke_width, color) } + #[must_use] + fn success_text(&self, text: impl Into) -> egui::RichText { + egui::RichText::new(text).color(SUCCESS_COLOR) + } + #[must_use] fn warning_text(&self, text: impl Into) -> egui::RichText { let style = self.ctx().style(); diff --git a/crates/viewer/re_ui/src/ui_ext.rs b/crates/viewer/re_ui/src/ui_ext.rs index 6accaeb5457c..840f86841816 100644 --- a/crates/viewer/re_ui/src/ui_ext.rs +++ b/crates/viewer/re_ui/src/ui_ext.rs @@ -18,6 +18,12 @@ pub trait UiExt { fn ui(&self) -> &egui::Ui; fn ui_mut(&mut self) -> &mut egui::Ui; + /// Show the label with a success color. + fn success_label(&mut self, text: &str) -> egui::Response { + let label = egui::Label::new(self.ui().ctx().success_text(text)); + self.ui_mut().add(label) + } + /// Shows a warning label. fn warning_label(&mut self, warning_text: &str) -> egui::Response { let label = egui::Label::new(self.ui().ctx().warning_text(warning_text)); diff --git a/crates/viewer/re_viewer/src/ui/settings_screen.rs b/crates/viewer/re_viewer/src/ui/settings_screen.rs index 0ccb650497f7..a41e71991bfb 100644 --- a/crates/viewer/re_viewer/src/ui/settings_screen.rs +++ b/crates/viewer/re_viewer/src/ui/settings_screen.rs @@ -1,7 +1,9 @@ -use egui::NumExt as _; +use egui::{NumExt as _, Ui}; +use std::path::Path; use re_log_types::TimeZone; use re_ui::UiExt as _; +use re_video::decode::{FFmpegVersion, FFmpegVersionParseError}; use re_viewer_context::AppOptions; pub fn settings_screen_ui(ui: &mut egui::Ui, app_options: &mut AppOptions, keep_open: &mut bool) { @@ -99,6 +101,9 @@ fn settings_screen_ui_impl(ui: &mut egui::Ui, app_options: &mut AppOptions, keep ui.strong("Map view"); ui.horizontal(|ui| { + // TODO(ab): needed for alignment, we should use egui flex instead + ui.set_height(19.0); + ui.label("Mapbox access token:").on_hover_ui(|ui| { ui.markdown_ui( "This token is used toe enable Mapbox-based map view backgrounds.\n\n\ @@ -116,9 +121,26 @@ fn settings_screen_ui_impl(ui: &mut egui::Ui, app_options: &mut AppOptions, keep // separator_with_some_space(ui); - ui.strong("Video"); + video_section_ui(ui, app_options); + + // + // Experimental features + // + + // Currently, the wasm target does not have any experimental features. If/when that changes, + // move the conditional compilation flag to the respective checkbox code. + #[cfg(not(target_arch = "wasm32"))] + { + separator_with_some_space(ui); + ui.strong("Experimental features"); + ui + .re_checkbox(&mut app_options.experimental_space_view_screenshots, "Space view screenshots") + .on_hover_text("Allow taking screenshots of 2D and 3D space views via their context menu. Does not contain labels."); + } +} +fn video_section_ui(ui: &mut Ui, app_options: &mut AppOptions) { #[cfg(not(target_arch = "wasm32"))] { ui.re_checkbox( @@ -135,6 +157,9 @@ fn settings_screen_ui_impl(ui: &mut egui::Ui, app_options: &mut AppOptions, keep ui.add_enabled_ui(app_options.video_decoder_override_ffmpeg_path, |ui| { ui.horizontal(|ui| { + // TODO(ab): needed for alignment, we should use egui flex instead + ui.set_height(19.0); + ui.label("Path:"); ui.add(egui::TextEdit::singleline( @@ -142,6 +167,8 @@ fn settings_screen_ui_impl(ui: &mut egui::Ui, app_options: &mut AppOptions, keep )); }); }); + + ffmpeg_path_status_ui(ui, app_options); } // This affects only the web target, so we don't need to show it on native. @@ -171,20 +198,38 @@ fn settings_screen_ui_impl(ui: &mut egui::Ui, app_options: &mut AppOptions, keep // entries outdate automatically. }); } +} - // - // Experimental features - // +fn ffmpeg_path_status_ui(ui: &mut Ui, app_options: &AppOptions) { + let path = app_options + .video_decoder_override_ffmpeg_path + .then(|| Path::new(&app_options.video_decoder_ffmpeg_path)); - // Currently, the wasm target does not have any experimental features. If/when that changes, - // move the conditional compilation flag to the respective checkbox code. - #[cfg(not(target_arch = "wasm32"))] - { - separator_with_some_space(ui); - ui.strong("Experimental features"); - ui - .re_checkbox(&mut app_options.experimental_space_view_screenshots, "Space view screenshots") - .on_hover_text("Allow taking screenshots of 2D and 3D space views via their context menu. Does not contain labels."); + if let Some(path) = path { + if !path.is_file() { + ui.error_label("The specified FFmpeg binary path does not exist or is not a file."); + return; + } + } + + let res = FFmpegVersion::for_executable(path); + match res { + 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 }) => { + ui.warning_label(&format!( + "FFmpeg binary found but unable to parse version: {raw_version}" + )); + } + + Err(err) => { + ui.error_label(&format!("Unable to check FFmpeg version: {err}")); + } } } diff --git a/crates/viewer/re_viewer_context/src/app_options.rs b/crates/viewer/re_viewer_context/src/app_options.rs index 5c986aa9816b..6f9afe532fd9 100644 --- a/crates/viewer/re_viewer_context/src/app_options.rs +++ b/crates/viewer/re_viewer_context/src/app_options.rs @@ -45,11 +45,13 @@ pub struct AppOptions { /// /// Implementation note: we avoid using `Option` here to avoid loosing the user-defined /// path when disabling the override. + #[allow(clippy::doc_markdown)] pub video_decoder_override_ffmpeg_path: bool, /// Custom path to the FFmpeg binary. /// /// Don't use this field directly, use [`AppOptions::video_decoder_settings`] instead. + #[allow(clippy::doc_markdown)] pub video_decoder_ffmpeg_path: String, /// Mapbox API key (used to enable Mapbox-based map view backgrounds).