Fix memory leaks and reduce memory usage in recording pipeline #1462
Conversation
WalkthroughRefactors image/sample buffer ownership and guards missing buffers; adds FFmpeg-based macOS segmented muxer with per-segment manifests and failure tracking; adjusts MP4 encoder finish/timing behavior; increases queue retry limits; adds memory-leak detector example and updates desktop diagnostics types. Changes
Sequence Diagram(s)sequenceDiagram
participant Pipeline as Capture Pipeline
participant Mux as MacOSSegmentedMuxer
participant SegEnc as Segment Encoder Thread
participant FFmpeg as FFmpeg Writer
participant Manifest as Manifest Writer
Pipeline->>Mux: send_video_frame(frame, timestamp)
activate Mux
Mux->>Mux: check_rotation_and_segment()
alt rotate_needed
Mux->>SegEnc: finish_nowait() (non-blocking)
SegEnc->>FFmpeg: write_trailer()/flush
FFmpeg-->>SegEnc: status (ok/err)
SegEnc-->>Mux: report_finish_result
Mux->>Manifest: update segment entry (is_failed/is_complete)
Mux->>Mux: spawn new Segment Encoder
end
Mux->>SegEnc: push_frame_to_channel(frame, adjusted_ts)
activate SegEnc
SegEnc->>FFmpeg: queue_video_frame()/queue_audio_frame()
FFmpeg-->>SegEnc: ok / error
deactivate SegEnc
Mux-->>Pipeline: ack (or backpressure/drop)
deactivate Mux
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.tsx📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx,js,jsx,rs}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📓 Common learnings⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
crates/enc-avfoundation/src/segmented.rs (2)
24-37: Consider logging sync failures for consistency.Both
dir.sync_all()(line 27) andfile.sync_all()(line 35) silently discard errors withlet _ =. While these are best-effort operations, other parts of this file log warnings on write failures. Consistent logging aids debugging.Suggested improvement
if let Some(parent) = path.parent() && let Ok(dir) = std::fs::File::open(parent) { - let _ = dir.sync_all(); + if let Err(e) = dir.sync_all() { + warn!("Directory fsync failed for {}: {e}", parent.display()); + } }fn sync_file(path: &Path) { - if let Ok(file) = std::fs::File::open(path) { - let _ = file.sync_all(); + if let Ok(file) = std::fs::File::open(path) + && let Err(e) = file.sync_all() + { + warn!("File fsync failed for {}: {e}", path.display()); } }
70-71: Unconventional skip_serializing_if predicate.
std::ops::Not::notworks here becauseimpl Not for &boolexists, but it's obscure. A named helper would be clearer.More readable alternative
fn is_false(v: &bool) -> bool { !*v }- #[serde(skip_serializing_if = "std::ops::Not::not")] + #[serde(skip_serializing_if = "is_false")] is_failed: bool,crates/recording/examples/memory-leak-detector.rs (3)
452-455: Unsafeset_varin multi-threaded context.
std::env::set_varis unsafe in Rust 2024 edition and can cause undefined behavior in multi-threaded programs. Since this is called beforetokio::maininitializes the runtime, it's technically safe here, but the approach is fragile.Consider using the
RUST_LOGenvironment variable externally or initializing the subscriber with explicit configuration:Suggested alternative
- unsafe { std::env::set_var("RUST_LOG", "info,cap_recording=debug") }; - tracing_subscriber::fmt::init(); + tracing_subscriber::fmt() + .with_env_filter("info,cap_recording=debug") + .init();
383-383: Intentional memory leak viastd::mem::forget.
std::mem::forget(dir)prevents cleanup of the temporary directory, which appears intentional so users can inspect the recording output. However, this leaks resources and the intent isn't immediately clear from the code.Since the coding guidelines prohibit comments, consider renaming or restructuring to make the intent explicit:
Suggested approach
- std::mem::forget(dir); + let output_dir = dir.into_path(); + println!("Recording preserved at: {}", output_dir.display());Using
into_path()explicitly transfers ownership and prevents cleanup while being self-documenting.
186-186: Use.last()instead of indexing withlen() - 1.This is more idiomatic and avoids potential issues if the vector were empty (though guarded by the check on line 180).
Suggested fix
let first = &self.samples[0]; - let last = &self.samples[self.samples.len() - 1]; + let last = self.samples.last().unwrap();crates/recording/src/output_pipeline/macos_segmented_ffmpeg.rs (5)
17-23: Consider addingis_failedfield to align with fragmented.rs.The
SegmentInfostruct is missing theis_failedfield that exists in thefragmented.rsimplementation (as seen in the relevant code snippets). This could limit proper failure tracking in manifests.🔎 Proposed fix
#[derive(Debug, Clone)] pub struct SegmentInfo { pub path: PathBuf, pub index: u32, pub duration: Duration, pub file_size: Option<u64>, + pub is_failed: bool, }
25-33: Missingis_failedfield in FragmentEntry.The
FragmentEntrystruct infragmented.rsincludes anis_failedfield for per-segment failure tracking. This implementation lacks that field, creating inconsistency between the two muxers' manifest formats.🔎 Proposed fix
#[derive(Serialize)] struct FragmentEntry { path: String, index: u32, duration: f64, is_complete: bool, #[serde(skip_serializing_if = "Option::is_none")] file_size: Option<u64>, + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + is_failed: bool, }
471-514: Detached thread for segment cleanup may accumulate.The cleanup thread spawned during rotation is not joined or tracked. If segment rotations occur faster than cleanup completes, threads could accumulate. Consider using a bounded thread pool or tracking spawned handles.
630-634: Audio frames are silently ignored.The
AudioMuxerimplementation is a no-op. If this is intentional (video-only segments), consider adding a trace log or documenting this limitation.
636-745: Extract duplicated frame conversion logic into a shared utility module.The
sample_buf_to_ffmpeg_frame()function andBaseAddrLockGuardstruct in this file duplicate nearly identical implementations fromcrates/scap-ffmpeg/src/screencapturekit.rs. Both files implement identical pixel format handling (NV12 and BGRA), stride calculation, and memory locking logic. Consider moving this conversion logic into a shared utility to reduce maintenance burden.crates/enc-avfoundation/src/mp4.rs (1)
518-522: Consider logging errors in Drop.While ignoring the
ResultinDropis unavoidable since errors cannot be propagated, consider logging any failure to aid debugging:🔎 Optional improvement
impl Drop for MP4Encoder { fn drop(&mut self) { - let _ = self.finish(None); + if let Err(e) = self.finish(None) { + error!("Failed to finish encoder in Drop: {:?}", e); + } } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.claude/settings.local.json(1 hunks).gitignore(1 hunks)crates/camera-ffmpeg/src/macos.rs(14 hunks)crates/camera/src/macos.rs(2 hunks)crates/enc-avfoundation/src/mp4.rs(12 hunks)crates/enc-avfoundation/src/segmented.rs(13 hunks)crates/enc-ffmpeg/src/lib.rs(1 hunks)crates/enc-ffmpeg/src/mux/fragmented_mp4.rs(1 hunks)crates/enc-ffmpeg/src/mux/mod.rs(1 hunks)crates/recording/examples/camera-benchmark.rs(4 hunks)crates/recording/examples/encoding-benchmark.rs(2 hunks)crates/recording/examples/memory-leak-detector.rs(1 hunks)crates/recording/examples/recording-benchmark.rs(1 hunks)crates/recording/src/capture_pipeline.rs(2 hunks)crates/recording/src/feeds/camera.rs(1 hunks)crates/recording/src/output_pipeline/fragmented.rs(2 hunks)crates/recording/src/output_pipeline/macos.rs(2 hunks)crates/recording/src/output_pipeline/macos_segmented_ffmpeg.rs(1 hunks)crates/recording/src/output_pipeline/mod.rs(1 hunks)crates/recording/src/sources/native_camera.rs(1 hunks)crates/recording/src/sources/screen_capture/macos.rs(1 hunks)crates/rendering/src/cpu_yuv.rs(3 hunks)crates/scap-ffmpeg/src/screencapturekit.rs(3 hunks)crates/scap-screencapturekit/src/capture.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Use duration.saturating_sub(other) instead of duration - other to avoid panics on underflow
Merge nested if statements: write 'if a && b { }' instead of 'if a { if b { } }'
Don't call .clone() on Copy types (integers, bools, etc.); copy them directly
Use function references directly: iter.map(foo) instead of iter.map(|x| foo(x))
Accept &[T] or &str instead of &Vec or &String in function parameters for flexibility
Use .is_empty() instead of .len() == 0 or .len() > 0 / .len() != 0
Don't assign () to a variable: write foo(); instead of let _ = foo(); or let x = foo(); when return is unit
Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Use 'for item in &collection' or 'for (i, item) in collection.iter().enumerate()' instead of 'for i in 0..collection.len()'
Use value.clamp(min, max) instead of manual if chains or .min(max).max(min) patterns
Always handle Result/Option or types marked #[must_use]; never ignore them
**/*.rs: Userustfmtand workspace clippy lints for Rust code formatting and linting
Use snake_case for Rust module names and kebab-case for crate names
Never usedbg!()macro in Rust code; use proper logging instead (Clippy:dbg_macro= deny)
Always handleResult/Optionor types marked#[must_use]; never ignore them (Rust compiler lint:unused_must_use= deny)
Never writelet _ = async_fn()which silently drops futures; await or explicitly handle them (Clippy:let_underscore_future= deny)
Usesaturating_subinstead of-forDurationto avoid panics (Clippy:unchecked_duration_subtraction= deny)
Merge nestedifstatements: useif a && b { }instead ofif a { if b { } }(Clippy:collapsible_if= deny)
Don't call.clone()onCopytypes; just copy them directly (Clippy:clone_on_copy= deny)
U...
Files:
crates/recording/src/sources/screen_capture/macos.rscrates/recording/src/feeds/camera.rscrates/recording/src/sources/native_camera.rscrates/recording/src/output_pipeline/fragmented.rscrates/rendering/src/cpu_yuv.rscrates/recording/examples/camera-benchmark.rscrates/enc-ffmpeg/src/mux/mod.rscrates/recording/src/output_pipeline/mod.rscrates/enc-ffmpeg/src/lib.rscrates/scap-ffmpeg/src/screencapturekit.rscrates/camera/src/macos.rscrates/camera-ffmpeg/src/macos.rscrates/recording/src/output_pipeline/macos_segmented_ffmpeg.rscrates/enc-avfoundation/src/segmented.rscrates/recording/examples/encoding-benchmark.rscrates/recording/examples/memory-leak-detector.rscrates/recording/examples/recording-benchmark.rscrates/scap-screencapturekit/src/capture.rscrates/recording/src/capture_pipeline.rscrates/recording/src/output_pipeline/macos.rscrates/enc-ffmpeg/src/mux/fragmented_mp4.rscrates/enc-avfoundation/src/mp4.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Never add comments to code (
//,/* */,///,//!,#, etc.); code must be self-explanatory through naming, types, and structure
Files:
crates/recording/src/sources/screen_capture/macos.rscrates/recording/src/feeds/camera.rscrates/recording/src/sources/native_camera.rscrates/recording/src/output_pipeline/fragmented.rscrates/rendering/src/cpu_yuv.rscrates/recording/examples/camera-benchmark.rscrates/enc-ffmpeg/src/mux/mod.rscrates/recording/src/output_pipeline/mod.rscrates/enc-ffmpeg/src/lib.rscrates/scap-ffmpeg/src/screencapturekit.rscrates/camera/src/macos.rscrates/camera-ffmpeg/src/macos.rscrates/recording/src/output_pipeline/macos_segmented_ffmpeg.rscrates/enc-avfoundation/src/segmented.rscrates/recording/examples/encoding-benchmark.rscrates/recording/examples/memory-leak-detector.rscrates/recording/examples/recording-benchmark.rscrates/scap-screencapturekit/src/capture.rscrates/recording/src/capture_pipeline.rscrates/recording/src/output_pipeline/macos.rscrates/enc-ffmpeg/src/mux/fragmented_mp4.rscrates/enc-avfoundation/src/mp4.rs
🧠 Learnings (4)
📓 Common learnings
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
📚 Learning: 2025-10-17T05:58:22.586Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1219
File: crates/enc-avfoundation/src/mp4.rs:350-373
Timestamp: 2025-10-17T05:58:22.586Z
Learning: In crates/enc-avfoundation/src/mp4.rs, the `finish()` method intentionally skips video extension when `is_paused` is true. This is correct behavior because if recording is paused, the video should not be extended beyond the pause point—the pause is user-initiated, unlike the case where ScreenCaptureKit stops providing frames during static content.
Applied to files:
crates/recording/src/sources/screen_capture/macos.rscrates/recording/src/output_pipeline/fragmented.rscrates/recording/src/output_pipeline/macos_segmented_ffmpeg.rscrates/enc-avfoundation/src/segmented.rscrates/scap-screencapturekit/src/capture.rscrates/recording/src/capture_pipeline.rscrates/recording/src/output_pipeline/macos.rscrates/enc-ffmpeg/src/mux/fragmented_mp4.rscrates/enc-avfoundation/src/mp4.rs
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/recording/src/sources/screen_capture/macos.rscrates/recording/src/feeds/camera.rscrates/recording/src/output_pipeline/fragmented.rscrates/recording/examples/camera-benchmark.rscrates/recording/src/output_pipeline/mod.rscrates/enc-ffmpeg/src/lib.rscrates/scap-ffmpeg/src/screencapturekit.rscrates/camera-ffmpeg/src/macos.rscrates/recording/src/output_pipeline/macos_segmented_ffmpeg.rscrates/enc-avfoundation/src/segmented.rscrates/scap-screencapturekit/src/capture.rscrates/recording/src/capture_pipeline.rscrates/recording/src/output_pipeline/macos.rscrates/enc-ffmpeg/src/mux/fragmented_mp4.rscrates/enc-avfoundation/src/mp4.rs
📚 Learning: 2025-12-07T14:29:19.180Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T14:29:19.180Z
Learning: Applies to **/*.rs : Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Applied to files:
crates/recording/examples/camera-benchmark.rs
🧬 Code graph analysis (4)
crates/recording/src/sources/screen_capture/macos.rs (1)
crates/scap-screencapturekit/src/capture.rs (1)
image_buf(110-112)
crates/scap-ffmpeg/src/screencapturekit.rs (2)
crates/scap-ffmpeg/src/lib.rs (1)
as_ffmpeg(15-15)crates/scap-screencapturekit/src/capture.rs (1)
image_buf(110-112)
crates/recording/src/output_pipeline/macos_segmented_ffmpeg.rs (2)
crates/recording/src/output_pipeline/fragmented.rs (2)
default(37-42)default(183-188)crates/recording/src/fragmentation/mod.rs (2)
current_index(86-88)total_duration(132-138)
crates/enc-avfoundation/src/segmented.rs (2)
crates/enc-avfoundation/src/mp4.rs (1)
wait_for_writer_finished(672-714)crates/recording/src/fragmentation/mod.rs (1)
sync_file(31-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (52)
crates/rendering/src/cpu_yuv.rs (3)
993-993: LGTM! Improved test assertion message readability.The named placeholders make test failure output clearer and easier to debug.
1063-1063: LGTM! Consistent test assertion formatting.Named placeholders maintain consistency with other test assertions and improve debugging experience.
1114-1114: LGTM! Clear and consistent test assertion.The named placeholders and descriptive prefix ("YUV420P") make it easy to distinguish test failures from different conversion functions.
crates/recording/examples/recording-benchmark.rs (1)
268-268: LGTM: Clean simplification of match arm.The explicit "full" pattern was redundant since the default branch executes the same logic. This change maintains identical behavior while simplifying the code.
crates/recording/examples/encoding-benchmark.rs (2)
195-195: LGTM: Modern formatting syntax.The change to named parameters in
println!is a minor style improvement with no functional impact.
409-409: LGTM: Consistent simplification.This matches the same cleanup applied in
recording-benchmark.rs, removing the redundant explicit "full" pattern while maintaining identical behavior through the default branch.crates/recording/examples/camera-benchmark.rs (1)
301-304: Improved error handling during drain phase.This change properly handles encoding errors during the drain phase by logging the error and recording the dropped output metric, making it consistent with the error handling in the main benchmark loop. Previously, these errors were silently ignored.
crates/enc-avfoundation/src/segmented.rs (8)
1-11: LGTM!Imports are appropriate for the new background finalization and failure tracking functionality.
179-189: File size may be inaccurate due to async finalization.
file_sizeis read immediately after spawning the background finalization thread. Sincewait_for_writer_finishedruns asynchronously, the file may not be fully written when metadata is queried, potentially yielding stale sizes in the manifest.If accurate sizes are needed for playback or stitching, consider either:
- Accepting
Nonefor in-progress segments and updating sizes post-finalization- Deferring size calculation to manifest finalization
205-219: LGTM!Good error handling with contextual logging for the segment index, enabling easier debugging if encoder creation fails during rotation.
325-378: Consistent failure tracking pattern.The finish method properly mirrors the rotation logic with explicit success/failure paths. The same file_size timing consideration applies here as noted for
rotate_segment.
380-424: Good failure summary and manifest finalization.The warning log for failed segments provides useful operational visibility. The
is_completeflag correctly indicates session completion while individualis_failedflags track per-segment status.
264-311: LGTM!The in-progress manifest correctly tracks both completed segments (with their failure states) and the current active segment. The
is_failed: falsefor the current segment is appropriate since it's still being written.
131-148: LGTM!Clear error propagation with
QueueFrameError::NoEncoderwhen the encoder is unavailable.
53-60: LGTM!The
is_failedfield inSegmentInfoenables clean per-segment failure tracking throughout the pipeline.crates/recording/src/feeds/camera.rs (1)
479-482: Good refactor: Using standard.clone()pattern.The change from
.retained()to.clone()aligns with standard Rust conventions for reference-counted types. Both methods properly increment the reference count, but.clone()is more idiomatic.crates/recording/src/output_pipeline/macos.rs (2)
38-38: Increased retry tolerance improves reliability under load.Increasing
MAX_QUEUE_RETRIESfrom 500 to 1500 extends the timeout window from 1 second to 3 seconds, giving the encoder more time to recover when under pressure. This complements the buffer size reductions elsewhere in the PR.
149-149: Consistent retry limit increase for camera muxer.The same retry limit increase applied to the camera muxer ensures consistent timeout behavior across both muxer types.
crates/recording/src/sources/screen_capture/macos.rs (1)
177-182: Proper guard against missing image buffer prevents potential crashes.The addition of the
let Some(image_buf) = frame.image_buf() else { return; }guard properly handles cases where the image buffer is unavailable, preventing potential panics. This defensive pattern aligns with the PR's goal of robust buffer lifecycle management.crates/recording/src/output_pipeline/fragmented.rs (2)
47-47: Consistent retry limit across all muxer variants.The retry limit increase matches the change in the non-fragmented muxers, ensuring consistent timeout behavior across all macOS encoding paths.
193-193: Camera muxer timeout aligned with screen muxer.Consistent retry limits across both fragmented muxer types ensure predictable behavior.
crates/camera/src/macos.rs (2)
102-104: Proper guard prevents construction of frames without image data.The early return when
image_buf()isNoneensures that only valid frames are passed to the callback, preventing downstream errors.
203-212: Memory optimization: stores only SampleBuf and extracts ImageBuf on demand.This refactor reduces memory usage by storing a single reference to
SampleBufinstead of separate references to bothImageBufandSampleBuf. Theimage_buf()method extracts the image buffer on demand and returns anOptionto handle cases where the buffer is unavailable. All call sites properly handle theOptionreturn value.crates/recording/src/sources/native_camera.rs (1)
32-32: Buffer size reduction requires load testing validation.The drop detection logic (lines 81-88) is correctly implemented with proper logging. However, buffer capacity at 8 frames should be validated under realistic encoding load conditions. The current code logs frame drops but doesn't prevent them. Without benchmarks or integration tests that measure frame drop rates at typical framerates and resolutions, the adequacy of this size remains unvalidated. Consider running the existing benchmark examples (crates/recording/examples/camera-benchmark.rs) with measured drop rates to confirm this buffer size doesn't cause excessive frame loss under expected conditions.
crates/recording/examples/memory-leak-detector.rs (2)
296-310: Camera lock not explicitly released on early return paths.The
CameraFeedis spawned and a lock is acquired, but if the recording fails later, the lock isn't explicitly released. While Rust's ownership will eventually clean up, for a memory leak detector tool, explicit cleanup would be more robust.Consider whether
camera::Unlockor similar cleanup should be performed in error paths.
1-528: LGTM overall for a development/diagnostic tool.The memory leak detector is well-structured with clear separation between memory tracking, test execution, and CLI handling. The use of
tracingfor logging follows the coding guidelines.crates/enc-ffmpeg/src/mux/fragmented_mp4.rs (2)
62-68: Unsafe FFmpeg option setting looks correct but fragile.The unsafe block correctly sets fragmented MP4 movflags. The
CString::new().unwrap()calls are safe since the strings are hardcoded ASCII without null bytes.
120-153: Good error handling pattern infinish().The implementation correctly:
- Continues to flush audio even if video flush fails
- Logs individual encoder failures while still attempting trailer write
- Returns structured
FinishResultwith per-encoder outcomesThis resilient approach prevents partial failures from causing resource leaks.
crates/enc-ffmpeg/src/mux/mod.rs (1)
1-5: LGTM.The new
fragmented_mp4module is correctly declared and maintains consistent ordering with existing modules.crates/enc-ffmpeg/src/lib.rs (1)
16-18: LGTM.The re-export pattern is consistent with the existing
segmented_audiomodule exposure, maintaining API consistency.crates/recording/src/output_pipeline/mod.rs (1)
6-7: LGTM.The new
macos_segmented_ffmpegmodule follows the established pattern for platform-specific modules, with proper#[cfg(target_os = "macos")]guards matching the existingfragmentedandmacosmodules.Also applies to: 14-15
crates/scap-screencapturekit/src/capture.rs (2)
28-32: LGTM!The early return for missing image buffer prevents processing invalid screen frames, and the simplified
VideoFrameconstruction avoids redundant buffer storage.
110-112: LGTM!Delegating to
sample_buf.image_buf()instead of storing a separate reference is a cleaner approach that aligns with the memory leak fixes.crates/recording/src/capture_pipeline.rs (2)
9-9: LGTM!Import updated to use the new
MacOSSegmentedMuxerfor the fragmented recording path.
89-89: LGTM!Consistent with the import change, switching to the new FFmpeg-based segmented muxer.
crates/scap-ffmpeg/src/screencapturekit.rs (3)
10-10: LGTM!New error variant properly handles the case when no image buffer is available.
15-24: LGTM!Good refactor: obtaining the image buffer reference once, checking for None early, and precomputing plane strides before the lock avoids redundant method calls during the copy loops.
30-91: LGTM!The pixel format matching and frame copying logic correctly uses the precomputed strides.
crates/camera-ffmpeg/src/macos.rs (3)
23-24: LGTM!NoImageBuffer error variant added consistently with the scap-ffmpeg implementation.
58-90: LGTM!Good refactor: obtaining the image buffer once, early error on None, and precomputing plane info before the lock. This avoids repeated method calls during frame processing.
291-319: LGTM!The y420 branch correctly validates
plane_count >= 3before iterating over all three planes using the precomputedplane_info.crates/recording/src/output_pipeline/macos_segmented_ffmpeg.rs (4)
162-194: LGTM!The
Muxer::setupimplementation correctly creates the segments directory and initializes state. Good use ofwith_contextfor error messages.
204-264: LGTM!The
finishmethod properly handles timeout-based thread joining and usessaturating_subfor duration calculation as per coding guidelines.
100-131: LGTM!The
PauseTracker::adjustmethod correctly handles pause/resume transitions with proper overflow checks usingchecked_subandchecked_add.
611-624: No accessibility issue.The
sample_buffield is public in theVideoFramestruct (crates/recording/src/sources/screen_capture/macos.rs), andscreen_captureis properly imported in this module. The field is accessible.Likely an incorrect or invalid review comment.
crates/enc-avfoundation/src/mp4.rs (7)
19-19: Excellent memory leak fix!Replacing
most_recent_frame(which stored both frame buffer and timestamp) with justlast_frame_timestampeliminates unnecessary retention of frame buffer references. This directly addresses the memory leak issue described in the PR objectives.Also applies to: 206-206
55-56: LGTM - Improved error diagnostics.The new error variants provide better error information:
WriterFailedcaptures the actual error from the asset writerNoEncoderclearly indicates when audio encoder is missingAlso applies to: 61-62
278-281: Verify error handling for frame copy.The explicit
drop(frame)is excellent for memory management—it immediately releases the original frame after creating the copy, reducing peak memory usage.However, the
.unwrap()on line 278 will panic ifcopy_with_new_timingfails. Consider proper error propagation:🔎 Suggested improvement
- let new_frame = frame.copy_with_new_timing(&[timing]).unwrap(); + let new_frame = frame.copy_with_new_timing(&[timing]) + .map_err(QueueFrameError::Construct)?; drop(frame);
300-302: LGTM - Clear error handling.Proper use of the new
NoEncodererror variant when audio input is not configured.
455-515: Excellent refactoring for async finalization support.The finish logic is well-separated:
finish()provides synchronous completionfinish_nowait()enables async patternsfinish_start()handles initializationThe use of
saturating_sub(line 500) correctly follows coding guidelines to prevent panics on duration underflow. The logic properly handles pause states and timestamp adjustments.
653-656: LGTM - Enhanced error diagnostics.The improved error handling captures and propagates the actual writer error when available, providing better debugging information.
668-714: Polling approach is appropriate given the available cidre bindings.The implementation correctly uses polling because the cidre bindings only expose a synchronous
finish_writing()method without callback support. The 10-second timeout, progressive 2-second logging, and proper error handling for all writer states make this a robust solution within the available API constraints. The blocking behavior is correct for sequential frame processing in this context.
| pub fn queue_audio_frame(&mut self, frame: frame::Audio) { | ||
| if self.is_finished { | ||
| return; | ||
| } | ||
|
|
||
| let Some(audio) = &mut self.audio else { | ||
| return; | ||
| }; | ||
|
|
||
| self.has_frames = true; | ||
| audio.send_frame(frame, &mut self.output); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find fragmented_mp4.rs file
find . -name "fragmented_mp4.rs" -type fRepository: CapSoftware/Cap
Length of output: 104
🏁 Script executed:
#!/bin/bash
# Search for AudioEncoder trait definition
rg "trait AudioEncoder" -A 10
# Search for queue_audio_frame method
rg "fn queue_audio_frame" -A 8
# Search for queue_video_frame method
rg "fn queue_video_frame" -A 8
# Search for send_frame method in audio encoder context
rg "fn send_frame" -B 2 -A 5Repository: CapSoftware/Cap
Length of output: 8616
Inconsistency: queue_audio_frame should return Result like queue_video_frame.
The queue_audio_frame method returns () while the analogous queue_video_frame returns Result<(), h264::QueueFrameError>. This inconsistency masks encoding errors. Although the AudioEncoder::send_frame trait method itself returns (), the implementations explicitly drop errors from internal methods using let _ = .... Align error handling by returning Result from queue_audio_frame:
- pub fn queue_audio_frame(&mut self, frame: frame::Audio) {
+ pub fn queue_audio_frame(&mut self, frame: frame::Audio) -> Result<(), Box<dyn std::error::Error>> {
if self.is_finished {
- return;
+ return Ok(());
}
let Some(audio) = &mut self.audio else {
- return;
+ return Ok(());
};
self.has_frames = true;
- audio.send_frame(frame, &mut self.output);
+ audio.send_frame(frame, &mut self.output);
+ Ok(())
}🤖 Prompt for AI Agents
In crates/enc-ffmpeg/src/mux/fragmented_mp4.rs around lines 107-118,
queue_audio_frame currently returns () while queue_video_frame returns
Result<(), h264::QueueFrameError>, which hides encoding errors; change
queue_audio_frame to return Result<(), h264::QueueFrameError>, update the call
to audio.send_frame to propagate its Result (or update the
AudioEncoder::send_frame trait and all its implementations to return Result if
they currently return ()), map/convert any internal encoder errors into
h264::QueueFrameError as needed, and update all callers of queue_audio_frame to
handle the Result so audio encoding failures are not silently ignored.
| impl Drop for FragmentedMP4File { | ||
| fn drop(&mut self) { | ||
| if let Err(e) = self.finish() { | ||
| error!("Failed to finish FragmentedMP4File in Drop: {e}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Drop implementation logs AlreadyFinished as error.
When finish() is called explicitly before drop, the Drop impl will call finish() again and log AlreadyFinished as an error, which is misleading since it's expected behavior.
Suggested fix
impl Drop for FragmentedMP4File {
fn drop(&mut self) {
- if let Err(e) = self.finish() {
- error!("Failed to finish FragmentedMP4File in Drop: {e}");
+ if !self.is_finished {
+ if let Err(e) = self.finish() {
+ error!("Failed to finish FragmentedMP4File in Drop: {e}");
+ }
}
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/enc-ffmpeg/src/mux/fragmented_mp4.rs around lines 164 to 169, the Drop
impl calls self.finish() and unconditionally logs any error, which causes an
misleading error log for the expected AlreadyFinished case; change the drop
logic to call finish(), match the returned error, and suppress or ignore the
AlreadyFinished/expected-no-op variant while only logging unexpected errors (or
map the AlreadyFinished to Ok) so Drop does not emit an error for the normal
already-finished path.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/recording/src/output_pipeline/macos_segmented_ffmpeg.rs (1)
219-237: Consider extracting the thread join-with-timeout pattern.The polling logic for waiting on encoder threads (with 5s timeout and 50ms intervals) appears in both
finish()androtate_segment(). Extracting this pattern into a helper function would reduce duplication and improve maintainability.🔎 Example helper function
fn join_encoder_thread_with_timeout( handle: JoinHandle<anyhow::Result<()>>, timeout: Duration, context: &str, ) { let start = std::time::Instant::now(); loop { if handle.is_finished() { if let Err(panic_payload) = handle.join() { warn!("Screen encoder thread panicked during {context}: {panic_payload:?}"); } break; } if start.elapsed() > timeout { warn!("Screen encoder thread did not finish within {timeout:?} during {context}, abandoning"); break; } std::thread::sleep(Duration::from_millis(50)); } }Also applies to: 474-493
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/utils/tauri.ts(2 hunks)crates/recording/src/output_pipeline/macos_segmented_ffmpeg.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use TanStack Query v5 for all client-side server state and data fetching in TypeScript files
Files:
apps/desktop/src/utils/tauri.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use strict TypeScript; avoid any type; leverage shared types from @cap/* packages
Follow camelCase naming for variables and functions; PascalCase for components; hooks must start with 'use' prefix
Use Biome for linting and formatting; match existing formatting conventions in the codebase
Use Tailwind CSS for styling in web components; stay consistent with spacing and tokens
Use static skeletons for loading states that mirror content; avoid bouncing animations
Memoize expensive work, code-split naturally, and use Next/Image for remote assets
**/*.{ts,tsx,js,jsx}: Use 2-space indent for TypeScript files; format with Biome usingpnpm format
Use Biome for code formatting and linting; runpnpm formatregularly
Use kebab-case for file names (e.g.,user-menu.tsx); use PascalCase for components
Files:
apps/desktop/src/utils/tauri.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never add any form of comments to code (single-line //, multi-line /* /, JSDoc /* */, or any other comment syntax); code must be self-explanatory through naming, types, and structure
Directory naming must use lowercase-dashed convention
Files:
apps/desktop/src/utils/tauri.ts
apps/desktop/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.ts: Use @tanstack/solid-query for server state management in SolidJS components
Use generated commands and events from tauri_specta for IPC; never manually construct IPC calls
Listen directly to generated events from tauri_specta and use typed event interfaces
Files:
apps/desktop/src/utils/tauri.ts
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Never add comments to code (
//,/* */,///,//!,#, etc.); code must be self-explanatory through naming, types, and structure
Files:
apps/desktop/src/utils/tauri.tscrates/recording/src/output_pipeline/macos_segmented_ffmpeg.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Use duration.saturating_sub(other) instead of duration - other to avoid panics on underflow
Merge nested if statements: write 'if a && b { }' instead of 'if a { if b { } }'
Don't call .clone() on Copy types (integers, bools, etc.); copy them directly
Use function references directly: iter.map(foo) instead of iter.map(|x| foo(x))
Accept &[T] or &str instead of &Vec or &String in function parameters for flexibility
Use .is_empty() instead of .len() == 0 or .len() > 0 / .len() != 0
Don't assign () to a variable: write foo(); instead of let _ = foo(); or let x = foo(); when return is unit
Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Use 'for item in &collection' or 'for (i, item) in collection.iter().enumerate()' instead of 'for i in 0..collection.len()'
Use value.clamp(min, max) instead of manual if chains or .min(max).max(min) patterns
Always handle Result/Option or types marked #[must_use]; never ignore them
**/*.rs: Userustfmtand workspace clippy lints for Rust code formatting and linting
Use snake_case for Rust module names and kebab-case for crate names
Never usedbg!()macro in Rust code; use proper logging instead (Clippy:dbg_macro= deny)
Always handleResult/Optionor types marked#[must_use]; never ignore them (Rust compiler lint:unused_must_use= deny)
Never writelet _ = async_fn()which silently drops futures; await or explicitly handle them (Clippy:let_underscore_future= deny)
Usesaturating_subinstead of-forDurationto avoid panics (Clippy:unchecked_duration_subtraction= deny)
Merge nestedifstatements: useif a && b { }instead ofif a { if b { } }(Clippy:collapsible_if= deny)
Don't call.clone()onCopytypes; just copy them directly (Clippy:clone_on_copy= deny)
U...
Files:
crates/recording/src/output_pipeline/macos_segmented_ffmpeg.rs
🧠 Learnings (2)
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/recording/src/output_pipeline/macos_segmented_ffmpeg.rs
📚 Learning: 2025-10-17T05:58:22.586Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1219
File: crates/enc-avfoundation/src/mp4.rs:350-373
Timestamp: 2025-10-17T05:58:22.586Z
Learning: In crates/enc-avfoundation/src/mp4.rs, the `finish()` method intentionally skips video extension when `is_paused` is true. This is correct behavior because if recording is paused, the video should not be extended beyond the pause point—the pause is user-initiated, unlike the case where ScreenCaptureKit stops providing frames during static content.
Applied to files:
crates/recording/src/output_pipeline/macos_segmented_ffmpeg.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (6)
apps/desktop/src/utils/tauri.ts (1)
438-438: LGTM!The
MacOSVersionInfotype definition is clear and appropriate for storing macOS version information.crates/recording/src/output_pipeline/macos_segmented_ffmpeg.rs (5)
630-634: Verify intentional audio omission.The
AudioMuxerimplementation silently discards all audio frames. Confirm this is intentional for the current scope, as the PR mentions fixing "camera buffer handling" which could include audio.If audio is not needed for this segmented muxer, consider adding a log message during setup to make this explicit:
fn setup(...) -> anyhow::Result<Self> { // ... existing setup ... info!("MacOSSegmentedMuxer: audio frames will not be encoded"); Ok(Self { ... }) }
100-131: LGTM: Robust timestamp adjustment.The pause tracking logic correctly uses
checked_subandchecked_addfor all Duration arithmetic with clear error messages, preventing panics on underflow/overflow.As per coding guidelines,
saturating_suborchecked_submust be used for Duration subtraction.
453-532: LGTM: Segment rotation with proper cleanup.The segment rotation correctly uses
saturating_subfor duration calculation (line 457) and spawns an async cleanup thread to avoid blocking the recording pipeline. The frame drop tracker is properly reset at segment boundaries.
636-712: LGTM: Correct pixel buffer conversion with stride handling.The conversion function properly:
- Checks for missing image buffers before access
- Uses RAII guard to ensure pixel buffer is locked during memory access
- Handles stride mismatches with row-by-row copying for both NV12 and BGRA formats
- Returns descriptive errors for unsupported formats
The row-by-row approach is necessary for correct handling of different stride values between source and destination.
726-740: Verify memory safety of pixel buffer access.The
unsafeslice creation inplane_data()relies oncidreproviding valid base addresses and sizes. Ensure that:
- The CVPixelBuffer lifetime extends beyond the guard's lifetime
- The plane data calculation
plane_bytes_per_row * plane_heightcorrectly represents the available memory- The
cidrelibrary provides accurate base address pointersThe current RAII pattern with lifetime
'ais correct for ensuring the lock is held during slice access. However, the underlying FFI assumptions should be validated.#!/bin/bash # Verify CVPixelBuffer memory access patterns in cidre usage rg -nP --type=rust -C3 'ImageBuf.*plane_base_address|plane_bytes_per_row'
| export type StudioRecordingMeta = { segment: SingleSegment } | { inner: MultipleSegments } | ||
| export type StudioRecordingStatus = { status: "InProgress" } | { status: "NeedsRemux" } | { status: "Failed"; error: string } | { status: "Complete" } | ||
| export type SystemDiagnostics = { windowsVersion: WindowsVersionInfo | null; gpuInfo: GpuInfoDiag | null; allGpus: AllGpusInfo | null; renderingStatus: RenderingStatus; availableEncoders: string[]; graphicsCaptureSupported: boolean; d3D11VideoProcessorAvailable: boolean } | ||
| export type SystemDiagnostics = { macosVersion: MacOSVersionInfo | null; availableEncoders: string[]; screenCaptureSupported: boolean } |
There was a problem hiding this comment.
Breaking change in SystemDiagnostics type requires verification.
The SystemDiagnostics type has been completely restructured from Windows/GPU-centric diagnostics to macOS-centric diagnostics. This breaking change removes several fields (e.g., windowsVersion, gpuInfo, allGpus, renderingStatus, graphicsCaptureSupported, d3D11VideoProcessorAvailable) and introduces new macOS-specific fields (macosVersion, screenCaptureSupported).
Ensure that all TypeScript consumers of this type have been updated to reference the new field names and structure.
#!/bin/bash
# Search for usages of removed SystemDiagnostics fields
echo "=== Searching for removed SystemDiagnostics fields ==="
rg -n --type=ts --type=tsx -C3 '\b(windowsVersion|gpuInfo|allGpus|renderingStatus|graphicsCaptureSupported|d3D11VideoProcessorAvailable)\b'
echo ""
echo "=== Searching for SystemDiagnostics type usage ==="
rg -n --type=ts --type=tsx -C3 'SystemDiagnostics'🤖 Prompt for AI Agents
In apps/desktop/src/utils/tauri.ts around line 501, the SystemDiagnostics type
was changed from a Windows/GPU-centric shape to a macOS-centric shape (removed
fields: windowsVersion, gpuInfo, allGpus, renderingStatus,
graphicsCaptureSupported, d3D11VideoProcessorAvailable; added macosVersion,
screenCaptureSupported), which is a breaking change; locate all TypeScript
usages (run the provided ripgrep commands), update each consumer to use the new
field names or accessors, or restore a compatibility union/interface or mapping
function that populates the old fields from the new diagnostics where
appropriate, and add unit/type tests to confirm no remaining references to
removed fields remain and compilation succeeds.
| impl Drop for BaseAddrLockGuard<'_> { | ||
| fn drop(&mut self) { | ||
| let _ = unsafe { self.0.unlock_lock_base_addr(self.1) }; | ||
| } |
There was a problem hiding this comment.
Log unlock failures in Drop.
The unlock_lock_base_addr result is silently ignored. While Drop cannot propagate errors, failures during unlock should be logged to aid debugging.
🔎 Proposed fix to log unlock errors
impl Drop for BaseAddrLockGuard<'_> {
fn drop(&mut self) {
- let _ = unsafe { self.0.unlock_lock_base_addr(self.1) };
+ if let Err(e) = unsafe { self.0.unlock_lock_base_addr(self.1) } {
+ error!("Failed to unlock pixel buffer base address: {:?}", e);
+ }
}
}As per coding guidelines, Results should not be ignored.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl Drop for BaseAddrLockGuard<'_> { | |
| fn drop(&mut self) { | |
| let _ = unsafe { self.0.unlock_lock_base_addr(self.1) }; | |
| } | |
| impl Drop for BaseAddrLockGuard<'_> { | |
| fn drop(&mut self) { | |
| if let Err(e) = unsafe { self.0.unlock_lock_base_addr(self.1) } { | |
| error!("Failed to unlock pixel buffer base address: {:?}", e); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/recording/src/output_pipeline/macos_segmented_ffmpeg.rs around lines
742 to 745, the Drop impl for BaseAddrLockGuard currently ignores the Result
from unlock_lock_base_addr; change it to call unlock_lock_base_addr inside the
unsafe block, check the Result, and log any Err using the project's logger
(include the base addr/self.1 and a clear message), but do not panic — just log
the failure since Drop cannot return errors.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.