[RFC] Implement CUBEB_STREAM_PREF_EXCLUSIVE

[spycrab]

A bad attempt at supporting Exclusive mode.

Issues:

• I end up with segfaults when the IAudioClient should be started for some reason.
• Format availability detection is probably still off
• Formatting issues / Debugging leftovers in a few places

Some of the changes are probably not necessary (e.g. the CLSCTX_ALL stuff).

If someone had some ideas regarding what I'm doing wrong, help would be greatly appreciated!

[spycrab]

This line also causes an access violation for some reason (when uncommented): https://github.com/kinetiknz/cubeb/pull/415/files#diff-2f55f506bd2dd918b7b8c6ceec8aee0dR1671

[kinetiknz]

Thanks for the PR. I think we'd add support for this as long as the changes are non-invasive and a new test is added to ensure the functionality isn't accidentally broken -- we won't use this inside Firefox, so without tests it'll end up broken fairly quickly.

I do wonder whether the user cases are better addressed via IAudioClient3 as discussed in issue #324 (although that's Windows 10 only). To clarify, is the reason you want this feature for lower latency?

For the PR:

• The crash in LOG at 1671 is possibly due to differences in setting up the remix/resample formats in handle_channel_layout with exclusive mode; that's the piece of code I'd expect more changes around to support exclusive mode. If you get stuck, report back and we can dig in when we have time. (this is your point 2, possibly also point 1)
• Following on from the above: does it make sense to allow remix/resampling for exclusive streams? Maybe cubeb should require the requested format matches the OS/hardware exactly, otherwise stream init fails.
• I'd remove the CLSCTX_ALL changes unless there's a reason for them.
• CUBEB_STREAM_PREF_WASAPI_EXCLUSIVE could be OS-agnostic and return errors from the other backends (until they support something similar, if that functionality makes sense on the platform); that'd allow removing WASAPI from the name and the ifdefs in cubeb.h.
• There's some code that assumes shared mode (e.g. https://github.com/kinetiknz/cubeb/blob/475d97f64567b292761bd9dddaecbd54749b3aa3/src/cubeb_wasapi.cpp#L623) that needs to be updated to work, or at least tested and known to fail in a sensible way.

[spycrab]

@kinetiknz Thanks for your quick response!

Changes I made:

• Removed the CLSCTX_ALL bits for now
• Made the LOG line default to CUBEB_LAYOUT_UNKNOWN which fixes the segfault
• Turned the flag into CUBEB_STREAM_EXCLUSIVE and added a backend check to cubeb.c

I took a look at quick look at handle_channel_layout and it didn't look as if it had to be changed for exclusive mode.

Not sure if get_input_buffer plays a role at this stage though.

That's all I know at the moment :/

[spycrab]

Regarding the tests: I'd suggest just adding them to test_audio via an ifdef, that's how I've tested it thus far.

EDIT: Sorry for the massive diff in cubeb_wasapi.cpp, will fix that tomorrow! Fixed

[kinetiknz]

Not sure if get_input_buffer plays a role at this stage though.

It's only used on the capture side, not playback.

Regarding the tests: I'd suggest just adding them to test_audio via an ifdef, that's how I've tested it thus far.

Sounds good.

[spycrab]

Fixed get_input_buffer now. But I'm not sure where to go from here to be honest.
I've already tried overriding mix_format with a known, working one but to no avail.

[kinetiknz]

The indenting is a bit weird in the latest version.

I noticed the LOG change, what are you getting in mix_params->layout?

Also still curious re paragraph 2 of: https://github.com/kinetiknz/cubeb/pull/415#issuecomment-364810077

[spycrab]

mix_params->layout is CUBEB_LAYOUT_STEREO.
Regarding paragraph 2, no that probably doesn't make sense but I've not encountered remapping / resampling yet (according to the logs at least).

[kinetiknz]

Sorry, I meant the question about why you want this functionality vs IAudioClient3's lower latency shared mode.

