Conversation
WalkthroughAdds a new exported macro Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Config as VideoSourceConfig
participant Capturer as Capturer / Capture Loop
participant Logger as Background Logger
participant Fail as cap_fail::fail_ret!
participant Shutdown as Shutdown (cancel/drop)
App->>Config: construct config (inner, capturer, error_rx, cancel_token, drop_guard, video_frame_counter)
Config->>Capturer: start (attach callbacks & error_rx)
Capturer->>Capturer: on_frame -> video_frame_counter.fetch_add(1)
Capturer->>Fail: on_skip_event -> cap_fail::fail_ret!("NAME")
alt should_fail true (debug)
Fail->>Capturer: print & return (early exit)
else should_fail false
Fail-->>Capturer: continue
end
par periodic logging (every 5s)
Logger->>Logger: read video_frame_counter.load()
Logger->>App: log frame count
end
App->>Shutdown: stop capture
Shutdown->>Logger: cancel_token triggered / drop_guard dropped
Logger-->>Logger: logger task exits
Shutdown->>App: log final frame count
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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: 1
🧹 Nitpick comments (3)
crates/fail/src/lib.rs (1)
60-79: fail_ret works; consider a value-returning variant and tracing over eprintln
- This returns unit; callers must be in ()-returning contexts. Consider adding an overload that returns a value for non-unit functions.
- Prefer tracing over eprintln to align with logging and avoid clippy “print” lints in debug builds.
Example additions:
#[macro_export] macro_rules! fail_ret { - ($name:literal) => { + ($name:literal) => { #[cfg(debug_assertions)] { const NAME: &'static str = concat!(env!("CARGO_PKG_NAME"), "::", $name); $crate::private::inventory::submit! { $crate::Fail { name: NAME } } let should_fail = $crate::private::should_fail(NAME); if should_fail { - eprintln!("Purposely returned at '{NAME}'"); + tracing::warn!("Purposely returned at '{NAME}'"); return; } } }; + ($name:literal, $ret:expr) => { + #[cfg(debug_assertions)] + { + const NAME: &'static str = concat!(env!("CARGO_PKG_NAME"), "::", $name); + $crate::private::inventory::submit! { $crate::Fail { name: NAME } } + if $crate::private::should_fail(NAME) { + tracing::warn!("Purposely returned value at '{NAME}'"); + return $ret; + } + } + }; }crates/recording/src/sources/screen_capture/windows.rs (1)
35-36: Use the LOG_INTERVAL constant consistentlyLOG_INTERVAL is defined but not used; replace the hardcoded 5s sleep with the constant.
-const LOG_INTERVAL: Duration = Duration::from_secs(5); +const LOG_INTERVAL: Duration = Duration::from_secs(5); - tokio::time::sleep(Duration::from_secs(5)).await; + tokio::time::sleep(LOG_INTERVAL).await;Also applies to: 260-264
crates/recording/src/sources/screen_capture/macos.rs (1)
363-376: Minor: extract a LOG_INTERVAL and reuse across platformsFor consistency with Windows and readability, factor the 5s literal into a LOG_INTERVAL constant and use it here. Optional.
-use std::{ +use std::{ sync::{ Arc, atomic::{self, AtomicBool, AtomicU32}, }, - time::Duration, + time::Duration, }; ... - loop { - tokio::time::sleep(Duration::from_secs(5)).await; + loop { + tokio::time::sleep(LOG_INTERVAL).await; debug!( "Captured {} frames", video_frame_count.load(atomic::Ordering::Relaxed) ); }And near the top:
const LOG_INTERVAL: Duration = Duration::from_secs(5);Also applies to: 385-392
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/fail/src/lib.rs(1 hunks)crates/recording/src/sources/screen_capture/macos.rs(8 hunks)crates/recording/src/sources/screen_capture/windows.rs(4 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/recording/src/sources/screen_capture/windows.rscrates/fail/src/lib.rscrates/recording/src/sources/screen_capture/macos.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/sources/screen_capture/windows.rscrates/fail/src/lib.rscrates/recording/src/sources/screen_capture/macos.rs
🧬 Code graph analysis (2)
crates/recording/src/sources/screen_capture/windows.rs (2)
crates/scap-direct3d/src/lib.rs (3)
new(154-346)settings(348-350)Capturer(145-151)crates/recording/src/output_pipeline/core.rs (1)
spawn(119-139)
crates/recording/src/sources/screen_capture/macos.rs (2)
crates/recording/src/sources/screen_capture/windows.rs (6)
std(183-183)setup(169-305)setup(353-391)start(311-320)start(397-400)video_info(307-309)crates/recording/src/output_pipeline/core.rs (16)
new(595-597)new(638-640)new(671-673)new(707-724)setup(606-621)setup(646-658)setup(680-686)setup(730-736)setup(756-765)source(550-552)start(690-692)start(740-742)spawn(119-139)video_info(580-582)video_info(623-625)video_info(688-688)
⏰ 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). (5)
- 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)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
crates/recording/src/sources/screen_capture/macos.rs (1)
159-183: Frame counter + fail_ret placement looks correctIncrementing the counter only for valid screen frames and skipping via fail_ret in debug builds is sound. Audio-path skip mirrors this.
| let drop_guard = cancel_token.drop_guard(); | ||
| tokio::spawn({ | ||
| let video_frame_count = video_frame_counter.clone(); | ||
| async move { | ||
| loop { | ||
| tokio::time::sleep(Duration::from_secs(5)).await; | ||
| debug!( | ||
| "Captured {} frames", | ||
| video_frame_count.load(atomic::Ordering::Relaxed) | ||
| ); | ||
| } | ||
| } | ||
| .with_cancellation_token_owned(cancel_token.clone()) | ||
| .in_current_span() | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
tokio::spawn from non-runtime thread can panic; use a captured Runtime Handle
This closure runs on a std::thread via spawn_thread. Calling tokio::spawn here will panic if the thread isn’t within a Tokio runtime. Capture a Handle in the async setup before spawning the thread and use handle.spawn inside the thread.
Apply this diff in this block:
- let drop_guard = cancel_token.drop_guard();
- tokio::spawn({
+ let drop_guard = cancel_token.drop_guard();
+ rt_handle.spawn({
let video_frame_count = video_frame_counter.clone();
async move {
loop {
- tokio::time::sleep(Duration::from_secs(5)).await;
+ tokio::time::sleep(LOG_INTERVAL).await;
debug!(
"Captured {} frames",
video_frame_count.load(atomic::Ordering::Relaxed)
);
}
}
.with_cancellation_token_owned(cancel_token.clone())
.in_current_span()
});Add this outside the spawned thread (inside setup), so it’s available to move into the closure:
// capture before ctx.tasks().spawn_thread(...)
let rt_handle = tokio::runtime::Handle::current();Also confirm that with_cancellation_token_owned is in scope (custom extension) in both targets.
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the file structure and the specific section
cat crates/recording/src/sources/screen_capture/windows.rs | head -300 | tail -100
Length of output: 3631
🏁 Script executed:
#!/bin/bash
# Search for spawn_thread calls in the file to understand thread context
rg -n "spawn_thread" crates/recording/src/sources/screen_capture/windows.rs -C 10Length of output: 1019
🏁 Script executed:
#!/bin/bash
# Search for LOG_INTERVAL definition
rg -n "LOG_INTERVAL" crates/recording/src/sources/screen_capture/windows.rsLength of output: 57
🏁 Script executed:
#!/bin/bash
# Search for rt_handle or Handle usage in file
rg -n "rt_handle|Handle::current" crates/recording/src/sources/screen_capture/windows.rs -C 3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check imports and trait extensions
head -50 crates/recording/src/sources/screen_capture/windows.rsLength of output: 1458
🏁 Script executed:
#!/bin/bash
# Search for with_cancellation_token_owned to verify it's imported/available
rg -n "with_cancellation_token_owned" crates/recording/src/sources/screen_capture/windows.rs -B 5Length of output: 269
🏁 Script executed:
#!/bin/bash
# Get broader context around lines 255-269
sed -n '230,290p' crates/recording/src/sources/screen_capture/windows.rsLength of output: 2219
Replace tokio::spawn with a captured runtime handle and use LOG_INTERVAL constant
The code calls tokio::spawn() from a non-runtime thread (inside spawn_thread at line 185), which will panic at runtime. Capture tokio::runtime::Handle::current() in the async setup function before spawning the thread, move it into the closure, and use it to spawn the task. Additionally, use the existing LOG_INTERVAL constant instead of Duration::from_secs(5).
Add before line 185 (before ctx.tasks().spawn_thread(...)):
let rt_handle = tokio::runtime::Handle::current();Apply this diff at lines 255-269:
let drop_guard = cancel_token.drop_guard();
- tokio::spawn({
+ rt_handle.spawn({
let video_frame_count = video_frame_counter.clone();
async move {
loop {
- tokio::time::sleep(Duration::from_secs(5)).await;
+ tokio::time::sleep(LOG_INTERVAL).await;
debug!(
"Captured {} frames",
video_frame_count.load(atomic::Ordering::Relaxed)
);
}
}
.with_cancellation_token_owned(cancel_token.clone())
.in_current_span()
});
1 participant
Summary by CodeRabbit
New Features
Refactor