Getting rid of the EventLoop

[ishitatsuyuki]

Close #278. I rushed for a buildable version so you can see it, there's still a lot of thing to do.

• Check for can_pause and optionally return an error
• Polymorphic struct versions of Traits will be explicitly !Sync, !Send
• Take an additional error callback as described in Better error handling interface #268 (comment)
• Update documentation
• Checkbox for ASIO backend so I don't miss it because it's without CI

[ishitatsuyuki]

Examples are updated.

[ishitatsuyuki]

The ALSA backend is ready for review.

Breaking change: when an error happens, the error callback will be called but that doesn't destroy the stream or thread. The user will have to drop the Stream object instead.

[ishitatsuyuki]

cc @mitchmindtree

[mitchmindtree]

Nice @ishitatsuyuki! I've only had a chance to look at the examples so far and I think they're looking great!

I haven't had a chance to dig into the ALSA implementation just yet, but in the meantime a couple things that come to mind are:

• I wonder if we should we make sure that Streams are Sync and Send for each platform? This is something that wasn't super clear to me w.r.t. the EventLoop. From memory, most hosts had an unsafe impl Send/Sync but for the most part it has been undocumented and none of the impls explained why it was safe. I think it's nice from a usability point of view, but 1. we should check that it's possible to do this in a safe manner for each platform and 2. maybe we should add a small test that tries to pass a Stream to a empty function that expects T: Send + Sync? I hope this isn't derailing this PR too much, it's just something that might be worth clearing up early as I imagine it might affect implementation details for newer Stream impls.
• I want to make sure we have a clear path ahead for handling the web audio API. Do you think it will be possible to spawn both an input and output stream with the stdweb restrictions mentioned here? My understanding of the restriction is still a little vague - I guess the main concern is that ensuring building a stream does not block the entire thread like EventLoop::run currently does due to calling stdweb::event_loop.

I will aim to do a more thorough review of the ALSA backend soon!

[ishitatsuyuki]

I wonder if we should we make sure that Streams are Sync and Send for each platform?

No, I intentionally exclude some of the platforms. Sync and Send are only implemented if you are allowed to access things within the callback, which rules out Android.

In general, the synchronization story is unclear on many platforms, and I think it's the best to code in a style that uses message passing to serialize access to the underlying API.

Do you think it will be possible to spawn both an input and output stream with the stdweb restrictions

My stance doesn't change from before, but I think that there is no problem doing that. Effectively we submit a task to the web event loop, instead of spawning a thread, and that perfectly fits into how the web API was supposed to work.

