Don't destroy voice when it is output to other voices

[gofman]

Cosmoteer (Steam game id 799600) sometimes destroys a submix voice which is an output for another voice. That leads to use after free access in audio client thread performing mixing which leads to obscure random crashes.

MS docs [1] suggest that IXAudio2Voice::DestroyVoice fails in such a case ("If any other voice is currently sending audio to this voice, the method fails.").

I made a test on top of Wine xaudio2 tests which indirectly confirms that (attaching a patch). I don't immediately see a direct way to test if DestroyVoice actually failed or not as it doesn't return any status. However, I found that after successful voice destruction IXAudio2SubmixVoice_GetVoiceDetails() for that voice crashes on Windows and using this fact to probe for actual voice destruction (the first one in the test doesn't crash as, the second one guarded with if (0) crashes for me on Win10).

1. https://learn.microsoft.com/en-us/windows/win32/api/xaudio2/nf-xaudio2-ixaudio2voice-destroyvoice

0001-xaudio2-tests-Test-destroying-a-voice-which-is-an-ou.txt

[flibitijibibo]

At a high level this makes sense to me, we may have to tweak things slightly though - I think this would actually fix #326 as well, so bringing in @thatcosmonaut for review.

[flibitijibibo]

I think this counts as an ABI break, so we may have to export a new function that includes the return value :(

[flibitijibibo]

Should this be E_FAIL and the one below be 0?

[flibitijibibo]

Actually it turns out the signature is correct...

https://learn.microsoft.com/en-us/windows/win32/api/xaudio2/nf-xaudio2-ixaudio2voice-destroyvoice

... but honestly I'd be up for exporting a new function since being able to see this failure seems important. DestroyVoiceSafeEXT maybe?

[gofman]

Yes, sure. I did it this way because technically it doesn't break the ABI, if old client uses void version and doesn't check value it is not broken with newer version returning the value. But it is looks cleaner to have a different function. I think both old and new one will check if the voice is in use and the difference will be only in the presence return value. Or do you want the old version work like before?

[flibitijibibo]

What we can probably do is have this in the header...

FAUDIOAPI void FAudioVoice_DestroyVoice(...);

FAUDIOAPI uint32_t FAudioVoice_DestroyVoiceSafeEXT(...);

... and then we'll rename the implementation we have now to DestroyVoiceSafeEXT, and DestroyVoice can call DestroyVoiceSafeEXT and discard the return value entirely. It's a bit roundabout, but we've done similar things with other extensions like voice filters and it's been easy enough to keep stable!