dolphin icon indicating copy to clipboard operation
dolphin copied to clipboard

Update cubeb to mozilla/cubeb@773f16b7ea308392c05be3e290163d1f636e6024

Open AdmiralCurtiss opened this issue 3 years ago • 8 comments

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 avatar Jan 04 '22 07:01 AdmiralCurtiss

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

shuffle2 avatar Jan 08 '22 09:01 shuffle2

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

AdmiralCurtiss avatar Jun 17 '22 23:06 AdmiralCurtiss

nice.

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

shuffle2 avatar Jun 18 '22 00:06 shuffle2

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...

AdmiralCurtiss avatar Jun 18 '22 00:06 AdmiralCurtiss

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

shuffle2 avatar Jun 18 '22 01:06 shuffle2

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

AdmiralCurtiss avatar Jun 18 '22 01:06 AdmiralCurtiss

7c8dd95bf6ab0abc43c6baddb29e9d341cff7f5a 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.

AdmiralCurtiss avatar Jun 18 '22 03:06 AdmiralCurtiss

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.

shuffle2 avatar Aug 27 '22 06:08 shuffle2

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.

AdmiralCurtiss avatar Nov 26 '22 04:11 AdmiralCurtiss

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

shuffle2 avatar Nov 26 '22 16:11 shuffle2

Well, don't mind me then...

AdmiralCurtiss avatar Nov 27 '22 02:11 AdmiralCurtiss

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

Zopolis4 avatar Nov 27 '22 02:11 Zopolis4

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 avatar Nov 27 '22 10:11 Filoppi

@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 avatar Nov 29 '22 22:11 Filoppi

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

I've done it here: https://github.com/dolphin-emu/dolphin/pull/11466

Filoppi avatar Jan 20 '23 19:01 Filoppi