Re-enable and polish IAudioClient3 to achieve lower latencies

[Filoppi]

I made this PR for Dolphin (link) and it's currently in testing, though I thought to propise to Mozilla as well, to potentially get some feedback.

This PR re-activates IAudioClient3 on WASAPI. This allows PCs with Windows 10+ to achieve lower latencies: Microsoft doc theoretically the latency should be pretty similar to the WASAPI exclusive mode (depending on the device driver), but without its limitations. If not supported, it will fall back to the classic/old IAudioClient audio mode.

Most of the code was already there, though Mozilla disabled it because it gave problems with input streams (bug report). For that reason, we are only using it with output streams, and additionally, I have polished the code a lot and added handling of a few edge/error cases that were not acknowledged before. Chromium has safely used IAudioClient3 for years, so I have analyzed its implementation, and multiple other open source applications, to find out what might be improved here.

One major problem was that the latency set by the user was in samples (48Khz on Dolphin), not in ms, but cubeb failed to convert it for device actual output sample rate, so the latency could have been set to extremely low or high values, which could have made it starve for samples. Also, cubeb failed to check if the mixer format it generated was fully supported by IAudioClient3, and thus sometimes it wouldn't initialize correctly. I'm not 100% sure about this, but I've been testing this with @eliasreid, who can confirm as his audio device only supports 24bit formats, and cubeb forced 16bit on it and Dolphin produced crackled sounds (it didn't seem to be a latency issue). See CHECK_MIXER_FORMAT_SUPPORT for more. (This was only partially true)

The code is kind of done, but there are quite a few things that could benefit from testing. I've added some defines at the top of the files, which are all meant for testing, and I think the current configuration is the best, but if something is wrong with your output (or IAudioClient3 fails to activate).

[Filoppi]

I'm in the process of polishing the code and I've already removed many of the test settings as they revealed useless. This does fix some bugs though, as just re-enabling IAudioClient3 without the additional changes can output cracking sounds on some devices. My next step kind of depends on you if you are interested. Specifically, I was wondering what's your stance on the user/client preferred latency. Is it "mandatory" to respect it, or it is just taken as "target" latency. IAudioClient(1) doesn't always seem to care about that value, so I suppose we should also kind of ignore it with IAudioClient3? Because it's unlikely that the latency asked by the user will be one of the ones accepted by the IAudioClient3 audio engine.

[kinetiknz]

Thanks for working on this! We'd like to ship IAC3 support in Firefox at some point, but haven't had a chance to revisit the issues discovered during the previous rollout attempt. I'll take a look at the changes so far over the next day or so and get back to you with any feedback.

[padenot]

My next step kind of depends on you if you are interested. Specifically, I was wondering what's your stance on the user/client preferred latency. Is it "mandatory" to respect it, or it is just taken as "target" latency. IAudioClient(1) doesn't always seem to care about that value, so I suppose we should also kind of ignore it with IAudioClient3? Because it's unlikely that the latency asked by the user will be one of the ones accepted by the IAudioClient3 audio engine.

It's more or less always been a "desirable target", not all backends can ensure this parameter is respected, so I don't think there's any problem here.

[Filoppi]

@kinetiknz @padenot I have polished the code and removed all the test branches that turned out unneccessary from testing. There are still some defines on the top of the code. The one that is of most interest to me is ALLOW_AUDIO_CLIENT_3_LATENCY_OVER_DEFAULT, because a comment in cubeb stated that creating a IAudioSession3 shared stream (not locked to a format) would also lock all IAudioSession(1) streams to its same latency. I haven't been able to verify that information, nor find it anywhere else. I don't think it is true, so I think we could safely keep the code as if ALLOW_AUDIO_CLIENT_3_LATENCY_OVER_DEFAULT was 1.

These are some good references for IAudioClient3 code: https://chromium.googlesource.com/chromium/src/media/+/master/audio/win/core_audio_util_win.cc https://chromium.googlesource.com/external/webrtc/+/HEAD/modules/audio_device/win/core_audio_utility_win.cc https://chromium.googlesource.com/external/webrtc/+/HEAD/modules/audio_device/win/core_audio_base_win.cc https://github.com/sidewayss/cubeb/blob/master/src/cubeb_wasapi.cpp (thanks @sidewayss) https://github.com/microsoft/Windows-universal-samples/blob/main/Samples/WindowsAudioSession/cpp/WASAPIRenderer.cpp https://github.com/microsoft/Windows-classic-samples/blob/main/Samples/Win7Samples/multimedia/audio/CaptureSharedEventDriven/WASAPICapture.cpp https://learn.microsoft.com/en-us/windows-hardware/drivers/audio/audio-processing-object-architecture https://learn.microsoft.com/en-us/windows-hardware/drivers/audio/audio-signal-processing-modes

[Filoppi]

