Repository navigation
perf(recording): bound camera & converter frame queues by byte budget and drain macOS autorelease pools (#109) - #3935
Conversation
… and drain macOS autorelease pools Fixes CapSoftware#109 Signed-off-by: Lucas Oliveira <kalopsia3445@gmail.com>
|
@frds3445 is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
| struct CameraLoopRuntime { | ||
| original_info: VideoInfo, | ||
| video_tx: mpsc::Sender<FFmpegVideoFrame>, | ||
| stall_health_tx: crate::output_pipeline::PipelineHealthSender, |
There was a problem hiding this comment.
CameraLoopRuntime uses crate::output_pipeline::PipelineHealthSender, but that type does not exist. The module exports HealthSender, which is also what SetupCtx::health_tx() returns. Use HealthSender here; the new field otherwise prevents cap-recording from compiling on every platform.
| stall_health_tx: crate::output_pipeline::PipelineHealthSender, | |
| stall_health_tx: crate::output_pipeline::HealthSender, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/recording/src/sources/camera.rs
Line: 103
Comment:
**Recording crate cannot build**
`CameraLoopRuntime` uses `crate::output_pipeline::PipelineHealthSender`, but that type does not exist. The module exports `HealthSender`, which is also what `SetupCtx::health_tx()` returns. Use `HealthSender` here; the new field otherwise prevents `cap-recording` from compiling on every platform.
```suggestion
stall_health_tx: crate::output_pipeline::HealthSender,
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| let max_total_frames = | ||
| (CONVERTER_POOL_BYTE_BUDGET / avg_frame_bytes).max(MIN_QUEUE_CAPACITY * 3); | ||
|
|
||
| let max_input = (max_total_frames / 3).max(MIN_QUEUE_CAPACITY); | ||
| let max_output = (max_total_frames / 3).max(MIN_QUEUE_CAPACITY); | ||
| let max_pool = (max_total_frames - max_input - max_output).max(MIN_QUEUE_CAPACITY); |
There was a problem hiding this comment.
Queues exceed their byte budgets
The minimum frame counts override both 64 MiB budgets for high-resolution cameras:
- A 3840×2160 BGRA frame uses 33,177,600 bytes, but the camera channel still admits eight frames—about 253 MiB.
- The same conversion gets four input slots, so that queue alone can hold about 127 MiB before counting output frames or workers.
Remove the floors that exceed the byte limits, account for worker-held frames, and make the tests check retained bytes rather than these minimum counts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/frame-converter/src/pool.rs
Line: 81-86
Comment:
**Queues exceed their byte budgets**
The minimum frame counts override both 64 MiB budgets for high-resolution cameras:
- A 3840×2160 BGRA frame uses 33,177,600 bytes, but the camera channel still admits eight frames—about 253 MiB.
- The same conversion gets four input slots, so that queue alone can hold about 127 MiB before counting output frames or workers.
Remove the floors that exceed the byte limits, account for worker-held frames, and make the tests check retained bytes rather than these minimum counts.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if matches!(self.drop_strategy, DropStrategy::DropOldest) { | ||
| let _ = self.input_rx.try_recv(); | ||
| let _ = input_tx.try_send(rejected); | ||
| } | ||
| self.record_input_drop(); |
There was a problem hiding this comment.
submit() counts a drop even when no frame was discarded. After the first send reports Full, workers can empty the queue before try_recv(). If that receive fails and the retry succeeds, every frame survives but frames_dropped still increases.
pending_frame_count() then undercounts unfinished work, so drain_with_timeout() can stop workers while frames remain. Count successful evictions and failed retries separately instead of always adding one drop.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/frame-converter/src/pool.rs
Line: 265-269
Comment:
**Drop counts can end draining**
`submit()` counts a drop even when no frame was discarded. After the first send reports `Full`, workers can empty the queue before `try_recv()`. If that receive fails and the retry succeeds, every frame survives but `frames_dropped` still increases.
`pending_frame_count()` then undercounts unfinished work, so `drain_with_timeout()` can stop workers while frames remain. Count successful evictions and failed retries separately instead of always adding one drop.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| dropped | ||
| ); | ||
| Err(flume::TrySendError::Full(rejected)) => { | ||
| // Evict the oldest queued input frame when configured for DropOldest. |
There was a problem hiding this comment.
The new eviction comment only narrates the next lines. AGENTS.md requires comments to capture non-obvious context and explicitly bans comments that describe what code does.
Remove it, along with the same pattern above bounded_for_conversion and estimate_video_frame_bytes; the pattern also appears elsewhere in the additions. This repository requirement must be satisfied before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/frame-converter/src/pool.rs
Line: 264
Comment:
**Comments repeat the code**
The new eviction comment only narrates the next lines. `AGENTS.md` requires comments to capture non-obvious context and explicitly bans comments that describe what code does.
Remove it, along with the same pattern above `bounded_for_conversion` and `estimate_video_frame_bytes`; the pattern also appears elsewhere in the additions. This repository requirement must be satisfied before merging.
**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!
…d count only actual evictions
…rop narrating comments Include worker_count in bounded_for_conversion retained byte accounting, allow 1-frame camera channel bounds for ultra-high-res formats, and remove narrating comments per AGENTS.md.
Summary
Bounds in-flight camera and frame-converter queues by explicit byte budgets and drains macOS Objective-C autorelease pools per frame during recording (#109):
crates/camera/src/macos.rs&crates/frame-converter/src/videotoolbox.rs: Wraps per-frame CoreMedia/CVPixelBuffer callbacks and VideoToolbox conversion passes in explicit autorelease pool drains to prevent retaining transient Objective-C buffers across long sessions.crates/frame-converter/src/pool.rs&crates/recording/src/sources/camera.rs: Enforces bounded byte budgets on converter and camera frame queues with deterministic backpressure metrics inmemory_profiling.rs.Fixes #109
/claim #109
Fix the missing camera-loop type, byte-budget limits, drop accounting, and comments-policy violation before merging.
Findings
Fix with agent prompt
Summary
The PR reduces camera and converter queue capacities, adds macOS camera autorelease pools, reuses VideoToolbox output buffers, and bounds retained timing samples.
Reviews (1) · Last reviewed commit: "perf(recording): bound camera & converte..." · Reviewed by Greptile