Add support for variable output latency in WASAPI audio driver

[benjarmstrong]

Fixed the audio 'output latency' project setting not appearing when using the WASAPI audio driver (see #30562). Added variable output latency support to the WASAPI audio driver for systems that support it.

Bugsquad edit: This closes #30562.

[benjarmstrong]

I've also implemented this in my own 3.2 branch for a musc/rhythm based project I am working on that requires this fix. Let me know if you'd like me to create a pull request for that.

[akien-mga]

Could you squash commits into one? See PR workflow.

[benjarmstrong]

@akien-mga

Could you squash commits into one? See PR workflow.

Apologies, will do and I'll review the PR workflow

[benjarmstrong]

Removed auto, added support for building via MinGW, fixed conflicts

[EIREXE]

GetBufferSize appears to return an incorrect value when using InitializeSharedAudioStream to set a buffer size different than default, this is probably due to window compatibility stuff?

I have a (I believe) fix here: EIRTeam@68e098a

[benjarmstrong]

GetBufferSize appears to return an incorrect value when using InitializeSharedAudioStream to set a buffer size different than default, this is probably due to window compatibility stuff?

I have a (I believe) fix here: EIRTeam@68e098a

Thank you, I'll give it a look.

Which Windows version(s) did you test on and what compiler(s) did you use?

[benjarmstrong]

@EIREXE

GetBufferSize appears to return an incorrect value when using InitializeSharedAudioStream to set a buffer size different than default, this is probably due to window compatibility stuff?

I can reproduce this, but only sometimes; IAudioClient3 behaves differently for different audio devices.

I have a (I believe) fix here: EIRTeam@68e098a

This works like a charm, thank you! I'll squash that into my PR.

P.S. Sorry about closing/reopening, accidentally hit 'close with comment'

[EIREXE]

@EIREXE

GetBufferSize appears to return an incorrect value when using InitializeSharedAudioStream to set a buffer size different than default, this is probably due to window compatibility stuff?

I can reproduce this, but only sometimes; IAudioClient3 behaves differently for different audio devices.

I have a (I believe) fix here: EIRTeam@68e098a

This works like a charm, thank you! I'll squash that into my PR.

P.S. Sorry about closing/reopening, accidentally hit 'close with comment'

You're welcome!

I've also done some testing on linux with pulse and it appears like the reported latency by get_output_latency is way too high (on the order of 50 ms) but it sure doesn't FEEL like it, I wonder what's going on.

[benjarmstrong]

@EIREXE

I've also done some testing on linux with pulse and it appears like the reported latency by get_output_latency is way too high (on the order of 50 ms) but it sure doesn't FEEL like it, I wonder what's going on.

I think I may have found part your problem.

From this page:

If PA_STREAM_INTERPOLATE_TIMING is passed when connecting the stream, pa_stream_get_time() and pa_stream_get_latency() will try to interpolate the current playback time/latency by estimating the number of samples that have been played back by the hardware since the last regular timing update. It is espcially useful to combine this option with PA_STREAM_AUTO_TIMING_UPDATE, which will enable you to monitor the current playback time/latency very precisely and very frequently without requiring a network round trip every time.

PA_STREAM_INTERPOLATE_TIMING and PA_STREAM_AUTO_TIMING_UPDATE are passed in Godot's pulse driver so this applies. PA_STREAM_ADJUST_LATENCY is also passed allowing pulse to adjust the latency of the stream as it sees fit. This means means that pa_stream_get_latency may return different results each call, but in AudioDriverPulseAudio pa_stream_get_latency is called exactly once then that result is used for every subsequent AudioDriverPulseAudio ::get_latency() call (this appears to be for performance reasons).

After realising this could be an issue I modified AudioDriverPulseAudio::get_latency() to not reuse the return value of pa_stream_get_latency and call it every time. Then I ran a project with this script:

extends Node

var latencies = []
var min_latency = null
var max_latency = null

func _process(_delta):
	var latency = AudioServer.get_output_latency()
	if min_latency == null or latency < min_latency:
		min_latency = latency
	if max_latency == null or latency > max_latency:
		max_latency = latency

func _exit_tree():
	print("AudioServer.get_output_latency() ranged between ", min_latency, " and ", max_latency)

And I got the following output:

AudioServer.get_output_latency() ranged between 0.014455 and 0.02272

When this was run the engine audio output latency was set to 5ms (256 audio frames).

So I guess in your case it's plausible that AudioDriverPulseAudio::get_latency() returned the correct latency at the time it was called, but Pulse eventually reduced it which is why it doesn't feel as high as Godot claims it is.

DISCLAIMER: I skimmed the Pulse Audio documentation extremely briefly.

Apologies for getting off topic. I guess this should submitted as an issue if it hasn't been already?

[EIREXE]

I'm starting to believe my change isn't entirely a good idea, users have reported pops and desyncs, so maybe we should actually believe getbuffersize after all.

[EIREXE]

Reading the documentation:

https://docs.microsoft.com/en-us/windows/win32/api/audioclient/nf-audioclient-iaudioclient-getbuffersize

This method retrieves the length of the endpoint buffer shared between the client application and the audio engine. The length is expressed as the number of audio frames the buffer can hold. The size in bytes of an audio frame is calculated as the number of channels in the stream multiplied by the sample size per channel. For example, the frame size is four bytes for a stereo (2-channel) stream with 16-bit samples.

so from what I understand getbuffersize is the size of the internal buffer that's between the application and WASPI, and wasapi might read this buffer with a periodicity that's higher than the actual buffer size
or something like this I have no idea about audio engineering stuff.

[fire]

I polled the audio channel on rocket chat.

This is a lot of Windows specific code and one person said they're not qualified to say if it's a good change or not because of that.

Is there a way to have an easier review?

[EIREXE]

I polled the audio channel on rocket chat.

This is a lot of Windows specific code and one person said they're not qualified to say if it's a good change or not because of that.

Is there a way to have an easier review?

I asked around for people who've written similar IAudioClient3 code for rhythm games (particularly project DIVA) and they say it appears to be spot on.
I also reviewed it when the PR came out and it looked correct comparing it to MS's documentation.
I've also been using it in production for a while and people report it's fine on w7 too (which is where the iaudioclient3 interface stops existing).

[fire]

I asked around for people who've written similar IAudioClient3 code for rhythm games (particularly project DIVA) and they say it appears to be spot on.

I also reviewed it when the PR came out and it looked correct comparing it to MS's documentation.

I've also been using it in production for a while and people report it's fine on w7 too (which is where the iaudioclient3 interface stops existing).

Approved because of review by @EIREXE.

Calinou marked this conversation as resolved.

[fire]

Thanks.

[benjarmstrong]

With this merged we should probably make an issue/PR for the docs that mention the limitations of the audio latency project setting. Depending on the operating system and hardware audio driver Godot may have to pick the nearest latency that the system allows. Examples:

• Some drivers may have a limited range of buffer sizes they support, or a set of fixed values (e.g. my motherboard audio driver will allow buffer sizes of 128 to 4096 in increments of 32)
• Some hardware audio drivers may only support one latency (e.g. the Vive pro audio driver on my Windows 10 machine only allows a buffer size of 441 samples)
• Windows versions prior to 10 will only allow one latency.

Should I go ahead and do this?

[clayjohn]

@benjarmstrong yes please, doc changes are usually included with PRs as appropriate. It would be great if you could make a docs PR as soon as convenient.

[benjarmstrong]

Will do

[akien-mga]

A backport for 3.x could also be considered if there's interest and it doesn't break compatibility.

[benjarmstrong]

Oh absolutely, I've been using my own backport of this for ages now. I'll get on that

[EIREXE]

I have a backport to current-ish 3.x here: EIRTeam@7df5185

[akien-mga]

A PR would be welcome yes :) Be sure to include the fix from #53319 too.

[akien-mga]

If a cherry-pick PR is made, #54880 should also be included to solve a regression.

[benjarmstrong]

Yep I've just confirmed that this PR triggers #54879

#54880 should probably be cherry-picked either way since it fixes a nasty little trip wire where if certain AudioDriverWASAPI methods aren't called in exactly the right order the audio engine explodes.