Skip to content

make audio capture send #958

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Brendonovich merged 2 commits into from
Aug 26, 2025
Merged

make audio capture send #958

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ca07fd
make audio capture send
Brendonovich Aug 26, 2025
6f71671
string
Brendonovich Aug 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions crates/recording/src/sources/screen_capture/windows.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,9 @@ enum SourceError {
#[error("CreateAudioCapture/{0}")]
CreateAudioCapture(scap_cpal::CapturerError),
#[error("StartCapturingAudio/{0}")]
StartCapturingAudio(SendError<audio::StartCapturing, cpal::PlayStreamError>),
StartCapturingAudio(
String, /* SendError<audio::StartCapturing, cpal::PlayStreamError> */
),
#[error("Closed")]
Closed,
}
Expand Down Expand Up @@ -289,7 +291,7 @@ impl PipelineSourceTask for ScreenCaptureSource<AVFrameCapture> {
audio_capture
.ask(audio::StartCapturing)
.await
.map_err(SourceError::StartCapturingAudio)?;
.map_err(|v| SourceError::StartCapturingAudio(v.to_string()))?;

Some(audio_capture)
} else {
Expand Down Expand Up @@ -467,6 +469,8 @@ pub mod audio {
capturer: scap_cpal::Capturer,
}

unsafe impl Send for WindowsAudioCapture {}

Comment on lines +472 to +473
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Unsafe Send for WindowsAudioCapture is likely unsound without proof; kameo/tokio may move the actor across threads

The actor holding WindowsAudioCapture can be polled on different Tokio worker threads (multi-thread runtime), which requires the actor state to be Send. By adding unsafe impl Send, we assert that moving WindowsAudioCapture (and thus its scap_cpal::Capturer) between threads is sound. That’s a high bar: CPAL on Windows (WASAPI/COM) often involves thread-affine resources; if scap_cpal::Capturer or anything it retains is !Send, this unsafely sidesteps the compiler and can lead to UB/data races.

Action items:

  • Either justify this unsafe impl with concrete invariants (and references to upstream trait impls) or avoid needing it.

Safer alternatives:

  • Prefer running this actor on a single-threaded executor or a LocalSet so it doesn’t need Send (e.g., a “local actor” variant if kameo supports it).
  • Offload the scap_cpal::Capturer to a dedicated OS thread and communicate with it via channels; keep the actor itself Send by only storing channel ends in the actor.
  • If it’s actually safe: document the invariants and add compile-time checks (see below). Even better, make scap_cpal::Capturer itself Send in its crate with a justified unsafe impl rather than layering the promise here.

Minimal immediate improvement (if you keep this):

  • Add a rigorous SAFETY comment that enumerates why moving across threads is sound, and what invariants must be upheld.

Proposed inline change (documentation only):

-    unsafe impl Send for WindowsAudioCapture {}
+    // SAFETY:
+    // - Explain why moving WindowsAudioCapture across threads is sound.
+    // - Demonstrate that all fields are Send or otherwise thread-agnostic.
+    // - State that methods are only called from one thread at a time (actor's mailbox),
+    //   and that underlying CPAL/WASAPI handles tolerate cross-thread moves.
+    unsafe impl Send for WindowsAudioCapture {}

If kameo supports a non-Send actor path, consider removing the impl instead:

-    unsafe impl Send for WindowsAudioCapture {}
+    // NOTE: Keep WindowsAudioCapture !Send and run this actor on a LocalSet/single-thread runtime.
+    // See follow-up in PR discussion for wiring.

Verification steps I recommend running in the repo to reduce guesswork:

And confirm upstream CPAL’s guarantees for the exact version in Cargo.lock:

If CPAL Stream is not Send for the version in use, this unsafe impl is a correctness bug and should be reverted in favor of a local actor/thread confinement approach.

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

echo "=== Locate scap_cpal::Capturer definition ==="
rg -nP --type=rust -C3 '^\s*(pub\s+)?struct\s+Capturer\b' || true

echo
echo "=== Check for existing Send/Sync impls on Capturer ==="
rg -nP --type=rust -C2 'impl\s+(unsafe\s+)?(Send|Sync)\s+for\s+Capturer' || true

echo
echo "=== What CPAL version are we on? ==="
rg -n 'name = "cpal"' Cargo.lock -n -C1 || true

echo
echo "=== Usage: where is WindowsAudioCapture spawned? (confirm multithread runtime) ==="
rg -nP --type=rust -C3 'WindowsAudioCapture::spawn\(|spawn\(\s*WindowsAudioCapture' || true

Length of output: 3397

🌐 Web query:

Does cpal::<version from Cargo.lock>::Stream implement Send on Windows (WASAPI)? Cite source lines from the CPAL docs or code.

Error during web search

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

echo "=== Locate WindowsAudioCapture struct ==="
rg -nP --type=rust 'pub struct WindowsAudioCapture' -C5 crates/recording/src/sources/screen_capture/windows.rs || true
rg -nP --type=rust 'struct WindowsAudioCapture' -C5 crates/recording/src/sources/screen_capture/windows.rs || true

echo
echo "=== Inspect WindowsAudioCapture fields for Capturer usage ==="
rg -nP --type=rust 'WindowsAudioCapture\s*{?[^;]*' -C5 crates/recording/src/sources/screen_capture/windows.rs || true

Length of output: 2895

🌐 Web query:

Does CPAL 0.15.3's Stream implement Send on Windows (WASAPI)? Please cite source code or docs.

Error during web search

Unsound unsafe impl Send for WindowsAudioCapture – Action Required

The unsafe impl Send for WindowsAudioCapture {} on lines 470–471 asserts that its sole field, scap_cpal::Capturer, is safe to move across threads. However:

  • The Capturer struct in crates/scap-cpal/src/lib.rs is defined as: 
    pub struct Capturer {
    stream: Stream,
    config: StreamConfig,
    _output_device: cpal::Device,
}
    and there are no Send implementations for it or its inner cpal::Stream on Windows. By default, CPAL’s WASAPI stream is not Send due to COM objects being thread-affine and requiring strict thread ownership.
  • Overriding this with an unsafe impl without a documented safety contract opens the door to UB and data races when Tokio’s multi-thread scheduler moves the actor between worker threads.

Next steps – choose one:

• Remove the unsafe impl Send and confine the actor to a single-threaded context:

- unsafe impl Send for WindowsAudioCapture {}
+ // NOTE: WindowsAudioCapture is !Send. Run this actor on a Tokio LocalSet
+ //       or single-thread runtime to maintain COM thread-affinity.

This ensures correct WASAPI usage without unsound thread-crossing.

• Justify and document the safety if truly safe to send:

- unsafe impl Send for WindowsAudioCapture {}
+ // SAFETY: Moving WindowsAudioCapture across threads is sound because:
+ // - `scap_cpal::Capturer` only holds thread-agnostic handles
+ //   (cpal::Device and StreamConfig implement Send/Sync, and
+ //    the underlying WASAPI COM objects are marshaled if moved).
+ // - All public methods synchronize access via the actor mailbox,
+ //   guaranteeing no concurrent calls.
+ unsafe impl Send for WindowsAudioCapture {}

And consider moving the Send impl upstream into the scap-cpal crate where the invariants can be centrally audited.

Without one of these fixes, the code unsafely sidesteps Rust’s guarantees. Please address as a critical fix.

impl WindowsAudioCapture {
pub fn new(
audio_tx: Sender<(ffmpeg::frame::Audio, f64)>,
Expand Down
Loading