WASAPI code improvements #8316

CookiePLMonster · 2019-08-17T09:53:17Z

This PR brings numerous improvements to WASAPI code in Dolphin, improving code readability as well as fixing numerous issues.

As a preparation, WIL has been added, since it's very useful for ensuring proper RAII-based destruction for several WinAPI types - in my case it was proper scoped calls to CoUninitialize and proper destruction of previously leaking (in some cases) PROPVARIANT.

All COM objects have been moved to use WRL, which ensures their proper release. Previously, there were numerous code paths where functions would return without cleaning up their COM objects, effectively leaking references.

device->GetId(&device_id); was present in several places, never using the return value nor freeing it - leaking memory for no reason.

Thread synchronization model for WASAPI Handler thread has been simplified by not detaching said thread. Not only it improved code readability, but also fixed a spinlock occuring if WASAPIStream::SetRunning(true) failed in any way - a following call to WASAPIStream::SetRunning(false) would then spin endlessly checking for m_stopped. Now this boolean is gone, and so is the problem.

std::string_view is now used where applicable. I opted for constructing a std::string out of std::string_view in HandleWinAPI, because it happens in code path which is taken relatively rarely - in a perfect case it's never taken.

I modified SoundLoop not to do volume adjustment unless absolutely necessary, saving on a relatively costly loop. For 100% volume, volume adjustment is not needed, and for 0% volume a AUDCLNT_BUFFERFLAGS_SILENT is now specified instead of zeroing the buffer. I also removed several nullptr checks which were not thread safe anyway, so only brought a false sense of safety.

CookiePLMonster · 2019-08-17T10:09:51Z

Build fails because VS2017 on the build server is too old. Seems to be the same error as reported here:
microsoft/wil#25

Apparently it can be worked around by defining __WI_LIBCPP_HAS_NO_IS_AGGREGATE, but I guess upgrading VS2017 on the build server (if possible) is better?

Source/Core/AudioCommon/WASAPIStream.cpp

lioncash · 2019-08-17T14:54:31Z

The AudioCommon CMakeLists file should also be modified to handle the use of WIL.

CookiePLMonster · 2019-08-17T15:01:02Z

The AudioCommon CMakeLists file should also be modified to handle the use of WIL.

I normally steer clear of CMake, so will

target_compile_definitions(audiocommon PRIVATE WIL_SUPPRESS_EXCEPTIONS)

be valid syntax? The other part about adding WIL\include to include paths seems easy enough.

Source/Core/AudioCommon/WASAPIStream.cpp

Source/Core/AudioCommon/WASAPIStream.h

Source/Core/AudioCommon/WASAPIStream.cpp

CookiePLMonster · 2019-08-18T11:02:57Z

I have fixed CMake definitions (and tested them), also reworded all mentioned comments. Also I added one more commits, because I just noticed a std::thread constructor with invoke semantics exists - so there is no need to involve a lambda capturing the object.

CookiePLMonster · 2019-09-06T09:29:03Z

So with build machine now migrated to Visual Studio 2019, and build succeeding, any blockers preventing from merging this in?

CookiePLMonster · 2019-09-07T13:36:24Z

Rebased on latest master and used the most up to date WIL. Doesn't bring any improvements to the PR but I think it makes sense to start off the most up to date submodule.

CookiePLMonster · 2019-09-24T15:19:38Z

Pushed a tiny change, since I noticed IID_PPV_ARGS can be used in one more place. Also rebased on latest master.

Source/Core/AudioCommon/WASAPIStream.cpp

CookiePLMonster · 2019-10-04T18:38:50Z

Updated HandleWinAPI, since I just had a realization that I can pass the string size to ERROR_LOG (since it uses sprintf internally) and not have to promote string_view to string just to gain a guaranteed null terminator.

CookiePLMonster · 2019-10-08T17:43:10Z

Rebased on latest master due to conflicts.

BhaaLseN

Code seems fine, untested.

CookiePLMonster · 2019-10-22T20:49:05Z

I've rebased against the latest master since I updated WIL to the newest version - makes sense to keep it up to date if it's to become a submodule.

However, since looks like this is not getting merged anytime soon and I really want to use WIL in other PR's, maybe I should split the commit adding WIL as submodule as a separate PR so I don't stall waiting for this to be merged?

@BhaaLseN @Warepire @lioncash thoughts?

BhaaLseN · 2019-10-23T17:32:09Z

Not sure how others are coming along with testing and their reviews; but if that still takes a while it probably isn't a bad idea to pull it out into its own PR.

CookiePLMonster · 2019-10-23T17:51:18Z

Postponed until #8427 gets merged.

leoetlino · 2020-03-15T22:30:30Z

#8427 has now been merged :)

CookiePLMonster · 2020-03-17T21:31:01Z

Rebased on latest master. Thankfully, all I had to do was remove the commit added WIL since it's now merged in separately!

shuffle2 · 2021-01-12T04:54:20Z

btw, thanks for adding WIL!
I just used it for https://github.com/dolphin-emu/dolphin/pull/9438/files , and I think it provides stuff (the COM wrappers, string wrappers, etc.) to make this PR even nicer, if you're willing to give it a shot...

CookiePLMonster · 2021-01-12T12:29:19Z

to make this PR even nicer, if you're willing to give it a shot...

Do you see any other places where WIL could be used in this PR? For COM pointers I personally prefer WRL because it is included in Windows headers, and so it is used here.

EDIT:
One thing I see is CoCreateInstance, I haven't realized WIL has a helper for that! I can take care of it when rebasing to resolve conflicts.

Filoppi · 2021-01-12T16:25:26Z

Shame I had not seen this before starting my WASAPI works (nor anyone told me), though it's not very encouraging to see this still pending given over a year has passed and I have a lot more changes.
I think I've made a lot of the same improvements as you even if I haven't fully checked your code, but my rewrite is much more extensive (it allows any setting to be changed at runtime for example).
#8983
While the WASAPI code there is finished, I still can't mark the PR as finished as I need to clean up other parts.

shuffle2 · 2021-01-12T17:30:04Z

@Filoppi , let's leave this PR specifically for WASAPI resource management improvements.

…rly, not just S_OK

This fixes numerous resource leaks, as not every return path cleaned every created resource Now they are all managed automatically and "commited" to WASAPIStream class fields only after it's certain they initialized properly

…ching WASAPI handler thread

… a full barrier

…e sense of safety

This skips a potentially costly loop if volume is 100% or 0%, as for former there is no need for volume adjustment, while latter can be solved by specifying a AUDCLNT_BUFFERFLAGS_SILENT flag

…n to greatly reduce code duplication

…ad of a lambda

Filoppi · 2021-01-12T18:46:48Z

Never said to not keep this even if merging is gonna be annoying. I think the only improvement that mine doesn't have is converting to the new ComPtr. I should have fixed all memory leaks and resource management stuff, plus the deadlocks (which weren't exclusive to WASAPI).
Also changed the devices to be saved by ID and not name string because that would break if you had two devices with the same name, and multiplied the WASAPI volume by windows mixer volume so it's directly controllable from there as well, as it should be.
Anyway, this is great stuff, why hasn't it been merged :(.

CookiePLMonster · 2021-01-12T19:08:38Z

Merge conflicts resolved. I decided against using wil::CoCreateInstance because:

it uses wil::com_ptr and I'm using WRL::ComPtr
It wouldn't improve code readability too much and we do make use of CoCreateInstance's result value, which wil::CoCreateInstanceNoThrow drops (by design)

Adamillo · 2021-03-15T21:03:57Z

Does this PR fix the dreaded audio issues on Wasapi?

Warepire reviewed Aug 17, 2019

View reviewed changes

Source/Core/AudioCommon/WASAPIStream.cpp Outdated Show resolved Hide resolved

Source/Core/AudioCommon/WASAPIStream.cpp Outdated Show resolved Hide resolved

BhaaLseN reviewed Aug 17, 2019

View reviewed changes

CookiePLMonster force-pushed the wasapi-code-improvements branch 2 times, most recently from b061e72 to 92eed7e Compare August 18, 2019 11:00

CookiePLMonster force-pushed the wasapi-code-improvements branch from 92eed7e to 4cfdde4 Compare September 7, 2019 13:35

CookiePLMonster force-pushed the wasapi-code-improvements branch from 4cfdde4 to 7e9bd6c Compare September 24, 2019 15:18

abhiaagarwal reviewed Sep 24, 2019

View reviewed changes

Source/Core/AudioCommon/WASAPIStream.cpp Show resolved Hide resolved

CookiePLMonster force-pushed the wasapi-code-improvements branch 2 times, most recently from d525349 to edf85fa Compare October 4, 2019 18:37

CookiePLMonster force-pushed the wasapi-code-improvements branch from edf85fa to 1c89ff7 Compare October 8, 2019 17:42

BhaaLseN approved these changes Oct 8, 2019

View reviewed changes

CookiePLMonster force-pushed the wasapi-code-improvements branch from 1c89ff7 to 228e5d8 Compare October 22, 2019 20:47

CookiePLMonster force-pushed the wasapi-code-improvements branch from 228e5d8 to 0fdef8c Compare October 23, 2019 17:30

CookiePLMonster mentioned this pull request Oct 23, 2019

Add Windows Implementation Libraries as an external library #8427

Merged

CookiePLMonster changed the title ~~WASAPI code improvements~~ [POSTPONED] WASAPI code improvements Oct 23, 2019

CookiePLMonster force-pushed the wasapi-code-improvements branch from 0fdef8c to 654616f Compare March 17, 2020 21:30

CookiePLMonster changed the title ~~[POSTPONED] WASAPI code improvements~~ WASAPI code improvements Mar 17, 2020

shuffle2 approved these changes Aug 21, 2020

View reviewed changes

CookiePLMonster added 9 commits January 12, 2021 19:18

AudioCommon: Make HandleWinAPI handle all success return values prope…

374629e

…rly, not just S_OK

AudioCommon/WASAPI: Use WRL/WIL whenever possible

c5a372a

This fixes numerous resource leaks, as not every return path cleaned every created resource Now they are all managed automatically and "commited" to WASAPIStream class fields only after it's certain they initialized properly

AudioCommon/WASAPI: Simplify thread synchronization model by not deta…

ee60be4

…ching WASAPI handler thread

AudioCommon/WASAPI: Use leaner memory model on m_running, no need for…

7d59ad2

… a full barrier

AudioCommon/WASAPI: Use std::string_view where applicable

5dbbf36

AudioCommon/WASAPI: Remove thread unsafe nullptr checks giving a fals…

991b3ba

…e sense of safety

AudioCommon/WASAPI: Do volume adjustment only when really needed

c373890

This skips a potentially costly loop if volume is 100% or 0%, as for former there is no need for volume adjustment, while latter can be solved by specifying a AUDCLNT_BUFFERFLAGS_SILENT flag

AudioCommon/WASAPI: Factorize device enumeration logic into a functio…

11c5150

…n to greatly reduce code duplication

AudioCommon/WASAPI: Construct std::thread with invoke semantics inste…

cb854d7

…ad of a lambda

CookiePLMonster force-pushed the wasapi-code-improvements branch from 654616f to cb854d7 Compare January 12, 2021 19:06

JosJuice merged commit 18d95df into dolphin-emu:master Mar 11, 2021

CookiePLMonster deleted the wasapi-code-improvements branch March 11, 2021 20:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WASAPI code improvements #8316

WASAPI code improvements #8316

CookiePLMonster commented Aug 17, 2019

CookiePLMonster commented Aug 17, 2019

lioncash commented Aug 17, 2019

CookiePLMonster commented Aug 17, 2019

CookiePLMonster commented Aug 18, 2019

CookiePLMonster commented Sep 6, 2019

CookiePLMonster commented Sep 7, 2019

CookiePLMonster commented Sep 24, 2019 •

edited

Loading

CookiePLMonster commented Oct 4, 2019

CookiePLMonster commented Oct 8, 2019

BhaaLseN left a comment

CookiePLMonster commented Oct 22, 2019

BhaaLseN commented Oct 23, 2019

CookiePLMonster commented Oct 23, 2019

leoetlino commented Mar 15, 2020

CookiePLMonster commented Mar 17, 2020

shuffle2 commented Jan 12, 2021

CookiePLMonster commented Jan 12, 2021 •

edited

Loading

Filoppi commented Jan 12, 2021

shuffle2 commented Jan 12, 2021

Filoppi commented Jan 12, 2021 •

edited

Loading

CookiePLMonster commented Jan 12, 2021 •

edited

Loading

Adamillo commented Mar 15, 2021

WASAPI code improvements #8316

WASAPI code improvements #8316

Conversation

CookiePLMonster commented Aug 17, 2019

CookiePLMonster commented Aug 17, 2019

lioncash commented Aug 17, 2019

CookiePLMonster commented Aug 17, 2019

CookiePLMonster commented Aug 18, 2019

CookiePLMonster commented Sep 6, 2019

CookiePLMonster commented Sep 7, 2019

CookiePLMonster commented Sep 24, 2019 • edited Loading

CookiePLMonster commented Oct 4, 2019

CookiePLMonster commented Oct 8, 2019

BhaaLseN left a comment

Choose a reason for hiding this comment

CookiePLMonster commented Oct 22, 2019

BhaaLseN commented Oct 23, 2019

CookiePLMonster commented Oct 23, 2019

leoetlino commented Mar 15, 2020

CookiePLMonster commented Mar 17, 2020

shuffle2 commented Jan 12, 2021

CookiePLMonster commented Jan 12, 2021 • edited Loading

Filoppi commented Jan 12, 2021

shuffle2 commented Jan 12, 2021

Filoppi commented Jan 12, 2021 • edited Loading

CookiePLMonster commented Jan 12, 2021 • edited Loading

Adamillo commented Mar 15, 2021

CookiePLMonster commented Sep 24, 2019 •

edited

Loading

CookiePLMonster commented Jan 12, 2021 •

edited

Loading

Filoppi commented Jan 12, 2021 •

edited

Loading

CookiePLMonster commented Jan 12, 2021 •

edited

Loading