Skip to content

perf(recording): reduce recording memory pressure #1831

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ibbxx wants to merge 1 commit into
base: main
Choose a base branch
from
Open

perf(recording): reduce recording memory pressure #1831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 44 additions & 10 deletions crates/recording/src/cursor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
use cap_cursor_capture::CursorCropBounds;
use cap_cursor_info::CursorShape;
use cap_project::{
CursorClickEvent, CursorEvents, CursorMoveEvent, KeyPressEvent, KeyboardEvents, XY,
CursorClickEvent, CursorMoveEvent, KeyPressEvent, KeyboardEvents, XY,
};
use cap_timestamp::Timestamps;
use futures::{FutureExt, future::Shared};
use std::{
collections::HashMap,
fs::File,
io::{BufWriter, Write},
path::{Path, PathBuf},
time::Instant,
};
Expand Down Expand Up @@ -51,15 +53,35 @@ impl CursorActor {
const CURSOR_FLUSH_INTERVAL_SECS: u64 = 5;

fn flush_cursor_data(output_path: &Path, moves: &[CursorMoveEvent], clicks: &[CursorClickEvent]) {
let events = CursorEvents {
clicks: clicks.to_vec(),
moves: moves.to_vec(),
#[derive(serde::Serialize)]
struct BorrowedCursorEvents<'a> {
clicks: &'a [CursorClickEvent],
moves: &'a [CursorMoveEvent],
}

let file = match File::create(output_path) {
Ok(file) => file,
Err(e) => {
tracing::error!(
"Failed to create cursor data file {}: {}",
output_path.display(),
e
);
return;
}
};
if let Ok(json) = serde_json::to_string_pretty(&events)
&& let Err(e) = std::fs::write(output_path, json)
{

let events = BorrowedCursorEvents { clicks, moves };
let mut writer = BufWriter::new(file);
if let Err(e) = serde_json::to_writer(&mut writer, &events) {
tracing::error!(
"Failed to serialize cursor data to {}: {}",
output_path.display(),
e
);
} else if let Err(e) = writer.write_all(b"\n").and_then(|_| writer.flush()) {
tracing::error!(
"Failed to write cursor data to {}: {}",
"Failed to flush cursor data to {}: {}",
output_path.display(),
e
);
Expand Down Expand Up @@ -226,6 +248,8 @@ pub fn spawn_cursor_recorder(

let mut last_flush = Instant::now();
let flush_interval = Duration::from_secs(CURSOR_FLUSH_INTERVAL_SECS);
let mut last_flushed_cursor_moves = 0;
let mut last_flushed_cursor_clicks = 0;
let mut last_cursor_id: Option<String> = None;

loop {
Expand Down Expand Up @@ -362,7 +386,13 @@ pub fn spawn_cursor_recorder(

if last_flush.elapsed() >= flush_interval {
if let Some(ref path) = incremental_outputs.cursor {
flush_cursor_data(path, &response.moves, &response.clicks);
if response.moves.len() != last_flushed_cursor_moves
|| response.clicks.len() != last_flushed_cursor_clicks
{
flush_cursor_data(path, &response.moves, &response.clicks);
last_flushed_cursor_moves = response.moves.len();
last_flushed_cursor_clicks = response.clicks.len();
}
}
if let Some(ref kb_path) = incremental_outputs.keyboard {
flush_keyboard_data(kb_path, &response.keyboard_presses);
Expand All @@ -374,7 +404,11 @@ pub fn spawn_cursor_recorder(
info!("cursor recorder done");

if let Some(ref path) = incremental_outputs.cursor {
flush_cursor_data(path, &response.moves, &response.clicks);
if response.moves.len() != last_flushed_cursor_moves
|| response.clicks.len() != last_flushed_cursor_clicks
{
flush_cursor_data(path, &response.moves, &response.clicks);
}
}

if let Some(ref kb_path) = incremental_outputs.keyboard {
Expand Down
103 changes: 77 additions & 26 deletions crates/recording/src/output_pipeline/macos.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use std::{
use tracing::*;

const DEFAULT_MP4_MUXER_BUFFER_SIZE: usize = 60;
const DEFAULT_MP4_MUXER_BUFFER_SIZE_INSTANT: usize = 240;
const DEFAULT_MP4_MUXER_BUFFER_SIZE_INSTANT: usize = 96;
const DEFAULT_MP4_AUDIO_FINISH_TIMEOUT: Duration = Duration::from_secs(2);
const DEFAULT_MP4_AUDIO_FINISH_TIMEOUT_INSTANT: Duration = Duration::from_secs(8);

Expand All @@ -51,9 +51,17 @@ fn get_available_disk_space_mb(path: &std::path::Path) -> Option<u64> {
}

fn get_mp4_muxer_buffer_size(instant_mode: bool) -> usize {
std::env::var("CAP_MP4_MUXER_BUFFER_SIZE")
.ok()
.and_then(|s| s.parse().ok())
let instant_override = instant_mode
.then(|| std::env::var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT").ok())
.flatten()
.and_then(|s| s.parse().ok());

instant_override
.or_else(|| {
std::env::var("CAP_MP4_MUXER_BUFFER_SIZE")
.ok()
.and_then(|s| s.parse().ok())
})
.unwrap_or(if instant_mode {
DEFAULT_MP4_MUXER_BUFFER_SIZE_INSTANT
} else {
Expand Down Expand Up @@ -1197,6 +1205,37 @@ mod tests {
mod mp4_muxer_buffer_size {
use super::*;

static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());

fn with_muxer_env<T>(
global: Option<&str>,
instant: Option<&str>,
test: impl FnOnce() -> T,
) -> T {
let _guard = ENV_LOCK.lock().unwrap();
Comment on lines 1207 to +1215
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Mutex poisoning cascades on test failure

If any test panics inside with_muxer_env (e.g. an assert_eq! fails), the thread unwinds while holding _guard. Rust marks the Mutex as poisoned when a thread panics while holding a lock. Every subsequent call to ENV_LOCK.lock().unwrap() will then panic with a PoisonError, causing all remaining tests in the module to fail regardless of their own correctness. Use .unwrap_or_else(|e| e.into_inner()) to tolerate a poisoned mutex.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: crates/recording/src/output_pipeline/macos.rs
Line: 1207-1215

Comment:
**Mutex poisoning cascades on test failure**

If any test panics inside `with_muxer_env` (e.g. an `assert_eq!` fails), the thread unwinds while holding `_guard`. Rust marks the `Mutex` as poisoned when a thread panics while holding a lock. Every subsequent call to `ENV_LOCK.lock().unwrap()` will then panic with a `PoisonError`, causing all remaining tests in the module to fail regardless of their own correctness. Use `.unwrap_or_else(|e| e.into_inner())` to tolerate a poisoned mutex.

How can I resolve this? If you propose a fix, please make it concise.

unsafe {
match global {
Some(value) => std::env::set_var("CAP_MP4_MUXER_BUFFER_SIZE", value),
None => std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE"),
}
match instant {
Some(value) => {
std::env::set_var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT", value)
}
None => std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT"),
}
}

let result = test();

unsafe {
std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE");
std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT");
}

result
}
Comment on lines +1229 to +1237
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Env vars set inside with_muxer_env are not cleaned up if test() panics. After the panic is caught by the test harness, CAP_MP4_MUXER_BUFFER_SIZE / CAP_MP4_MUXER_BUFFER_SIZE_INSTANT remain set, which can pollute later tests. Wrapping test() in catch_unwind and resuming after cleanup ensures the vars are always removed.

Suggested change
let result = test();
unsafe {
std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE");
std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT");
}
result
}
let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(test));
unsafe {
std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE");
std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT");
}
match result {
Ok(v) => v,
Err(e) => std::panic::resume_unwind(e),
}
}
Prompt To Fix With AI 
This is a comment left during a code review.
Path: crates/recording/src/output_pipeline/macos.rs
Line: 1229-1237

Comment:
Env vars set inside `with_muxer_env` are not cleaned up if `test()` panics. After the panic is caught by the test harness, `CAP_MP4_MUXER_BUFFER_SIZE` / `CAP_MP4_MUXER_BUFFER_SIZE_INSTANT` remain set, which can pollute later tests. Wrapping `test()` in `catch_unwind` and resuming after cleanup ensures the vars are always removed.

```suggestion
            let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(test));

            unsafe {
                std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE");
                std::env::remove_var("CAP_MP4_MUXER_BUFFER_SIZE_INSTANT");
            }

            match result {
                Ok(v) => v,
                Err(e) => std::panic::resume_unwind(e),
            }
        }
```

How can I resolve this? If you propose a fix, please make it concise.


#[test]
fn instant_mode_buffer_is_larger_than_normal() {
let instant = DEFAULT_MP4_MUXER_BUFFER_SIZE_INSTANT;
Expand All @@ -1210,8 +1249,8 @@ mod tests {
}

#[test]
fn instant_mode_default_is_240() {
assert_eq!(DEFAULT_MP4_MUXER_BUFFER_SIZE_INSTANT, 240);
fn instant_mode_default_is_96() {
assert_eq!(DEFAULT_MP4_MUXER_BUFFER_SIZE_INSTANT, 96);
}

#[test]
Expand All @@ -1221,30 +1260,42 @@ mod tests {

#[test]
fn env_override_takes_precedence() {
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);
with_muxer_env(Some("500"), None, || {
let normal = get_mp4_muxer_buffer_size(false);
let instant = get_mp4_muxer_buffer_size(true);
assert_eq!(normal, 500);
assert_eq!(instant, 500);
});
}

#[test]
fn instant_env_override_takes_precedence_over_global_override() {
with_muxer_env(Some("500"), Some("120"), || {
let normal = get_mp4_muxer_buffer_size(false);
let instant = get_mp4_muxer_buffer_size(true);
assert_eq!(normal, 500);
assert_eq!(instant, 120);
});
}

#[test]
fn invalid_instant_override_falls_back_to_global_override() {
with_muxer_env(Some("500"), Some("not_a_number"), || {
let normal = get_mp4_muxer_buffer_size(false);
let instant = get_mp4_muxer_buffer_size(true);
assert_eq!(normal, 500);
assert_eq!(instant, 500);
});
}

#[test]
fn invalid_env_falls_back_to_defaults() {
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);
with_muxer_env(Some("not_a_number"), None, || {
let normal = get_mp4_muxer_buffer_size(false);
let instant = get_mp4_muxer_buffer_size(true);
assert_eq!(normal, DEFAULT_MP4_MUXER_BUFFER_SIZE);
assert_eq!(instant, DEFAULT_MP4_MUXER_BUFFER_SIZE_INSTANT);
});
}
}

Expand Down