diff --git a/apps/filmcraft/src/audio.rs b/apps/filmcraft/src/audio.rs --- a/apps/filmcraft/src/audio.rs +++ b/apps/filmcraft/src/audio.rs @@ -60,6 +60,32 @@ impl Note { } } +/// Whether the settings name a sample rate the device does not already run at, so its supported +/// configurations have to be listed. `0` is the settings' "device default". +fn needs_rate_search(default_rate: u32, want: u32) -> bool { + want != 0 && want != default_rate +} + +/// The stream configuration the settings ask for, falling back to the device default. +/// +/// Listing a device's configurations means probing every format and rate in the driver: +/// 150-215 ms on WASAPI (measured), while opening the stream itself costs ~20 ms warm (0.5-1 s +/// from a cold audio engine). So the list is only asked for when the settings name a rate the +/// device does not already run at (#2633). +fn stream_config(hw: &AudioHardwarePrefs, document_rate: Option, dev: &cpal::Device) -> Result { + let default = dev.default_output_config().map_err(|e| e.to_string())?; + let want = if hw.force_document_rate { document_rate.unwrap_or(hw.sample_rate) } else { hw.sample_rate }; + if !needs_rate_search(default.sample_rate().0, want) { + return Ok(default); + } + let exact = dev.supported_output_configs().ok().and_then(|it| { + it.filter(|c| c.channels() > 0 && (c.min_sample_rate().0..=c.max_sample_rate().0).contains(&want)) + .max_by_key(|c| (c.channels() == default.channels(), c.sample_format() == cpal::SampleFormat::F32, c.sample_format() == default.sample_format())) + .and_then(|c| c.try_with_sample_rate(cpal::SampleRate(want))) + }); + Ok(exact.unwrap_or(default)) +} + /// The host named in the settings (empty or unknown = the default host). fn host(name: &str) -> cpal::Host { if !name.is_empty() @@ -157,18 +183,9 @@ impl CpalOut { } } - /// The stream configuration the settings ask for, falling back to the device default. + /// The stream configuration the settings ask for (see [`stream_config`]). fn config(&self, dev: &cpal::Device) -> Result { - let default = dev.default_output_config().map_err(|e| e.to_string())?; - let want = if self.hw.force_document_rate { self.document_rate.unwrap_or(self.hw.sample_rate) } else { self.hw.sample_rate }; - let exact = dev.supported_output_configs().ok().and_then(|it| { - it.filter(|c| c.channels() > 0 && (c.min_sample_rate().0..=c.max_sample_rate().0).contains(&want)) - .max_by_key(|c| { - (c.channels() == default.channels(), c.sample_format() == cpal::SampleFormat::F32, c.sample_format() == default.sample_format()) - }) - .and_then(|c| c.try_with_sample_rate(cpal::SampleRate(want))) - }); - Ok(exact.unwrap_or(default)) + stream_config(&self.hw, self.document_rate, dev) } fn converted_stream(&self, dev: &cpal::Device, config: &cpal::StreamConfig, fill: Arc>) -> Result @@ -471,6 +488,15 @@ mod tests { assert!(!Note::NoDevice.text().is_empty()); } + /// #2633: the rate search costs ~190 ms on WASAPI, so the device default is taken as is. + #[test] + fn the_device_default_rate_is_used_without_listing_every_configuration() { + assert!(!needs_rate_search(48_000, 48_000), "the settings ask for the rate the device runs at"); + assert!(!needs_rate_search(44_100, 0), "0 is the device default"); + assert!(needs_rate_search(48_000, 44_100), "another rate has to be searched for"); + assert!(needs_rate_search(48_000, 96_000)); + } + #[test] fn integer_conversion_clips_and_preserves_interleaved_channels() { let mut signed = [0_i16; 8]; diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -1060,6 +1060,22 @@ impl FilmcraftApp { Ok(seconds) } + /// Whether [`FilmcraftApp::stop`] has a transport to stop: Program or Source playback, a + /// preroll, or a recording that runs one. A press that only moves the playhead uses this so it + /// does not close an audio stream that is already open (#2633): dropping the output stream + /// makes the next scrub grain open the device again, and on WASAPI that open is slow. + pub fn transport_running(&self) -> bool { + self.playback.playing + // A panel that panicked clears `playing` but leaves the output playing, and the clock + // stays on: the press still has that stream to stop. + || self.playback.audio_clock + || self.source_playback.clock.playing + || self.playback.preroll.is_some() + || self.session.mixrec.active() + || self.session.mcrec.active() + || self.session.voiceover.recording() + } + pub fn stop(&mut self) { self.playback.playing = false; self.playback.stop_at = None; @@ -2564,6 +2580,50 @@ mod audio_recovery_tests { app.stop(); } + /// #2633: a press that only moves the playhead has nothing to stop, so it must not close the + /// audio stream (closing it makes the next scrub grain open the device again). + #[test] + fn only_a_running_transport_has_something_to_stop() { + let mut session = Session::default(); + session.execute("file.newSequence", json!({"width": 16, "height": 16})).unwrap(); + let mut app = FilmcraftApp::new(session); + assert!(!app.transport_running(), "nothing runs in a new project"); + app.playback.playing = true; + assert!(app.transport_running(), "Program playback runs a transport"); + app.playback.playing = false; + app.source_playback.clock.playing = true; + assert!(app.transport_running(), "Source playback runs one"); + app.source_playback.clock.playing = false; + app.playback.preroll = Some(0.0); + assert!(app.transport_running(), "a preroll runs one"); + app.playback.preroll = None; + assert!(!app.transport_running()); + app.playback.audio_clock = true; + assert!(app.transport_running(), "an output a panicked frame left playing is stopped too"); + app.playback.audio_clock = false; + assert!(!app.transport_running()); + // A recording pass runs a transport too, and `stop()` ends it. + let seq = app.session.state.active_sequence.unwrap(); + app.session.mixrec.seq = Some(seq); + assert!(app.transport_running(), "an automation pass runs one"); + app.session.mixrec.seq = None; + app.session.mcrec.seq = Some(seq); + assert!(app.transport_running(), "a multicam pass runs one"); + app.session.mcrec.seq = None; + app.session.voiceover.rec = Some(filmcraft_engine::voiceover::Recording { + seq, + track: filmcraft_project::TrackId(0), + record_start: filmcraft_time::Tick::ZERO, + capture_start: filmcraft_time::Tick::ZERO, + punch_out: None, + format: filmcraft_engine::voiceover::InputFormat { sample_rate: 48_000, channels: 1 }, + channel: 0, + }); + assert!(app.transport_running(), "a voice-over take runs one"); + app.session.voiceover.rec = None; + assert!(!app.transport_running()); + } + /// Capture the block primed by the real shuttle playback path, without relying on a /// physical device or consuming the producer faster than a device would. #[test] diff --git a/crates/ui-egui/src/panels/timeline.rs b/crates/ui-egui/src/panels/timeline.rs --- a/crates/ui-egui/src/panels/timeline.rs +++ b/crates/ui-egui/src/panels/timeline.rs @@ -2397,7 +2397,15 @@ pub(crate) fn wheel_input(ctx: &egui::Context) -> WheelInput { /// Puts the playhead where the pointer is on the ruler: what a press does before any drag (#1090). fn scrub_playhead(app: &mut FilmcraftApp, seq: &Sequence, layout: &Layout, p: Pos2, shift: bool) { - app.stop(); + // A press on the ruler stops playback, but it must not close a scrub stream that is already + // open: `stop()` drops the output stream, and the grain a frame later has to open the device + // again (slow on WASAPI, #2633). A press while nothing runs has nothing to stop. + // The Source monitor runs on its own clock and `stop()` leaves it alone, so it stops first; + // the two checks stay separate because a press can have stops for both. + app.stop_source(); + if app.transport_running() { + app.stop(); + } let mut tt = seq.settings.frame_rate.snap_nearest(layout.tick_at(p.x).max(Tick::ZERO)); if shift || app.session.prefs.timeline.snap_playhead { tt = snap_playhead(app, seq, layout, tt); diff --git a/crates/ui-egui/tests/it/main.rs b/crates/ui-egui/tests/it/main.rs --- a/crates/ui-egui/tests/it/main.rs +++ b/crates/ui-egui/tests/it/main.rs @@ -50,6 +50,7 @@ mod project_panel_ui; mod properties_scroll_ui; mod remix_ui; mod scripted; +mod scrub_stop_ui; mod sequence_tabs_ui; mod sequence_ui; mod settings_ui; diff --git a/crates/ui-egui/tests/it/scrub_stop_ui.rs b/crates/ui-egui/tests/it/scrub_stop_ui.rs new file mode 100644 --- /dev/null +++ b/crates/ui-egui/tests/it/scrub_stop_ui.rs @@ -0,0 +1,193 @@ +//! #2633: a press on the Timeline ruler that only moves the playhead must not close the audio +//! output. +//! +//! `scrub_playhead` called `app.stop()` on every press, and `stop()` closes the output stream. The +//! press runs twice in one drag, so the grain the scrub had just started lost its stream two frames +//! later, and the next grain opened the device again: 0.5-1 s on WASAPI, on the UI thread, which +//! froze the playhead at the start of every click and drag. The press now stops only a running +//! transport. + +use std::sync::Arc; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::mpsc::{Sender, channel}; + +use egui_kittest::Harness; +use filmcraft_engine::Session; +use filmcraft_ui_egui::control::ControlRequest; +use filmcraft_ui_egui::{AudioOut, FilmcraftApp}; +use serde_json::{Value, json}; + +type Fill = Box; + +/// The playback output: counts the streams that were closed. +struct Out { + stops: Arc, +} + +impl AudioOut for Out { + fn start(&mut self, _: Fill) -> Result { + Ok(48_000) + } + fn stop(&mut self) { + self.stops.fetch_add(1, Ordering::Relaxed); + } + fn sample_rate(&self) -> u32 { + 48_000 + } + fn played_frames(&self) -> Option { + None + } +} + +struct Driver { + harness: Harness<'static, FilmcraftApp>, + tx: Sender, + stops: Arc, +} + +impl Driver { + /// An empty 3V/3A sequence with scrub audio on, and an output that counts its stops. + fn empty_sequence() -> Self { + let mut session = Session::default(); + session.execute("file.newProject", json!({"name": "press"})).expect("project"); + session.execute("file.newSequence", json!({"name": "Press", "video": 3, "audio": 3})).expect("sequence"); + session.prefs.audio.scrub_audio = true; + let (tx, rx) = channel(); + let mut app = FilmcraftApp::new(session).with_control(rx); + let stops = Arc::new(AtomicUsize::new(0)); + app.audio = Some(Box::new(Out { stops: stops.clone() })); + let harness = Harness::builder().with_size(egui::vec2(1600.0, 980.0)).with_step_dt(1.0 / 60.0).with_max_steps(10_000).build_eframe(move |_cc| app); + let mut d = Self { harness, tx, stops }; + d.frames(4); + // The first frame applies Settings ▸ Audio Hardware, which closes the output once: these + // tests count the stops a press causes, not that one. + d.stops.store(0, Ordering::Relaxed); + d + } + + fn frames(&mut self, n: usize) { + for _ in 0..n { + let ctx = self.harness.ctx.clone(); + let mut raw = std::mem::take(self.harness.input_mut()); + eframe::App::raw_input_hook(self.harness.state_mut(), &ctx, &mut raw); + *self.harness.input_mut() = raw; + self.harness.step(); + } + } + + fn ok(&mut self, method: &str, params: Value) -> Value { + let (req, reply) = ControlRequest::new(method, params.clone()); + self.tx.send(req).unwrap(); + for _ in 0..600 { + self.frames(1); + if let Ok(v) = reply.try_recv() { + assert_eq!(v["ok"], json!(true), "{method} {params} failed: {v}"); + return v["result"].clone(); + } + } + panic!("no reply to {method} {params}"); + } + + fn rect(&mut self, id: &str) -> egui::Rect { + let v = self.ok("ui.elements", json!({"prefix": id})); + let e = v.as_array().unwrap().iter().find(|e| e["id"] == id).unwrap_or_else(|| panic!("no element {id}")).clone(); + let r: Vec = e["rect"].as_array().unwrap().iter().map(|x| x.as_f64().unwrap() as f32).collect(); + egui::Rect::from_min_size(egui::pos2(r[0], r[1]), egui::vec2(r[2], r[3])) + } + + fn send(&mut self, e: egui::Event) { + self.harness.input_mut().events.push(e); + self.frames(1); + } + + /// Press and release on the Timeline ruler without moving the pointer: the press still puts the + /// playhead where the pointer is, and starts no grain. + fn click_ruler(&mut self) { + let r = self.rect("timeline.ruler"); + let pos = egui::pos2(r.min.x + 40.0, r.center().y); + self.send(egui::Event::PointerMoved(pos)); + self.send(egui::Event::PointerButton { pos, button: egui::PointerButton::Primary, pressed: true, modifiers: egui::Modifiers::NONE }); + self.send(egui::Event::PointerButton { pos, button: egui::PointerButton::Primary, pressed: false, modifiers: egui::Modifiers::NONE }); + self.frames(2); + } + + fn stops(&self) -> usize { + self.stops.load(Ordering::Relaxed) + } + + /// Puts a four-second clip in a new sequence and returns its media item. An empty sequence has + /// no duration: a press could arrive after the transport had already run into its end and + /// stopped by itself, and the Source monitor needs an item to open. + fn with_contents(&mut self) -> u64 { + let session = &mut self.harness.state_mut().session; + let r = session.execute("file.newColorMatte", json!({"color": "#3366ff", "width": 144, "height": 108, "seconds": 4.0})).unwrap(); + let item = r["item"].as_u64().unwrap(); + session.execute("file.newSequenceFromClip", json!({"items": [item]})).unwrap(); + self.frames(2); + item + } + + /// Runs playback until the preroll is over, so the transport is really moving, not waiting for + /// its first frames. + fn until_playing(&mut self) { + for _ in 0..120 { + self.frames(1); + if self.harness.state().playback.preroll.is_none() { + return; + } + } + panic!("the preroll did not end"); + } +} + +/// #2633: with nothing playing there is nothing for the press to stop, so the output stream the +/// scrub is using stays open. +#[test] +fn a_press_on_the_ruler_while_stopped_does_not_close_the_audio_output() { + let mut d = Driver::empty_sequence(); + // A drag opens the scrub stream; the press that follows must leave it open, or the next grain + // has to open the device again (the freeze this fixes). + let r = d.rect("timeline.ruler"); + let from = egui::pos2(r.min.x + 100.0, r.center().y); + let moved = egui::pos2(r.min.x + 120.0, r.center().y); + d.send(egui::Event::PointerMoved(from)); + d.send(egui::Event::PointerButton { pos: from, button: egui::PointerButton::Primary, pressed: true, modifiers: egui::Modifiers::NONE }); + d.send(egui::Event::PointerMoved(moved)); + assert!(d.harness.state().scrub.is_open(), "the drag leaves its stream open"); + d.send(egui::Event::PointerButton { pos: moved, button: egui::PointerButton::Primary, pressed: false, modifiers: egui::Modifiers::NONE }); + assert!(d.harness.state().scrub.is_open(), "the stream lingers after the release"); + let stops = d.stops(); + let before = d.harness.state().session.playhead(); + d.click_ruler(); + assert_ne!(d.harness.state().session.playhead(), before, "the press moved the playhead"); + assert_eq!(d.stops(), stops, "nothing was playing, so the press had nothing to stop"); + assert!(d.harness.state().scrub.is_open(), "the open stream survived the press"); +} + +/// The press still stops a running transport: that is what `stop()` is for. +#[test] +fn a_press_on_the_ruler_while_playing_stops_the_transport() { + let mut d = Driver::empty_sequence(); + d.with_contents(); + d.harness.state_mut().play(1.0); + d.until_playing(); + assert!(d.harness.state().playback.playing, "playback is still running before the press"); + let before = d.stops(); + d.click_ruler(); + assert!(!d.harness.state().playback.playing, "the press stopped playback"); + assert!(d.stops() > before, "the press closed the output the transport was using"); +} + +/// #2633: `transport_running()` counts the Source monitor, so the press must stop it too; the +/// program `stop()` leaves its clock alone. +#[test] +fn a_press_on_the_ruler_while_the_source_plays_stops_it() { + let mut d = Driver::empty_sequence(); + let item = d.with_contents(); + d.harness.state_mut().session.execute("source.open", json!({"item": item})).unwrap(); + d.harness.state_mut().play_source().unwrap(); + d.frames(1); + assert!(d.harness.state().source_playback.clock.playing, "the Source monitor is playing"); + d.click_ruler(); + assert!(!d.harness.state().source_playback.clock.playing, "the press stopped the Source monitor"); +} diff --git a/docs/performance.md b/docs/performance.md --- a/docs/performance.md +++ b/docs/performance.md @@ -905,3 +905,52 @@ decode 2642 / 2925 → 2506 / 2064; playback 5246 / 7903 → 3379 / 3121; scrub Not feasible / not done: reduced-resolution decode for H.264 / HEVC (inter prediction needs full-resolution references, so it cannot be bit-exact; ½/¼ playback already decimates after decode); AV1 internals untouched (measured only); export encoders not optimised. + +## Results (#2633: a playhead press re-opened the audio device, Windows) + +Reported on Windows: the Timeline playhead felt laggy and jumped on every click and drag, on an +empty sequence too. The Timeline UI is not the cost (~3 ms a pass, below); the stalls were audio +device opens on the UI thread. + +`ScrubAudio` plays a grain while the playhead moves with a button held (default on, +`audio.scrubAudio`), and `scrub_playhead` called `app.stop()` on every press. The press path runs +twice in one drag, so the stream the first grain had just opened was torn down two frames later and +opened again. `CpalOut::config` also listed `supported_output_configs()` on every open, even when +the settings ask for the rate the device already runs at. + +Measured on this machine (Realtek WASAPI, cpal 0.16). "Warm" is a stream built while another one is +already playing; "cold" is the first stream after the audio engine went idle, which is the normal +case between two gestures: + +| step | warm | cold | +|---|---|---| +| `default_output_device` | 2.7 ms | 3.6 ms | +| `default_output_config` | 1.1 ms | 0.9 ms | +| `supported_output_configs` | 191.3 ms | - | +| `build_output_stream` | 19.3-23.1 ms | **558.7 ms** (494.9-1039.3) | + +The configuration list was measured once, warm: it is the driver enumerating what it supports, not +the audio engine starting. A cold drag's ~1.5 s of freeze is 2 x (558.7 + 191.3) ms, so the list +costs the same there. + +So caching the device or its configuration buys nothing: the cost is `IAudioClient::Initialize` +starting the Windows audio engine, and it is paid once per stream. + +Timeline UI pass, empty 3V/3A sequence at 1600x980 (`egui_kittest`): idle p50 2.2 ms / max 3.6; a +120-step playhead drag p50 3.1 ms / max 6.9. The whole frame is cheap, so the device open was the +only stall. + +Fixed: + +- the press path opened the device twice per drag (once, then `app.stop()` closed it two frames + later and the next grain opened it again): the press stops a transport only when one runs; +- `config()` listed every supported configuration (~190 ms) even when the settings ask for the rate + the device already runs at: the device default is taken as is then. + +A cold drag went from ~1.5 s of frozen playhead (two opens of 558 + 191 ms) to ~0.6 s (one open of +558 ms). That last open is still on the UI thread; moving it off is the next step of #2633. + +Test: `crates/ui-egui/tests/it/scrub_stop_ui.rs` — a press that only moves the playhead closes no +stream, and a press while playing still stops the transport. `apps/filmcraft` (`audio::tests`) — +the device default rate needs no configuration search. +