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

Update cubeb to mozilla/cubeb@27d2a102b0b75d9e49d43bc1ea516233fb87d778 #10343

Merged
merged 4 commits into from Nov 27, 2022

Conversation

AdmiralCurtiss
Copy link
Contributor

Our current revision is from October 2017, so it's reasonable to update it. This fixes a long-standing issue where Windows would not remember Dolphin's volume as set in the Volume Mixer between sessions.

What complicates this is that cubeb at some point changed so it requires the user to initialize COM on Windows rather than doing it by itself, and also (at least via documentation, although it seems in practice this isn't a hard rule -- still, one should abide by the documentation...) requires that all calls to the cubeb API be only done on threads where COM has been initialized in MTA (CoInitializeEx(nullptr, COINIT_MULTITHREADED)) mode.

I've done this by adding a RunInCubebContext() function that tries to initialize in MTA mode, and if that fails creates a temporary thread to do it there instead. This is inspired by a long comment thread over in #8920, the last attempt at this, though with my own spin on it. That attempt never got merged, but maybe this one will be good enough?

Some extra notes:

  • The cubeb sources are 1:1 copied from the linked commit, except for the CMakeLists.txt, which was slightly modified to work with our CMake build scripts.
  • Two extra libraries are required to be linked on Windows compared to the October 2017 version, which have been added to the relevant .csproj files.

@AdmiralCurtiss AdmiralCurtiss force-pushed the cubeb-2021 branch 2 times, most recently from 051cef6 to 54706a0 Compare January 4, 2022 08:08
@shuffle2
Copy link
Contributor

shuffle2 commented Jan 8, 2022

Worth noting that it seems Mozilla deprecated (some parts of) the library in favor of the rust version: https://github.com/mozilla/cubeb/wiki/Backend-Support
Not that it means we shouldn't use it, but just good to be aware.

I would recommend using wil for the COM management , since it nicely solves the problem(s) and we already include it (maybe time to update that External, as well :))

Otherwise, lgtm

@AdmiralCurtiss
Copy link
Contributor Author

Hey this actually builds now with the build server upgrade, great!

@shuffle2
Copy link
Contributor

nice.

in addition to using WIL, what do you think about using submodule instead of copying the sources?

@AdmiralCurtiss
Copy link
Contributor Author

AdmiralCurtiss commented Jun 18, 2022

Yeah I think we can use a submodule.

Regarding WIL, I'm not quite sure how to design this as-is with WIL? I guess it would in general be better to always use a separate thread here since we don't want to randomly enable MTA mode permanently on whatever thread happens to call a Cubeb function... let's see...

@shuffle2
Copy link
Contributor

hrm, starting a thread to handle a single function call seems a bit smelly :p but yea, maybe it doesn't fit.

@AdmiralCurtiss
Copy link
Contributor Author

We could have a cubeb worker thread that gets function calls to execute via a queue, if that's better.

@AdmiralCurtiss AdmiralCurtiss marked this pull request as draft June 18, 2022 03:30
@AdmiralCurtiss AdmiralCurtiss force-pushed the cubeb-2021 branch 3 times, most recently from 574f91f to 7c8dd95 Compare June 18, 2022 03:35
@AdmiralCurtiss
Copy link
Contributor Author

7c8dd95 is a variant using a worker thread. It's a bit ridiculous, but maybe actually the better option? Avoids the CoInitialize on the main thread and never opens any temporary secondary threads.

@shuffle2
Copy link
Contributor

i was looking at c++/winrt stuff recently and noticed it has functionality to forward coroutines/callbacks onto a threadpool (for cross-apartment invocations). I wouldn't be surprised if WIL has a similar thing (or maybe it's better to just use the code in winrt header, anyway - it's <winrt/base.h>). Generally it just provides c++ syntactic sugar on com.

AdmiralCurtiss and others added 4 commits November 26, 2022 05:05
…ule.

CMakeLists.txt has been extracted and modified a bit to work with Dolphin's typical build settings.
…ialize state of whatever thread happens to call a Cubeb function.
@AdmiralCurtiss AdmiralCurtiss changed the title Update cubeb to mozilla/cubeb@773f16b7ea308392c05be3e290163d1f636e6024 Update cubeb to mozilla/cubeb@27d2a102b0b75d9e49d43bc1ea516233fb87d778 Nov 26, 2022
@AdmiralCurtiss
Copy link
Contributor Author

I'll be honest, I'm not sure how either WIL or WinRT help me here. If you have an idea how to do this better please go ahead, because this is the best I can come up with.

@shuffle2
Copy link
Contributor

I’m not going to get to that and the pr has been around forever so if you think it’s good go for it

@AdmiralCurtiss
Copy link
Contributor Author

Well, don't mind me then...

@AdmiralCurtiss AdmiralCurtiss merged commit 3cdc6e3 into dolphin-emu:master Nov 27, 2022
11 checks passed
@AdmiralCurtiss AdmiralCurtiss deleted the cubeb-2021 branch November 27, 2022 02:26
@Zopolis4
Copy link
Contributor

Depending on how the deprecation goes we could presumably use the C++ bindings to cubeb-rs, btw.

@Filoppi
Copy link
Contributor

Filoppi commented Nov 27, 2022

Happy to see this got merged. Thanks for quoting your references. I have resumed work on my other audio PRs, so this should make things a little simpler.

@Filoppi
Copy link
Contributor

Filoppi commented Nov 29, 2022

@AdmiralCurtiss
It seems like cubeb still hasn't re-enabled IAudioClient3 by default, which would allow for lower latency on Win 10/11:
PR commit
cubeb bug thread
I'd say we should give it a try given that it should sensibly reduce audio lag, and almost make it on par with exclusive mode WASAPI. If any issues arise, we could disable it again, but it's been a while since that bug was opened, so maybe Windows updates fixed it, and it seems like it had always been a problem exclusively with input stream, which are still prevented from using IAudioClient3 with my change.

@Filoppi
Copy link
Contributor

Filoppi commented Jan 20, 2023

@AdmiralCurtiss It seems like cubeb still hasn't re-enabled IAudioClient3 by default

I've done it here: #11466

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