@kinetiknz don't really want to bother, though it would be cool to know what's your plan with this. If the chances of it getting merged any time soon are small, then I'll consider making a custom branch for Dolphin (as there isn't one now). Thanks!

[padenot]

I want to re-enable this, but haven't had time to sit down and look at your code (thanks for working on this, btw!). I've requested the review for myself, so I'll be reminded to look at it, and I'll find some time to do so, hopefully soon.

[Filoppi]

I want to re-enable this, but haven't had time to sit down and look at your code (thanks for working on this, btw!). I've requested the review for myself, so I'll be reminded to look at it, and I'll find some time to do so, hopefully soon.

I'm sorry. I'm going to ask for an ETA again. If it's still unknown, I'll try to branch this on the dolphin repo. Thanks.

[padenot]

Sorry for the trouble. We can probably land something as long as we can keep the current behaviour, so that we can both do QA and experiment in our own timelines.

[padenot]

I've done a push on a separate branch to run CI, since it runs on Windows since last week: https://github.com/mozilla/cubeb/actions/runs/4863054212.

[Filoppi]

Nice. I had tested this on a number of devices with very different properties and it seemed to work fine. I'm not sure what more I could do from my side :).

[padenot]

I don't think you can do anything. I want to give it a quick run on the Firefox CI, because it's very thorough, but it's having an outage at the moment, and refuses new push, I just about could push https://github.com/mozilla/cubeb/pull/542 before it closed.

Does your workload contain a lot of duplex (input+output) streams? It seems like it's easier to have it working well when you're only running one side, but I don't remember exactly what we saw.

[Filoppi]

I did not test IAudioClient3 on input, but it's not enabled by default ALLOW_AUDIO_CLIENT_3_FOR_INPUT. You can either test it or leave it off for now. It should be coded correctly, but to avoid delays in merging this I left it off. Ultimately it's not as important as output.

[sidewayss]

fyi - For many users, like me, the entire reason for low latency is for input, so that you can play along with the output by plugging in your electric guitar or keyboard or whatever. With the currently latency that is simply implausible.

[padenot]

We can merge support for output only, and continue iterating on the input side if need be, we've taken too much time to merge this.

Lots of people need only output, lots of people need both -- Firefox needs both, for example, but for if cubeb is used for the audio of a video game, it's plausible that low latency output matters a lot more than the input (or maybe the game doesn't use input at all).

The outage on our CI is resolved, here's the push: https://treeherder.mozilla.org/jobs?repo=try&revision=c46c26bb245374e3acdc24652c271562718bfede.

[Filoppi]

We can merge support for output only, and continue iterating on the input side if need be, we've taken too much time to merge this.

All good? Do I need to do anything for this to proceed?

[padenot]

All good? Do I need to do anything for this to proceed?

All good, in the sense that it's green both here, and on the Firefox infra, but that's not real machine with their diverse hardware, drivers and other moving bits. I'm having a final look at the code now. I'm thinking maybe we want to make it a bit more configurable than the current code, I'll take care of that by adding patches on top of your branch. Using flags maybe, Windows only.

All that to say that I'm looking into it, I don't think there's much to do but wait, sorry it takes so long.

[Filoppi]

@padenot is there anything specific you'd still like me to address?

[Filoppi]

@padenot sorry to bother again, any updates on this?

[padenot]

Sorry for the delay. We're almost done with fixing the issues found after merging https://github.com/mozilla/cubeb/pull/542 (on WASAPI, there's still a crash that eludes us), and we're busy fixing two issues on this library: macOS low-latency audio glitches when running duplex streams on specific configurations, and quite a number of crashes on Android. Crashes and existing quality issues take precedence over optimizing existing well functioning code.

That said, I've been playing with this a bit more, and it looks like this is behaving better on recent Windows, so it's likely that when we've fixed those two issues on macOS and Android, we'll get this merged. Having so many users with so diverse hardware running Firefox makes even rare bugs important, and I think it's better for everybody to have stable code on the master branch, even if this means some downstream users having to carry patches, or have performance that could be slightly better.

Has this code been running somewhere in production?

[Filoppi]

@padenot Nice. No, this code has only been tested by me and a couple of testers online.

[padenot]

Yeah, that's the real issue with all this. I appeared to work fine on the machine we have, so we shipped it, and the latencies were indeed lower, but it was a train wreck in the field, and we had to disable it in emergency.

It was a long time ago though.

[Filoppi]

@padenot

Yeah. I understand, but it was like 4-5 years ago now? There's been years of development by MS, and they should have fixed any bugs with it. Also I think my implementation tackles some issues with the original one and is a bit more safe.

[padenot]

Yeah, I've been chatting with other folks that are doing real-time audio on Windows and deploy to lots of random systems, and it seems to be that it's in better shape, so I'm hopeful.