I had a quick look and have a few pointers for where to look next to get this working. There's some sample code with explananations at https://blogs.msdn.microsoft.com/matthew_van_eerde/2009/04/03/sample-wasapi-exclusive-mode-event-driven-playback-app-including-the-hd-audio-alignment-dance/ that might be useful.

Testing with test_sanity (simplest test; plays a tone - updated to pass the exclusive pref flag), IAudioClient::Initialize fails with AUDCLNT_E_BUFFER_SIZE_ERROR. Since we're passing 0 for the hnsPeriodicity (MSDN says this must be non-zero for exclusive + event streams), that's probably the first thing to fix.

From the linked blog post above, the other thing we'll likely need to handle is the "alignment dance" to calculate buffer sizing compatible with the harder.

[spycrab]

Sorry, for some reason I interpreted paragraph two meaning point two (entirely my fault).
Anyway regarding your question:
Yeah, the main reason I want exclusive mode support is for lower latency. While IAudioClient3 looks interesting the Windows 10 only part of it kind of kills my interest.
(It'd also be interesting to compare latency between IAudioClient exclusive mode and IAudioClient3 shared mode).

hnsPeriodicity was set correctly before, I've must have messed this up while trying to fix the formatting (Not used to VS).

We should probably handle this, yes. Although I'll have to find a test case that requires that.

Thanks for your patience.

[spycrab]

@kinetiknz Fixed hnsPeriodicity.
Sadly test_sanity just segfaults where test_audio does :/

[padenot]

We're having a look at IAudioClient3 as well (independently, as you can imagine, we can't really use exclusive mode for Firefox). I certainly helps a lot when it's available.

[spycrab]

When it's available, it's probably superior. If only for the fact that it runs in shared mode

[kinetiknz]

Sadly test_sanity just segfaults where test_audio does :/

Would you mind running it under a debugger and providing more details?

With your latest changes and test_sanity passing the exclusive flag, IAudioClient::Initialize now returns AUDCLNT_E_BUFFER_SIZE_NOT_ALIGNED, which will need to be addressed by the "alignment dance" stuff I mentioned earlier.

[spycrab]

Turns out they crash at different sections now.
Anyhow here's what I could find:

test_audio

Caused by:

test_audio.exe!`anonymous namespace'::stream_start_one_side(cubeb_stream * stm, `anonymous-namespace'::StreamDirection dir) Line 2081 (In cubeb_wasapi.cpp)

Error Message:

Exception thrown at 0x6A33E20E (AudioSes.dll) in test_audio.exe: 0xC0000005: Access violation reading location 0x04DF0034. occurred

Library Function

AudioSes.dll!CAudioClient::Start()

test_sanity

Caused by:

