Repository navigation
perf(recording): drain macOS autorelease pools and reuse video frames in-place - #3625
Conversation
|
@duongynhi000005-oss is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
| let keyframe_emitted_ref = &mut keyframe_emitted; | ||
| let encode_result = encoder.encode_frame_reusable( | ||
| frame, | ||
| &mut converted_frame, |
There was a problem hiding this comment.
Recovery keeps the wrong buffer
converted_frame now survives an encoder rebuild, but build_standalone() can fall back to another codec with a different output pixel format. A scaled BGRA recording can switch from VideoToolbox to libx264, which requires YUV420P. encode_frame_reusable() keeps the old buffer because it is already Some, so later conversions can fail and drop every remaining frame.
Clear converted_frame when replacing the encoder, or recreate it when the new encoder needs a different format.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/recording/src/output_pipeline/oop_fragmented_m4s.rs
Line: 438
Comment:
**Recovery keeps the wrong buffer**
`converted_frame` now survives an encoder rebuild, but `build_standalone()` can fall back to another codec with a different output pixel format. A scaled BGRA recording can switch from VideoToolbox to `libx264`, which requires YUV420P. `encode_frame_reusable()` keeps the old buffer because it is already `Some`, so later conversions can fail and drop every remaining frame.
Clear `converted_frame` when replacing the encoder, or recreate it when the new encoder needs a different format.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| unsafe { | ||
| ffmpeg::ffi::av_frame_make_writable(converted.as_mut_ptr()); | ||
| } |
There was a problem hiding this comment.
Failed buffer checks are ignored
The new av_frame_make_writable() calls discard their return values. If FFmpeg cannot allocate a private buffer, conversion still writes into the old buffer, which may remain shared with the encoder. Under memory pressure, this can overwrite a frame the encoder still uses.
Return the error before calling convert_or_scale(). Apply this to the added checks in both h264.rs and h264_packet.rs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/enc-ffmpeg/src/video/h264.rs
Line: 1038-1040
Comment:
**Failed buffer checks are ignored**
The new `av_frame_make_writable()` calls discard their return values. If FFmpeg cannot allocate a private buffer, conversion still writes into the old buffer, which may remain shared with the encoder. Under memory pressure, this can overwrite a frame the encoder still uses.
Return the error before calling `convert_or_scale()`. Apply this to the added checks in both `h264.rs` and `h264_packet.rs`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if let Some(ref mut frame) = self.frame { | ||
| if unsafe { ffmpeg::ffi::av_frame_make_writable(frame.as_mut_ptr()) } < 0 { | ||
| self.frame = None; | ||
| } | ||
| } |
There was a problem hiding this comment.
Nested conditions break repository rules
get_frame() adds a nested if let and if. The repository guide requires combining nested conditions with && and lists collapsible_if among the denied checks. Use an if let chain here. This repository requirement must be satisfied before merging.
| if let Some(ref mut frame) = self.frame { | |
| if unsafe { ffmpeg::ffi::av_frame_make_writable(frame.as_mut_ptr()) } < 0 { | |
| self.frame = None; | |
| } | |
| } | |
| if let Some(ref mut frame) = self.frame | |
| && unsafe { ffmpeg::ffi::av_frame_make_writable(frame.as_mut_ptr()) } < 0 | |
| { | |
| self.frame = None; | |
| } |
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/recording/src/output_pipeline/macos_frame_convert.rs
Line: 57-61
Comment:
**Nested conditions break repository rules**
`get_frame()` adds a nested `if let` and `if`. The repository guide requires combining nested conditions with `&&` and lists `collapsible_if` among the denied checks. Use an `if let` chain here. This repository requirement must be satisfied before merging.
```suggestion
if let Some(ref mut frame) = self.frame
&& unsafe { ffmpeg::ffi::av_frame_make_writable(frame.as_mut_ptr()) } < 0
{
self.frame = None;
}
```
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Fixes #109
Summary
This PR addresses the three primary sources of sustained CPU and memory growth in the macOS recording pipeline reported in #109:
Drain Objective-C autorelease pools on per-frame worker threads and AVFoundation camera capture queues (crates/camera/src/macos.rs, crates/recording/src/output_pipeline/macos.rs, macos_fragmented_m4s.rs, oop_fragmented_m4s.rs):
Reuse AVFrame backing buffers in-place across the software M4S and OOP M4S pipelines (crates/recording/src/output_pipeline/macos_frame_convert.rs, macos_fragmented_m4s.rs, oop_fragmented_m4s.rs, crates/enc-ffmpeg/src/video/h264.rs, h264_packet.rs, crates/enc-ffmpeg/src/mux/segmented_stream.rs):
Testing
Fix the converted frame's lifetime across encoder rebuilds and satisfy the required Rust condition style before merging.
Findings
Fix with agent prompt
Summary
This PR reduces repeated allocations and drains temporary Objective-C objects during macOS recording.
Reviews (1) · Last reviewed commit: "perf(recording): drain macOS autorelease..." · Reviewed by Greptile