Skip to content
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.

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

Implement Send for Stream on certain platforms #840

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bushrat011899
Copy link

Objective

Solution

Moved NotSendSyncAcrossAllPlatforms into its own module, maybe_send (for clarity) and implemented Send for an instance where (I believe) it is safe to do so. I've deliberately left the cfg statement controlling this implementation verbose to make it clear how other platforms could be included where it is confirmed to be safe to do so:

#[cfg(any(
    // Windows with WASAPI allows for a Send Stream
    all(windows, not(feature = "asio"))
))]
unsafe impl Send for NotSendSyncAcrossAllPlatforms {}

To add this support to Linux but only on FreeBSD using Jack (as an example) then a single statement can be added without modifying any of the others:

#[cfg(any(
    // Windows with WASAPI allows for a Send Stream
    all(windows, not(feature = "asio"))
    // FreeBSD using Jack allows for a Send Stream
    all(target_os = "freebsd", feature = "jack"),
))]
unsafe impl Send for NotSendSyncAcrossAllPlatforms {}

I have deliberately only implemented Send on Windows under WASAPI in this pull request as I believe it is known to be Send safe and should be relatively straight-forward to test. I would encourage others to test on platforms they can also confirm the behaviour on and then make follow-up PRs to increase support.

Motivation

I personally ran into this issue myself when working with Bevy and thought it was a little annoying. Not a deal breaker by any stretch of the imagination, the workarounds are well documented and perfectly reasonable. This is more of a quality of life improvement than anything else.

@bushrat011899 bushrat011899 marked this pull request as draft February 19, 2024 04:22
@bushrat011899
Copy link
Author

As a small test, the beep example can be modified like below to demonstrate the send-nature of a Stream (on supported platforms):

pub fn run<T>(device: &cpal::Device, config: &cpal::StreamConfig) -> Result<(), anyhow::Error>
where
    T: SizedSample + FromSample<f32>,
{
    // snip

    // Create the stream on the main thread...
    let stream = device.build_output_stream(
        config,
        move |data: &mut [T], _: &cpal::OutputCallbackInfo| {
            write_data(data, channels, &mut next_value)
        },
        err_fn,
        None,
    )?;

    // ...then send it to a new thread and play it...
    let stream = std::thread::spawn(move || {
        stream.play()?;
    
        std::thread::sleep(std::time::Duration::from_millis(1000));

        Ok::<_, anyhow::Error>(stream)
    });

    // ...get it back and then pause and play it again.
    let stream = stream.join().unwrap()?;

    stream.pause()?;

    std::thread::sleep(std::time::Duration::from_millis(1000));
    
    stream.play()?;

    std::thread::sleep(std::time::Duration::from_millis(1000));

    Ok(())
}

@bushrat011899 bushrat011899 marked this pull request as ready for review February 19, 2024 04:29
@est31 est31 mentioned this pull request Mar 1, 2024
ameknite added a commit to ameknite/cpal that referenced this pull request Mar 1, 2024
@n1ght-hunter
Copy link

is there anyway we can get this merged?
seems like a really useful PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants