wasapi: Allow both threading models and switch the default to STA #597

ishitatsuyuki · 2021-08-06T15:16:14Z

COM can prevent undefined behavior in either concurrency model by
performing marshaling when necessary. As a result, CoInitializeEx can be
called with either concurrency model, and in this case STA provides better
compatibility with other code requiring STA, e.g. ASIO backend or winit
drag-and-drop.

To dive into the detail, the entry point of WASAPI, MMDeviceEnumerator, is
registered with "both" threading model, which means that COM objects are
created with whatever the thread's concurrency model is set to. This raises
the concern that when STA is used, marshaling might make audio buffer
operations block on the main thread, breaking continuous audio processing.
However, the implementation actually uses free-threaded marshaller for
interfaces dealing with buffer operations, which effectively bypasses COM's
compatibility marshaling behavior and perform any API calls on the caller's
thread instead. Therefore, the interfaces would operate just fine on either
concurrency model.

For more details on COM's threading model, see [1].

[1] https://thrysoee.dk/InsideCOM+/ch04d.htm

Co-Authored-By: Henrik Rydgård hrydgard@gmail.com

Close #59
Close #348
Close #504
Close #530
Close #538
Close #572

ishitatsuyuki · 2021-08-06T15:17:40Z

cc @hrydgard @JoshuaBatty

Ralith · 2021-08-07T18:45:06Z

Thanks for doing all that research and sorting this out, this is awesome!

maroider

Echoing what @Ralith said: Thanks for fixing this long-standing issue!

I think this PR also fixes #530 and #572.

maroider · 2021-08-07T18:56:42Z

src/host/wasapi/com.rs

-struct ComInitialized(*mut ());
+struct ComInitialized {
+    result: HRESULT,
+    _ptr: *mut (),


Suggested change

_ptr: *mut (),

_marker: PhantomData<*mut ()>,

Using PhantomData<*mut T> avoids the need to store a pointer while still removing Send/Sync.

maroider · 2021-08-07T18:56:45Z

src/host/wasapi/com.rs

+        // Try to initialize COM with STA by default to avoid compatibility issues with the ASIO
+        // backend (where CoInitialize() is called by the ASIO SDK) or winit (where drag and drop
+        // requires STA).
+        // this call can fail with RPC_E_CHANGED_MODE if another library initialized COM with MTA.


Suggested change

// this call can fail with RPC_E_CHANGED_MODE if another library initialized COM with MTA.

// This call can fail with RPC_E_CHANGED_MODE if another library initialized COM with MTA.

nit: capitalization

COM can prevent undefined behavior in either concurrency model by performing marshaling when necessary. As a result, CoInitializeEx can be called with either concurrency model, and in this case STA provides better compatibility with other code requiring STA, e.g. ASIO backend or winit drag-and-drop. To dive into the detail, the entry point of WASAPI, MMDeviceEnumerator, is registered with "both" threading model, which means that COM objects are created with whatever the thread's concurrency model is set to. This raises the concern that when STA is used, marshaling might make audio buffer operations block on the main thread, breaking continuous audio processing. However, the implementation actually uses free-threaded marshaller for interfaces dealing with buffer operations, which effectively bypasses COM's compatibility marshaling behavior and perform any API calls on the caller's thread instead. Therefore, the interfaces would operate just fine on either concurrency model. For more details on COM's threading model, see [1]. [1] https://thrysoee.dk/InsideCOM+/ch04d.htm Co-Authored-By: Henrik Rydgård <hrydgard@gmail.com> Close RustAudio#59 Close RustAudio#348 Close RustAudio#504 Close RustAudio#530 Close RustAudio#538 Close RustAudio#572

ishitatsuyuki · 2021-08-08T03:04:04Z

Thanks @maroider for the suggested changes!

est31

Thanks!

est31 · 2021-08-08T05:05:45Z

This is a big one. i think it warrants a new release, don't you think?

ishitatsuyuki · 2021-08-08T06:01:03Z

Yeah, let's have a release!

ishitatsuyuki · 2021-08-08T16:53:47Z

@est31 I don't have publish perms on crates.io, would you mind doing the release? (just a friendly reminder)

est31 · 2021-08-08T17:17:36Z

@ishitatsuyuki I think it should be enough to file a PR that increases the version. That will trigger the auto publish machinery.

ishitatsuyuki · 2021-08-08T17:26:39Z

Got it, will do that tomorrow unless someone beat me to it.

est31 · 2021-08-08T20:37:36Z

0.13.4 is out: #598

danwilhelm · 2021-08-09T02:23:16Z

@ishitatsuyuki Thanks for thoroughly researching this issue!

JoshuaBatty · 2021-08-09T04:06:26Z

Thanks @ishitatsuyuki for doing this. Very much appreciated!

It was initialized on a separate thread since 6178dd9, as a workaround for Windows. But now thanks to RustAudio/cpal#597, this is no longer needed, and cpal can be initialized on the same thread as before.

# Objective - Fixes #2096 ## Solution - This PR enables the drag-and-drop feature for winit on windows again, as the collision issue between cpal and winit has been fixed in RustAudio/cpal#597. I confirmed the drag and drop example working on windows 10 with this change. - ~~It also bumps the rodio version, though this is not strictly necessary.~~

CSoundInterface previously called CoInitializeEx and CoUninitialize in its constructor and destructor. However this failed on Windows 7: On startup or after changing configuration, CSoundInterface::OpenChannel () failed to call IAudioClient::Initialize(), and it instead returned 0x800401f0 CO_E_NOTINITIALIZED. This is because CSoundInterface() called CoInitializeEx() on the GUI thread, while CSoundInterface::OpenChannel() is called on the audio thread. This is a program bug, and Windows 10 and Wine happened to mask this error for some reason. To avoid this issue, call CoInitializeEx and CoUninitialize on the GUI/audio threads' startup/teardown functions (CFamiTrackerApp and CSoundGen::InitInstance/ExitInstance()), rather than the CSoundInterface object's constructor and destructor. Hopefully there aren't problems with accessing COM objects on the wrong thread. It doesn't matter where IMMDeviceEnumerator/ IMMDeviceCollection/IMMDevice are constructed and accessed, since those types are only accessed in startup/teardown(which don't need to be real-time). All real-time logic talks to IAudioClient and IAudioRenderClient, which are created on the audio thread in CSoundInterface::OpenChannel() (and hopefully belong to the audio thread, and if not they use a free-threaded marshaller anyway) and only accessed on the audio thread. COM threading is hard. RustAudio/cpal#597, mozilla/cubeb#534

# Objective - Fixes bevyengine#2096 ## Solution - This PR enables the drag-and-drop feature for winit on windows again, as the collision issue between cpal and winit has been fixed in RustAudio/cpal#597. I confirmed the drag and drop example working on windows 10 with this change. - ~~It also bumps the rodio version, though this is not strictly necessary.~~

# Objective - Fixes bevyengine/bevy#2096 ## Solution - This PR enables the drag-and-drop feature for winit on windows again, as the collision issue between cpal and winit has been fixed in RustAudio/cpal#597. I confirmed the drag and drop example working on windows 10 with this change. - ~~It also bumps the rodio version, though this is not strictly necessary.~~

ishitatsuyuki force-pushed the com branch 2 times, most recently from a08b4c9 to 494ef00 Compare August 6, 2021 16:38

hrydgard approved these changes Aug 6, 2021

View reviewed changes

maroider suggested changes Aug 7, 2021

View reviewed changes

ishitatsuyuki force-pushed the com branch from 494ef00 to 03947af Compare August 8, 2021 03:03

est31 approved these changes Aug 8, 2021

View reviewed changes

est31 merged commit 508618d into RustAudio:master Aug 8, 2021

PolyMeilex mentioned this pull request Aug 8, 2021

Opening a file dialog does nothing half of the time in Windows PolyMeilex/rfd#11

Closed

est31 mentioned this pull request Aug 8, 2021

Release 0.13.4 #598

Merged

danwilhelm mentioned this pull request Aug 9, 2021

Remove Windows drag-and-drop method nannou-org/nannou#785

Merged

ishitatsuyuki deleted the com branch August 9, 2021 08:24

hrydgard mentioned this pull request Aug 9, 2021

Embark's pull requests EmbarkStudios/rust-ecosystem#20

Open

26 tasks

Herohtar mentioned this pull request Aug 18, 2021

PistonWindow doesn't work with rodio on Windows 10 due to incompatible thread modes PistonDevelopers/piston_window#297

Closed

relrelb mentioned this pull request Sep 3, 2021

desktop: Revert cpal initialization on a separate thread ruffle-rs/ruffle#5215

Merged

Veykril mentioned this pull request Jan 26, 2022

[Merged by Bors] - Enable drag-and-drop events on windows bevyengine/bevy#3772

Closed

nyanpasu64 mentioned this pull request May 14, 2022

Replace DirectSound backend with WASAPI Dn-Programming-Core-Management/Dn-FamiTracker#124

Merged

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wasapi: Allow both threading models and switch the default to STA #597

wasapi: Allow both threading models and switch the default to STA #597

ishitatsuyuki commented Aug 6, 2021 •

edited

Loading

ishitatsuyuki commented Aug 6, 2021

Ralith commented Aug 7, 2021

maroider left a comment

maroider Aug 7, 2021

maroider Aug 7, 2021

ishitatsuyuki commented Aug 8, 2021

est31 left a comment

est31 commented Aug 8, 2021

ishitatsuyuki commented Aug 8, 2021

ishitatsuyuki commented Aug 8, 2021

est31 commented Aug 8, 2021

ishitatsuyuki commented Aug 8, 2021

est31 commented Aug 8, 2021

danwilhelm commented Aug 9, 2021

JoshuaBatty commented Aug 9, 2021

	// this call can fail with RPC_E_CHANGED_MODE if another library initialized COM with MTA.
	// This call can fail with RPC_E_CHANGED_MODE if another library initialized COM with MTA.

wasapi: Allow both threading models and switch the default to STA #597

wasapi: Allow both threading models and switch the default to STA #597

Conversation

ishitatsuyuki commented Aug 6, 2021 • edited Loading

ishitatsuyuki commented Aug 6, 2021

Ralith commented Aug 7, 2021

maroider left a comment

Choose a reason for hiding this comment

maroider Aug 7, 2021

Choose a reason for hiding this comment

maroider Aug 7, 2021

Choose a reason for hiding this comment

ishitatsuyuki commented Aug 8, 2021

est31 left a comment

Choose a reason for hiding this comment

est31 commented Aug 8, 2021

ishitatsuyuki commented Aug 8, 2021

ishitatsuyuki commented Aug 8, 2021

est31 commented Aug 8, 2021

ishitatsuyuki commented Aug 8, 2021

est31 commented Aug 8, 2021

danwilhelm commented Aug 9, 2021

JoshuaBatty commented Aug 9, 2021

ishitatsuyuki commented Aug 6, 2021 •

edited

Loading