From 37aa68fc34edd5652924722fb917789e838342b1 Mon Sep 17 00:00:00 2001 From: shitobu Date: Wed, 11 Nov 2026 16:38:04 -0600 Subject: [PATCH 1/2] perf(recording): reduce in-recording memory pressure and sub-pixel cursor jitter - Deduplicate sub-pixel cursor move jitter via CursorActorResponse::push_move_deduped (MIN_CURSOR_MOVE_DELTA_SQ = 1e-7), preventing unbounded event allocations on high-polling mice and long recordings while preserving shape-change sensitivity. - Eliminate full-history cursor string allocations at finalization by streaming borrowed CursorEventsRef directly via write_atomically_with and BufWriter. - Add CAP_MP4_MUXER_BUFFER_SIZE_INSTANT env override with thread-safe test synchronization to prevent test races under parallel test runners. - Add unit tests for cursor deduplication, sensor jitter benchmark, and instant muxer buffer sizing. --- crates/recording/src/cursor.rs | 162 ++++++++++++++++-- crates/recording/src/output_pipeline/macos.rs | 46 +++++ crates/recording/src/studio_recording.rs | 25 ++- 3 files changed, 213 insertions(+), 20 deletions(-) diff --git a/crates/recording/src/cursor.rs b/crates/recording/src/cursor.rs index e8aa1c71aed..9a166b0b1b5 100644 --- a/crates/recording/src/cursor.rs +++ b/crates/recording/src/cursor.rs @@ -35,6 +35,31 @@ pub struct CursorActorResponse { pub keyboard_presses: Vec, } +/// Minimum squared delta in normalized coordinates (~0.316 sub-pixel on 1080p) +/// required to push a new cursor move event, preventing unbounded heap growth from +/// sub-pixel sensor noise and high-polling mice during long recordings. +pub const MIN_CURSOR_MOVE_DELTA_SQ: f64 = 1e-7; + +impl CursorActorResponse { + pub fn push_move_deduped(&mut self, event: CursorMoveEvent) -> bool { + let should_push = match self.moves.last() { + Some(last) => { + let dx = last.x - event.x; + let dy = last.y - event.y; + (dx * dx + dy * dy) >= MIN_CURSOR_MOVE_DELTA_SQ || last.cursor_id != event.cursor_id + } + None => true, + }; + + if should_push { + self.moves.push(event); + true + } else { + false + } + } +} + pub struct CursorActor { stop: Option, stop_wakeup: Option>, @@ -237,28 +262,34 @@ fn prefers_wayland_portal_cursor() -> bool { .is_ok_and(|session| session.eq_ignore_ascii_case("wayland")) } -/// Replaces `path` with `parts` through a temporary file and a rename, so a -/// crash or kill mid-write leaves the previous complete file for recovery. A -/// refused rename, such as a reader holding the file open on Windows, falls -/// back to writing in place as before. -pub(crate) fn write_atomically(path: &Path, parts: &[&[u8]]) -> std::io::Result<()> { - use std::io::Write; - - let write = |path: &Path| { - let mut file = std::fs::File::create(path)?; - parts.iter().try_for_each(|part| file.write_all(part)) - }; +/// Replaces `path` by running `write_fn` against a temporary file and renaming +/// it over `path`, so a crash or kill mid-write leaves the previous complete +/// file for recovery. A refused rename, such as a reader holding the file open +/// on Windows, falls back to writing in place as before. +pub(crate) fn write_atomically_with( + path: &Path, + write_fn: impl Fn(&Path) -> std::io::Result<()>, +) -> std::io::Result<()> { let mut temp_path = path.as_os_str().to_owned(); temp_path.push(".tmp"); let temp_path = PathBuf::from(temp_path); - if let Err(error) = write(&temp_path) { + if let Err(error) = write_fn(&temp_path) { let _ = std::fs::remove_file(&temp_path); return Err(error); } std::fs::rename(&temp_path, path).or_else(|error| { let _ = std::fs::remove_file(&temp_path); tracing::warn!(path = %path.display(), %error, "Atomic replace failed, writing in place"); - write(path) + write_fn(path) + }) +} + +/// Replaces `path` with `parts` through a temporary file and a rename. +pub(crate) fn write_atomically(path: &Path, parts: &[&[u8]]) -> std::io::Result<()> { + use std::io::Write; + write_atomically_with(path, |target| { + let mut file = std::fs::File::create(target)?; + parts.iter().try_for_each(|part| file.write_all(part)) }) } @@ -882,7 +913,7 @@ pub fn spawn_cursor_recorder( x, y, }; - response.moves.push(mouse_event); + response.push_move_deduped(mouse_event); } } @@ -2413,4 +2444,107 @@ mod cursor_image_tests { assert_eq!(response.cursors.len(), 1); assert!(cursor_path.is_file()); } + + #[test] + fn cursor_subpixel_moves_are_deduplicated() { + let mut response = empty_response(); + + // Initial position + let pushed_first = response.push_move_deduped(CursorMoveEvent { + active_modifiers: vec![], + cursor_id: "0".to_string(), + time_ms: 0.0, + x: 0.5, + y: 0.5, + }); + assert!(pushed_first); + assert_eq!(response.moves.len(), 1); + + // Subpixel jitter (< 1e-7 squared distance: ~0.0001 distance) + let pushed_jitter = response.push_move_deduped(CursorMoveEvent { + active_modifiers: vec![], + cursor_id: "0".to_string(), + time_ms: 16.0, + x: 0.50005, + y: 0.50005, + }); + assert!(!pushed_jitter, "Sub-pixel jitter should be deduplicated"); + assert_eq!(response.moves.len(), 1); + + // Meaningful move (> 1e-7 squared distance: ~0.005 distance) + let pushed_move = response.push_move_deduped(CursorMoveEvent { + active_modifiers: vec![], + cursor_id: "0".to_string(), + time_ms: 32.0, + x: 0.505, + y: 0.505, + }); + assert!(pushed_move, "Meaningful move should be recorded"); + assert_eq!(response.moves.len(), 2); + + // Cursor shape change triggers move event even if position is unchanged + let pushed_shape_change = response.push_move_deduped(CursorMoveEvent { + active_modifiers: vec![], + cursor_id: "1".to_string(), + time_ms: 48.0, + x: 0.505, + y: 0.505, + }); + assert!( + pushed_shape_change, + "Cursor ID/shape change must force a move event" + ); + assert_eq!(response.moves.len(), 3); + } + + #[test] + fn benchmark_cursor_deduplication_reduces_event_count_under_sensor_jitter() { + let mut response = empty_response(); + let total_samples = 10_000; + let mut raw_events_count = 0; + + // Simulate 10,000 samples of high-polling cursor with optical micro-jitter + // interspersed with genuine movements. + let mut current_x = 0.5f64; + let mut current_y = 0.5f64; + + for i in 0..total_samples { + raw_events_count += 1; + // Every 100 ticks, introduce genuine cursor motion; otherwise optical subpixel noise + if i % 100 == 0 { + current_x += 0.005; + current_y += 0.005; + } else { + // Jitter around current position (0.00005) + let jitter = ((i % 5) as f64 - 2.0) * 0.00002; + current_x += jitter; + current_y += jitter; + } + + response.push_move_deduped(CursorMoveEvent { + active_modifiers: vec![], + cursor_id: "0".to_string(), + time_ms: i as f64 * 16.6, + x: current_x, + y: current_y, + }); + } + + let deduped_count = response.moves.len(); + let reduction_pct = (1.0 - (deduped_count as f64 / raw_events_count as f64)) * 100.0; + + println!( + "\n=== Cursor Deduplication Benchmark ===\n\ + Raw input events: {}\n\ + Retained dedup events: {}\n\ + Reduction: {:.1}%\n\ + ======================================\n", + raw_events_count, deduped_count, reduction_pct + ); + + assert!( + reduction_pct >= 80.0, + "Expected at least 80% reduction under jitter, got {reduction_pct:.1}% ({deduped_count}/{raw_events_count})" + ); + } } diff --git a/crates/recording/src/output_pipeline/macos.rs b/crates/recording/src/output_pipeline/macos.rs index 0fd2e282a9b..2633681a6e4 100644 --- a/crates/recording/src/output_pipeline/macos.rs +++ b/crates/recording/src/output_pipeline/macos.rs @@ -64,6 +64,14 @@ fn check_disk_space_to_start(output_path: &std::path::Path) -> anyhow::Result<() } fn get_mp4_muxer_buffer_size(instant_mode: bool) -> usize { + if instant_mode { + if let Some(instant_val) = std::env::var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT") + .ok() + .and_then(|s| s.parse().ok()) + { + return instant_val; + } + } std::env::var("CAP_MP4_MUXER_BUFFER_SIZE") .ok() .and_then(|s| s.parse().ok()) @@ -1547,6 +1555,8 @@ mod tests { mod mp4_muxer_buffer_size { use super::*; + static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + #[test] fn instant_mode_buffer_is_larger_than_normal() { let instant = DEFAULT_MP4_MUXER_BUFFER_SIZE_INSTANT; @@ -1569,6 +1579,7 @@ mod tests { #[test] fn env_override_takes_precedence() { + let _guard = ENV_LOCK.lock().unwrap(); unsafe { std::env::set_var("CAP_MP4_MUXER_BUFFER_SIZE", "500"); } @@ -1583,6 +1594,7 @@ mod tests { #[test] fn invalid_env_falls_back_to_defaults() { + let _guard = ENV_LOCK.lock().unwrap(); unsafe { std::env::set_var("CAP_MP4_MUXER_BUFFER_SIZE", "not_a_number"); } @@ -1594,6 +1606,40 @@ mod tests { assert_eq!(normal, DEFAULT_MP4_MUXER_BUFFER_SIZE); assert_eq!(instant, DEFAULT_MP4_MUXER_BUFFER_SIZE_INSTANT); } + + #[test] + fn instant_env_override_takes_precedence_over_global_override() { + let _guard = ENV_LOCK.lock().unwrap(); + unsafe { + std::env::set_var("CAP_MP4_MUXER_BUFFER_SIZE", "500"); + std::env::set_var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT", "120"); + } + let normal = get_mp4_muxer_buffer_size(false); + let instant = get_mp4_muxer_buffer_size(true); + unsafe { + std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE"); + std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT"); + } + assert_eq!(normal, 500); + assert_eq!(instant, 120); + } + + #[test] + fn invalid_instant_override_falls_back_to_global_override() { + let _guard = ENV_LOCK.lock().unwrap(); + unsafe { + std::env::set_var("CAP_MP4_MUXER_BUFFER_SIZE", "300"); + std::env::set_var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT", "invalid"); + } + let normal = get_mp4_muxer_buffer_size(false); + let instant = get_mp4_muxer_buffer_size(true); + unsafe { + std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE"); + std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT"); + } + assert_eq!(normal, 300); + assert_eq!(instant, 300); + } } mod channel_pressure_tracker { diff --git a/crates/recording/src/studio_recording.rs b/crates/recording/src/studio_recording.rs index be6d6cfc74a..83dad77eb4d 100644 --- a/crates/recording/src/studio_recording.rs +++ b/crates/recording/src/studio_recording.rs @@ -141,14 +141,27 @@ fn persist_segment_input_events( failure: &mut Option, ) -> (Cursors, u32) { if let Some(path) = cursor_path { - let result = serde_json::to_string_pretty(&CursorEvents { - clicks: response.clicks, - moves: response.moves, + #[derive(serde::Serialize)] + struct CursorEventsRef<'a> { + clicks: &'a [CursorClickEvent], + moves: &'a [CursorMoveEvent], + } + + let result = crate::cursor::write_atomically_with(path, |target| { + use std::io::Write; + let file = std::fs::File::create(target)?; + let mut writer = std::io::BufWriter::new(file); + serde_json::to_writer( + &mut writer, + &CursorEventsRef { + clicks: &response.clicks, + moves: &response.moves, + }, + ) + .map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e))?; + writer.flush() }) .map_err(anyhow::Error::from) - .and_then(|events| { - crate::cursor::write_atomically(path, &[events.as_bytes()]).map_err(anyhow::Error::from) - }) .with_context(|| format!("Could not save cursor events to {}", path.display())); accumulate_finalization_error(failure, result); } From d1c87350d84f146c98f677f1c7ae8711c47cf1ff Mon Sep 17 00:00:00 2001 From: shitobu Date: Wed, 11 Nov 2026 21:19:12 -0600 Subject: [PATCH 2/2] fix(recording): address review feedback on imports, formatting, clippy, and threshold - Import cap_project::{CursorClickEvent, CursorMoveEvent} in studio_recording.rs. - Use serde_json::to_writer_pretty in persist_segment_input_events for byte-for-byte test compatibility with to_string_pretty. - Flatten nested if in macos.rs get_mp4_muxer_buffer_size to satisfy clippy collapsible_if. - Fine-tune MIN_CURSOR_MOVE_DELTA_SQ to 1e-8 to preserve 1px moves on 4K+ displays. - Add EnvVarReset RAII helper in macos.rs tests to restore inherited environment variables. - Remove obvious code-narrating comments from tests. --- crates/recording/src/cursor.rs | 39 ++++------ crates/recording/src/output_pipeline/macos.rs | 71 +++++++++++++------ crates/recording/src/studio_recording.rs | 6 +- 3 files changed, 65 insertions(+), 51 deletions(-) diff --git a/crates/recording/src/cursor.rs b/crates/recording/src/cursor.rs index 9a166b0b1b5..68bb35d1522 100644 --- a/crates/recording/src/cursor.rs +++ b/crates/recording/src/cursor.rs @@ -35,10 +35,10 @@ pub struct CursorActorResponse { pub keyboard_presses: Vec, } -/// Minimum squared delta in normalized coordinates (~0.316 sub-pixel on 1080p) -/// required to push a new cursor move event, preventing unbounded heap growth from -/// sub-pixel sensor noise and high-polling mice during long recordings. -pub const MIN_CURSOR_MOVE_DELTA_SQ: f64 = 1e-7; +/// Minimum squared delta in normalized coordinates required to push a new cursor +/// move event, preventing unbounded heap growth from sub-pixel sensor noise while +/// preserving 1-pixel moves on 4K+ displays (1px on 4K is ~6.78e-8). +pub const MIN_CURSOR_MOVE_DELTA_SQ: f64 = 1e-8; impl CursorActorResponse { pub fn push_move_deduped(&mut self, event: CursorMoveEvent) -> bool { @@ -2449,7 +2449,6 @@ mod cursor_image_tests { fn cursor_subpixel_moves_are_deduplicated() { let mut response = empty_response(); - // Initial position let pushed_first = response.push_move_deduped(CursorMoveEvent { active_modifiers: vec![], cursor_id: "0".to_string(), @@ -2460,40 +2459,34 @@ mod cursor_image_tests { assert!(pushed_first); assert_eq!(response.moves.len(), 1); - // Subpixel jitter (< 1e-7 squared distance: ~0.0001 distance) let pushed_jitter = response.push_move_deduped(CursorMoveEvent { active_modifiers: vec![], cursor_id: "0".to_string(), time_ms: 16.0, - x: 0.50005, - y: 0.50005, + x: 0.50002, + y: 0.50002, }); - assert!(!pushed_jitter, "Sub-pixel jitter should be deduplicated"); + assert!(!pushed_jitter); assert_eq!(response.moves.len(), 1); - // Meaningful move (> 1e-7 squared distance: ~0.005 distance) let pushed_move = response.push_move_deduped(CursorMoveEvent { active_modifiers: vec![], cursor_id: "0".to_string(), time_ms: 32.0, - x: 0.505, - y: 0.505, + x: 0.501, + y: 0.501, }); - assert!(pushed_move, "Meaningful move should be recorded"); + assert!(pushed_move); assert_eq!(response.moves.len(), 2); - // Cursor shape change triggers move event even if position is unchanged let pushed_shape_change = response.push_move_deduped(CursorMoveEvent { active_modifiers: vec![], cursor_id: "1".to_string(), time_ms: 48.0, - x: 0.505, - y: 0.505, + x: 0.501, + y: 0.501, }); - assert!( - pushed_shape_change, - "Cursor ID/shape change must force a move event" - ); + assert!(pushed_shape_change); assert_eq!(response.moves.len(), 3); } @@ -2503,20 +2496,16 @@ mod cursor_image_tests { let total_samples = 10_000; let mut raw_events_count = 0; - // Simulate 10,000 samples of high-polling cursor with optical micro-jitter - // interspersed with genuine movements. let mut current_x = 0.5f64; let mut current_y = 0.5f64; for i in 0..total_samples { raw_events_count += 1; - // Every 100 ticks, introduce genuine cursor motion; otherwise optical subpixel noise if i % 100 == 0 { current_x += 0.005; current_y += 0.005; } else { - // Jitter around current position (0.00005) - let jitter = ((i % 5) as f64 - 2.0) * 0.00002; + let jitter = ((i % 5) as f64 - 2.0) * 0.00001; current_x += jitter; current_y += jitter; } diff --git a/crates/recording/src/output_pipeline/macos.rs b/crates/recording/src/output_pipeline/macos.rs index 2633681a6e4..c931c600beb 100644 --- a/crates/recording/src/output_pipeline/macos.rs +++ b/crates/recording/src/output_pipeline/macos.rs @@ -64,13 +64,12 @@ fn check_disk_space_to_start(output_path: &std::path::Path) -> anyhow::Result<() } fn get_mp4_muxer_buffer_size(instant_mode: bool) -> usize { - if instant_mode { - if let Some(instant_val) = std::env::var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT") + if instant_mode + && let Some(instant_val) = std::env::var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT") .ok() .and_then(|s| s.parse().ok()) - { - return instant_val; - } + { + return instant_val; } std::env::var("CAP_MP4_MUXER_BUFFER_SIZE") .ok() @@ -1557,6 +1556,46 @@ mod tests { static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + struct EnvVarReset { + _guard: std::sync::MutexGuard<'static, ()>, + prev_global: Option, + prev_instant: Option, + } + + impl EnvVarReset { + fn new() -> Self { + let guard = ENV_LOCK.lock().unwrap(); + let prev_global = std::env::var_os("CAP_MP4_MUXER_BUFFER_SIZE"); + let prev_instant = std::env::var_os("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT"); + unsafe { + std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE"); + std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT"); + } + Self { + _guard: guard, + prev_global, + prev_instant, + } + } + } + + impl Drop for EnvVarReset { + fn drop(&mut self) { + unsafe { + if let Some(ref val) = self.prev_global { + std::env::set_var("CAP_MP4_MUXER_BUFFER_SIZE", val); + } else { + std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE"); + } + if let Some(ref val) = self.prev_instant { + std::env::set_var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT", val); + } else { + std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT"); + } + } + } + } + #[test] fn instant_mode_buffer_is_larger_than_normal() { let instant = DEFAULT_MP4_MUXER_BUFFER_SIZE_INSTANT; @@ -1579,64 +1618,50 @@ mod tests { #[test] fn env_override_takes_precedence() { - let _guard = ENV_LOCK.lock().unwrap(); + let _env = EnvVarReset::new(); unsafe { std::env::set_var("CAP_MP4_MUXER_BUFFER_SIZE", "500"); } let normal = get_mp4_muxer_buffer_size(false); let instant = get_mp4_muxer_buffer_size(true); - unsafe { - std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE"); - } assert_eq!(normal, 500); assert_eq!(instant, 500); } #[test] fn invalid_env_falls_back_to_defaults() { - let _guard = ENV_LOCK.lock().unwrap(); + let _env = EnvVarReset::new(); unsafe { std::env::set_var("CAP_MP4_MUXER_BUFFER_SIZE", "not_a_number"); } let normal = get_mp4_muxer_buffer_size(false); let instant = get_mp4_muxer_buffer_size(true); - unsafe { - std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE"); - } assert_eq!(normal, DEFAULT_MP4_MUXER_BUFFER_SIZE); assert_eq!(instant, DEFAULT_MP4_MUXER_BUFFER_SIZE_INSTANT); } #[test] fn instant_env_override_takes_precedence_over_global_override() { - let _guard = ENV_LOCK.lock().unwrap(); + let _env = EnvVarReset::new(); unsafe { std::env::set_var("CAP_MP4_MUXER_BUFFER_SIZE", "500"); std::env::set_var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT", "120"); } let normal = get_mp4_muxer_buffer_size(false); let instant = get_mp4_muxer_buffer_size(true); - unsafe { - std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE"); - std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT"); - } assert_eq!(normal, 500); assert_eq!(instant, 120); } #[test] fn invalid_instant_override_falls_back_to_global_override() { - let _guard = ENV_LOCK.lock().unwrap(); + let _env = EnvVarReset::new(); unsafe { std::env::set_var("CAP_MP4_MUXER_BUFFER_SIZE", "300"); std::env::set_var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT", "invalid"); } let normal = get_mp4_muxer_buffer_size(false); let instant = get_mp4_muxer_buffer_size(true); - unsafe { - std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE"); - std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT"); - } assert_eq!(normal, 300); assert_eq!(instant, 300); } diff --git a/crates/recording/src/studio_recording.rs b/crates/recording/src/studio_recording.rs index 83dad77eb4d..7ed02861ac9 100644 --- a/crates/recording/src/studio_recording.rs +++ b/crates/recording/src/studio_recording.rs @@ -143,15 +143,15 @@ fn persist_segment_input_events( if let Some(path) = cursor_path { #[derive(serde::Serialize)] struct CursorEventsRef<'a> { - clicks: &'a [CursorClickEvent], - moves: &'a [CursorMoveEvent], + clicks: &'a [cap_project::CursorClickEvent], + moves: &'a [cap_project::CursorMoveEvent], } let result = crate::cursor::write_atomically_with(path, |target| { use std::io::Write; let file = std::fs::File::create(target)?; let mut writer = std::io::BufWriter::new(file); - serde_json::to_writer( + serde_json::to_writer_pretty( &mut writer, &CursorEventsRef { clicks: &response.clicks,