And a disclaimer: I made some pretty wild refactor when modifying the ALSA backend (the code moved isn't preserved as-is), sorry if it seems hard to review.

[ishitatsuyuki]

Rebased, but... I'm still struggling to motivate myself to work on the other backends.

[gentoid]

I may try to help with Windows platform. I have a machine with Win 10. Though please expect that there may be many questions :-D

[ishitatsuyuki]

This is my first try at writing mentoring instructions, sorry if it's not detailed enough! Feel free to ask any questions.

So the API changes are like this:

• StreamId is removed; the internal loop no longer handles multiple streams
• EventLoop is removed; build_input/output_stream are moved onto Device, play/pause are moved onto a new Stream struct (and trait)
• The callback methods are now passed to build_input/output_stream and they are now two different methods for data/error instead of one.

We first start with changing the main loop:

1. Change RunContext so it only contains one Arc<StreamInner>, not the Vec.
   

   cpal/src/host/wasapi/stream.rs

   Line 58 in 9e3a19c

   streams: Vec<StreamInner>,

2. Move the run_inner function out as a free function. The signature should be fn run_inner(run_context: RunContext, data_callback: &mut dyn FnMut(StreamData), error_callback: &mut dyn FnMut(StreamResult) (returns () instead of !).
   

   cpal/src/host/wasapi/stream.rs

   Line 453 in 9e3a19c

   fn run_inner(&self, callback: &mut dyn FnMut(StreamId, StreamDataResult)) -> ! {

3. Delete these lines.
   

   cpal/src/host/wasapi/stream.rs

   Lines 455 to 465 in 9e3a19c

   	// We keep `run_context` locked forever, which guarantees that two invocations of
   	// `run()` cannot run simultaneously.
   	let mut run_context = self.run_context.lock().unwrap();
   	// Force a deref so that borrow checker can operate on each field independently.
   	// Shadow the name because we don't use (or drop) it otherwise.
   	let run_context = &mut *run_context;
   	// Keep track of the set of streams that should be removed due to some error occurring.
   	//
   	// Checked at the start of each loop.
   	let mut streams_to_remove: Vec<(StreamId, StreamError)> = vec![];

   
   

   cpal/src/host/wasapi/stream.rs

   Lines 468 to 478 in 9e3a19c

   	// Remove any failed streams.
   	for (stream_id, err) in streams_to_remove.drain(..) {
   	match run_context.streams.iter().position(|s| s.id == stream_id) {
   	None => continue,
   	Some(p) => {
   	run_context.handles.remove(p + 1);
   	run_context.streams.remove(p);
   	callback(stream_id, Err(err.into()));
   	},
   	}
   	}

4. Whenever you encounter a long, old error handling template, replace it with error_callback(err); break 'stream_loop. You may need to use into, remove Err(..) (because callback no longer takes a Result) so it compiles.

5. There is only one stream, so delete these two lines.
   

   cpal/src/host/wasapi/stream.rs

   Lines 502 to 503 in 9e3a19c

   	let stream_idx = handle_idx - 1;
   	let stream = &mut run_context.streams[stream_idx];

6. Remove this line.
   https://github.com/RustAudio/cpal/blob/master/src/host/wasapi/stream.rs#L627

7. Also alter process_commands. Remove the NewStream and DestroyStream variant as described below, and change the error handling as described above.

And now let's go on to the other API changes:

1. Create the new Stream struct. It should contain:

• thread: Option<JoinHandle<()>>, this is the thread that we create with this new API.
• commands, pending_scheduled_event: copy these from EventLoop.

1. Remove the NewStream and DestroyStream variant from Command.

2. Move the build_input/output_stream methods onto Device. You need to alter function signature and move referenced function (some of used functions are in stream.rs). When constructing a Stream, create a std::thread::Thread that indirectly calls run_inner, and use the JoinHandle returned. The construction of other fields should be copied from EventLoop::new.
   For how to create the thread, see the ALSA implementation for an example:
   

   cpal/src/host/alsa/mod.rs

   Lines 706 to 708 in 0ee93de

   	let thread = thread::spawn(move || {
   	stream_worker(rx, &*stream, &mut data_callback, &mut error_callback);
   	});

3. Implement Drop for Device. It should contain two parts: one is from EventLoop
   

   cpal/src/host/wasapi/stream.rs

   Lines 653 to 660 in 9e3a19c

   	impl Drop for EventLoop {
   	#[inline]
   	fn drop(&mut self) {
   	unsafe {
   	handleapi::CloseHandle(self.pending_scheduled_event);
   	}
   	}
   	}

   
   And another to join the thread, like the ALSA implementation.
   

   cpal/src/host/alsa/mod.rs

   Lines 719 to 720 in 0ee93de

   	self.trigger.wakeup();
   	self.thread.take().unwrap().join().unwrap();

   
   Use this snippet instead of trigger.wakeup().
   

   cpal/src/host/wasapi/stream.rs

   Lines 646 to 649 in 9e3a19c

   	unsafe {
   	let result = synchapi::SetEvent(self.pending_scheduled_event);
   	assert!(result != 0);
   	}

At this point I think I've covered most of the changes. There might be other compile errors so fix them if they're straightforward. I'll be ready to answer questions otherwise.

[gentoid]

woah, great writeup! This will help a lot! Thanks

[gentoid]

Just to let you know. I'm working on that, and the instruction is really helpful 👍

[ishitatsuyuki]

I haven't tested the WASAPI backend, I will get to that next time I use Windows if you don't beat me to it.

[gentoid]

I'm actually happy to contribute! Maybe I'll have a chance to have a look at it today. Thank you!

[gentoid]

I've fixed all the warning raised by Clippy in the wasapi module - pushed to my branch. Except mem::uninitialized, I saw there's an issue there for that.

Clippy also throws 2 errors (while cargo check doesn't):

    //     error: this comparison involving the minimum or maximum element for this type contains a case that is always true or always false
    //    --> src\host\wasapi\stream.rs:227:19
    //     |
    // 227 |     debug_assert!(result >= winbase::WAIT_OBJECT_0);
    //     |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    //     |
    //     = note: `#[deny(clippy::absurd_extreme_comparisons)]` on by default
    //     = help: because winbase::WAIT_OBJECT_0 is the minimum value for this type, this comparison is always true
    //     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#absurd_extreme_comparisons


error: casting from `*const [u8; 16]` to a more-strictly-aligned pointer (`*const usize`) (1 < 8 bytes)
   --> src\host\wasapi\device.rs:361:37
    |
361 |             let ptr_usize: usize = *(&property_value.data as *const _ as *const usize);
    |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[deny(clippy::cast_ptr_alignment)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#cast_ptr_alignment

[gentoid]

I guess I could help with ASIO too. Will try to update it using the same instruction

[ishitatsuyuki]

Merged that.

I removed the first assertion warned by clippy. We shouldn't rely on Windows constants being a particular value, but in this case the debug assertion is useless because a subtraction happens afterward which is checked in debug. So this justify the change.

Regarding the second alignment error, this is pretty much a hack and I think we'd better leave it as-is. I looked up a bit and it seems that API is old so hopefully we can replace that with calls to newer APIs that requires less hacky conversion.

If you can help with the ASIO backend too, great and thanks a lot! I'll check later if there's anything that need special handling.

[ishitatsuyuki]

I fixed the WASAPI backend to the point that beep works. Others are unverified.

[crusadergo]

Hi there! I'm newbie with Rust and I have a Mac machine. I'd like to help you with this cpal project, because I believe, it will help me to learn Rust more deeply :)

[ishitatsuyuki]

I finished the CoreAudio backend myself since it's quite different from other platforms and somewhat more simple to do.

@crusadergo Sorry I've already finished the code, but if you can please help with testing! Checkout the PR as described here and then run cargo run --example beep (and maybe other examples too) to verify that the code works.

[icefoxen]

Tested the current branch on Windows 10, it seems to work but with caveats. The beep example works fine, trying to port it to a ggez program produces something sounding Wrong, maybe more sawtooth-y. May well be my program doing something wrong in terms of timing, code is here for reference. I'll play with giving the playing sound its own thread or something.

Tested on Linux, on Ubuntu it seems to work fine (even my ggez program), on Debian the no-eventloop beep example works fine-ish but seems to occasionally stutter a little on startup (haven't checked if the old one did that). On Debian my blep example doesn't output sound at all (though PulseAudio's equalizer shows it outputting). NO IDEA where to start debugging that.

[ishitatsuyuki]

@icefoxen I inspected your code and it seems to be calling sin in a nested manner: is that what you meant to write?

When getting no audio, the thing I usually do is to insert a debug println inside the callback. Although if you're getting mixer indicators that might be useless.

[Ralith]

on Debian the no-eventloop beep example works fine-ish but seems to occasionally stutter a little on startup (haven't checked if the old one did that).

The beep example has been glitchy for ages; see #272.

[icefoxen]

it seems to be calling sin in a nested manner: is that what you meant to write?

It was, until I'd forgotten I'd written that way. Unfortunately, fixing it only makes the sound differently wrong. XD I'll investigate more later when I have time.

[ishitatsuyuki]

Closing in favor of #354.