feat: Major fragmented mp4 optimisation #1457
Conversation
WalkthroughAdds manifest versioning and per-fragment file_size, introduces atomic JSON write and fsync helpers, records segment sizes during rotation/finish, adds frame-drop tracking in Windows muxers, hardens recovery validation/cleanup, and applies minor example output/formatting tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Encoder
participant Muxer as SegmentedMuxer
participant FS as FileSystem
participant ManifestWriter
Note over Encoder,Muxer: Segment rotation / finalize flow
Encoder->>Muxer: Request finish current segment
Muxer->>Encoder: Flush encoder / write trailer
Muxer->>FS: fsync segment file descriptor
Muxer->>FS: stat segment file -> get file_size
FS-->>Muxer: file_size
Muxer->>Muxer: update SegmentInfo with file_size
Muxer->>ManifestWriter: build in‑progress/final manifest (versioned, include file_size)
ManifestWriter->>FS: atomic write (temp -> final)
FS->>FS: sync parent directory
Muxer->>Encoder: initialize next segment / continue
sequenceDiagram
participant Recovery
participant FS as FileSystem
participant Manifest as ManifestReader
participant Probe as MediaProbe
Note over Recovery,Manifest: Fragment discovery & validation
Recovery->>FS: list fragment dirs
Recovery->>Manifest: read manifest.json
Manifest-->>Recovery: manifest (version, fragments)
alt manifest.version > CURRENT_MANIFEST_VERSION
Recovery->>Recovery: warn about newer manifest version
end
Recovery->>FS: stat fragment file -> actual_size
FS-->>Recovery: actual_size
alt expected_size present && mismatch
Recovery->>Recovery: warn size mismatch
end
alt fragment is mp4 (video)
Recovery->>Probe: probe_video_can_decode(path)
Probe-->>Recovery: decodable? (ok / fail)
else audio/other
Recovery->>Probe: probe_media_valid(path)
Probe-->>Recovery: valid? (ok / fail)
end
Recovery->>Recovery: include only validated, complete fragments
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas that deserve extra attention:
Possibly related PRs
Suggested reviewers
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/enc-mediafoundation/examples/cli.rs (1)
130-130: Remove commented-out code; it violates the coding guideline against using comment markers in code.Lines 130, 137, and 148 contain
//comment markers applied to code, which violates the requirement that "code must be self-explanatory through naming, types, and structure." The callback on lines 146-149 also becomes non-functional as a result. Either restore the full functionality by uncommenting and fixing the code, or remove this incomplete example entirely.crates/recording/src/output_pipeline/win_segmented.rs (1)
515-520: Swallowed muxer write error.The
write_sampleerror is mapped to a string but the result is discarded withlet _ =. If muxing fails, encoding continues silently. This could lead to corrupted or incomplete segments without any indication. Consider at least logging the error:- let _ = muxer - .write_sample(&output_sample, &mut output) - .map_err(|e| format!("WriteSample: {e}")); + if let Err(e) = muxer.write_sample(&output_sample, &mut output) { + tracing::warn!("WriteSample failed: {e}"); + }
🧹 Nitpick comments (12)
crates/mediafoundation-ffmpeg/examples/usage.rs (2)
33-42: Consider documenting or removing the unusedwrite_samplemethod.The
#[allow(dead_code)]attribute indicates this method isn't used in the example. If it's kept as a reference for users who want to extend the example, consider adding a comment explaining its purpose and usage.
59-77: Example doesn't demonstrate writing data.The example initializes the muxer and finishes immediately without writing any video or audio samples. While this shows the basic lifecycle, it doesn't demonstrate the core functionality of the library.
Consider either:
- Adding a simple example of writing at least one sample (even if synthetic/dummy data)
- Adding a comment explaining this is just a minimal lifecycle example and referring users to more complete examples or tests
crates/enc-avfoundation/src/segmented.rs (1)
12-36: Consider extracting shared utilities to avoid duplication.These
atomic_write_jsonandsync_filefunctions are identical to the public versions incrates/recording/src/fragmentation/mod.rs. While the duplication is likely intentional to avoid a circular dependency (enc-avfoundation → recording → enc-avfoundation), consider extracting these utilities into a shared lower-level crate (e.g.,cap-utilsorcap-io) that both can depend on.crates/enc-ffmpeg/src/mux/segmented_audio.rs (1)
11-35: Same duplication as enc-avfoundation—consider shared utility crate.These functions are identical to those in
enc-avfoundation/src/segmented.rsandrecording/src/fragmentation/mod.rs. A shared utility crate would eliminate this triplication.crates/recording/src/output_pipeline/win_segmented_camera.rs (3)
60-94: Significant code duplication withwin_segmented.rs.
FrameDropTrackeris essentially identical between this file andwin_segmented.rs(lines 63-94 there). Consider extracting this to a shared module to avoid maintaining duplicate implementations.Additionally, there's a minor logic gap in
record_drop: if 30 drops occur butlast_warning.elapsed()is still under 5 seconds, the warning is suppressed. Subsequent drops in that same window will keep incrementing the count past 30 without ever warning. Consider whether you want to emit the warning as soon as 30 drops are reached regardless of the time window, or keep accumulating:fn record_drop(&mut self) { self.count += 1; - if self.count >= 30 && self.last_warning.elapsed() > Duration::from_secs(5) { + if self.last_warning.elapsed() > Duration::from_secs(5) && self.count >= 30 { warn!( "Dropped {} camera frames due to encoder backpressure", self.count ); self.count = 0; self.last_warning = std::time::Instant::now(); } }This keeps the same semantics but makes the time-gate primary. If you want to warn immediately upon hitting 30 drops (rate-limited to once per 5s), you could restructure as a separate condition.
19-46: Duplicate type definitions across Windows muxer files.
SegmentInfo,FragmentEntry,Manifest, andMANIFEST_VERSIONare duplicated betweenwin_segmented_camera.rsandwin_segmented.rs. Consider extracting these to a shared module (e.g.,win_common.rsor reusing types from thefragmentationmodule) to ensure consistency and reduce maintenance burden.
578-623:write_in_progress_manifestduplicateswrite_manifestlogic.The fragment entry mapping is nearly identical in
write_manifest,finalize_manifest, andwrite_in_progress_manifest. Consider extracting a helper likefn build_fragment_entries(&self) -> Vec<FragmentEntry>to reduce duplication within this file.crates/recording/src/fragmentation/mod.rs (2)
11-29: Atomic write implementation looks correct, but temp file may be left on serialization failure.If
serde_json::to_string_prettyfails (line 13-14), the function returns early without creating the temp file, so that's fine. However, iffile.write_allorfile.sync_allfails after the file is created, the temp file (*.json.tmp) will remain on disk. Consider cleaning up on error:pub fn atomic_write_json<T: Serialize>(path: &Path, data: &T) -> std::io::Result<()> { let temp_path = path.with_extension("json.tmp"); let json = serde_json::to_string_pretty(data) .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; - let mut file = std::fs::File::create(&temp_path)?; - file.write_all(json.as_bytes())?; - file.sync_all()?; + let result = (|| { + let mut file = std::fs::File::create(&temp_path)?; + file.write_all(json.as_bytes())?; + file.sync_all() + })(); + + if result.is_err() { + let _ = std::fs::remove_file(&temp_path); + return result; + } std::fs::rename(&temp_path, path)?; ... }
31-35:sync_filesilently ignores all errors.This is acceptable for a best-effort durability hint, but makes debugging I/O issues harder. Consider at least a
trace!log on failure for observability:pub fn sync_file(path: &Path) { if let Ok(file) = std::fs::File::open(path) { - let _ = file.sync_all(); + if let Err(e) = file.sync_all() { + tracing::trace!("sync_file failed for {}: {e}", path.display()); + } } }crates/recording/src/output_pipeline/win_segmented.rs (3)
63-94: DuplicateFrameDropTrackerimplementation.This is identical to the implementation in
win_segmented_camera.rs. Extract to a shared module to avoid maintaining two copies.
22-49: Duplicate struct definitions.
SegmentInfo,FragmentEntry, andManifestare duplicated fromwin_segmented_camera.rs. These should be consolidated into a shared module, potentially reusing or extending types fromcrates/recording/src/fragmentation/manifest.rs.
636-681:write_in_progress_manifestmirrorswrite_manifestclosely.Consider extracting shared fragment entry mapping logic to reduce duplication within this file.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
crates/camera-directshow/examples/cli.rs(1 hunks)crates/camera-mediafoundation/examples/cli.rs(1 hunks)crates/camera-windows/examples/cli.rs(1 hunks)crates/enc-avfoundation/src/segmented.rs(12 hunks)crates/enc-ffmpeg/src/mux/segmented_audio.rs(14 hunks)crates/enc-mediafoundation/examples/cli.rs(1 hunks)crates/mediafoundation-ffmpeg/examples/usage.rs(2 hunks)crates/recording/src/fragmentation/manifest.rs(3 hunks)crates/recording/src/fragmentation/mod.rs(3 hunks)crates/recording/src/output_pipeline/win_segmented.rs(15 hunks)crates/recording/src/output_pipeline/win_segmented_camera.rs(16 hunks)crates/recording/src/recovery.rs(12 hunks)crates/recording/src/studio_recording.rs(1 hunks)crates/scap-ffmpeg/examples/cli.rs(1 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/camera-mediafoundation/examples/cli.rscrates/enc-mediafoundation/examples/cli.rscrates/recording/src/studio_recording.rscrates/camera-windows/examples/cli.rscrates/recording/src/recovery.rscrates/camera-directshow/examples/cli.rscrates/enc-ffmpeg/src/mux/segmented_audio.rscrates/scap-ffmpeg/examples/cli.rscrates/recording/src/fragmentation/mod.rscrates/enc-avfoundation/src/segmented.rscrates/recording/src/output_pipeline/win_segmented.rscrates/mediafoundation-ffmpeg/examples/usage.rscrates/recording/src/output_pipeline/win_segmented_camera.rscrates/recording/src/fragmentation/manifest.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/camera-mediafoundation/examples/cli.rscrates/enc-mediafoundation/examples/cli.rscrates/recording/src/studio_recording.rscrates/camera-windows/examples/cli.rscrates/recording/src/recovery.rscrates/camera-directshow/examples/cli.rscrates/enc-ffmpeg/src/mux/segmented_audio.rscrates/scap-ffmpeg/examples/cli.rscrates/recording/src/fragmentation/mod.rscrates/enc-avfoundation/src/segmented.rscrates/recording/src/output_pipeline/win_segmented.rscrates/mediafoundation-ffmpeg/examples/usage.rscrates/recording/src/output_pipeline/win_segmented_camera.rscrates/recording/src/fragmentation/manifest.rs
🧠 Learnings (6)
📚 Learning: 2025-12-07T14:29:40.743Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.743Z
Learning: Applies to **/*.rs : Use `.is_empty()` instead of `.len() == 0` or `.len() > 0` (Clippy: `len_zero` = deny)
Applied to files:
crates/camera-mediafoundation/examples/cli.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 : Use .is_empty() instead of .len() == 0 or .len() > 0 / .len() != 0
Applied to files:
crates/camera-mediafoundation/examples/cli.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/enc-mediafoundation/examples/cli.rscrates/camera-directshow/examples/cli.rscrates/enc-ffmpeg/src/mux/segmented_audio.rscrates/enc-avfoundation/src/segmented.rscrates/recording/src/output_pipeline/win_segmented.rscrates/mediafoundation-ffmpeg/examples/usage.rscrates/recording/src/output_pipeline/win_segmented_camera.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/enc-mediafoundation/examples/cli.rscrates/enc-avfoundation/src/segmented.rscrates/recording/src/output_pipeline/win_segmented.rscrates/mediafoundation-ffmpeg/examples/usage.rscrates/recording/src/output_pipeline/win_segmented_camera.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/camera-windows/examples/cli.rs
📚 Learning: 2025-12-07T14:29:40.743Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.743Z
Learning: Applies to **/*.rs : Never use `dbg!()` macro in Rust code; use proper logging instead (Clippy: `dbg_macro` = deny)
Applied to files:
crates/camera-windows/examples/cli.rs
🧬 Code graph analysis (4)
crates/recording/src/recovery.rs (1)
crates/enc-ffmpeg/src/remux.rs (2)
probe_video_can_decode(276-347)probe_media_valid(272-274)
crates/camera-directshow/examples/cli.rs (2)
crates/recording/src/sources/screen_capture/windows.rs (1)
timestamp(62-64)crates/recording/src/sources/screen_capture/macos.rs (1)
timestamp(61-63)
crates/enc-avfoundation/src/segmented.rs (2)
crates/recording/src/fragmentation/mod.rs (4)
atomic_write_json(11-29)new(45-52)sync_file(31-35)total_duration(132-138)crates/recording/src/output_pipeline/win_segmented.rs (3)
new(69-74)new(97-103)write_in_progress_manifest(636-681)
crates/mediafoundation-ffmpeg/examples/usage.rs (2)
crates/enc-ffmpeg/src/mux/mp4.rs (1)
init(45-81)crates/mediafoundation-ffmpeg/src/h264.rs (1)
new(38-87)
⏰ 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 (27)
crates/camera-windows/examples/cli.rs (1)
35-41: LGTM! Appropriate use ofprintln!for example code.The change from
dbg!toprintln!is correct for this example file. While the coding guidelines prohibitdbg!and recommend structured logging for production code,println!is the right choice for examples in theexamples/directory where the goal is to demonstrate library usage and show clear output to users.crates/recording/src/studio_recording.rs (1)
231-233: Excellent error handling improvement.This change properly handles the Result from
pipeline.stop().awaitinstead of silently ignoring it, which aligns with the coding guidelines. Logging a warning during cancel is appropriate since this is a best-effort cleanup operation.crates/camera-directshow/examples/cli.rs (1)
117-121: The change looks good, but appears unrelated to the PR objectives.The updated output formatting is a reasonable improvement for this CLI example. However, this change seems inconsistent with the PR's stated focus on "fragmented mp4 optimisation," manifest versioning, and changes in the enc-avfoundation and enc-ffmpeg crates. This file is part of the camera-directshow crate and doesn't relate to MP4 segmentation or manifest generation.
crates/scap-ffmpeg/examples/cli.rs (5)
6-7: LGTM!Import additions are appropriate and used in the updated implementation.
9-18: LGTM!The capturer initialization with the updated API is clean and idiomatic. Using
unwrap()ontry_as_capture_item()is acceptable in example code for simplicity.
19-32: Excellent change – follows coding guidelines!Replacing
dbg!with formattedprintln!output aligns with the project's coding guidelines that prohibitdbg!()macro usage. The frame information output is clear and appropriate for example code.
33-36: LGTM!The capturer construction with error handling closure and optional parameter is well-structured. The use of
unwrap()is acceptable for example code.
38-42: LGTM!The updated start/stop lifecycle with direct method calls on the capturer is cleaner and more intuitive than the previous handle-based approach.
crates/camera-mediafoundation/examples/cli.rs (1)
24-27: LGTM!Good alignment with Clippy's
len_zerolint by using.is_empty()instead of.len() == 0. Based on coding guidelines.crates/recording/src/fragmentation/manifest.rs (2)
4-22: Well-designed manifest versioning.Good backward compatibility approach: defaulting to version 1 when deserializing older manifests that lack the version field, while new manifests use
CURRENT_MANIFEST_VERSION = 2.
36-37: LGTM!Appropriate use of
skip_serializing_ifto keep the JSON clean whenfile_sizeisNone.crates/enc-avfoundation/src/segmented.rs (3)
70-79: LGTM!Manifest versioning and structure align well with the
FragmentManifestin the fragmentation module.
146-188: LGTM on segment rotation logic.Good use of
saturating_subfor duration calculation (line 148), proper file sync before capturing file size, and atomic manifest writes. The in-progress manifest write after creating the new encoder correctly reflects the current state.
287-312: LGTM!Clean implementation following the same pattern as
win_segmented.rs(per relevant snippets). The in-progress manifest correctly includes both completed segments and the current incomplete segment.crates/recording/src/recovery.rs (4)
213-222: Good forward compatibility handling.Warning when encountering a newer manifest version without blocking recovery is the right approach—it allows graceful degradation if the manifest format evolves.
224-273: Thorough fragment validation.Good layered validation: existence check → file size check → decode capability check. This catches truncated files and corrupt media before attempting recovery.
285-289: LGTM!Simple, focused helper with case-insensitive extension matching.
409-433: Good resilience improvement.Wrapping cleanup operations in
if let Errwith debug logging prevents non-critical failures (leftover directories, locked files) from aborting an otherwise successful recovery. The same pattern is correctly applied throughout the file for all cleanup operations.crates/enc-ffmpeg/src/mux/segmented_audio.rs (3)
189-227: LGTM on segment rotation.Correct use of
saturating_sub(line 191), proper sync before file size capture, and atomic manifest writes. The warning-level logs for flush/trailer errors during rotation are appropriate since these aren't fatal.
313-359: LGTM on finish logic.Good handling of empty segments—writing trailer before removal, using trace-level logging for cleanup failures. The file sync and size capture for non-empty segments mirrors the rotation logic correctly.
408-440: LGTM!Final manifest correctly includes
total_durationandis_complete: truewith atomic write.crates/recording/src/output_pipeline/win_segmented_camera.rs (2)
249-262: Segment is only recorded ifsegment_startisSome.If
segment_start_timewas never set (i.e., no frames were received), the segment won't be added tocompleted_segments, andfinalize_manifest()will produce a manifest missing this segment. This appears intentional (no frames = no valid segment), but verify this edge case is acceptable for your recovery logic.
225-230: Good: Panic payload is now logged instead of silently ignored.The change from ignoring thread join errors to logging them as warnings is a good improvement for debuggability.
crates/recording/src/fragmentation/mod.rs (2)
22-26: Parent directory sync result is discarded.Similar to
sync_file, thedir.sync_all()result on line 24 is silently ignored. This is typically acceptable since directory sync is a durability optimization that may not be supported on all filesystems, but consider adding a trace log for consistency.
108-118: Good use of atomic writes for manifest durability.The switch to
atomic_write_jsonensures manifest files are not corrupted on crash mid-write.crates/recording/src/output_pipeline/win_segmented.rs (2)
258-268: Segment only added ifsegment_startisSome.Same pattern as in
win_segmented_camera.rs— if no frames were received, the segment is not recorded. Ensure this is the expected behavior for edge cases like immediate stop after start.
580-585: Good: Encoder thread panic is now logged.Improved observability for debugging encoder thread failures during rotation.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/enc-ffmpeg/src/mux/segmented_audio.rs(14 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/enc-ffmpeg/src/mux/segmented_audio.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/enc-ffmpeg/src/mux/segmented_audio.rs
🧠 Learnings (1)
📚 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/enc-ffmpeg/src/mux/segmented_audio.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 (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
crates/enc-ffmpeg/src/mux/segmented_audio.rs (5)
61-61: LGTM! Well-designed additions for manifest versioning and metadata.The additions of
file_sizefields and manifest versioning enable better downstream validation and schema evolution. The use ofOption<u64>andskip_serializing_ifkeeps the manifest clean while handling cases where file size isn't available.Also applies to: 71-71, 74-74, 78-78
120-133: LGTM! Early manifest write improves observability.Writing an in-progress manifest immediately after initialization provides early visibility into the recording state, which is valuable for monitoring and crash recovery scenarios.
189-227: LGTM! Robust segment rotation with proper state management.The rotation logic correctly:
- Captures the segment path before taking the encoder (avoiding borrow issues)
- Handles errors gracefully with warnings while continuing (intentional degradation)
- Syncs the file before capturing size (ensuring accuracy)
- Updates both completed and in-progress manifests (maintaining consistency)
The error handling pattern with
tracing::warnis appropriate here—the segment remains usable even if flush or trailer operations encounter issues.
234-311: LGTM! Atomic manifest writes with proper versioning.Both
write_manifestandwrite_in_progress_manifestcorrectly:
- Use atomic writes to prevent corruption during crashes
- Include manifest versioning for future schema evolution
- Handle errors gracefully with logging (fire-and-forget pattern is appropriate here)
- Maintain consistency between completed and in-progress states
The in-progress manifest provides valuable visibility into the current recording state for monitoring and recovery tools.
313-406: LGTM! Consistent finish logic with proper cleanup.Both finish methods correctly:
- Capture necessary values before taking the encoder (avoiding borrow issues)
- Sync files before capturing size (ensuring accuracy)
- Handle empty segments by removing them with trace-level logging
- Include file_size in final SegmentInfo
- Call finalize_manifest to mark completion
The consistent error handling and state management patterns across both methods make the code maintainable and predictable.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/enc-avfoundation/src/segmented.rs (1)
12-36: Consider extracting common utilities to reduce duplication.The
atomic_write_jsonandsync_filefunctions are duplicated between this file andcrates/enc-ffmpeg/src/mux/segmented_audio.rs. Consider extracting these to a shared utility module to reduce maintenance burden and ensure consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/enc-avfoundation/src/segmented.rs(12 hunks)crates/enc-ffmpeg/src/mux/segmented_audio.rs(14 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/enc-avfoundation/src/segmented.rscrates/enc-ffmpeg/src/mux/segmented_audio.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/enc-avfoundation/src/segmented.rscrates/enc-ffmpeg/src/mux/segmented_audio.rs
🧠 Learnings (6)
📚 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/enc-avfoundation/src/segmented.rscrates/enc-ffmpeg/src/mux/segmented_audio.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/enc-avfoundation/src/segmented.rs
📚 Learning: 2025-12-07T14:29:40.743Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.743Z
Learning: Applies to **/*.rs : Never write `let _ = async_fn()` which silently drops futures; await or explicitly handle them (Clippy: `let_underscore_future` = deny)
Applied to files:
crates/enc-ffmpeg/src/mux/segmented_audio.rs
📚 Learning: 2025-12-07T14:29:40.743Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.743Z
Learning: Applies to **/*.rs : Always handle `Result`/`Option` or types marked `#[must_use]`; never ignore them (Rust compiler lint: `unused_must_use` = deny)
Applied to files:
crates/enc-ffmpeg/src/mux/segmented_audio.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 write let _ = async_fn() which silently drops futures; await or explicitly handle them
Applied to files:
crates/enc-ffmpeg/src/mux/segmented_audio.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 : Always handle Result/Option or types marked #[must_use]; never ignore them
Applied to files:
crates/enc-ffmpeg/src/mux/segmented_audio.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 (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (16)
crates/enc-avfoundation/src/segmented.rs (7)
52-58: LGTM - File size tracking added to segment metadata.The addition of
file_size: Option<u64>appropriately tracks segment file sizes, aligning with the PR objectives for enhanced segment metadata.
60-79: LGTM - Manifest versioning and metadata enhancements.The additions of manifest versioning (
MANIFEST_VERSION = 2) and optionalfile_sizefields with conditional serialization are well-designed. This enables backward-compatible manifest evolution while enriching segment metadata.
94-109: LGTM - In-progress manifest initialization.Calling
write_in_progress_manifest()during initialization provides immediate visibility into the segmentation state, which is valuable for monitoring and recovery scenarios.
228-273: LGTM - In-progress manifest implementation.The
write_in_progress_manifest()function correctly reflects the current state by including completed segments (markedis_complete: true) and the active segment (markedis_complete: falsewithduration: 0.0). This provides real-time visibility during encoding.
146-189: LGTM - Segment rotation with proper cleanup and tracking.The rotation logic correctly:
- Finishes and logs errors from the encoder
- Syncs the completed segment file to disk
- Captures file size with graceful fallback to
None- Updates both the completed manifest and in-progress manifest
This ensures data integrity during segment transitions.
287-312: LGTM - Proper final segment handling.The
finish()method correctly:
- Syncs the final segment file
- Conditionally adds the final segment only when
segment_startexists- Captures file size and calculates final duration accurately
This prevents invalid manifest entries for edge cases.
314-346: LGTM - Complete manifest finalization with atomic writes.The
finalize_manifest()correctly marks the manifest as complete, includes versioning and total duration, and usesatomic_write_jsonto ensure durability. Error logging viatracing::warn!provides appropriate visibility for manifest write failures.crates/enc-ffmpeg/src/mux/segmented_audio.rs (9)
11-34: LGTM - Atomic JSON write with proper Result handling.The
atomic_write_jsonimplementation correctly handles all Results, including the directory fsync on lines 25-30. The fix properly logs failures while continuing execution, which is appropriate for this best-effort operation after the critical atomic rename.
36-42: LGTM - File sync with proper error handling.The
sync_filefunction correctly handles the Result fromfile.sync_all()on lines 38-40, logging failures appropriately. This addresses the previous review concern.
63-90: LGTM - Consistent manifest versioning and metadata.The additions of
file_sizetracking andMANIFEST_VERSION = 2are consistent with the changes incrates/enc-avfoundation/src/segmented.rs, providing uniform manifest behavior across encoders.
127-141: LGTM - In-progress manifest on initialization.Initializing with an in-progress manifest provides immediate visibility into the segmentation state, consistent with the pattern in
crates/enc-avfoundation/src/segmented.rs.
273-318: LGTM - In-progress manifest reflects current encoding state.The
write_in_progress_manifest()correctly includes completed segments and the active segment (marked incomplete with zero duration), providing real-time encoding state visibility.
196-234: LGTM - Segment rotation with comprehensive error handling.The rotation logic properly:
- Flushes and writes trailer with warnings on failure
- Syncs the completed segment
- Captures file size
- Updates both standard and in-progress manifests
The error handling via
tracing::warn!is appropriate for these best-effort operations during rotation.
320-366: LGTM - Proper finish handling with empty segment cleanup.The
finish()method correctly:
- Handles segments with frames by flushing, writing trailer, syncing, and capturing metadata
- Removes empty segments (lines 350-360) to avoid invalid manifest entries
- Uses appropriate logging levels (
warnfor encoder issues,tracefor empty segment cleanup)This ensures clean finalization without spurious segment files.
368-413: LGTM - Timestamp-based finish with consistent logic.The
finish_with_timestamp()method maintains the same robust error handling and empty segment cleanup asfinish(), with the timestamp explicitly provided for duration calculation. The consistent implementation reduces maintenance burden.
415-447: LGTM - Atomic final manifest with complete metadata.The
finalize_manifest()correctly marks the manifest as complete, includes version information and total duration, and uses atomic writes to ensure durability. Error logging provides visibility for manifest write failures.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/recording/src/fragmentation/mod.rs (2)
22-26: Handle the Result from directory sync for consistency.Line 25 uses
let _ = dir.sync_all();which ignores theResult. The same pattern incrates/enc-ffmpeg/src/mux/segmented_audio.rswas updated to log failures. Apply the same fix here for consistency and to satisfy coding guidelines.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() { + tracing::trace!("Directory fsync failed for {}: {e}", parent.display()); + } }Based on coding guidelines: "Always handle Result/Option or types marked #[must_use]; never ignore them."
31-35: Handle the Result from file sync for consistency.Line 33 uses
let _ = file.sync_all();which ignores theResult. The same pattern incrates/enc-ffmpeg/src/mux/segmented_audio.rslogs failures. Apply the same fix here.pub fn sync_file(path: &Path) { if let Ok(file) = std::fs::File::open(path) { - let _ = file.sync_all(); + if let Err(e) = file.sync_all() { + tracing::trace!("File fsync failed for {}: {e}", path.display()); + } } }Based on coding guidelines: "Always handle Result/Option or types marked #[must_use]; never ignore them."
crates/recording/src/output_pipeline/win_segmented_camera.rs (1)
60-94: Consider extracting FrameDropTracker to a shared module.The
FrameDropTrackerimplementation is nearly identical betweenwin_segmented.rsandwin_segmented_camera.rs. Consider extracting it to a shared module to reduce duplication. The message text could be parameterized.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/enc-ffmpeg/src/mux/segmented_audio.rs(14 hunks)crates/recording/src/fragmentation/mod.rs(3 hunks)crates/recording/src/output_pipeline/win_segmented.rs(15 hunks)crates/recording/src/output_pipeline/win_segmented_camera.rs(16 hunks)crates/recording/src/recovery.rs(12 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/output_pipeline/win_segmented_camera.rscrates/recording/src/fragmentation/mod.rscrates/enc-ffmpeg/src/mux/segmented_audio.rscrates/recording/src/recovery.rscrates/recording/src/output_pipeline/win_segmented.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/output_pipeline/win_segmented_camera.rscrates/recording/src/fragmentation/mod.rscrates/enc-ffmpeg/src/mux/segmented_audio.rscrates/recording/src/recovery.rscrates/recording/src/output_pipeline/win_segmented.rs
🧠 Learnings (6)
📚 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/win_segmented_camera.rscrates/enc-ffmpeg/src/mux/segmented_audio.rscrates/recording/src/output_pipeline/win_segmented.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/win_segmented_camera.rscrates/recording/src/output_pipeline/win_segmented.rs
📚 Learning: 2025-12-07T14:29:40.743Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.743Z
Learning: Applies to **/*.rs : Never write `let _ = async_fn()` which silently drops futures; await or explicitly handle them (Clippy: `let_underscore_future` = deny)
Applied to files:
crates/enc-ffmpeg/src/mux/segmented_audio.rs
📚 Learning: 2025-12-07T14:29:40.743Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.743Z
Learning: Applies to **/*.rs : Always handle `Result`/`Option` or types marked `#[must_use]`; never ignore them (Rust compiler lint: `unused_must_use` = deny)
Applied to files:
crates/enc-ffmpeg/src/mux/segmented_audio.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 write let _ = async_fn() which silently drops futures; await or explicitly handle them
Applied to files:
crates/enc-ffmpeg/src/mux/segmented_audio.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 : Always handle Result/Option or types marked #[must_use]; never ignore them
Applied to files:
crates/enc-ffmpeg/src/mux/segmented_audio.rs
🧬 Code graph analysis (4)
crates/recording/src/output_pipeline/win_segmented_camera.rs (1)
crates/recording/src/fragmentation/mod.rs (3)
new(45-52)sync_file(31-35)atomic_write_json(11-29)
crates/recording/src/fragmentation/mod.rs (2)
crates/enc-avfoundation/src/segmented.rs (3)
atomic_write_json(12-30)sync_file(32-36)write_manifest(196-226)crates/recording/src/output_pipeline/win_segmented.rs (3)
new(69-74)new(97-103)write_manifest(283-313)
crates/recording/src/recovery.rs (1)
crates/enc-ffmpeg/src/remux.rs (2)
probe_video_can_decode(276-347)probe_media_valid(272-274)
crates/recording/src/output_pipeline/win_segmented.rs (1)
crates/recording/src/fragmentation/mod.rs (3)
new(45-52)sync_file(31-35)atomic_write_json(11-29)
⏰ 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 (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (33)
crates/recording/src/recovery.rs (7)
204-222: LGTM!The manifest version check with a default of 1 for older manifests and a warning for unsupported newer versions is a sensible forward-compatibility approach.
224-273: LGTM!The layered validation (existence → size match → decodability) is robust. The differentiation between video files using
probe_video_can_decodeand other media usingprobe_media_validis appropriate.
284-288: LGTM!The case-insensitive mp4 extension check is appropriate for the fragment naming convention used in this codebase.
299-318: LGTM!The extension-based routing with debug logging for skipped fragments is well-structured. Using
debug!rather thanwarn!is appropriate for discovery-phase filtering.
326-346: LGTM!The early existence check and consistent routing based on file type is clean and efficient.
407-432: Hardened cleanup is a good improvement.Using guarded removals with debug logging instead of unconditional operations prevents panics during recovery and provides diagnostic information without alarming users.
608-625: LGTM!Graceful handling of invalid camera videos with guarded removal and appropriate logging levels.
crates/recording/src/fragmentation/mod.rs (3)
68-84: LGTM!The signature extension to include
file_sizeis clean and maintains backward compatibility throughOption.
98-106: LGTM!Consistent signature update with
rotatemethod.
108-130: LGTM!The addition of manifest versioning and atomic writes improves durability and enables compatibility checking during recovery.
crates/enc-ffmpeg/src/mux/segmented_audio.rs (8)
11-33: LGTM!The atomic write helper properly handles errors with logging, and the directory sync failure is now logged as requested in the previous review.
35-41: LGTM!The sync_file helper now properly logs failures as requested in the previous review.
62-89: LGTM!The addition of
file_sizetracking and manifest versioning is well-structured with appropriate serde attributes.
126-139: LGTM!Writing an in-progress manifest at initialization ensures the manifest exists from the start, aiding recovery scenarios.
195-231: LGTM!The rotation flow is well-structured: flush → trailer → sync → get size → record → manifest. Warning on non-fatal errors is appropriate.
272-317: LGTM!The in-progress manifest correctly tracks both completed segments (with full metadata) and the current incomplete segment (with placeholder values).
319-412: LGTM!Both finish methods follow a consistent pattern with proper file sync before size measurement and graceful handling of empty segments.
414-446: LGTM!The finalize_manifest correctly marks the manifest as complete with total duration and uses atomic writes.
crates/recording/src/output_pipeline/win_segmented.rs (9)
22-49: LGTM!The struct extensions for file_size and manifest versioning are consistent with other segmented encoders.
63-94: LGTM!The FrameDropTracker provides useful backpressure monitoring with appropriate rate limiting to avoid log flooding.
157-157: LGTM!Clean integration of FrameDropTracker into the muxer.
209-268: LGTM!The enhanced error handling with trace logs for expected channel closures and warnings for thread panics provides good diagnostics.
283-312: LGTM!Good reuse of the shared
fragmentation::atomic_write_jsonhelper for manifest durability.
315-347: LGTM!Correct finalization with total duration and completion flag.
565-634: LGTM!The rotation flow correctly syncs, records metadata, resets drop counters, and writes manifests at appropriate points.
636-681: LGTM!The in-progress manifest correctly reflects the current recording state for potential recovery.
719-726: LGTM!Frame drop tracking is properly integrated with the try_send error handling.
crates/recording/src/output_pipeline/win_segmented_camera.rs (6)
19-46: LGTM!The struct definitions are consistent with
win_segmented.rs.
152-152: LGTM!Consistent integration of FrameDropTracker.
203-268: LGTM!The finish flow is consistent with
win_segmented.rswith proper file sync and metadata collection.
277-341: LGTM!Manifest writing is consistent with other segmented encoders.
503-623: LGTM!The rotation and in-progress manifest logic is consistent with
win_segmented.rs.
659-666: LGTM!Frame drop tracking is properly integrated in the camera muxer.
Improvements and new features related to segmented media encoding and manifest generation, primarily in the
enc-avfoundationandenc-ffmpegcrates. The key changes include atomic and safer manifest file writing, manifest versioning, and enhanced segment metadata (such as file size reporting).Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.