Removing the `EventLoop` - rebased #354

mitchmindtree · 2019-12-13T15:41:11Z

This PR is a rebase of @ishitatsuyuki's work in #301. Originally opened at ishitatsuyuki#3 but closed in favour opening a new PR here.

Backends

Other TODO

Polymorphic struct versions of Traits will be explicitly !Sync, !Send
Docs

ALSA multiple streams bug

Edit: This bug seems to have been addressed in the rebase.

After testing each of the examples with the ALSA backend on my Arch Linux system, I noticed that the feedback.rs example no longer works. No audio output was occurring and after some investigation it seems the output stream callback only seems to get called once. beep.rs and record_wav.rs both work fine, which leads me to wonder if there's an issue with the way multiple streams are being handled. feedback.rs also works fine on master, so this has likely been introduced in this PR.

@ishitatsuyuki does anything come to mind as a potential cause for this? I did notice that we're now spawning a whole OS thread per stream. I wonder if there's some kind of race going on in the stream loop? I might see if I can find anything about thread safety in the ALSA docs. I also wonder if we can reduce the number of threads necessary by just using a single thread on which all of the streams run, similar to the old EventLoop::run impl. If there is some race going on, maybe using a single thread for all streams would help to avoid this.

Also, I was reading through PortAudio's non-blocking ALSA code and noticed that, when spinning up a thread for a new stream, they also attempt to set the thread's priority to the highest possible. It might be worth seeing if there is some way we can do this too, though this work can probably wait for a follow-up PR.

ishitatsuyuki · 2019-12-14T03:18:52Z

some quick facts:

libalsa is thread safe with a flag, which is enabled by default on most distros. In my implementation I assume it is enabled.
there are two ways to configure real-time privileges:
- with ulimits. This is simpler and apparently more popular.
- with RealtimeKit. It provides additional monitoring to ensure a process doesn't over-use realtime budgets, but requires D-Bus, and because it's not very popular it has been almost a sibling of PulseAudio.

ishitatsuyuki · 2019-12-14T07:34:55Z

Oh hey, and I tested the (feedback) examples and apparently it works. I'm running PulseAudio with Arch if that matters.

mitchmindtree · 2019-12-14T10:55:29Z

Oh strange, it's working for me too now! Even switching through all my different available audio devices seems to work.

Ahh, I can recreate the error by switching back to the non-rebased branch. It looks like one of the ALSA patches introduced in the rebase must have addressed whatever was going on here.

ishitatsuyuki · 2019-12-14T11:22:17Z

Feels like #327.

MOZGIII · 2019-12-15T08:56:52Z

Polymorphic struct versions of Traits will be explicitly !Sync, !Send

What does this mean (compared to the currently existing event loop APIs)? And why not make them Send + Sync?
I'm asking cause things being Send + Sync is important for my code...

ishitatsuyuki · 2019-12-15T09:40:38Z

Polymorphic struct versions of Traits will be explicitly !Sync, !Send ...

@MOZGIII Because it’s not portable. Android is a clear obstacle in this case, and there are also some platforms where thread safety is unknown.

If you need the Send/Sync version, then you need to use the concrete (platform specific) types.

MOZGIII · 2019-12-15T09:44:11Z

Oh, ok. I'll still be able to use generics, right?

Are the structs this is about just to abstract away things to dynamic dispatch while hiding it as an implementation detail?

MOZGIII · 2019-12-15T09:46:11Z

Actually, it'd be perfect if some lower-level tweaking abilities were exposed if I were to use concrete types - like buffer sizes etc. Any plans on enabling that?

ishitatsuyuki · 2019-12-15T09:55:48Z

I was thinking of exposing those platform specific options, but I'm not in charge of development at the moment. It's completely possible and a matter of being implemented or not.

MOZGIII · 2019-12-15T10:00:53Z

Sounds good. I'd look into exposing the things I need, but there's way too much turbulence going on with the code base, so I'll just wait for this to land first.

mitchmindtree · 2019-12-15T13:50:59Z

@msiglreith I just had a read through your ASIO update and it looks good to me!

From my understanding reading through the thread at ishitatsuyuki#2 it looks like the remaining steps are:

