Encoder error handling improvements #1305
Conversation
WalkthroughRenames many encoder finish APIs to flush with Result returns; consolidates queue-frame errors and adds macOS retrying; exposes several modules publicly; changes Muxer finish to nested Result types; updates recording/audio APIs and tests to async/actor patterns; workspace lint and CI cache branch guards added. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Muxer
participant VideoEnc as VideoEncoder
participant AudioEnc as AudioEncoder
participant Writer as TrailerWriter
Note left of Caller: New high-level finish/flush flow
Caller->>Muxer: finish(timestamp) -> Result<FinishResult, FinishError>
Muxer->>VideoEnc: flush(output) -> Result<(), ffmpeg::Error>
VideoEnc-->>Muxer: Ok / Err
Muxer->>AudioEnc: flush(output) -> Result<(), ffmpeg::Error>
AudioEnc-->>Muxer: Ok / Err
Muxer->>Writer: write_trailer() -> Result<(), ffmpeg::Error>
Writer-->>Muxer: Ok / Err
Muxer-->>Caller: Ok(FinishResult{video_result,audio_result}) or Err(FinishError)
sequenceDiagram
participant AV as AVFoundationMp4Muxer
participant MP4 as MP4Encoder
participant Caller
Note left of AV: macOS queue retry loop (3 attempts, 3ms)
Caller->>AV: send_video_frame(frame)
AV->>MP4: queue_video_frame(frame) [try 1]
alt NotReadyForMore
MP4-->>AV: NotReadyForMore
AV->>MP4: queue_video_frame(frame) [try 2 after 3ms]
MP4-->>AV: Ok / Err
end
AV-->>Caller: Ok / Err (mapped with context)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing careful review:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
crates/recording/examples/camera.rs (2)
32-36: Remove no-op property calls and guard the receive with a timeout.The unused calls likely trigger clippy’s
unused_must_use. Add a timeout to avoid hanging when no frames arrive.- let frame = rx.recv_async().await.unwrap().inner; - frame.format(); - frame.width(); - frame.height(); + use tokio::time::{timeout, Duration}; + let frame = timeout(Duration::from_secs(5), rx.recv_async()) + .await + .expect("timed out waiting for a camera frame") + .expect("frame channel closed") + .inner;
47-59: Do not assume contiguous RGB24; respect linesize/stride when encoding JPEG.FFmpeg frames can have padding per row. Slicing
data(0)[0..(w*h*3)]risks corrupted output on aligned buffers. Pack rows usinglinesize(0)before encoding.- println!( - "Converted frame data len: {}", - converted_frame.data(0).len() - ); - let mut file = std::fs::File::create("./out.jpeg").unwrap(); - jpeg::JpegEncoder::new(&mut file) - .encode( - &converted_frame.data(0)[0..(frame.width() * frame.height() * 3) as usize], - frame.width(), - frame.height(), - ColorType::Rgb8.into(), - ) - .unwrap(); + let (w, h) = (converted_frame.width(), converted_frame.height()); + let row_bytes = (w * 3) as usize; + let line_size = converted_frame.linesize(0) as usize; + let src = converted_frame.data(0); + let mut packed = Vec::with_capacity(row_bytes * h as usize); + for y in 0..h as usize { + let start = y * line_size; + let end = start + row_bytes; + packed.extend_from_slice(&src[start..end]); + } + println!("Packed RGB size: {}", packed.len()); + let mut file = std::fs::File::create("./out.jpeg").expect("create out.jpeg"); + jpeg::JpegEncoder::new(&mut file) + .encode(&packed, w, h, ColorType::Rgb8.into()) + .expect("JPEG encode failed");crates/export/src/mp4.rs (1)
75-95: Bug: base is moved into the spawn_blocking closure, then used laterThe move || captures base due to base.output_path usage, causing use-after-move. Capture only output_path instead.
Apply:
- let encoder_thread = tokio::task::spawn_blocking(move || { + let output_path_cloned = output_path.clone(); + let encoder_thread = tokio::task::spawn_blocking(move || { @@ - let mut encoder = MP4File::init( - "output", - base.output_path.clone(), + let mut encoder = MP4File::init( + "output", + output_path_cloned.clone(), @@ - Ok::<_, String>(base.output_path) + Ok::<_, String>(output_path_cloned)Also applies to: 118-118
crates/enc-ffmpeg/src/audio/aac.rs (1)
33-35: Bitrate constant/comment mismatch320_000 != “128k”. Fix either the value or the comment.
- const OUTPUT_BITRATE: usize = 320 * 1000; // 128k + // 128 kbps + const OUTPUT_BITRATE: usize = 128 * 1000;crates/enc-ffmpeg/src/video/h264.rs (1)
296-310: Remove invalid encoder option "vsync" at line 307"vsync" is not a libx264 encoder option—it's a muxer/FFmpeg CLI-level setting. Passing it via the encoder's AVDictionary will be ignored or cause errors. Proper timestamp synchronization is already handled by the existing
set_time_base(),set_frame_rate(), and stream rate configuration (lines 181, 199–200). Delete line 307.crates/recording/src/output_pipeline/ffmpeg.rs (1)
126-149: Fix the associated type mismatch in OggMuxer::finish.The
Finishtype is declared asResult<(), ffmpeg::Error>but the implementation returnsanyhow::Result<()>via the?operator inOk(self.0.finish()?), which flattens the nested Result. This creates a type inconsistency where the declared type suggests a nested Result but the actual return is flat.Either:
- Change
type Finish = ();to match what's actually returned, or- Properly return nested Result:
self.0.finish().map_err(|e| anyhow!(e))The current pattern works because callers treat it as a flat Result, but the misleading type signature should be corrected.
🧹 Nitpick comments (10)
crates/scap-ffmpeg/examples/cli.rs (1)
75-75: Consider documenting why errors are ignored or removing the callback.The empty callback
|_, _| {}silently ignores all stop-with-error events. Since this is an example file that users may reference, consider either:
- Adding a comment explaining that errors are now handled internally by the builder, or
- Removing the
.with_stop_with_err_cb()call entirely if it's no longer necessary.This would better demonstrate best practices for users of the library.
Example with documentation:
+ // Errors are handled internally by the capturer; custom callback not needed .with_stop_with_err_cb(|_, _| {})Or if the callback is optional, consider removing it:
- .with_stop_with_err_cb(|_, _| {}) .build()crates/recording/examples/camera.rs (3)
14-14: Make type inference explicit and handle “no cameras” early.Avoid potential inference hiccups and provide a clearer UX when zero devices are found.
- let cameras = cap_camera::list_cameras().map(CameraSelection).collect(); + let cameras: Vec<_> = cap_camera::list_cameras().map(CameraSelection).collect(); + if cameras.is_empty() { + eprintln!("No cameras found."); + return; + }
19-27: Double-await is fine, but replaceunwrapwith propagation orexpect.Prevent example panics and improve diagnostics.
- feed.ask(camera::SetInput { - id: DeviceOrModelID::from_info(&device.0), - }) - .await - .unwrap() - .await - .unwrap(); + feed.ask(camera::SetInput { + id: DeviceOrModelID::from_info(&device.0), + }) + .await + .expect("failed to send SetInput to CameraFeed") + .await + .expect("CameraFeed failed to apply selected device");Optional: switch
async fn main() -> anyhow::Result<()>and use?instead.
28-31: Avoid potential hangs; add a timeout and consider a slightly larger channel.A bounded(1) mailbox plus a slow consumer can block the producer. Add a timeout to first-frame wait to avoid indefinite await if the camera/encoder stalls.
- let (tx, rx) = flume::bounded(1); + let (tx, rx) = flume::bounded(2); // small buffer to reduce backpressure @@ - feed.ask(camera::AddSender(tx)).await.unwrap(); + feed.ask(camera::AddSender(tx)) + .await + .expect("failed to add frame sender");And receive with timeout below (see next comment).
crates/scap-screencapturekit/examples/cli.rs (2)
37-41: Output callback simplification is fineIgnoring frames is acceptable for the example; consider leaving a commented log for quick debugging.
42-42: Don’t swallow stop errors; at least log themSwallowing stream/error hides failures during demos. Minimal visibility helps troubleshooting.
Apply:
- .with_stop_with_err_cb(|_, _| {}) + .with_stop_with_err_cb(|stream, err| { + eprintln!("capture stopped: stream={:?}, err={:?}", stream, err); + })As per coding guidelines.
crates/recording/src/output_pipeline/win.rs (1)
278-278: Flush errors are silently ignored.The audio encoder's
flush()error is explicitly discarded. While the PR description mentions continuing even if flushing fails, silent failure could make debugging difficult.Consider logging the error:
- let _ = audio_encoder.flush(&mut output); + if let Err(e) = audio_encoder.flush(&mut output) { + tracing::error!("Failed to flush audio encoder: {}", e); + }crates/enc-ffmpeg/src/audio/aac.rs (1)
117-119: Avoid confusion between inherent and trait flushBe explicit to call the inherent method from the trait impl.
- fn flush(&mut self, output: &mut format::context::Output) -> Result<(), ffmpeg::Error> { - self.flush(output) - } + fn flush(&mut self, output: &mut format::context::Output) -> Result<(), ffmpeg::Error> { + AACEncoder::flush(self, output) + }crates/recording/src/output_pipeline/macos.rs (2)
97-106: Mirror the video retry improvements for audioApply the same lock-scoping/backoff/jitter/metric suggestions here to avoid blocking video when audio is busy, and vice versa.
89-90: Include timestamp in error context for faster triageAdd the timestamp to anyhow messages.
- .map_err(|e| anyhow!("send_video_frame/{e}")) + .map_err(|e| anyhow!("send_video_frame ts={:?} / {e}", timestamp)) @@ - .map_err(|e| anyhow!("send_audio_frame/{e}")) + .map_err(|e| anyhow!("send_audio_frame ts={:?} / {e}", timestamp))Also applies to: 106-107
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
Cargo.toml(1 hunks)crates/audio/src/latency.rs(1 hunks)crates/enc-avfoundation/src/mp4.rs(9 hunks)crates/enc-ffmpeg/src/audio/aac.rs(2 hunks)crates/enc-ffmpeg/src/audio/audio_encoder.rs(1 hunks)crates/enc-ffmpeg/src/audio/base.rs(1 hunks)crates/enc-ffmpeg/src/audio/mod.rs(1 hunks)crates/enc-ffmpeg/src/audio/opus.rs(3 hunks)crates/enc-ffmpeg/src/lib.rs(1 hunks)crates/enc-ffmpeg/src/mux/mod.rs(1 hunks)crates/enc-ffmpeg/src/mux/mp4.rs(4 hunks)crates/enc-ffmpeg/src/mux/ogg.rs(2 hunks)crates/enc-ffmpeg/src/video/h264.rs(1 hunks)crates/enc-ffmpeg/src/video/mod.rs(0 hunks)crates/export/src/mp4.rs(2 hunks)crates/recording/Cargo.toml(1 hunks)crates/recording/examples/camera.rs(1 hunks)crates/recording/src/output_pipeline/core.rs(4 hunks)crates/recording/src/output_pipeline/ffmpeg.rs(7 hunks)crates/recording/src/output_pipeline/macos.rs(4 hunks)crates/recording/src/output_pipeline/win.rs(1 hunks)crates/recording/src/sources/audio_mixer.rs(3 hunks)crates/rendering/src/zoom.rs(1 hunks)crates/scap-ffmpeg/examples/cli.rs(1 hunks)crates/scap-screencapturekit/Cargo.toml(1 hunks)crates/scap-screencapturekit/examples/cli.rs(2 hunks)crates/timestamp/src/macos.rs(0 hunks)
💤 Files with no reviewable changes (2)
- crates/timestamp/src/macos.rs
- crates/enc-ffmpeg/src/video/mod.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
crates/recording/src/output_pipeline/win.rscrates/enc-ffmpeg/src/audio/audio_encoder.rscrates/rendering/src/zoom.rscrates/scap-screencapturekit/examples/cli.rscrates/recording/src/sources/audio_mixer.rscrates/recording/src/output_pipeline/macos.rscrates/enc-ffmpeg/src/video/h264.rscrates/audio/src/latency.rscrates/enc-ffmpeg/src/audio/base.rscrates/enc-ffmpeg/src/audio/aac.rscrates/enc-ffmpeg/src/mux/mod.rscrates/enc-ffmpeg/src/mux/ogg.rscrates/recording/examples/camera.rscrates/enc-ffmpeg/src/mux/mp4.rscrates/recording/src/output_pipeline/core.rscrates/scap-ffmpeg/examples/cli.rscrates/recording/src/output_pipeline/ffmpeg.rscrates/enc-ffmpeg/src/audio/mod.rscrates/enc-avfoundation/src/mp4.rscrates/export/src/mp4.rscrates/enc-ffmpeg/src/lib.rscrates/enc-ffmpeg/src/audio/opus.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/recording/src/output_pipeline/win.rscrates/enc-ffmpeg/src/audio/audio_encoder.rscrates/rendering/src/zoom.rscrates/recording/src/sources/audio_mixer.rscrates/recording/src/output_pipeline/macos.rscrates/enc-ffmpeg/src/video/h264.rscrates/audio/src/latency.rscrates/enc-ffmpeg/src/audio/base.rscrates/enc-ffmpeg/src/audio/aac.rscrates/enc-ffmpeg/src/mux/mod.rscrates/enc-ffmpeg/src/mux/ogg.rscrates/enc-ffmpeg/src/mux/mp4.rscrates/recording/src/output_pipeline/core.rscrates/recording/src/output_pipeline/ffmpeg.rscrates/enc-ffmpeg/src/audio/mod.rscrates/enc-avfoundation/src/mp4.rscrates/export/src/mp4.rscrates/enc-ffmpeg/src/lib.rscrates/enc-ffmpeg/src/audio/opus.rs
🧠 Learnings (1)
📚 Learning: 2025-10-17T05:58:22.586Z
Learnt from: Brendonovich
PR: CapSoftware/Cap#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.rs
🧬 Code graph analysis (13)
crates/enc-ffmpeg/src/audio/audio_encoder.rs (4)
crates/enc-ffmpeg/src/audio/aac.rs (2)
flush(107-109)flush(117-119)crates/enc-ffmpeg/src/audio/base.rs (1)
flush(39-45)crates/enc-ffmpeg/src/audio/opus.rs (2)
flush(101-103)flush(119-121)crates/enc-ffmpeg/src/audio/buffered_resampler.rs (2)
flush(264-271)output(54-56)
crates/scap-screencapturekit/examples/cli.rs (1)
crates/scap-screencapturekit/src/capture.rs (1)
builder(78-87)
crates/recording/src/sources/audio_mixer.rs (1)
crates/recording/src/output_pipeline/core.rs (5)
new(619-621)new(662-664)new(695-697)new(739-756)timestamp(782-782)
crates/recording/src/output_pipeline/macos.rs (2)
crates/recording/src/output_pipeline/core.rs (2)
finish(802-802)timestamp(782-782)crates/recording/src/output_pipeline/ffmpeg.rs (3)
finish(74-93)finish(148-150)timestamp(22-24)
crates/enc-ffmpeg/src/video/h264.rs (4)
crates/enc-ffmpeg/src/audio/aac.rs (2)
flush(107-109)flush(117-119)crates/enc-ffmpeg/src/audio/audio_encoder.rs (1)
flush(12-12)crates/enc-ffmpeg/src/audio/base.rs (1)
flush(39-45)crates/enc-ffmpeg/src/audio/opus.rs (2)
flush(101-103)flush(119-121)
crates/enc-ffmpeg/src/audio/base.rs (4)
crates/enc-ffmpeg/src/audio/aac.rs (2)
flush(107-109)flush(117-119)crates/enc-ffmpeg/src/audio/audio_encoder.rs (1)
flush(12-12)crates/enc-ffmpeg/src/audio/opus.rs (2)
flush(101-103)flush(119-121)crates/enc-ffmpeg/src/audio/buffered_resampler.rs (2)
flush(264-271)output(54-56)
crates/enc-ffmpeg/src/audio/aac.rs (4)
crates/enc-ffmpeg/src/audio/audio_encoder.rs (1)
flush(12-12)crates/enc-ffmpeg/src/audio/base.rs (1)
flush(39-45)crates/enc-ffmpeg/src/audio/opus.rs (2)
flush(101-103)flush(119-121)crates/enc-ffmpeg/src/audio/buffered_resampler.rs (2)
flush(264-271)output(54-56)
crates/enc-ffmpeg/src/mux/ogg.rs (3)
crates/enc-ffmpeg/src/mux/mp4.rs (2)
finish(111-144)drop(156-158)crates/recording/src/output_pipeline/ffmpeg.rs (2)
finish(74-93)finish(148-150)crates/recording/src/output_pipeline/win.rs (1)
finish(275-281)
crates/recording/examples/camera.rs (3)
apps/cli/src/main.rs (1)
cap_camera(113-113)apps/desktop/src-tauri/src/recording.rs (1)
list_cameras(208-210)crates/recording/src/feeds/camera.rs (1)
from_info(140-144)
crates/enc-ffmpeg/src/mux/mp4.rs (4)
crates/enc-avfoundation/src/mp4.rs (1)
finish(440-481)crates/enc-ffmpeg/src/mux/ogg.rs (1)
finish(48-61)crates/recording/src/output_pipeline/ffmpeg.rs (2)
finish(74-93)finish(148-150)crates/mediafoundation-ffmpeg/src/h264.rs (1)
finish(151-163)
crates/recording/src/output_pipeline/core.rs (8)
crates/recording/src/feeds/microphone.rs (1)
deref(255-257)crates/recording/src/feeds/camera.rs (1)
deref(120-122)crates/enc-ffmpeg/src/mux/mp4.rs (1)
finish(111-144)crates/enc-ffmpeg/src/mux/ogg.rs (1)
finish(48-61)crates/recording/src/output_pipeline/ffmpeg.rs (3)
finish(74-93)finish(148-150)timestamp(22-24)crates/recording/src/output_pipeline/macos.rs (1)
finish(55-61)crates/recording/src/output_pipeline/win.rs (1)
finish(275-281)crates/mediafoundation-ffmpeg/src/h264.rs (1)
finish(151-163)
crates/recording/src/output_pipeline/ffmpeg.rs (4)
crates/enc-ffmpeg/src/mux/mp4.rs (1)
finish(111-144)crates/enc-ffmpeg/src/mux/ogg.rs (1)
finish(48-61)crates/recording/src/output_pipeline/core.rs (2)
finish(802-802)timestamp(782-782)crates/mediafoundation-ffmpeg/src/h264.rs (1)
finish(151-163)
crates/enc-ffmpeg/src/audio/opus.rs (4)
crates/enc-ffmpeg/src/audio/aac.rs (4)
flush(107-109)flush(117-119)send_frame(98-105)send_frame(113-115)crates/enc-ffmpeg/src/audio/audio_encoder.rs (2)
flush(12-12)send_frame(11-11)crates/enc-ffmpeg/src/audio/base.rs (2)
flush(39-45)send_frame(21-37)crates/enc-ffmpeg/src/audio/buffered_resampler.rs (2)
flush(264-271)output(54-56)
⏰ 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). (4)
- GitHub Check: Clippy
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (39)
crates/rendering/src/zoom.rs (1)
243-245: LGTM! Explicit lifetime annotation improves clarity.The explicit lifetime parameter makes the borrowing relationship between the input slice and returned
SegmentsCursorclear and likely addresses a Clippy lint.crates/audio/src/latency.rs (1)
618-618: LGTM! Appropriate suppression for test code.The
unchecked_duration_subtractionlint suppression is reasonable for this test module. The tests use small, known durations (60-300ms) when backdatingInstantvalues for test setup, which are safe operations in controlled test environments.crates/recording/examples/camera.rs (3)
1-4: Imports look good.Module paths and re-exports are idiomatic; no issues.
7-8: LGTM on new imports.Bringing in
kameo::ActorandDisplayis appropriate for the actor flow and selection wrapper.
62-68: Display impl is clean.Using
CameraInfo::display_name()keeps the prompt user-friendly.crates/scap-screencapturekit/examples/cli.rs (1)
8-8: Import of cidre::sc looks goodRequired for ShareableContent; consistent with other examples.
crates/scap-screencapturekit/Cargo.toml (1)
24-24: LGTM!The addition of tokio as a dev-dependency is appropriate for enabling async capabilities in examples and tests.
Cargo.toml (1)
67-67: LGTM!Adding
unused_must_use = "deny"is a good practice that ensures Results and other must-use types are properly handled across the workspace, which complements the error handling improvements in this PR.crates/enc-avfoundation/src/mp4.rs (3)
51-60: LGTM!The consolidation of frame queuing errors into a unified
QueueFrameErrorenum improves consistency. The new variantsConstructandNotReadyForMoreprovide clearer error semantics for distinguishing transient busy states from fatal errors.
217-220: Good addition: Early detection of busy encoder.The check for
is_ready_for_more_media_data()before queuing enables callers to detect when the encoder is busy and implement retry logic, which aligns with the PR's improved AVFoundation error handling.
615-623: No changes needed — handling ofOk(false)is correct.Returning
falsefromappendSampleBufferjust means that particular sample was not accepted and does not always mean the writer has failed. Per Apple documentation, you must checkAVAssetWriter.status; if status is.failedthenAVAssetWriter.errorwill explain why, otherwise the failure can be transient or due to the sample/input state.Your code correctly implements this: it checks
writer.status() == Failedafterappend_sample_bufreturnsOk(false)and only returns an error if the writer has truly failed. Otherwise, it allows the function to returnOk(()), treating the rejection as a transient condition (backpressure, format/timing mismatch, etc.) that will be retried or resolved by caller logic.crates/recording/Cargo.toml (1)
51-51: LGTM!The addition of the
retrycrate supports the implementation of retry logic for handling transient encoder busy states, as described in the PR objectives.crates/enc-ffmpeg/src/lib.rs (1)
2-4: LGTM!The module reorganization and addition of
pub use audio::*;makes the audio module's public API accessible from the crate root, consistent with the existing patterns for video and mux modules.crates/enc-ffmpeg/src/audio/base.rs (1)
39-45: LGTM!The rename from
finishtoflushmore accurately describes the operation (flushing buffered frames), and the addition ofprocess_eofensures proper encoder finalization with error propagation.crates/enc-ffmpeg/src/audio/audio_encoder.rs (1)
12-12: LGTM!The rename to
flushand the addition of Result return type enables proper error propagation during encoder finalization, aligning with FFmpeg conventions where "flush" indicates draining buffered frames.crates/enc-ffmpeg/src/audio/aac.rs (1)
107-109: Flush rename aligns with base; goodDelegation to base.flush with Result is correct.
crates/recording/src/output_pipeline/macos.rs (1)
55-61: Finish signature change acknowledgedReturning Ok(()) as associated Finish keeps Muxer uniform. No issues.
crates/export/src/mp4.rs (1)
3-3: Import paths are valid and require no changesThe
cap_enc_ffmpegcrate properly re-exports all modules at the crate root via wildcard statements:
pub use audio::*;makesaac::AACEncoderaccessiblepub use video::*;makesh264::H264Encoderaccessiblepub use mux::*;makesmp4::*accessibleAudioEncoderis exposed through the audio re-export chainThe import statement at line 3 of
crates/export/src/mp4.rsis correct and does not require alternative paths.Likely an incorrect or invalid review comment.
crates/enc-ffmpeg/src/video/h264.rs (1)
265-266: Rename to flush with error propagation confirmed completeVerification confirms all
H264Encoder::finishcall sites have been updated toflush. The method is properly called atcrates/enc-ffmpeg/src/mux/mp4.rs:120with correct error propagation pattern matching audio encoders. No orphanedfinishreferences remain in the codebase.crates/enc-ffmpeg/src/audio/mod.rs (1)
4-9: Visibility reorganization verified; changes are safe and backward-compatibleThe module visibility changes are coherent:
basekept private—no code imports it (verified safe)buffered_resampler,aac,opusare public—aligns with new API intent- Glob re-export at
lib.rsensures backward compatibility; callers can use bothcap_enc_ffmpeg::audio::opusandcap_enc_ffmpeg::opus- Existing internal imports (e.g.,
crate::audio::opusinmux/ogg.rs) continue to work unchangedNo breaking changes detected.
crates/enc-ffmpeg/src/mux/mod.rs (1)
1-2: Public submodules are an implementation detail; no breaking change hereThe crate already re-exports mux contents at the root level via
pub use mux::*;in lib.rs. Makingpub mod mp4/oggadds an alternative path but doesn't change existing imports (cap_enc_ffmpeg::mp4,cap_enc_ffmpeg::ogg). No breaking change occurs, and compat re-exports are unnecessary.Likely an incorrect or invalid review comment.
crates/recording/src/sources/audio_mixer.rs (1)
482-682: LGTM! Test migration to async is well-executed.The test suite has been successfully migrated from synchronous flume channels to asynchronous tokio mpsc channels. All test assertions remain unchanged, and the async operations (
.send().await,.next(),.try_next()) are correctly applied.crates/recording/src/output_pipeline/ffmpeg.rs (4)
6-6: Good practice: Narrow imports improve clarity.Replacing the glob import with explicit encoder imports makes dependencies clearer and reduces namespace pollution.
33-93: Improved error handling: Per-stream results are now captured.The new
Finishstruct enables callers to inspect individual encoder flush results while ensuring the trailer is always written (unless trailer write itself fails). This aligns with the PR objective: "Muxer finish now writes the header even if encoder flushing fails, ensuring resulting MP4 files are valid."
105-105: Error propagation added for video frame queueing.Good improvement—encoder errors are now properly surfaced to the caller.
115-115: Error propagation added for audio frame sending.Consistent with the video path; encoder errors are now properly surfaced.
crates/enc-ffmpeg/src/audio/opus.rs (2)
11-13: Import update aligns with trait implementation.The addition of
AudioEncoderto imports supports the new trait implementation below.
101-122: Renamed finish→flush and added AudioEncoder trait impl.The method rename from
finishtoflushcorrectly reflects muxer vs. encoder semantics, as noted in the PR description. The AudioEncoder trait implementation is consistent with the AAC encoder pattern (usingDuration::MAXas a placeholder timestamp).crates/enc-ffmpeg/src/mux/mp4.rs (4)
4-4: Import simplified to glob import.The change from specific imports to
tracing::*is acceptable for this crate's usage pattern.
9-9: Import path updated for H264Encoder.The module path change aligns with the refactored encoder module structure.
31-42: Well-structured error and result types.
FinishErrorandFinishResultprovide clear separation between muxer-level failures and per-stream encoder results.
157-157: Drop safely ignores finish errors.This is appropriate for Drop implementations to avoid panics.
crates/recording/src/output_pipeline/core.rs (3)
16-16: Import added for Deref implementation.Supports the new Deref impl for AudioFrame below.
700-706: Deref implementation improves ergonomics.Allowing
AudioFrameto dereference to the innerffmpeg::frame::Audiosimplifies access patterns. The implementation follows the same pattern as other source types in the codebase.
787-802: Associated Finish type enables flexible error reporting.The addition of
type Finishand the updatedfinishsignature allow different muxer implementations to return appropriate finish results—from simple unit returns to detailed per-stream encoder results. This is a well-designed abstraction.crates/enc-ffmpeg/src/mux/ogg.rs (4)
4-4: Import path updated for OpusEncoder.Aligns with the refactored audio module structure.
12-18: Clear error type for finish failures.
FinishErrorprovides explicit variants for muxer-level failures, making error handling more predictable.
48-61: Finish implementation correctly handles errors.The implementation properly:
- Guards against double-finish with
AlreadyFinished- Captures encoder flush results
- Propagates trailer write failures via
WriteTrailerFailed- Returns the encoder flush result for caller inspection
The nested
Result<Result<...>, ...>structure distinguishes muxer-level failures from encoder-level issues, which aligns with the PR's error handling improvements.
66-66: Drop safely ignores finish errors.Appropriate for Drop implementations to avoid panics.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/recording/src/sources/screen_capture/windows.rs (1)
26-28: Replace hardcoded duration withLOG_INTERVALconstant, and remove or clarify unused constants.Verified findings:
- Line 251 hardcodes
Duration::from_secs(5)instead of usingLOG_INTERVAL(defined at line 27)WINDOW_DURATION(line 26) has no usages in this fileMAX_DROP_RATE_THRESHOLD(line 28) has no usages in this fileApply this fix:
tokio_rt.spawn( async move { loop { - tokio::time::sleep(Duration::from_secs(5)).await; + tokio::time::sleep(LOG_INTERVAL).await; debug!( "Captured {} frames", video_frame_counter.load(atomic::Ordering::Relaxed)Remove
WINDOW_DURATIONandMAX_DROP_RATE_THRESHOLDfrom lines 26 and 28, or add documentation if they're intended for future use.
🧹 Nitpick comments (4)
crates/mediafoundation-ffmpeg/src/h264.rs (1)
3-3: Consider using explicit imports instead of wildcard.The file only uses the
info!macro from the tracing crate. Using an explicit import (use tracing::info;) would make the dependencies clearer and align with Rust best practices, which generally discourage wildcard imports for maintainability.Apply this diff to use an explicit import:
-use tracing::*; +use tracing::info;crates/camera-directshow/src/lib.rs (1)
13-13: Consider using explicit imports instead of wildcard.The file only uses the
trace!macro from the tracing crate. Using an explicit import (use tracing::trace;) would make the dependencies clearer and align with Rust best practices, which generally discourage wildcard imports for maintainability.Apply this diff to use an explicit import:
-use tracing::*; +use tracing::trace;crates/audio/src/latency.rs (1)
619-619: Consider usingchecked_sub().expect()for explicit test setup.The module-level suppression of
clippy::unchecked_duration_subtractionis acceptable for test code where the operations are intentional test setup. However, for better clarity and error messages, consider usingchecked_sub().expect()instead:-estimator.last_update_at = Some(Instant::now() - Duration::from_millis(60)); +estimator.last_update_at = Some( + Instant::now() + .checked_sub(Duration::from_millis(60)) + .expect("test duration too large") +);This makes the intent explicit and provides clearer panic messages if the operation fails. That said, the current approach with
#[allow]is acceptable for this test module.crates/recording/src/output_pipeline/win.rs (1)
280-291: Consider adding logging for better observability.The implementation correctly captures the audio encoder flush result and propagates trailer write errors, aligning with the PR objectives. However, for consistency with other muxers (e.g.,
mp4.rs) and improved observability, consider adding tracing logs when flushing the audio encoder and writing the trailer.Apply this diff to add logging:
fn finish(&mut self, _: Duration) -> anyhow::Result<Self::Finish> { let mut output = self.output.lock()?; - let audio_result = self - .audio_encoder - .as_mut() - .map(|enc| enc.flush(&mut output)) - .unwrap_or(Ok(())); + let audio_result = self + .audio_encoder + .as_mut() + .map(|enc| { + tracing::info!("WindowsMuxer: Flushing audio encoder"); + enc.flush(&mut output).inspect_err(|e| { + tracing::error!("Failed to flush audio encoder: {e:#}"); + }) + }) + .unwrap_or(Ok(())); + tracing::info!("WindowsMuxer: Writing trailer"); output.write_trailer()?; Ok(Finish { audio_result }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
crates/audio/src/latency.rs(2 hunks)crates/camera-directshow/src/lib.rs(1 hunks)crates/camera-windows/src/lib.rs(1 hunks)crates/enc-avfoundation/src/mp4.rs(9 hunks)crates/enc-mediafoundation/src/lib.rs(1 hunks)crates/enc-mediafoundation/src/unsafe_send.rs(0 hunks)crates/enc-mediafoundation/src/video/h264.rs(0 hunks)crates/mediafoundation-ffmpeg/src/h264.rs(1 hunks)crates/recording/src/instant_recording.rs(2 hunks)crates/recording/src/lib.rs(2 hunks)crates/recording/src/output_pipeline/win.rs(4 hunks)crates/recording/src/sources/screen_capture/macos.rs(1 hunks)crates/recording/src/sources/screen_capture/mod.rs(0 hunks)crates/recording/src/sources/screen_capture/windows.rs(1 hunks)
💤 Files with no reviewable changes (3)
- crates/recording/src/sources/screen_capture/mod.rs
- crates/enc-mediafoundation/src/video/h264.rs
- crates/enc-mediafoundation/src/unsafe_send.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
crates/camera-windows/src/lib.rscrates/camera-directshow/src/lib.rscrates/recording/src/instant_recording.rscrates/audio/src/latency.rscrates/mediafoundation-ffmpeg/src/h264.rscrates/enc-mediafoundation/src/lib.rscrates/recording/src/lib.rscrates/enc-avfoundation/src/mp4.rscrates/recording/src/sources/screen_capture/macos.rscrates/recording/src/output_pipeline/win.rscrates/recording/src/sources/screen_capture/windows.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/camera-windows/src/lib.rscrates/camera-directshow/src/lib.rscrates/recording/src/instant_recording.rscrates/audio/src/latency.rscrates/mediafoundation-ffmpeg/src/h264.rscrates/enc-mediafoundation/src/lib.rscrates/recording/src/lib.rscrates/enc-avfoundation/src/mp4.rscrates/recording/src/sources/screen_capture/macos.rscrates/recording/src/output_pipeline/win.rscrates/recording/src/sources/screen_capture/windows.rs
🧬 Code graph analysis (3)
crates/recording/src/instant_recording.rs (1)
crates/recording/src/studio_recording.rs (1)
with_excluded_windows(387-390)
crates/recording/src/lib.rs (1)
apps/desktop/src/utils/tauri.ts (2)
LogicalBounds(418-418)WindowId(490-490)
crates/recording/src/output_pipeline/win.rs (6)
crates/enc-ffmpeg/src/video/h264.rs (1)
builder(234-236)crates/recording/src/output_pipeline/core.rs (2)
builder(39-46)finish(802-802)crates/mediafoundation-ffmpeg/src/h264.rs (1)
finish(151-163)crates/enc-ffmpeg/src/mux/mp4.rs (1)
finish(111-144)crates/enc-ffmpeg/src/mux/ogg.rs (1)
finish(48-61)crates/recording/src/output_pipeline/ffmpeg.rs (2)
finish(74-93)finish(148-150)
⏰ 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: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (22)
crates/camera-windows/src/lib.rs (1)
9-9: LGTM! Unused import removed.This cleanup removes the unused
Instantimport while retainingDuration, which is correctly used for frame timestamps.crates/recording/src/sources/screen_capture/macos.rs (1)
10-10: LGTM! Import explicitly declared.Making the
Timestampimport explicit improves code clarity and reduces reliance on the parent module's prelude. The import is correctly positioned and necessary for all theTimestampusages throughout this file.crates/audio/src/latency.rs (1)
233-234: LGTM! Proper platform-specific constant.The
#[cfg(target_os = "macos")]attribute correctly gates this constant to macOS-only builds, where it's used in the fallback latency calculation for wireless devices. The 200ms fallback value is appropriate for wireless audio latency.crates/enc-mediafoundation/src/lib.rs (1)
5-5: ****All four modules in
lib.rs(d3d, media, mft, and video) have public visibility and are treated uniformly. The mft module's public status is consistent with the existing crate architecture, not a unique exposure. The module already contains public items (EncoderDevice struct, enumerate functions), so there is no API expansion concern.Likely an incorrect or invalid review comment.
crates/recording/src/sources/screen_capture/windows.rs (5)
2-2: LGTM! Import cleanup improves clarity.The import reorganization removes unused dependencies and introduces the necessary
CancellationTokenfor the new cancellation-aware logging task.Also applies to: 8-8, 21-21, 23-23
202-203: LGTM! Proper initialization of counter and cancellation token.The use of
Arc<AtomicU32>for the frame counter enables thread-safe counting across closures, and theCancellationTokenis correctly initialized for managing the logging task lifecycle.
209-219: LGTM! Frame counter integration is correct.The atomic counter increment with
Relaxedordering is appropriate for this metrics use case, and the integration doesn't disrupt the existing frame processing logic.
248-260: LGTM! Cancellation-aware logging task is well-structured.The periodic logging task correctly uses
with_cancellation_token_ownedfor graceful cancellation andin_current_span()to maintain tracing context. This provides valuable observability into frame capture performance.
261-261: LGTM! Drop guard pattern ensures proper task cleanup.The
drop_guardensures the logging task is cancelled when the capture thread exits. Explicitly dropping it at line 282 after processing the Stop control provides deterministic cleanup timing.Also applies to: 282-282
crates/recording/src/lib.rs (1)
15-15: LGTM! Import refactoring is correct.The addition of the explicit
LogicalBoundsimport is appropriate and used at line 65.crates/recording/src/instant_recording.rs (2)
254-256: LGTM! Field type qualification is consistent.The field type update aligns with the changes in
lib.rsand maintains consistency across the macOS-specific recording API.
286-290: LGTM! Method signature update is consistent.The
with_excluded_windowsmethod signature correctly reflects the fully qualified type, maintaining consistency with theActorBuilderfield and the broader refactoring.crates/enc-avfoundation/src/mp4.rs (8)
51-60: LGTM! Good error type consolidation.Renaming to
QueueFrameErrorand addingConstructandNotReadyForMorevariants provides better error granularity for both video and audio paths.
213-215: Verify the intentional difference in early-return logic.The video path returns early only when
is_paused(line 213), allowing it to start the writing session (lines 221-225). However, the audio path (lines 282-284) returns early whenis_paused || !is_writing. This means:
- Video frames can initiate the session
- Audio frames are dropped if the session hasn't started
Confirm this asymmetry is intentional.
277-279: LGTM! Good performance optimization.Changing the signature to accept
&frame::Audioinstead offrame::Audioavoids unnecessary copying.
286-288: LGTM! Correct error handling.Returning
QueueFrameError::Failedwhenaudio_inputisNoneis the appropriate behavior.
297-299: LGTM! Past review concern addressed.Returning
Err(QueueFrameError::NotReadyForMore)when the audio input is not ready provides proper backpressure signaling and prevents silent frame drops, aligning with the video path behavior.
310-377: LGTM! Consistent error mapping.Using
QueueFrameError::Constructfor allcidre::os::Errorresults from buffer and format descriptor operations provides consistent error handling throughout the audio path.
402-417: LGTM! Appropriate method signature.Converting
audio_frame_durationto a static method is correct since it doesn't require instance state.
610-626: Good consolidation, but verify theOk(false)case.The helper function centralizes error handling effectively. However, when
append_sample_bufreturnsOk(false)and the writer status is notFailed(line 618), the function returnsOk(())(line 625). This means the caller receives a success signal even though the frame may not have been appended.Since readiness is checked before calling this helper (lines 217-219, 297-299), the
Ok(false)case likely represents an async operation in progress rather than a "not ready" condition. Please confirm this interpretation is correct and that returningOk(())in this scenario won't lead to frame drops.crates/recording/src/output_pipeline/win.rs (2)
44-44: LGTM: Correct trait association.The associated
Finishtype correctly implements theMuxertrait requirement.
3-3: LGTM: Import path updates.The import paths have been updated to use more specific module paths (
aac::AACEncoderandh264::H264Encoder), which aligns with the library reorganization and makes the module structure more explicit.Also applies to: 114-114
Brendonovich
force-pushed
the
handle-all-recording-errors
branch
from
ddfd128 to
11668ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
crates/recording/src/sources/screen_capture/windows.rs (1)
185-185: Fix indentation: replace tabs with spaces.Verification confirms tab characters are present in the file. Rust code must use 4-space indentation per rustfmt standards. The grep output shows tabs in multiple locations:
- Closure body (lines 191–201)
- Async logging task (lines 204–209, 231–241)
- Drop guard setup area (lines 230–264)
Run
cargo fmtto automatically fix all indentation before merging.crates/recording/src/sources/audio_mixer.rs (6)
247-256: Silence filling uses 0 bytes for all formats; U8 silence should be 128.Zeroing bytes yields DC offset for unsigned PCM (e.g., U8). Fill per sample format to avoid audible artifacts and clipping during resample/mix.
Apply a helper and use it at all three sites:
@@ - for i in 0..frame.planes() { - frame.data_mut(i).fill(0); - } + fill_silence(&mut frame, source.info.sample_format); @@ - for i in 0..frame.planes() { - frame.data_mut(i).fill(0); - } + fill_silence(&mut frame, source.info.sample_format); @@ - for i in 0..frame.planes() { - frame.data_mut(i).fill(0); - } + fill_silence(&mut frame, source.info.sample_format);Add the helper near other free fns:
+fn fill_silence(frame: &mut ffmpeg::frame::Audio, sample_format: cap_media_info::Sample) { + // For unsigned PCM, mid‑level (128) is silence; others use 0. + let byte = match sample_format { + cap_media_info::Sample::U8(_) => 128u8, + _ => 0u8, + }; + for i in 0..frame.planes() { + frame.data_mut(i).fill(byte); + } +}Also applies to: 301-312, 367-373
356-382: Start-gap prefill order is reversed due to push_front inside the loop.Pushing each generated frame to the front produces newest→oldest ordering. Drain will feed frames in reverse chronological order.
Prefill into a Vec in forward time, then push_front in reverse:
- let mut remaining = elapsed_since_start; - while remaining > buffer_timeout { + let mut remaining = elapsed_since_start; + let mut to_prepend: Vec<AudioFrame> = Vec::new(); + while remaining > buffer_timeout { let chunk_samples = samples_for_timeout(rate, buffer_timeout); let frame_duration = duration_from_samples(chunk_samples, rate); @@ - source.buffer_last = Some((timestamp, frame_duration)); - source.buffer.push_front(AudioFrame::new(frame, timestamp)); + source.buffer_last = Some((timestamp, frame_duration)); + to_prepend.push(AudioFrame::new(frame, timestamp)); @@ - } + } + for f in to_prepend.into_iter().rev() { + source.buffer.push_front(f); + }
416-422: Backpressure: treating any try_send error as terminal stops the mixer.Distinguish Full (transient) from Closed (terminal). On Full, yield and continue next tick.
+use futures::channel::mpsc::TrySendError; @@ - if self - .output - .try_send(AudioFrame::new(filtered, Timestamp::Instant(timestamp))) - .is_err() - { - return Err(()); - } + match self + .output + .try_send(AudioFrame::new(filtered, Timestamp::Instant(timestamp))) + { + Ok(()) => {} + Err(TrySendError::Full(_)) => { + // Drop frame or yield; here we yield and try again next tick. + debug!("AudioMixer output channel full; yielding"); + break; + } + Err(TrySendError::Closed(_)) => { + info!("AudioMixer output channel closed; stopping"); + return Err(()); + } + }
171-203: Timestamp origin mismatch: run() computes ‘start’ before build(); tick() should align with mixer.timestamps.Small drift can occur if build() advances time before self.timestamps is captured. Use mixer.timestamps for tick origin.
- let start = Timestamps::now(); - - let mut mixer = match self.build(output) { + let mut mixer = match self.build(output) { Ok(mixer) => mixer, @@ - let _ = ready_tx.send(Ok(())); + let _ = ready_tx.send(Ok(())); + let start = mixer.timestamps;Optionally, remove the start param from tick() and use self.timestamps internally to avoid future misuse.
Also applies to: 411-413
529-531: Unsafe transmute reads incorrect length; use proper byte count or cast_slice.byte_count omits sizeof::(), so the &[f32] slice length is wrong, causing UB.
- let byte_count = frame.samples() * frame.channels() as usize; - let samples: &[f32] = unsafe { std::mem::transmute(&frame.data(0)[0..byte_count]) }; + let bytes = frame.data(0); + // Each f32 is 4 bytes; cast with alignment check. + let samples: &[f32] = bytemuck::try_cast_slice(bytes) + .expect("Audio frame data not f32‑aligned/len not multiple of 4");Add a dev-dependency and import in the test module:
+[dev-dependencies] +bytemuck = "1"- use futures::{SinkExt, StreamExt}; + use futures::{SinkExt, StreamExt}; + use bytemuck;If avoiding new deps, compute length via from_raw_parts with correct len = bytes.len()/4 and alignment assertions.
68-111: Fix unhandled error cases in filter graph wiring and validate empty sources.The code currently ignores errors from
link()calls (lines 106, 139, 142, 143) despite callingfilter_graph.add()andfilter_graph.validate()with?operators elsewhere. Additionally,filter::find()uses.expect()(lines 92, 98, 113, 127, 133), which panics if a filter isn't found, and there's no validation for empty sources before constructingamixwithinputs=abuffers.len().
- Add an early guard: if
self.sources.is_empty(), return an appropriate error rather than passinginputs=0to amix.- Propagate
link()errors with?operator (all four calls).- Replace
.expect()onfilter::find()with proper error handling (e.g.,.ok_or(...)with an appropriate error variant).Verify the correct
ffmpeg::Errorvariant to use for filter-not-found and empty-sources cases in your ffmpeg-next fork before finalizing.
♻️ Duplicate comments (1)
crates/enc-ffmpeg/src/mux/mp4.rs (1)
120-132: Fix log messages: use “flush”, and correct the audio path label.Use terminology consistent with encoder APIs and fix the copy/paste in the audio branch.
Apply this diff:
- let video_finish = self.video.flush(&mut self.output).inspect_err(|e| { - error!("Failed to finish video encoder: {e:#}"); - }); + let video_finish = self.video.flush(&mut self.output).inspect_err(|e| { + error!("Failed to flush video encoder: {e:#}"); + }); @@ - enc.flush(&mut self.output).inspect_err(|e| { - error!("Failed to finish video encoder: {e:#}"); - }) + enc.flush(&mut self.output).inspect_err(|e| { + error!("Failed to flush audio encoder: {e:#}"); + })Run to find any remaining “Failed to finish … encoder” logs:
#!/bin/bash rg -nP 'Failed to finish (video|audio) encoder'
🧹 Nitpick comments (10)
crates/recording/src/sources/screen_capture/windows.rs (1)
26-27: Remove commented constants or use them.Lines 26-27 comment out
LOG_INTERVAL, yet Line 233 hardcodesDuration::from_secs(5). Either uncomment and use the constant for maintainability, or remove the commented-out constants entirely.Apply this diff to use the constant:
-// const WINDOW_DURATION: Duration = Duration::from_secs(3); -// const LOG_INTERVAL: Duration = Duration::from_secs(5); +const LOG_INTERVAL: Duration = Duration::from_secs(5); const MAX_DROP_RATE_THRESHOLD: f64 = 0.25;Then at Line 233:
- tokio::time::sleep(Duration::from_secs(5)).await; + tokio::time::sleep(LOG_INTERVAL).await;Alternatively, remove the commented constants if they're no longer needed.
Also applies to: 233-233
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
189-208: Consider logging Windows API failures for debugging.The Windows API calls now explicitly discard their return values to satisfy Clippy warnings. While this is acceptable for these UI operations (which can legitimately fail in normal circumstances), adding error logging would help with debugging when window focusing doesn't work as expected.
Consider applying this diff to add logging:
if GetWindowPlacement(hwnd, &mut wp).is_ok() { // Restore using the previous placement to avoid resizing wp.showCmd = SW_RESTORE.0 as u32; - let _ = SetWindowPlacement(hwnd, &wp); + if let Err(e) = SetWindowPlacement(hwnd, &wp) { + error!("Failed to restore window placement: {e}"); + } } else { // Fallback to simple restore if placement fails - let _ = ShowWindow(hwnd, SW_RESTORE); + if ShowWindow(hwnd, SW_RESTORE) == false.into() { + error!("Failed to show window"); + } } } // Always try to bring to foreground - let _ = SetForegroundWindow(hwnd); + if SetForegroundWindow(hwnd) == false.into() { + error!("Failed to set window to foreground"); + }Can you confirm whether silent failures are acceptable here, or if logging would be beneficial for debugging window focus issues?
crates/enc-avfoundation/src/mp4.rs (1)
646-655: Consider making the polling interval configurable.The 2ms sleep in the polling loop is hardcoded. While reasonable for most cases, making this configurable could be beneficial for testing or different performance profiles.
+const WRITER_POLL_INTERVAL_MS: u64 = 2; + fn wait_for_writer_finished(writer: &av::AssetWriter) -> Result<(), ()> { use av::asset::writer::Status; loop { match writer.status() { Status::Completed | Status::Cancelled => return Ok(()), Status::Failed | Status::Unknown => return Err(()), - Status::Writing => std::thread::sleep(Duration::from_millis(2)), + Status::Writing => std::thread::sleep(Duration::from_millis(WRITER_POLL_INTERVAL_MS)), } } }crates/recording/src/output_pipeline/ffmpeg.rs (2)
33-36: Remove unused Finish struct.Dead code; will trigger clippy’s dead_code.
Apply this diff:
-pub struct Finish { - pub video_result: Result<(), ffmpeg::Error>, - pub audio_result: Result<(), ffmpeg::Error>, -}
159-160: Flatten Ok wrapping for clarity.Avoid Ok(self.0.queue_frame(...)?) to reduce cognitive load.
Apply this diff:
- Ok(self.0.queue_frame(frame.inner, timestamp)?) + self.0.queue_frame(frame.inner, timestamp)?; + Ok(())crates/recording/src/sources/audio_mixer.rs (5)
388-396: Unify timestamp base inside tick().tick(start, now) mixes ‘start’ with self.timestamps. Prefer using self.timestamps consistently; or assert start == self.timestamps to catch misuse.
- fn tick(&mut self, start: Timestamps, now: Timestamp) -> Result<(), ()> { + fn tick(&mut self, start: Timestamps, now: Timestamp) -> Result<(), ()> { @@ - let timestamp = start.instant() + start_timestamp.duration_since(start) + elapsed; + let timestamp = self.timestamps.instant() + + start_timestamp.duration_since(self.timestamps) + + elapsed;Optionally remove the start parameter entirely in a follow-up.
Also applies to: 411-413
522-526: Use mixer.timestamps in tests for consistency with production path.You already do this elsewhere; mirror it here to avoid timestamp origin drift.
- let _ = mixer.tick( - start, + let _ = mixer.tick( + mixer.timestamps, Timestamp::Instant(start.instant() + Duration::from_secs_f64(4.0 / SAMPLE_RATE as f64)), );
482-486: Minor: tests import only what’s needed.You can drop SinkExt in tests that never call send().await, but keep it where used. Keeps clippy happy.
56-66: Builder silently accepts zero buffer_size/sample_rate.buffer_timeout_for falls back to default, which is fine. Consider logging at debug level when defaults kick in to aid troubleshooting device quirks.
121-130: Nit: aformat args can be computed once as constants.Given INFO is const, you can make aformat args a &'static str via lazy const to avoid formatting per build.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
.github/workflows/ci.yml(2 hunks).github/workflows/publish.yml(1 hunks)Cargo.toml(1 hunks)apps/desktop/src-tauri/src/target_select_overlay.rs(1 hunks)crates/audio/src/latency.rs(2 hunks)crates/camera-directshow/src/lib.rs(1 hunks)crates/camera-windows/src/lib.rs(1 hunks)crates/enc-avfoundation/src/mp4.rs(12 hunks)crates/enc-ffmpeg/src/audio/aac.rs(2 hunks)crates/enc-ffmpeg/src/audio/audio_encoder.rs(1 hunks)crates/enc-ffmpeg/src/audio/base.rs(1 hunks)crates/enc-ffmpeg/src/audio/mod.rs(1 hunks)crates/enc-ffmpeg/src/audio/opus.rs(3 hunks)crates/enc-ffmpeg/src/lib.rs(1 hunks)crates/enc-ffmpeg/src/mux/mod.rs(1 hunks)crates/enc-ffmpeg/src/mux/mp4.rs(4 hunks)crates/enc-ffmpeg/src/mux/ogg.rs(2 hunks)crates/enc-ffmpeg/src/video/h264.rs(1 hunks)crates/enc-ffmpeg/src/video/mod.rs(0 hunks)crates/enc-mediafoundation/src/lib.rs(1 hunks)crates/enc-mediafoundation/src/unsafe_send.rs(0 hunks)crates/enc-mediafoundation/src/video/h264.rs(0 hunks)crates/export/src/mp4.rs(2 hunks)crates/mediafoundation-ffmpeg/src/h264.rs(1 hunks)crates/recording/Cargo.toml(1 hunks)crates/recording/examples/camera.rs(1 hunks)crates/recording/src/instant_recording.rs(2 hunks)crates/recording/src/lib.rs(2 hunks)crates/recording/src/output_pipeline/core.rs(4 hunks)crates/recording/src/output_pipeline/ffmpeg.rs(6 hunks)crates/recording/src/output_pipeline/macos.rs(3 hunks)crates/recording/src/output_pipeline/win.rs(3 hunks)crates/recording/src/sources/audio_mixer.rs(3 hunks)crates/recording/src/sources/screen_capture/macos.rs(1 hunks)crates/recording/src/sources/screen_capture/mod.rs(0 hunks)crates/recording/src/sources/screen_capture/windows.rs(1 hunks)crates/recording/src/studio_recording.rs(2 hunks)crates/rendering/src/zoom.rs(1 hunks)crates/scap-ffmpeg/examples/cli.rs(1 hunks)crates/scap-screencapturekit/Cargo.toml(1 hunks)crates/scap-screencapturekit/examples/cli.rs(2 hunks)crates/timestamp/src/macos.rs(0 hunks)
💤 Files with no reviewable changes (5)
- crates/enc-ffmpeg/src/video/mod.rs
- crates/timestamp/src/macos.rs
- crates/enc-mediafoundation/src/video/h264.rs
- crates/recording/src/sources/screen_capture/mod.rs
- crates/enc-mediafoundation/src/unsafe_send.rs
🚧 Files skipped from review as they are similar to previous changes (15)
- crates/camera-directshow/src/lib.rs
- .github/workflows/publish.yml
- crates/scap-screencapturekit/Cargo.toml
- crates/camera-windows/src/lib.rs
- crates/enc-ffmpeg/src/mux/mod.rs
- crates/mediafoundation-ffmpeg/src/h264.rs
- crates/recording/examples/camera.rs
- crates/scap-ffmpeg/examples/cli.rs
- crates/enc-ffmpeg/src/lib.rs
- crates/recording/src/sources/screen_capture/macos.rs
- crates/enc-ffmpeg/src/audio/aac.rs
- crates/enc-ffmpeg/src/video/h264.rs
- crates/export/src/mp4.rs
- crates/enc-mediafoundation/src/lib.rs
- .github/workflows/ci.yml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/target_select_overlay.rscrates/recording/src/lib.rscrates/enc-ffmpeg/src/audio/audio_encoder.rscrates/recording/src/studio_recording.rscrates/recording/src/output_pipeline/win.rscrates/recording/src/sources/audio_mixer.rscrates/enc-ffmpeg/src/mux/ogg.rscrates/recording/src/instant_recording.rscrates/scap-screencapturekit/examples/cli.rscrates/recording/src/output_pipeline/macos.rscrates/recording/src/output_pipeline/ffmpeg.rscrates/recording/src/output_pipeline/core.rscrates/recording/src/sources/screen_capture/windows.rscrates/audio/src/latency.rscrates/rendering/src/zoom.rscrates/enc-avfoundation/src/mp4.rscrates/enc-ffmpeg/src/mux/mp4.rscrates/enc-ffmpeg/src/audio/mod.rscrates/enc-ffmpeg/src/audio/opus.rscrates/enc-ffmpeg/src/audio/base.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/recording/src/lib.rscrates/enc-ffmpeg/src/audio/audio_encoder.rscrates/recording/src/studio_recording.rscrates/recording/src/output_pipeline/win.rscrates/recording/src/sources/audio_mixer.rscrates/enc-ffmpeg/src/mux/ogg.rscrates/recording/src/instant_recording.rscrates/recording/src/output_pipeline/macos.rscrates/recording/src/output_pipeline/ffmpeg.rscrates/recording/src/output_pipeline/core.rscrates/recording/src/sources/screen_capture/windows.rscrates/audio/src/latency.rscrates/rendering/src/zoom.rscrates/enc-avfoundation/src/mp4.rscrates/enc-ffmpeg/src/mux/mp4.rscrates/enc-ffmpeg/src/audio/mod.rscrates/enc-ffmpeg/src/audio/opus.rscrates/enc-ffmpeg/src/audio/base.rs
🧠 Learnings (3)
📚 Learning: 2025-10-28T08:07:55.375Z
Learnt from: Brendonovich
PR: CapSoftware/Cap#1305
File: crates/scap-screencapturekit/examples/cli.rs:32-36
Timestamp: 2025-10-28T08:07:55.375Z
Learning: Example files (in paths containing `/examples/`) don't need to follow the same strict patterns as production code. Pragmatic and simpler patterns are acceptable in examples even if they wouldn't be ideal for production use.
Applied to files:
crates/scap-screencapturekit/examples/cli.rs
📚 Learning: 2025-10-28T08:39:42.216Z
Learnt from: Brendonovich
PR: CapSoftware/Cap#1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.216Z
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.rscrates/recording/src/output_pipeline/ffmpeg.rscrates/enc-avfoundation/src/mp4.rs
📚 Learning: 2025-10-17T05:58:22.586Z
Learnt from: Brendonovich
PR: CapSoftware/Cap#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.rs
🧬 Code graph analysis (15)
crates/recording/src/lib.rs (1)
apps/desktop/src/utils/tauri.ts (2)
LogicalBounds(418-418)WindowId(490-490)
crates/enc-ffmpeg/src/audio/audio_encoder.rs (4)
crates/enc-ffmpeg/src/audio/aac.rs (2)
flush(107-109)flush(117-119)crates/enc-ffmpeg/src/audio/base.rs (1)
flush(39-45)crates/enc-ffmpeg/src/audio/opus.rs (2)
flush(101-103)flush(119-121)crates/enc-ffmpeg/src/audio/buffered_resampler.rs (2)
flush(264-271)output(54-56)
crates/recording/src/studio_recording.rs (1)
crates/recording/src/instant_recording.rs (1)
with_excluded_windows(287-290)
crates/recording/src/output_pipeline/win.rs (5)
crates/recording/src/output_pipeline/core.rs (1)
finish(807-807)crates/enc-ffmpeg/src/mux/ogg.rs (1)
finish(52-65)crates/recording/src/output_pipeline/ffmpeg.rs (2)
finish(73-95)finish(149-154)crates/recording/src/output_pipeline/macos.rs (1)
finish(54-61)crates/enc-ffmpeg/src/mux/mp4.rs (1)
finish(111-144)
crates/recording/src/sources/audio_mixer.rs (2)
crates/recording/src/output_pipeline/core.rs (7)
new(625-627)new(668-670)new(701-703)new(745-762)start(728-730)start(778-780)timestamp(788-788)crates/timestamp/src/lib.rs (2)
now(103-112)duration_since(24-33)
crates/enc-ffmpeg/src/mux/ogg.rs (2)
crates/enc-ffmpeg/src/audio/opus.rs (1)
queue_frame(92-99)crates/recording/src/output_pipeline/ffmpeg.rs (3)
timestamp(22-24)finish(73-95)finish(149-154)
crates/recording/src/instant_recording.rs (1)
crates/recording/src/studio_recording.rs (1)
with_excluded_windows(386-389)
crates/scap-screencapturekit/examples/cli.rs (1)
crates/scap-screencapturekit/src/capture.rs (1)
builder(78-87)
crates/recording/src/output_pipeline/macos.rs (3)
crates/enc-ffmpeg/src/mux/ogg.rs (1)
finish(52-65)crates/recording/src/output_pipeline/core.rs (2)
finish(807-807)timestamp(788-788)crates/recording/src/output_pipeline/ffmpeg.rs (3)
finish(73-95)finish(149-154)timestamp(22-24)
crates/recording/src/output_pipeline/ffmpeg.rs (6)
crates/enc-ffmpeg/src/mux/ogg.rs (1)
finish(52-65)crates/recording/src/output_pipeline/core.rs (2)
finish(807-807)timestamp(788-788)crates/recording/src/output_pipeline/macos.rs (1)
finish(54-61)crates/recording/src/output_pipeline/win.rs (1)
finish(275-289)crates/enc-avfoundation/src/mp4.rs (1)
finish(450-496)crates/enc-ffmpeg/src/mux/mp4.rs (1)
finish(111-144)
crates/recording/src/output_pipeline/core.rs (4)
crates/enc-ffmpeg/src/mux/ogg.rs (1)
finish(52-65)crates/recording/src/output_pipeline/ffmpeg.rs (3)
finish(73-95)finish(149-154)timestamp(22-24)crates/recording/src/output_pipeline/macos.rs (1)
finish(54-61)crates/recording/src/output_pipeline/win.rs (1)
finish(275-289)
crates/enc-avfoundation/src/mp4.rs (4)
crates/recording/src/output_pipeline/core.rs (2)
timestamp(788-788)finish(807-807)crates/recording/src/output_pipeline/ffmpeg.rs (3)
timestamp(22-24)finish(73-95)finish(149-154)crates/recording/src/output_pipeline/macos.rs (1)
finish(54-61)crates/recording/src/output_pipeline/win.rs (1)
finish(275-289)
crates/enc-ffmpeg/src/mux/mp4.rs (2)
crates/enc-ffmpeg/src/mux/ogg.rs (1)
finish(52-65)crates/recording/src/output_pipeline/ffmpeg.rs (2)
finish(73-95)finish(149-154)
crates/enc-ffmpeg/src/audio/opus.rs (4)
crates/enc-ffmpeg/src/audio/aac.rs (4)
flush(107-109)flush(117-119)send_frame(98-105)send_frame(113-115)crates/enc-ffmpeg/src/audio/audio_encoder.rs (2)
flush(12-12)send_frame(11-11)crates/enc-ffmpeg/src/audio/base.rs (2)
flush(39-45)send_frame(21-37)crates/enc-ffmpeg/src/audio/buffered_resampler.rs (2)
flush(264-271)output(54-56)
crates/enc-ffmpeg/src/audio/base.rs (4)
crates/enc-ffmpeg/src/audio/aac.rs (2)
flush(107-109)flush(117-119)crates/enc-ffmpeg/src/audio/audio_encoder.rs (1)
flush(12-12)crates/enc-ffmpeg/src/audio/opus.rs (2)
flush(101-103)flush(119-121)crates/enc-ffmpeg/src/audio/buffered_resampler.rs (2)
flush(264-271)output(54-56)
⏰ 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: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (41)
crates/rendering/src/zoom.rs (1)
243-243: LGTM! Explicit lifetime improves clarity.The explicit lifetime annotation makes the borrow relationship between the input slice and returned cursor more obvious, and likely addresses a clippy warning mentioned in the PR objectives.
Cargo.toml (1)
67-67: LGTM! Enforcing must_use handling improves code quality.The addition of
unused_must_use = "deny"at the workspace level is an excellent practice that aligns with the PR's focus on improving error handling. This will ensure that Result types and other#[must_use]values are explicitly handled across the codebase.crates/audio/src/latency.rs (2)
233-234: LGTM! macOS-specific wireless latency constant is appropriate.The addition of
WIRELESS_FALLBACK_LATENCY_SECSfor macOS provides a more accurate fallback for wireless audio devices (200ms vs 30ms for wired). This aligns with the documented wireless audio delays mentioned in the module header (120ms+ for Bluetooth).
619-619: LGTM! Test lint suppression is appropriate.Suppressing
clippy::unchecked_duration_subtractionin tests is reasonable, as test code controls the timing values and ensures they don't underflow. This aligns with the workspace-level lint enforcement being added in this PR.crates/recording/src/instant_recording.rs (2)
255-255: LGTM! Consistent type qualification for WindowId.The change to
Vec<scap_targets::WindowId>provides explicit type qualification and aligns with the broader refactoring to unify WindowId usage across macOS-specific code paths.
287-290: LGTM! Method signature updated consistently.The
with_excluded_windowsmethod signature correctly uses the fully qualifiedVec<scap_targets::WindowId>type, maintaining consistency with the struct field and other modules.crates/recording/src/lib.rs (2)
15-15: LGTM! Import simplified to use qualified WindowId.Removing
WindowIdfrom the import statement and using the fully qualifiedscap_targets::WindowIdpath throughout the codebase improves clarity and consistency.
51-51: LGTM! Type qualification is consistent with other modules.The change to
Vec<scap_targets::WindowId>aligns with the refactoring across instant_recording.rs and studio_recording.rs. Note that a previous review comment indicated thatscreen_capture/mod.rsmay also need similar updates, though that file is not included in this review batch.crates/recording/src/studio_recording.rs (2)
348-348: LGTM! Struct field type updated consistently.The
excluded_windowsfield now uses the fully qualifiedVec<scap_targets::WindowId>type, consistent with the changes ininstant_recording.rsandlib.rs.
386-389: LGTM! Method signature updated to match struct field.The
with_excluded_windowsmethod signature correctly usesVec<scap_targets::WindowId>, maintaining consistency across the recording module's public API.crates/recording/Cargo.toml (1)
51-51: Retry dependency is properly used in the codebase.Verification confirms the
retrycrate is actively used incrates/recording/src/output_pipeline/macos.rswith proper imports and multiple usage sites for error handling. The dependency aligns with the PR objectives for improved AVFoundation error handling.crates/scap-screencapturekit/examples/cli.rs (1)
8-53: Example code patterns are acceptable per learning.The changes update this example to use async patterns with
#[tokio::main]and introducecidre::scusage. While there are mixing concerns withblock_on/.await, display shadowing, andstd::thread::sleepin async context, these were previously raised and you indicated that pragmatic patterns are acceptable in example files.Based on learnings.
crates/recording/src/sources/screen_capture/windows.rs (1)
2-2: LGTM: Import cleanup aligns with new cancellation pattern.The import changes appropriately remove unused symbols and add
CancellationTokensupport for the new task cancellation scaffolding.Also applies to: 8-8, 21-21, 23-23
crates/recording/src/output_pipeline/core.rs (3)
365-371: LGTM: Nested Result handling correctly distinguishes muxer vs encoder failures.The code correctly extracts the inner
Resultfrom muxer finish and logs encoder flush failures as warnings, allowing the pipeline to complete successfully even when encoder flushing encounters errors. This aligns with the PR objective of ensuring MP4 files remain valid even if encoder flushing fails.
706-712: LGTM: Idiomatic Deref implementation.The
Derefimplementation enables ergonomic access to the underlyingffmpeg::frame::Audio, allowing callers to useAudioFrameas if it were anAudiodirectly.
807-807: LGTM: Nested Result signature enables proper error layering.The updated signature
anyhow::Result<anyhow::Result<()>>allows implementations to distinguish between muxer-level failures (outerResult) and encoder stream failures (innerResult), supporting the PR's goal of writing valid MP4 trailers even when encoder flushing fails.crates/enc-avfoundation/src/mp4.rs (4)
218-283: LGTM: Video frame queuing with proper backpressure.The early
NotReadyForMorecheck properly signals backpressure to callers before attempting to append frames, enabling the retry logic in the muxer layer to function correctly.
287-395: LGTM: Audio queuing now consistent with video path.The audio path now returns
Err(QueueFrameError::NotReadyForMore)when the encoder is busy, resolving the inconsistency flagged in the previous review where audio silently dropped frames while video signaled backpressure.
450-496: LGTM: Finish implementation with proper error handling.The implementation correctly handles edge cases and propagates errors appropriately. Best-effort final frame queuing with error logging ensures video extension attempts don't block successful completion.
499-503: LGTM: Drop safely ignores finish result.Ignoring the
finishresult inDropis correct, as destructors must not panic and any errors will have been logged by thefinishmethod itself.crates/enc-ffmpeg/src/audio/audio_encoder.rs (1)
12-12: LGTM: Method rename aligns with encoder terminology.Renaming
finishtoflushcorrectly reflects that encoders flush buffered frames, while muxers finish writing. This aligns with the PR objective of distinguishing between encoder and muxer concepts.crates/enc-ffmpeg/src/audio/base.rs (1)
39-45: LGTM: Flush implementation preserves correct EOF handling.The renamed
flushmethod correctly processes remaining buffered frames and sends EOF to the encoder, ensuring clean stream termination.crates/recording/src/output_pipeline/win.rs (2)
3-3: LGTM: Import paths updated for new module structure.The import path change to
cap_enc_ffmpeg::aac::AACEncoderand tocap_enc_ffmpeg::h264::H264Encodercorrectly reflect the module reorganization where audio/video encoder submodules are now explicitly public rather than re-exported.Also applies to: 109-109
275-289: LGTM: Windows muxer finish correctly implements nested Result pattern.The implementation flushes the audio encoder, writes the trailer, then returns the encoder result as the inner
Result. This ensures the trailer is written even if encoder flushing fails, producing a valid (though potentially incorrect) MP4 file as intended by the PR objectives.crates/enc-ffmpeg/src/audio/mod.rs (1)
4-9: LGTM: Explicit public submodules improve API clarity.Making
buffered_resampler,aac, andopusexplicitly public (while keepingbaseprivate) provides better API encapsulation and allows consumers to import specific audio encoder types directly rather than through blanket re-exports.crates/enc-ffmpeg/src/audio/opus.rs (3)
11-13: LGTM: Import paths updated for explicit submodule structure.The import paths now explicitly reference
audio_encoder,base, andbuffered_resamplerfromcrate::audio, consistent with the module visibility changes inaudio/mod.rs.
101-103: LGTM: Flush method correctly delegates to base.The renamed method maintains the same delegation pattern to
base.flush, ensuring consistent frame flushing behavior across all audio encoders.
114-122: LGTM: AudioEncoder trait implementation with expected delegation.The trait implementation provides
send_frameandflushmethods that correctly delegate to the encoder's internal methods. TheDuration::MAXtimestamp insend_frameis likely handled appropriately by the base encoder for timestamp-agnostic encoding scenarios.crates/enc-ffmpeg/src/mux/ogg.rs (4)
4-4: LGTM: updated Opus import path.Matches the new crate::audio::opus layout.
12-18: LGTM: concise FinishError surface.Good idempotency variant and trailer error mapping.
52-65: LGTM: finish is idempotent; trailer errors propagate.Flush result is preserved even if writing the trailer succeeds/fails appropriately.
70-71: Drop behavior is fine.Ignoring finish result in Drop avoids panics on shutdown.
crates/enc-ffmpeg/src/mux/mp4.rs (5)
31-37: LGTM: FinishError mirrors ogg and propagates trailer failures.
39-43: LGTM: FinishResult captures per-stream flush outcomes.
111-114: LGTM: finish is idempotent.
135-139: LGTM: trailer write errors are returned.
157-157: LGTM: swallow finish in Drop.crates/recording/src/output_pipeline/ffmpeg.rs (4)
73-95: LGTM: nested Result finish semantics with trailer propagation.Matches muxer policy: write trailer even if flush fails; propagate trailer failure.
107-107: LGTM: propagate video queue_frame errors.
117-117: LGTM: propagate audio send_frame errors.
149-154: LGTM: delegate Ogg finish with proper error mapping.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/export/src/mp4.rs (1)
114-123: Handle per-stream finish results without early return; log warnings for partial failures.The current implementation returns an error on the first failed stream (video or audio), which doesn't align with the PR objective: "Muxer 'finish' now writes the header even if encoder flushing fails, ensuring resulting MP4 files are valid (though possibly incorrect)."
According to the PR description and the past review comment, the muxer should complete the finish process and log warnings for individual stream failures rather than failing early. The nested
Result<FinishResult, FinishError>structure supports this - the outer error represents unrecoverable muxer failures (like trailer write failures), while the innerFinishResultcontains per-stream results that should be logged but not propagate as fatal errors.Apply this diff to log per-stream errors without failing the export:
let res = encoder .finish() .map_err(|e| format!("Failed to finish encoding: {e}"))?; - if let Err(e) = res.video_finish { - return Err(format!("Video encoding failed: {e}")); - } - if let Err(e) = res.audio_finish { - return Err(format!("Audio encoding failed: {e}")); - } + if let Err(e) = res.video_finish { + warn!("Video flush error during finish (file is still finalized): {e}"); + } + if let Err(e) = res.audio_finish { + warn!("Audio flush error during finish (file is still finalized): {e}"); + }
🧹 Nitpick comments (3)
crates/enc-ffmpeg/src/mux/mp4.rs (1)
4-4: Consider explicit tracing imports for clarity.Using glob imports (
use tracing::*;) can reduce code clarity by obscuring which items are imported. While this is a common pattern for tracing macros, consider importing specific items explicitly.Apply this diff if you prefer explicit imports:
-use tracing::*; +use tracing::{error, info, trace};crates/recording/src/output_pipeline/ffmpeg.rs (1)
6-6: Consider explicit imports for h264 and ogg modules.Using glob imports (
h264::*,ogg::*) can reduce clarity by obscuring which items are imported. While this works, consider importing specific items explicitly.For example:
use cap_enc_ffmpeg::{ aac::AACEncoder, h264::{H264Encoder, QueueFrameError}, ogg::OggFile, opus::OpusEncoder };crates/enc-avfoundation/src/mp4.rs (1)
5-5: Consider explicit tracing imports for clarity.Using glob imports (
use tracing::*;) can reduce code clarity by obscuring which items are imported. While this is a common pattern for tracing macros, consider importing specific items explicitly.Apply this diff if you prefer explicit imports:
-use tracing::*; +use tracing::{debug, error, info, trace, warn};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/enc-avfoundation/src/mp4.rs(12 hunks)crates/enc-ffmpeg/src/mux/mp4.rs(4 hunks)crates/export/src/mp4.rs(2 hunks)crates/recording/src/output_pipeline/ffmpeg.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
crates/export/src/mp4.rscrates/enc-ffmpeg/src/mux/mp4.rscrates/enc-avfoundation/src/mp4.rscrates/recording/src/output_pipeline/ffmpeg.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/export/src/mp4.rscrates/enc-ffmpeg/src/mux/mp4.rscrates/enc-avfoundation/src/mp4.rscrates/recording/src/output_pipeline/ffmpeg.rs
🧠 Learnings (1)
📚 Learning: 2025-10-28T08:39:42.216Z
Learnt from: Brendonovich
PR: CapSoftware/Cap#1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.216Z
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/enc-avfoundation/src/mp4.rscrates/recording/src/output_pipeline/ffmpeg.rs
🧬 Code graph analysis (3)
crates/enc-ffmpeg/src/mux/mp4.rs (3)
crates/recording/src/output_pipeline/ffmpeg.rs (2)
finish(68-90)finish(144-149)crates/enc-ffmpeg/src/mux/ogg.rs (1)
finish(52-65)crates/mediafoundation-ffmpeg/src/h264.rs (1)
finish(151-163)
crates/enc-avfoundation/src/mp4.rs (2)
crates/recording/src/output_pipeline/core.rs (2)
timestamp(788-788)finish(807-807)crates/recording/src/output_pipeline/macos.rs (1)
finish(54-61)
crates/recording/src/output_pipeline/ffmpeg.rs (6)
crates/enc-ffmpeg/src/mux/mp4.rs (1)
finish(111-144)crates/enc-ffmpeg/src/mux/ogg.rs (1)
finish(52-65)crates/recording/src/output_pipeline/macos.rs (2)
finish(54-61)send_audio_frame(94-107)crates/recording/src/output_pipeline/win.rs (2)
finish(275-289)send_audio_frame(305-313)crates/recording/src/output_pipeline/core.rs (3)
finish(807-807)timestamp(788-788)send_audio_frame(811-811)crates/mediafoundation-ffmpeg/src/h264.rs (1)
finish(151-163)
⏰ 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). (4)
- GitHub Check: Clippy
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (17)
crates/export/src/mp4.rs (1)
3-3: LGTM! Import paths correctly reflect the new module structure.The updated imports correctly reference the refactored module structure where encoders are now under their specific submodules (aac::AACEncoder, h264::H264Encoder, mp4::*).
crates/enc-ffmpeg/src/mux/mp4.rs (3)
9-9: LGTM! Import paths correctly updated.The import path correctly reflects the refactored module structure where H264Encoder is now under video::h264.
31-42: LGTM! Error types well-designed for nested result handling.The new
FinishErrorandFinishResulttypes properly distinguish between unrecoverable muxer errors (outerResult) and per-stream encoder flush results (inner fields), aligning with the PR's goal to allow partial failures while still completing the muxer finish.
111-144: LGTM! Finish method correctly implements nested error handling.The finish method properly:
- Guards against double-finish with
FinishError::AlreadyFinished- Flushes both video and audio encoders, capturing errors without early returns
- Logs encoder flush failures via
inspect_err- Propagates trailer write failures as unrecoverable errors via
FinishError::WriteTrailerFailed- Returns nested results allowing callers to handle per-stream failures appropriately
This implementation aligns with the PR objective that the muxer finishes even if encoder flushing fails, ensuring valid MP4 files.
crates/recording/src/output_pipeline/ffmpeg.rs (5)
68-90: LGTM! Finish method correctly implements nested error handling.The finish method properly:
- Flushes both video and audio encoders without early returns
- Writes the trailer (outer
Resultpropagates unrecoverable errors)- Returns nested
Ok(Ok(()))when all encoders succeed- Returns nested
Ok(Err(...))when encoder flush fails, preserving details for both streamsThis implementation aligns with the trait signature in
output_pipeline/core.rsand the PR objective to complete muxer finish even when encoder flushing fails.
96-106: LGTM! Error propagation correctly implemented.The video frame queuing properly propagates errors using the
?operator, aligning with the updatedqueue_framesignature that now returnsResult.
109-117: LGTM! Error propagation correctly implemented.The audio frame sending properly propagates errors using the
?operator, aligning with the updatedsend_framesignature that now returnsResult.
144-149: LGTM! OggMuxer finish correctly delegates and converts errors.The finish method properly:
- Delegates to the underlying
OggFile::finish()- Converts both outer
FinishErrorand innerffmpeg::Errortoanyhow::ErrorusingInto- Maintains the nested
Resultstructure required by theMuxertraitThis implementation is consistent with the
Mp4Muxerfinish pattern.
152-156: LGTM! Error propagation correctly implemented.The audio frame queuing properly propagates errors from
queue_frameusing the?operator and wraps the result appropriately.crates/enc-avfoundation/src/mp4.rs (8)
51-70: LGTM! Error types well-defined with distinct messages.The error enums properly define failure modes:
QueueFrameErrorcovers append failures, construction errors, and backpressureFinishErrorhas distinct error messages for each variant (NotWriting, NoFrames, Failed)The past review concern about duplicate error messages has been addressed.
218-229: LGTM! Early return logic correctly handles paused state and backpressure.The early checks properly:
- Return early if paused without queueing
- Check encoder readiness before processing, returning
NotReadyForMorefor backpressureThis aligns with the retry logic in
crates/recording/src/output_pipeline/macos.rsthat handlesQueueFrameError::NotReadyForMore.Based on learnings
277-277: LGTM! Centralized append logic via helper function.The call to the new
append_sample_bufhelper properly delegates frame appending logic and propagates errors consistently.
287-309: LGTM! Audio frame queuing correctly aligned with video path.The updated method properly:
- Takes
&frame::Audioby reference for efficiency- Returns
Result<(), QueueFrameError>for proper error propagation- Checks encoder readiness and returns
NotReadyForMorefor backpressure, consistent with the video pathThe past review concern about inconsistent error handling between audio and video paths has been addressed.
311-395: LGTM! Consistent error mapping for construction operations.All construction operations (
BlockBuf::with_mem_block,as_mut_slice,AudioFormatDesc::with_asbd,SampleBuf::create) properly map errors toQueueFrameError::Construct, providing clear error categorization and consistent error handling.
450-496: LGTM! Finish method correctly implements AVFoundation finalization.The finish method properly:
- Guards against invalid states (
NotWriting,NoFrames)- Extends the video to the provided timestamp when appropriate
- Finalizes the asset writer by ending the session and marking inputs as finished
- Waits for the writer to complete using the
wait_for_writer_finishedhelper- Maps completion failures to
FinishError::FailedThis implementation aligns with AVFoundation's asynchronous writing model.
499-503: LGTM! Drop implementation properly finalizes resources.The Drop implementation correctly calls
finish(None)to ensure resources are cleaned up. Ignoring the result withlet _ =is appropriate in Drop since errors cannot be propagated, preventing potential panics.
625-655: LGTM! Helper functions properly encapsulate error handling and polling logic.The helper functions are well-designed:
append_sample_buf(lines 625-644):
- Centralizes frame appending with consistent error handling
- Maps
append_sample_bufoutcomes to appropriateQueueFrameErrorvariants- Checks writer status to distinguish between transient (NotReadyForMore) and fatal (Failed) errors
- Retains exception objects for error reporting
wait_for_writer_finished(lines 646-655):
- Polls the asset writer status until completion
- Returns success for Completed or Cancelled states
- Returns error for Failed or Unknown states
- Uses appropriate sleep interval (2ms) while Writing
Summary by CodeRabbit
Bug Fixes
API Changes
Dependencies
Chores