Move windows capturer onto dedicated thread #1113
Conversation
WalkthroughEnables system audio in the recording CLI example. Refactors Windows screen capture to run in a dedicated thread with readiness and stop coordination via channels, replaces the in-process capturer handle, and changes StopCapturing reply type to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant ScreenCaptureActor as ScreenCaptureActor
participant CapturerThread as "Capturer Thread"
participant Capturer
Caller->>ScreenCaptureActor: StartCapturing
activate ScreenCaptureActor
ScreenCaptureActor->>CapturerThread: spawn thread (ready oneshot + stop mpsc)
activate CapturerThread
CapturerThread->>Capturer: initialize/start capturer
CapturerThread-->>ScreenCaptureActor: send ready via oneshot
note right of Capturer: capture loop runs, frames -> handlers
deactivate ScreenCaptureActor
Caller->>ScreenCaptureActor: StopCapturing
activate ScreenCaptureActor
ScreenCaptureActor->>CapturerThread: send oneshot Sender over stop mpsc
CapturerThread->>Capturer: stop()
CapturerThread-->>ScreenCaptureActor: send stop result (Result<(), StopCapturerError>)
ScreenCaptureActor-->>Caller: Reply mapped to Result<(), String>
deactivate ScreenCaptureActor
deactivate CapturerThread
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (4)crates/**/src/**/*.rs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx,js,jsx,rs}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
crates/*/src/**/*📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)crates/recording/src/sources/screen_capture/windows.rs (3)
⏰ 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)
🔇 Additional comments (4)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/recording/examples/recording-cli.rs(1 hunks)crates/recording/src/sources/screen_capture/windows.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
crates/recording/examples/recording-cli.rscrates/recording/src/sources/screen_capture/windows.rs
**/*.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/examples/recording-cli.rscrates/recording/src/sources/screen_capture/windows.rs
crates/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
For desktop IPC, use tauri_specta derive/macros on Rust types/events; do not hand-roll bindings
Files:
crates/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/recording/src/sources/screen_capture/windows.rs
🧬 Code graph analysis (1)
crates/recording/src/sources/screen_capture/windows.rs (3)
crates/recording/src/feeds/camera.rs (11)
oneshot(336-336)handle(326-400)handle(406-422)handle(428-430)handle(436-453)handle(459-465)handle(471-487)handle(503-539)handle(545-569)handle(575-591)handle(597-610)crates/scap-direct3d/src/lib.rs (4)
new(140-335)d3d_device(193-193)d3d_device(345-347)d3d_device(458-460)crates/mediafoundation-utils/src/lib.rs (1)
thread_init(18-21)
⏰ 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)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(1 hunks)crates/recording/src/sources/screen_capture/windows.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
crates/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
For desktop IPC, use tauri_specta derive/macros on Rust types/events; do not hand-roll bindings
Files:
crates/recording/src/sources/screen_capture/windows.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
crates/recording/src/sources/screen_capture/windows.rs
**/*.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.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.rs
🧬 Code graph analysis (1)
crates/recording/src/sources/screen_capture/windows.rs (4)
crates/recording/src/feeds/camera.rs (11)
oneshot(336-336)handle(326-400)handle(406-422)handle(428-430)handle(436-453)handle(459-465)handle(471-487)handle(503-539)handle(545-569)handle(575-591)handle(597-610)crates/scap-direct3d/src/lib.rs (4)
new(140-335)d3d_device(193-193)d3d_device(345-347)d3d_device(458-460)crates/mediafoundation-utils/src/lib.rs (1)
thread_init(18-21)crates/recording/src/feeds/microphone.rs (3)
handle(245-365)handle(371-383)handle(389-391)
⏰ 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 (5)
.github/workflows/ci.yml (1)
100-100: LGTM! Clippy component properly configured.The addition of the
clippycomponent ensures the linting tool is available during the CI build, supporting the Clippy check action that runs at line 128.crates/recording/src/sources/screen_capture/windows.rs (4)
9-9: LGTM! Required imports for thread coordination.The
oneshotchannel andStopCapturerErrortype support the new thread-based coordination model with ready signaling and stop result propagation.Also applies to: 11-11
373-373: LGTM! Field change supports thread coordination.The
stop_txfield correctly replaces the direct capturer handle with a channel for coordinating stop requests and results with the dedicated thread.Also applies to: 381-381
429-481: Thread coordination pattern correctly implements the fix.The dedicated thread with
thread_init()at line 436 addresses the COM marshalling issue mentioned in the PR objectives. The ready/stop channel coordination ensures proper lifecycle management:
- Thread initializes COM via
thread_init()(fixes marshalling)- Creates and starts capturer
- Signals readiness via
ready_tx- Blocks on
stop_rxfor shutdown signal- Stops capturer and reports result
Frame and closed callbacks correctly use thread-safe channels (
Recipientandflume::Sender), and the capturer is properly cleaned up when the thread exits.
496-519: LGTM! Stop coordination properly awaits worker thread result.The
StopCapturinghandler correctly:
- Takes ownership of
stop_tx(line 503) ensuring single stop- Sends a oneshot channel to the worker thread (line 508)
- Awaits the stop result via
done_rx(lines 512-514)- Propagates errors from
capturer.stop()as stringsThe error at line 509 is logged when the worker thread has already exited, which is an acceptable fallback.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/recording/src/sources/screen_capture/windows.rs (1)
503-503: Consider retaining typed error information.The reply type changed from
Result<(), StopCapturingError>toResult<(), String>, which loses type information and prevents callers from matching on specific error variants. While this simplification aligns with the current usage (where errors are ignored at line 328), consider whether preserving the typed error would benefit future callers or debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/recording/src/sources/screen_capture/windows.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
crates/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
For desktop IPC, use tauri_specta derive/macros on Rust types/events; do not hand-roll bindings
Files:
crates/recording/src/sources/screen_capture/windows.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
crates/recording/src/sources/screen_capture/windows.rs
**/*.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.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.rs
🧬 Code graph analysis (1)
crates/recording/src/sources/screen_capture/windows.rs (4)
crates/recording/src/feeds/camera.rs (11)
oneshot(336-336)handle(326-400)handle(406-422)handle(428-430)handle(436-453)handle(459-465)handle(471-487)handle(503-539)handle(545-569)handle(575-591)handle(597-610)crates/scap-direct3d/src/lib.rs (4)
new(140-335)d3d_device(193-193)d3d_device(345-347)d3d_device(458-460)crates/mediafoundation-utils/src/lib.rs (1)
thread_init(18-21)crates/recording/src/feeds/microphone.rs (3)
handle(245-365)handle(371-383)handle(389-391)
⏰ 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 (3)
crates/recording/src/sources/screen_capture/windows.rs (3)
9-11: LGTM!The new imports are correctly used for thread coordination (
oneshot) and error handling (StopCapturerError).
373-381: LGTM!The field replacement correctly reflects the new threading model, storing a channel to signal stop and receive results from the dedicated capturer thread.
416-499: LGTM!The implementation correctly moves the Direct3D capturer onto a dedicated thread with proper ready/stop coordination:
- Thread initialization calls
thread_init()to set up COM/Media Foundation- Ready signal is awaited before returning, ensuring initialization errors surface synchronously
stop_txis only set after successful startup, preventing stop attempts on failed initialization- Stop coordination properly waits for the signal, stops the capturer, and returns the result
This addresses the threading/marshalling issue mentioned in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/recording/src/sources/screen_capture/windows.rs (1)
519-525: Fix: Propagate channel drop error in stop coordination.The
if let Ok(res)pattern at line 519 silently ignores theErrcase whendone_rx.awaitfails (e.g., if the worker thread panics or drops the channel). This causesStopCapturingto incorrectly returnOk(())at line 525 even though the stop result was never confirmed.Apply this diff to propagate the error:
- if let Ok(res) = done_rx.await { - res.map_err(|e| e.to_string())?; - } + match done_rx.await { + Ok(res) => res.map_err(|e| e.to_string())?, + Err(_) => return Err("Capturer thread dropped stop channel".to_string()), + } info!("stopped windows capturer"); Ok(())
🧹 Nitpick comments (1)
crates/recording/src/sources/screen_capture/windows.rs (1)
514-517: Consider propagating stop signal send failure.If
stop_tx.send(done_tx)fails at line 515, the worker thread has already exited (channel disconnected). The current code logs the error but continues to awaitdone_rx, which will never receive a value. Consider returning an error immediately instead of awaiting a channel that will never respond.Apply this diff:
let (done_tx, done_rx) = oneshot::channel(); - if let Err(e) = stop_tx.send(done_tx) { - error!("Silently failed to stop Windows capturer: {}", e); - } + stop_tx + .send(done_tx) + .map_err(|_| "Worker thread already exited".to_string())?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/recording/src/sources/screen_capture/windows.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
crates/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
For desktop IPC, use tauri_specta derive/macros on Rust types/events; do not hand-roll bindings
Files:
crates/recording/src/sources/screen_capture/windows.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
crates/recording/src/sources/screen_capture/windows.rs
**/*.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.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.rs
🧬 Code graph analysis (1)
crates/recording/src/sources/screen_capture/windows.rs (4)
crates/recording/src/feeds/camera.rs (11)
oneshot(336-336)handle(326-400)handle(406-422)handle(428-430)handle(436-453)handle(459-465)handle(471-487)handle(503-539)handle(545-569)handle(575-591)handle(597-610)crates/scap-direct3d/src/lib.rs (4)
new(140-335)d3d_device(193-193)d3d_device(345-347)d3d_device(458-460)crates/mediafoundation-utils/src/lib.rs (1)
thread_init(18-21)crates/recording/src/feeds/microphone.rs (3)
handle(245-365)handle(371-383)handle(389-391)
⏰ 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 (4)
crates/recording/src/sources/screen_capture/windows.rs (4)
9-9: LGTM: Import additions support thread coordination.The
futures::channel::oneshotandscap_direct3d::StopCapturerErrorimports are necessary for the new thread-based capturer lifecycle and stop coordination.Also applies to: 11-11
373-373: LGTM: Field change aligns with thread-based architecture.The replacement of
capture_handlewithstop_txcorrectly reflects the new thread-based capturer lifecycle where stop coordination happens via channels rather than direct method calls.Also applies to: 381-381
416-499: LGTM: StartCapturing correctly implements thread-based initialization.The implementation properly:
- Spawns a dedicated thread with COM initialization via
cap_mediafoundation_utils::thread_init()- Signals readiness via oneshot channel after successful capturer creation
- Awaits and propagates both success and failure from the ready channel before setting
stop_tx- Blocks the worker thread waiting for stop coordination
This addresses the marshalling concerns mentioned in the PR description.
503-503: LGTM: Reply type simplified to String.Changing the reply type from
Result<(), StopCapturingError>toResult<(), String>simplifies error handling. The caller at line 328 already ignores the result, so this change is non-breaking.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
We've been getting error reports from some Windows devices about an interface not being marshalled on the right thread, it seems to be coming from stopping the recording. I've moved the direct3d capturer onto a dedicated thread just in case.
Summary by CodeRabbit
New Features
Refactor
Chores