Ensure Streams don't become invalid if the Device is dropped first. It looks like we can solve this by sharing the Device's driver and asio_streams Arcs with the Streams themselves.
Ensure that when a Stream is dropped that its callback is also removed from the main ASIO loop. We could possibly achieve this by having the asio::Driver::set_callback method return a unique Id associated with the callback which can be stored by the Stream. The Id can then be used by the Stream upon drop to remove its callback from the list, perhaps via some new asio::Driver::remove_callback(callback_id) method. It might be worth changing the asio::Driver::set_callback method name to push_callback or add_callback to make its behaviour clearer.
Work out some way to stop stream creation/removal from blocking the processing of audio. As there's not an easy, clear solution for this just yet I'm happy to leave this for a future PR, particularly seeing as this is how it was behaving before anyway. Landing this PR is probably a bigger priority as it's a blocker for quite a few other issues.

Does this sound about right? Would you be interested in finishing this off? If not, let me know and I'll rebase and make these changes myself next time I get access to a Windows machine.

msiglreith · 2019-12-15T14:22:08Z

Hi 👋,

1st and 2nd point are rather design decision imo. It was not clear to me how the lifetime of the objects are coupled in the API. The suggested approach looks viable.

For the 3rd one, there are two aspects:

Recreation of the buffers in the asio driver layer when adding a new stream, which could be addressed on the cpal API side
Adding/removing streams in the internal event loop, which is not really an issue in my opinion. Using lightweight mutexes should be enough

Would you be interested in finishing this off? If not, let me know and I'll rebase and make these changes myself next time I get access to a Windows machine.

The earliest date for me to start working on it again would most likely be at end of the year.

mitchmindtree · 2019-12-15T15:07:13Z

Thanks for the quick response!

The earliest date for me to start working on it again would most likely be at end of the year.

No worries! I'll try to get onto this in the next day or two.

msiglreith · 2019-12-29T16:20:42Z

examples/feedback.rs


    // Run for 3 seconds before closing.
    println!("Playing for 3 seconds... ");
    std::thread::sleep(std::time::Duration::from_secs(3));
+    drop(input_stream);


why an explicit drop here?

The stream terminates when it's dropped, and this makes it explicit.

msiglreith · 2019-12-29T16:34:41Z

src/host/wasapi/stream.rs