Test_sanity.exe!`anonymous namespace'::com_ptr<IAudioRenderClient>::release() Line 131`` (In cubeb_wasapi.cpp)

Error Message:

Exception thrown at 0x6A3466C2 (AudioSes.dll) in test_sanity.exe: 0xC0000005: Access violation reading location 0x03810110. occurred

Library Function:

AudioSes.dll!CAudioRenderClient::Cleanup()

Hope this helps!

[kinetiknz]

Right, so you're getting past the AUDCLNT_E_BUFFER_SIZE_NOT_ALIGNED issue somehow. Did you alter the stream params in the test or are we testing different versions of the code?

Anyway, if I hardcode suitable buffer sizing to avoid AUDCLNT_E_BUFFER_SIZE_NOT_ALIGNED, I hit the same crash in stream_start_one_side. That was tricky to track down... it turns out IAudioClient's initialized with AUDCLNT_SHAREMODE_EXCLUSIVE care if COM is still initialized (they hold a reference to some global/thread-specific COM data and use it during IAudioClient::Start). The cubeb tests never initialize COM, so we were relying on the auto_com initialization inside cubeb_wasapi.cpp. Since COM wasn't initialized at a higher level (meaning auto_com would just shuffle a ref count), auto_com completely destroys the COM global data each time... since the IAudioClient is holding a reference to this (and uses it in AUDCLNT_SHAREMODE_EXCLUSIVE mode), it becomes invalid and crashes.

I note that wasapi_stream_start/stop are missing auto_com, but adding them doesn't help because the IAudioClient still holds the reference to the old, destroyed COM global data.

So I think the correct fix for this is to initialize COM in the cubeb tests, and to document that COM must be initialized before calling cubeb APIs. That may also mean the auto_com stuff becomes completely obsolete.

[kinetiknz]

Next issue to address: after adding a variant of auto_com to test/common.h and using it inside test_tone, the stream starts and stops as expected, but the data_callback is never called, so no tone is produced and the test's assert fails.

Looks like this is caused by cubeb's calls to IAudioRenderClient::GetBuffer not yet handling exclusive-mode streams.

[spycrab]

Right, so you're getting past the AUDCLNT_E_BUFFER_SIZE_NOT_ALIGNED issue somehow. Did you alter the stream params in the test or are we testing different versions of the code?

No, I didn't. Anyway I implemented the alignment dance now, not sure if it works properly though.

Anyway, if I hardcode suitable buffer sizing to avoid AUDCLNT_E_BUFFER_SIZE_NOT_ALIGNED, I hit the same crash in stream_start_one_side. That was tricky to track down... it turns out IAudioClient's initialized with AUDCLNT_SHAREMODE_EXCLUSIVE care if COM is still initialized (they hold a reference to some global/thread-specific COM data and use it during IAudioClient::Start). The cubeb tests never initialize COM, so we were relying on the auto_com initialization inside cubeb_wasapi.cpp. Since COM wasn't initialized at a higher level (meaning auto_com would just shuffle a ref count), auto_com completely destroys the COM global data each time... since the IAudioClient is holding a reference to this (and uses it in AUDCLNT_SHAREMODE_EXCLUSIVE mode), it becomes invalid and crashes.

Thanks for figuring this one out! I added a CoInitialize call to test/common.h temporarily.

Next issue to address: after adding a variant of auto_com to test/common.h and using it inside test_tone, the stream starts and stops as expected, but the data_callback is never called, so no tone is produced and the test's assert fails.

Sorry but I can't reproduce this for some reason :/
The test passes fine for me.

Also added my version of test/test_audio.cpp which passes all tests 🎉

[kinetiknz]

Sorry but I can't reproduce this for some reason :/

Possibly for the same reason you didn't hit the buffer alignment issues... your hardware/configuration might happen to be compatible with the configuration requested (or more flexible in accepting configurations). So we'll need to test this with various configurations and shake out the remaining issues.

[spycrab]

@kinetiknz Yeah, my audio setup is kinda weird (USB microphone with a builtin "sound card" and a headset out) :/

[spycrab]

So any pointers regarding what I can still do to move this along?

[kinetiknz]

Either (or both) extensive testing to shake out what else needs adjusting or reviewing all of the IAudioClient/IAudioRenderClient/IAudioCaptureClient APIs to see what restrictions they have for exclusive mode. As mentioned in an earlier comment, GetBuffer fails with some configurations as the code is now.

[spycrab]

Rebased. Also did some further testing with the devices available to me.
None of them seemed to succumb to the error mentioned here.

So I'm kinda unsure what to do next.

[kinetiknz]

I need to find some time to debug the next issue, I guess.

In the mean time:

• The indentation is different to the file in a lot of your changes (might be tabs vs spaces, hard to tell with GitHub diffs)
• CoInitialize isn't needed in common.h now
• The logging on 1551 shouldn't need to be different for exclusive mode, so that can be merged
• The channel layout comments were removed in the mixer rework, don't add them back (rebase issue?)

[Adamillo]

What's the state of this PR?

[padenot]

It's never been finished and it doesn't apply anymore, there's been a lot of changes since then to the WASAPI backend.

I wonder how useful are exclusive streams these days.

[Adamillo]

Well, exclusive WASAPI in Cubeb reduces latency by a lot, so that's why it's very useful.