From fc8c800a9eaa3e53744ef1d31a5007adf6e26750 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Wed, 12 Aug 2026 13:30:33 +0200 Subject: [PATCH] fix(studio): re-analyse audio on every scenario load, and report failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The studio analysed audio tracks exactly once, in run_preview_root. Every later load path — the file watcher, opening a scenario from the library, undo — replaced the model without touching the analysis, so a scenario that gained a track, or whose track path was corrected, kept the previous (or no) analysis for the whole session. waveform and audio_spectrum then drew their flat fallback with nothing to explain it, because analyze_scenario_audio turned a failed decode into a bare `continue`. Move the call into StudioModel::new, the constructor every load path already funnels through, and return the tracks that could not be analysed instead of swallowing them. The encoders print them as a warning; the studio shows them in the topbar next to the write-error indicator. The cache is keyed by path alone (the painters look tracks up that way), so also record what each entry was computed from — file length, mtime, fps — and re-analyse when that changes. Re-exporting a mix under the same name while the studio is open used to keep serving the first envelope. --- crates/rustmotion-studio/src/app/mod.rs | 4 +- crates/rustmotion-studio/src/editor/topbar.rs | 8 ++ crates/rustmotion-studio/src/editor/view.rs | 4 +- .../rustmotion-studio/src/scenario/model.rs | 17 ++++ .../rustmotion/src/encode/audio_analysis.rs | 90 +++++++++++++++++-- crates/rustmotion/src/encode/video/ffmpeg.rs | 4 +- crates/rustmotion/src/encode/video/h264.rs | 8 +- crates/rustmotion/src/tests.rs | 85 ++++++++++++++++++ 8 files changed, 209 insertions(+), 11 deletions(-) diff --git a/crates/rustmotion-studio/src/app/mod.rs b/crates/rustmotion-studio/src/app/mod.rs index 423f6b2e..7bde8fa5 100644 --- a/crates/rustmotion-studio/src/app/mod.rs +++ b/crates/rustmotion-studio/src/app/mod.rs @@ -69,7 +69,9 @@ pub fn run_preview_root( engine::prefetch_icons(&view.scenes); engine::preextract_video_frames(&view.scenes, scenario.video.fps); } - rustmotion::encode::audio_analysis::analyze_scenario_audio(&scenario); + // Audio analysis is NOT done here: it belongs to `StudioModel::new`, which + // every load path goes through. Doing it once at launch left a scenario + // opened or reloaded later with the wrong (or no) analysis for the session. if !scenario.fonts.is_empty() { engine::renderer::load_custom_fonts(&scenario.fonts); } diff --git a/crates/rustmotion-studio/src/editor/topbar.rs b/crates/rustmotion-studio/src/editor/topbar.rs index 6c7a9f28..1fe8853a 100644 --- a/crates/rustmotion-studio/src/editor/topbar.rs +++ b/crates/rustmotion-studio/src/editor/topbar.rs @@ -95,6 +95,7 @@ pub fn TopBar( show_hits: Signal, comment_count: usize, write_error: Option, + audio_error: Option, diff_active: Signal, diff_side: Signal, ) -> Element { @@ -159,6 +160,13 @@ pub fn TopBar( "Changes not saved: {msg}" } } + if let Some(ref msg) = audio_error { + span { + title: "{msg}", + style: "color:var(--rm-error); font-size:11px; white-space:nowrap; max-width:220px; overflow:hidden; text-overflow:ellipsis;", + "Audio not analysed: {msg}" + } + } Button { variant: ButtonVariant::Ghost, size: ButtonSize::IconSm, diff --git a/crates/rustmotion-studio/src/editor/view.rs b/crates/rustmotion-studio/src/editor/view.rs index 043dced0..13db46df 100644 --- a/crates/rustmotion-studio/src/editor/view.rs +++ b/crates/rustmotion-studio/src/editor/view.rs @@ -157,7 +157,7 @@ pub fn StudioApp(view: Signal) -> Element { diff_side, ); - let (total, err, write_err, title, annotations) = { + let (total, err, write_err, audio_err, title, annotations) = { let m = shared.lock().unwrap_or_else(|e| e.into_inner()); let title = m .path @@ -170,6 +170,7 @@ pub fn StudioApp(view: Signal) -> Element { m.total_frames, m.error.clone(), m.write_error.clone(), + m.audio_error.clone(), title, list_annotations(&m.raw), ) @@ -320,6 +321,7 @@ pub fn StudioApp(view: Signal) -> Element { show_hits, comment_count, write_error: write_err, + audio_error: audio_err, diff_active, diff_side, } diff --git a/crates/rustmotion-studio/src/scenario/model.rs b/crates/rustmotion-studio/src/scenario/model.rs index 403025bd..6fe74582 100644 --- a/crates/rustmotion-studio/src/scenario/model.rs +++ b/crates/rustmotion-studio/src/scenario/model.rs @@ -20,6 +20,10 @@ pub struct StudioModel { /// Disk-write error surfaced as a topbar indicator. Cleared on the next /// successful write or on model reload. pub write_error: Option, + /// Audio tracks the analyser could not read, surfaced as a topbar + /// indicator. Without it an undecodable track just makes `waveform` and + /// `audio_spectrum` draw their flat fallback, with nothing to explain it. + pub audio_error: Option, /// Bumped on every hot-reload so the UI can detect a change. pub generation: u64, /// Path to the scenario file (for inspector write-back). @@ -66,12 +70,25 @@ impl StudioModel { .unwrap_or(serde_json::Value::Null); let tasks = rustmotion::encode::build_frame_tasks(&scenario); let total_frames = tasks.len() as u32; + // Analyse here rather than once at launch: every reload path — the + // watcher, opening a file from the library, undo — funnels through this + // constructor, and a scenario that gained a track or had its path fixed + // must not keep the previous scenario's (or no) analysis. + let failures = rustmotion::encode::audio_analysis::analyze_scenario_audio(&scenario); + let audio_error = (!failures.is_empty()).then(|| { + failures + .iter() + .map(|f| f.to_string()) + .collect::>() + .join(" · ") + }); Self { scenario: Arc::new(scenario), tasks: Arc::new(tasks), total_frames, error, write_error: None, + audio_error, generation: 0, path, raw, diff --git a/crates/rustmotion/src/encode/audio_analysis.rs b/crates/rustmotion/src/encode/audio_analysis.rs index bf0a54ec..8dabef13 100644 --- a/crates/rustmotion/src/encode/audio_analysis.rs +++ b/crates/rustmotion/src/encode/audio_analysis.rs @@ -1,4 +1,5 @@ -use std::sync::Arc; +use std::collections::HashMap; +use std::sync::{Arc, Mutex, OnceLock}; use rustfft::{num_complex::Complex, FftPlanner}; use rustmotion_core::engine::renderer::audio_analysis::{audio_analysis_cache, AudioAnalysis}; @@ -7,6 +8,51 @@ use rustmotion_core::schema::ResolvedScenario; const FFT_SIZE: usize = 2048; const NUM_BANDS: usize = 16; +/// A track that could not be analysed, and why. +/// +/// Returned rather than logged so each caller decides how loud to be: an +/// encode prints a warning and carries on, the studio shows it in the topbar. +/// Swallowing it leaves `waveform`/`audio_spectrum` drawing their flat +/// fallback with nothing anywhere saying why. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AudioAnalysisFailure { + pub src: String, + pub reason: String, +} + +impl std::fmt::Display for AudioAnalysisFailure { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}: {}", self.src, self.reason) + } +} + +/// What a cached analysis was computed from: file size, mtime, and the fps it +/// was bucketed at. The cache is keyed by path alone (the painters look tracks +/// up that way), so without this a track whose *content* changed under a stable +/// path — the normal case when someone re-exports a mix while the studio is +/// open — would keep serving the old envelope forever. +type SourceFingerprint = (u64, u128, u32); + +static FINGERPRINTS: OnceLock>> = OnceLock::new(); + +fn fingerprints() -> &'static Mutex> { + FINGERPRINTS.get_or_init(Default::default) +} + +/// `None` when the file cannot be stat'ed — treated as "changed", so the next +/// analysis attempt runs and reports a real decode error instead of silently +/// reusing a stale entry. +fn source_fingerprint(src: &str, fps: u32) -> Option { + let meta = std::fs::metadata(src).ok()?; + let mtime = meta + .modified() + .ok()? + .duration_since(std::time::UNIX_EPOCH) + .ok()? + .as_nanos(); + Some((meta.len(), mtime, fps)) +} + /// Build the 16 log-spaced band frequency boundaries (Hz) from 20..16000. fn band_boundaries() -> [(f32, f32); NUM_BANDS] { let mut bounds = [(0.0f32, 0.0f32); NUM_BANDS]; @@ -28,14 +74,18 @@ fn hann_window(n: usize) -> Vec { } /// Analyze all audio tracks in the scenario and populate the global cache. -/// Already-cached tracks are skipped (idempotent). -pub fn analyze_scenario_audio(scenario: &ResolvedScenario) { +/// Idempotent: a track is re-analysed only when its file changed on disk or +/// the fps did. Returns the tracks that could not be analysed — an empty vec +/// means every track in the scenario now has an entry in the cache. +pub fn analyze_scenario_audio(scenario: &ResolvedScenario) -> Vec { + let mut failures = Vec::new(); let tracks = &scenario.audio; if tracks.is_empty() { - return; + return failures; } let fps = scenario.video.fps; let cache = audio_analysis_cache(); + let fps_of = fingerprints(); let band_bounds = band_boundaries(); let hann = hann_window(FFT_SIZE); let mut planner = FftPlanner::::new(); @@ -43,14 +93,29 @@ pub fn analyze_scenario_audio(scenario: &ResolvedScenario) { for track in tracks { let src = &track.src; - if cache.contains_key(src) { + let fingerprint = source_fingerprint(src, fps); + let cached_and_current = cache.contains_key(src) + && fingerprint.is_some() + && fps_of + .lock() + .unwrap_or_else(|e| e.into_inner()) + .get(src) + .copied() + == fingerprint; + if cached_and_current { continue; } // Decode to PCM f32 let (samples, sample_rate, channels) = match crate::encode::audio::decode_audio_file(src) { Ok(v) => v, - Err(_) => continue, // graceful degradation: skip undecodable track + Err(e) => { + failures.push(AudioAnalysisFailure { + src: src.clone(), + reason: e.to_string(), + }); + continue; + } }; // Downmix to mono @@ -145,5 +210,18 @@ pub fn analyze_scenario_audio(scenario: &ResolvedScenario) { bands: bands_all, }); cache.insert(src.clone(), analysis); + let mut fps_of = fps_of.lock().unwrap_or_else(|e| e.into_inner()); + match fingerprint { + Some(fp) => { + fps_of.insert(src.clone(), fp); + } + // Un-stat'able but decodable: don't record a fingerprint, so the + // next call re-analyses rather than trusting an entry it cannot + // check. + None => { + fps_of.remove(src); + } + } } + failures } diff --git a/crates/rustmotion/src/encode/video/ffmpeg.rs b/crates/rustmotion/src/encode/video/ffmpeg.rs index 567f1aaa..b91208c6 100644 --- a/crates/rustmotion/src/encode/video/ffmpeg.rs +++ b/crates/rustmotion/src/encode/video/ffmpeg.rs @@ -401,7 +401,9 @@ fn encode_with_ffmpeg_hw_impl( for view in &scenario.views { prefetch_icons(&view.scenes); } - analyze_scenario_audio(scenario); + for failure in analyze_scenario_audio(scenario) { + eprintln!("rustmotion: audio-reactive: {failure} — waveform/audio_spectrum will render flat for this track."); + } let (tasks, full_total_frames, segment_start_frame) = match frame_range { Some((start, end)) => { diff --git a/crates/rustmotion/src/encode/video/h264.rs b/crates/rustmotion/src/encode/video/h264.rs index 0a2c0ea9..12a70569 100644 --- a/crates/rustmotion/src/encode/video/h264.rs +++ b/crates/rustmotion/src/encode/video/h264.rs @@ -67,7 +67,9 @@ fn encode_video_impl( preextract_video_frames(&view.scenes, fps); prefetch_icons(&view.scenes); } - analyze_scenario_audio(scenario); + for failure in analyze_scenario_audio(scenario) { + eprintln!("rustmotion: audio-reactive: {failure} — waveform/audio_spectrum will render flat for this track."); + } let (tasks, full_total_frames, segment_start_frame) = match frame_range { Some((start, end)) => { @@ -171,7 +173,9 @@ pub fn encode_video_incremental( preextract_video_frames(&view.scenes, fps); prefetch_icons(&view.scenes); } - analyze_scenario_audio(scenario); + for failure in analyze_scenario_audio(scenario) { + eprintln!("rustmotion: audio-reactive: {failure} — waveform/audio_spectrum will render flat for this track."); + } let num_slots = slots.len(); let scene_hashes: Vec = slots diff --git a/crates/rustmotion/src/tests.rs b/crates/rustmotion/src/tests.rs index c51487dc..f0e6da95 100644 --- a/crates/rustmotion/src/tests.rs +++ b/crates/rustmotion/src/tests.rs @@ -1949,6 +1949,91 @@ mod audio_tests { count } + /// A track that cannot be decoded must be *reported*, not swallowed: + /// silence here leaves `waveform`/`audio_spectrum` on their flat fallback + /// with nothing anywhere saying why. + #[test] + fn analyze_scenario_audio_reports_an_undecodable_track() { + let missing = std::env::temp_dir() + .join(format!("rustmotion_test_absent_{}.wav", nanos())) + .to_str() + .unwrap() + .to_string(); + + let json = serde_json::json!({ + "video": {"width": 32, "height": 32, "fps": 30}, + "audio": [{"src": missing}], + "scenes": [{"duration": 1.0, "children": []}] + }) + .to_string(); + let scenario = + crate::loader::load_scenario_from_source(None, Some(&json)).expect("load scenario"); + + let failures = crate::encode::audio_analysis::analyze_scenario_audio(&scenario); + assert_eq!(failures.len(), 1, "the missing track must be reported"); + assert_eq!(failures[0].src, missing); + assert!( + !failures[0].reason.is_empty(), + "a failure must carry a reason, got {failures:?}" + ); + assert!( + audio_analysis_cache().get(&missing).is_none(), + "a failed decode must not leave an entry behind" + ); + } + + /// The cache is keyed by path, so a track re-exported under the same name + /// used to keep serving the first envelope for the life of the process — + /// exactly what a studio session does when the mix is updated. + #[test] + fn analyze_scenario_audio_reruns_when_the_file_changes() { + let sample_rate = 44100u32; + let wav_path = + std::env::temp_dir().join(format!("rustmotion_test_refresh_{}.wav", nanos())); + let wav_str = wav_path.to_str().unwrap().to_string(); + + // First: a full second of sine — amplitude high throughout. + std::fs::write( + &wav_path, + make_sine_wav(sample_rate, sample_rate, 440.0, sample_rate), + ) + .expect("write first fixture"); + + let json = serde_json::json!({ + "video": {"width": 32, "height": 32, "fps": 30}, + "audio": [{"src": wav_str}], + "scenes": [{"duration": 1.0, "children": []}] + }) + .to_string(); + let scenario = + crate::loader::load_scenario_from_source(None, Some(&json)).expect("load scenario"); + assert!(crate::encode::audio_analysis::analyze_scenario_audio(&scenario).is_empty()); + let late_before = audio_analysis_cache().get(&wav_str).unwrap().amplitude[25]; + + // Rewrite the same path with sine only in the first half. mtime has + // 1 ns resolution on the platforms we target, but the length differs + // too, so the fingerprint changes either way. + std::fs::write( + &wav_path, + make_sine_wav(sample_rate * 2, sample_rate / 2, 440.0, sample_rate), + ) + .expect("write second fixture"); + + assert!(crate::encode::audio_analysis::analyze_scenario_audio(&scenario).is_empty()); + let late_after = audio_analysis_cache().get(&wav_str).unwrap().amplitude[25]; + std::fs::remove_file(&wav_path).ok(); + + assert!( + late_before > 0.5, + "frame 25 of the first take is inside the sine, got {late_before}" + ); + assert!( + late_after < 0.1, + "frame 25 of the second take is silence — a stale analysis would \ + still report {late_before}, got {late_after}" + ); + } + #[test] fn analyze_scenario_audio_computes_amplitude_and_440hz_band() { let sample_rate = 44100u32;