-    // TODO: use AtomicU64 instead
-    next_stream_id: AtomicUsize,
+pub struct Stream {
+    /// The high-priority audio processing thread calling callbacks.


These comments feel kind of misleading as priority for these threads are never set

msiglreith · 2019-12-29T16:36:52Z

src/lib.rs

+//! > **Note**: Creating and running a stream will *not* block the thread. On modern platforms, the
+//! > given callback is called by a dedicated, high-priority thread responsible for delivering
+//! > audio data to the system's audio device in a timely manner. On older platforms that only
+//! > provide a blocking API (e.g. ALSA), CPAL will create a thread in order to consistently


should be noted that it's currently one thread per stream

- Shares the `Device`'s `driver` and `asio_streams` `Arc`s with the `Stream`s to ensure they remain valid if the `Host` or `Device` are dropped early. - Ensures that a stream's callback is removed upon `Drop`.

mitchmindtree · 2019-12-31T15:54:21Z

Thanks for the review @msiglreith!

I think this is just about ready to go now other than:

Polymorphic struct versions of Traits will be explicitly !Sync, !Send

@ishitatsuyuki off the top of your head, do you remember if this is necessary for all of those types? Or is it e.g. just the Host that requires this? Or just the Stream? It would be nice if we can make this restriction only where absolutely necessary as it is a bit of an inconvenience to have on the API. Edit: also are you sure we also need !Send? Or is it just !Sync that we need?

ishitatsuyuki · 2020-01-01T02:13:05Z

My original intention was AAudio, which is not only not thread safe but also prohibits calling certain functions within a callback. Therefore I went for setting the !Send requirement. (Apparently I can't confirm the callback limits anymore, which used to be explicitly checked in the library code - maybe I'm just missing where it's implemented or Google might have patched this.)

AAudio on the other hand is stateless for stream creation, so the !Send and !Sync bound is only required for Stream.

Though, I think ASIO is not thread safe at all? Therefore we might also need !Sync (but not !Send) on Host and Device.

TannerRogalsky · 2020-01-05T13:38:34Z

src/host/wasapi/stream.rs

-                        let buffer_len = frames_available as usize
-                            * stream.bytes_per_frame as usize / sample_size;
+        let thread =
+            thread::spawn(move || run_inner(run_context, &mut data_callback, &mut error_callback));


Should this thread initialize the COM library to COINIT_MULTITHREADED? And, if so, does this mean that the calling thread can have it's requirement on that threading model relaxed since the device setup is less timing-sensitive?

COM is initialised lazily using thread_local.

Threading model will not be relaxed AFAIK because what matters is the thread that created the handle. As long as the handle is created before the thread spawns, we'll need to use MTA when doing it.

This is in order to ensure consistent restrictions across platforms in a manner that ensures thread safety across each of the supported platforms. Please see added comments in the diff for details on which platforms impose these restrictions.

This makes some tweaks to the ASIO backend in order to fix some cases where races may have occured. This should allow us to remove the `Sync` bound on the `Device` and `Host` types.

Originally this restriction was placed due to uncertainty around the thread safety of the ASIO API. While the ASIO API itself makes no thread-safety guarantees that we are aware of, the `asio-sys` high-level bindings enforce synchronised access to the API and state transitions via a mutex.

Explicitly make dynamically dispatched API !Send + !Sync

mitchmindtree · 2020-01-13T11:49:59Z

OK this should be good to go!

MOZGIII · 2020-01-13T13:28:00Z

Any plans on publishing a new version to crates.io in the near future?

Ralith · 2020-01-13T23:32:08Z

Thank you for your hard work in bringing this to completion!

mitchmindtree force-pushed the no-eventloop-rebased branch from 2df33a6 to ef39ea9 Compare December 14, 2019 15:45

ishitatsuyuki mentioned this pull request Dec 15, 2019

Getting rid of the EventLoop #301

Closed

5 tasks

mitchmindtree force-pushed the no-eventloop-rebased branch from 771d53d to 20b914e Compare December 15, 2019 19:10

msiglreith approved these changes Dec 29, 2019

View reviewed changes

ishitatsuyuki and others added 13 commits December 31, 2019 15:42

Remove EventLoop and port the ALSA backend

c97d1dd

Change callback interface so that it takes a dedicated error callback

3cce3e4

Add one missing error handling

508e7d8

Suppress warnings in null backend

e782ca1

Ignore RLS log files

f57e7c7

Move DeviceTrait implementation to wasapi::device

37e7b5b

Rename "StreamDataResult" to "StreamData"

cffd2da

Keep a single stream only

463540f

Extract "run_inner" as a standalone function

b00630b

Fix "run_inner" fn

28cf3fe

Adjust "process_commands" and "Command"

dd418c0

Move code from stream to device

e25c44a

Add Stream struct

0541bf8

mitchmindtree and others added 7 commits December 31, 2019 15:45

Remove redundant from backend

2724544

Update docs for removal of the EventLoop

f34a062

Remove EventLoop and port the ALSA backend

cede7b4

Move code from stream to device

64ba84d

Port ASIO host to new stream-based API

bfda575

asio: Re-add possibility to create stream individually

972cce0

RAII cleanup wthin the ASIO backend

39ade49

- Shares the `Device`'s `driver` and `asio_streams` `Arc`s with the `Stream`s to ensure they remain valid if the `Host` or `Device` are dropped early. - Ensures that a stream's callback is removed upon `Drop`.

mitchmindtree force-pushed the no-eventloop-rebased branch from 20b914e to 39ade49 Compare December 31, 2019 14:46

Clarify the current state of stream behaviour in docs and comments

2afebb2

TannerRogalsky reviewed Jan 5, 2020

View reviewed changes

mitchmindtree added 4 commits January 5, 2020 17:43

Fix state transition synchronisation in ASIO

ca2aceb

This makes some tweaks to the ASIO backend in order to fix some cases where races may have occured. This should allow us to remove the `Sync` bound on the `Device` and `Host` types.

Merge pull request #2 from mitchmindtree/no-eventloop-send-sync

4fb1c3f

Explicitly make dynamically dispatched API !Send + !Sync

mitchmindtree merged commit 59ac088 into RustAudio:master Jan 13, 2020

mitchmindtree deleted the no-eventloop-rebased branch January 13, 2020 14:29

This was referenced Jan 13, 2020

wasapi: emphasize eventloop abstraction vs stream handling #350

Closed

[coreaudio] Start voices on run #184

Closed

Move SampleFormat check out of the main stream loop #119

Closed

Ralith mentioned this pull request Jan 13, 2020

The state of audio/sound libraries rust-gamedev/wg#40

Open

This was referenced Jan 14, 2020

Getting rid of the EventLoop #278

Closed

Allowing EventLoop::run to return - Discussion / Proposal for a new EventLoop::run #245

Closed

Osspial mentioned this pull request Jan 16, 2020

Windows: Survive a OleInitialize failure. Fixes #1255 rust-windowing/winit#1260

Closed

5 tasks

simlay mentioned this pull request Jan 16, 2020

Fix undeclared module errors on Mac. #360

Closed

mitchmindtree mentioned this pull request Jan 24, 2020

support wasm32-unknown-unknown target #212

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing the `EventLoop` - rebased #354

Removing the `EventLoop` - rebased #354

mitchmindtree commented Dec 13, 2019 •

edited

ishitatsuyuki commented Dec 14, 2019

ishitatsuyuki commented Dec 14, 2019 •

edited

mitchmindtree commented Dec 14, 2019

ishitatsuyuki commented Dec 14, 2019

MOZGIII commented Dec 15, 2019 •

edited

ishitatsuyuki commented Dec 15, 2019 •

edited

MOZGIII commented Dec 15, 2019

MOZGIII commented Dec 15, 2019 •

edited

ishitatsuyuki commented Dec 15, 2019

MOZGIII commented Dec 15, 2019

mitchmindtree commented Dec 15, 2019

msiglreith commented Dec 15, 2019

mitchmindtree commented Dec 15, 2019

msiglreith Dec 29, 2019

ishitatsuyuki Dec 30, 2019

msiglreith Dec 29, 2019

msiglreith Dec 29, 2019

mitchmindtree commented Dec 31, 2019 •

edited

ishitatsuyuki commented Jan 1, 2020 •

edited

TannerRogalsky Jan 5, 2020

ishitatsuyuki Jan 5, 2020

mitchmindtree commented Jan 13, 2020

MOZGIII commented Jan 13, 2020

Ralith commented Jan 13, 2020

Removing the EventLoop - rebased #354

Removing the EventLoop - rebased #354

Conversation

mitchmindtree commented Dec 13, 2019 • edited

Backends

Other TODO

ALSA multiple streams bug

ishitatsuyuki commented Dec 14, 2019

ishitatsuyuki commented Dec 14, 2019 • edited

mitchmindtree commented Dec 14, 2019

ishitatsuyuki commented Dec 14, 2019

MOZGIII commented Dec 15, 2019 • edited

ishitatsuyuki commented Dec 15, 2019 • edited

MOZGIII commented Dec 15, 2019

MOZGIII commented Dec 15, 2019 • edited

ishitatsuyuki commented Dec 15, 2019

MOZGIII commented Dec 15, 2019

mitchmindtree commented Dec 15, 2019

msiglreith commented Dec 15, 2019

mitchmindtree commented Dec 15, 2019

msiglreith Dec 29, 2019

Choose a reason for hiding this comment

ishitatsuyuki Dec 30, 2019

Choose a reason for hiding this comment

msiglreith Dec 29, 2019

Choose a reason for hiding this comment

msiglreith Dec 29, 2019

Choose a reason for hiding this comment

mitchmindtree commented Dec 31, 2019 • edited

ishitatsuyuki commented Jan 1, 2020 • edited

TannerRogalsky Jan 5, 2020

Choose a reason for hiding this comment

ishitatsuyuki Jan 5, 2020

Choose a reason for hiding this comment

mitchmindtree commented Jan 13, 2020

MOZGIII commented Jan 13, 2020

Ralith commented Jan 13, 2020

Removing the `EventLoop` - rebased #354

Removing the `EventLoop` - rebased #354

mitchmindtree commented Dec 13, 2019 •

edited

ishitatsuyuki commented Dec 14, 2019 •

edited

MOZGIII commented Dec 15, 2019 •

edited

ishitatsuyuki commented Dec 15, 2019 •

edited

MOZGIII commented Dec 15, 2019 •

edited

mitchmindtree commented Dec 31, 2019 •

edited

ishitatsuyuki commented Jan 1, 2020 •

edited