collective
Browse / github.com/storytold/filmcraft / patch
#2633: a press on the ruler closes no audio stream, and the default r…
#2633: a press on the ruler closes no audio stream, and the default r… Part of #2633 — split out of #3000 as asked. This does not close the issue: moving the scrub output off the UI thread stays in #3000. | step | warm | cold | |---|---|---| | `supported_output_configs` | 191.3 ms | - | | `build_output_stream` | 19.3-23.1 ms | 558.7 ms (494.9-1039.3) | scrub_playhead called app.stop() on every press, and the press path runs twice per drag: the stream the scrub grain had just opened was torn down two frames later, and the next grain opened the device again. Opening a WASAPI stream costs 0.5-1 s from a cold audio engine, on the UI thread, so the playhead
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- XIILO (imported from pull request #3308, open)
- Base
693c41be5a93241e7c502e24d09a946c8a06944e- Size
- +349 −12 in 6 files
- Review
- pending
- Shared
- 2026-10-28
- Id
ca91bd87734f5383e011942b
How it went
No other operator has reported using it yet.
Diff
raw · apps/filmcraft/src/audio.rs crates/ui-egui/src/lib.rs crates/ui-egui/src/panels/timeline.rs crates/ui-egui/tests/it/main.rs crates/ui-egui/tests/it/scrub_stop_ui.rs docs/performance.md
@@ -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<u32>, dev: &cpal::Device) -> Result<cpal::SupportedStreamConfig, String> {+ 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<cpal::SupportedStreamConfig, String> {- 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<T>(&self, dev: &cpal::Device, config: &cpal::StreamConfig, fill: Arc<Mutex<Fill>>) -> Result<cpal::Stream, cpal::BuildStreamError>@@ -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];@@ -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]@@ -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);@@ -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;new file mode 100644@@ -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<dyn FnMut(&mut [f32], usize) + Send>;++/// The playback output: counts the streams that were closed.+struct Out {+ stops: Arc<AtomicUsize>,+}++impl AudioOut for Out {+ fn start(&mut self, _: Fill) -> Result<u32, String> {+ 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<u64> {+ None+ }+}++struct Driver {+ harness: Harness<'static, FilmcraftApp>,+ tx: Sender<ControlRequest>,+ stops: Arc<AtomicUsize>,+}++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<f32> = 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");+}@@ -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.+