Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WASAPI audio driver produces ringing artifact when project's bit rate doesn't match device's sample rate #46595

Open
Tracked by #76797
wingedadventurer opened this issue Mar 2, 2021 · 18 comments

Comments

@wingedadventurer
Copy link
Contributor

Godot version:
3.2.4-rc3
3.2.3-stable (*)

OS/device including version:
Windows 10, 64 bit
WASAPI audio driver
tested on: laptop speakers with 3.5mm headphones; Steelseries Arctis 7 headphones

Issue description:
When mix rate property in Project Settings (which defaults to 44100 Hz) is different from current audio device's sample rate setting in Windows Sound Control Panel (which, for my 2 devices I tested on, defaults to 48000 Hz), you can hear a ringing effect in any audio played in Godot. The ringing is more noticeable when playing low-end frequencies. This happens with WAV, OGG, and as of 3.2.4, MP3.

I have also tried to set devices to 44100 Hz and Godot to 48000 Hz, with same results.
Setting both to either 44100 Hz or 48000 Hz removes the ringing.

I also tested samples with sample rate set to 44100 Hz and 48000 Hz, but they don't make any difference for this issue.

Here is the video demonstrating the issue (listen with headphones to hear low-end frequencies better):

demonstration.mp4

I played these samples in FL Studio, VLC, and Audacity, and they all sound the same.

In case it is expected that Godot's and device's sample rates should match, shouldn't that be automatic? The tooltip for Mix Rate setting says: In general, it's better to not touch this and leave it to the host operating system. so I'd assume the audio should play just fine with all defaults.

This is a continuation of #23544. CC @Zylann @marcelofg55 @akien-mga

Steps to reproduce:

Try playing audio samples provided below in the minimal reproduction project. You don't need a node, just play them from the editor.

Mix Rate setting in Godot is located in Project Settings > audio/mix_rate.

Device sample rate in Windows 10 is located in Settings > System > Sound > Sound Control Panel. Double-click your device, go to Advanced tab, and you should see your device's sample rate:

nav2

(*) note that on 3.2.3-stable, you can hear this ringing for OGG even with matching sample rates. #46086 fixes this issue and is merged in 3.2.4-rc3.

Minimal reproduction project:

godot_wasapi_ringing.zip

@benjarmstrong
Copy link
Contributor

If #46086 fixes the issue then could it be possible that this is a problem on all audio drivers (not just WASAPI)?

@griffinjennings
Copy link

After doing a little testing, it looks like the information about project bitrate is stored in the .pck file of a given exported project.

I'm not a programmer really, so this might not be feasible, but would it be possible to write in some kind of check at runtime that:

  1. Reads the user's device's sample rate
    2a. Writes that sample rate to the PCK file just before loading it, OR
    2b. Reads the rest of the PCK file with the system's sample rate, and disregards the sample info in the PCK file?

@starry-abyss
Copy link
Contributor

starry-abyss commented Jul 19, 2021

From what I googled WASAPI can't change user-specified sample rate to something else. So I think the options are:

  1. adjust Godot project sample rate to what's selected by user in Windows settings
  2. continuously resample from Godot selected rate to Windows selected rate
  3. similar to 2, but ask Windows to resample
  4. add DirectSound support back to Godot

Here are some examples from other projects of how it could be done:
soundradix/JUCE@1162b8d
thestk/rtaudio#119
thestk/rtaudio#88

@ellenhp
Copy link
Contributor

ellenhp commented Jul 31, 2021

It might make sense to resample all output in the audio driver before sending it to WASAPI, if the sample rates do not match. I'll see if I can get something put together that does this.

@devalexx
Copy link

adjust Godot project sample rate to what's selected by user in Windows settings

to be honest I don't care about this option (I guess 99% of users too). that's why the best option for me would be this. Like putting 0 there or have an extra checkbox to read the global windows value and use it.
this ringing is really annoying.

PS: FYI I've just tested and in HTML5 exported project everything is ok

@Calinou
Copy link
Member

Calinou commented Jan 16, 2022

Would using a sample rate of 48,000 Hz by default alleviate this issue? It seems 48,000 Hz is a more common default on Windows compared to 44,100 Hz.

Reading the Windows-provided value and performing resampling would be ideal, but I don't know how much work it requires.

@benjarmstrong
Copy link
Contributor

@Calinou

Reading the Windows-provided value and performing resampling would be ideal, but I don't know how much work it requires.

I've looked into this idea and briefly implemented it on our private branch. It worked, but introduced a small amount of latency so I scrapped it. This was using resampling code I copy-pasted from elsewhere in the engine that added 256 frames of latency; it was unacceptable for our use case but it may be fine for general purposes users.

@ellenhp
Copy link
Contributor

ellenhp commented Jan 22, 2022

Someone may have already suggested this but what if we just ignore the sample rate setting on some platforms and use whatever sample rate the platform wants us to use. We already resample every audio stream played through the engine, maybe we should just resample them to the correct sample rate instead of the one selected by the game developer at build-time? To benjamin's point we probably don't want to be resampling the final mix (again)

@Calinou
Copy link
Member

Calinou commented Jan 22, 2022

Someone may have already suggested this but what if we just ignore the sample rate setting on some platforms and use whatever sample rate the platform wants us to use. We already resample every audio stream played through the engine, maybe we should just resample them to the correct sample rate instead of the one selected by the game developer at build-time? To benjamin's point we probably don't want to be resampling the final mix (again)

I agree this is likely the better choice, which will also result in better quality.

@ellenhp
Copy link
Contributor

ellenhp commented Jan 22, 2022

The only argument against this that I can think of is if some audiophile comes along and says no, the sample rate for my game must be 192khz, but that's not even on my radar as a possibility right now because of this specific bug. We need to solve it before audio in Godot sounds good on Windows.

Side-note: I'd be willing to bet that WASAPI has this bug for the same reason that Godot had it. There's a stackoverflow answer somewhere about how to do cubic resampling that has this bug. They used the wrong coefficients for the polynomial. I bet some WASAPI developer used that code instead of doing all the math themselves 😆

@griffinjennings
Copy link

The only argument against this that I can think of is if some audiophile comes along and says no, the sample rate for my game must be 192khz

Would it be possible to replace the current Sample Rate setting with a "Force Sample Rate" setting, kinda like the Force Fps setting in debug? where setting it to zero = just use whatever the platform is using?

@ellenhp
Copy link
Contributor

ellenhp commented Jan 22, 2022

Another option I'd almost prefer is to keep the current setting and add a "force sample rate" checkbox (default false). The ability to set a sample rate preference on some platforms may be useful, so on every platform except for Windows, the text box will retain its behavior. But you have the checkbox just in case you want to specify the sample rate on Windows too. The reason I like this more is because it breaks compatibility less. No need to change defaults or names of settings. Existing projects will continue to open and be magically fixed once the Godot version is updated. Something like this could be cherry picked for 3.x with no issues, but renaming a setting might be a 4.x only thing.

I've fixed a lot of Godot's audio bugs but I don't have a windows machine so even if I had time this isn't something I can do. But the basic idea behind either one of these fixes is to change the interface of the AudioDriver class so that it can surface a platform preference for sample rate to the AudioServer. All of the audio drivers except for WASAPI will simply specify zero as their sample rate preference indicating they don't care. The AudioServer should take this into consideration along with whatever preference/forcing options are or are not specified by the user at build time. Once the audio server decides on a sample rate, all the AudioStream classes will handle resampling to the decided-upon rate, and everything should work :)

@ellenhp
Copy link
Contributor

ellenhp commented Jan 22, 2022

@benjarmstrong If I got a branch that set up the AudioDriver interface and AudioServer changes for the plan in #46595 (comment), would you be willing to forklift your WASAPI sample rate detection code onto it and then test? I know you're very busy but I imagine this is something you need fixed if you're releasing a game on Windows, so we might be able to solve it with our powers combined :)

@benjarmstrong
Copy link
Contributor

@ellenhp

would you be willing to forklift your WASAPI sample rate detection code onto it and then test?

I noticed the ringing when I used that resampling code that I copied from elsewhere in the engine, and it was gone when I got rid of it (should have mentioned that). I think you may be correct in your assessment that a bad coefficient is used, or my implementation was sloppy

I plan to eventually rework it though. I'll be sure to keep that in mind when I do

@ellenhp
Copy link
Contributor

ellenhp commented Jan 22, 2022

Oh, I don't think your implementation was sloppy, I think WASAPI's internal implementation of resampling is sloppy! Sorry if that wasn't clear, oops.

I went to go look into fixing this and found some interesting stuff in the docs.

The AUDCLNT_STREAMFLAGS_RATEADJUST flag enables an application to get a reference to the IAudioClockAdjustment interface that is used to set the sample rate for the stream. To get a pointer to this interace, an application must initialize the audio client with this flag and then call IAudioClient::GetService by specifying the IID_IAudioClockAdjustment identifier. To set the new sample rate, call IAudioClockAdjustment::SetSampleRate. This flag is valid only for a rendering device. Otherwise the GetService call fails with the error code AUDCLNT_E_WRONG_ENDPOINT_TYPE. The application must also set the ShareMode parameter to AUDCLNT_SHAREMODE_SHARED during the Initialize call. SetSampleRate fails if the audio client is not in shared mode.

We're definitely not doing any of that. I set up a branch to try and do what they described and I'm hoping it'll compile and maybe even fix the bug, but without a windows machine it's hard to know for sure whether it'll work.

@benjarmstrong
Copy link
Contributor

benjarmstrong commented Jan 23, 2022

Oh I meant my private implementation of resampling was sloppy. I never committed it, I was just tinkering with the engine.

without a windows machine it's hard to know for sure whether it'll work

For Windows-specific commits I use virtual machines running Windows 7 and 10 respectively since the WASAPI driver behaves differently on Windows 10 onwards, and Godot aims to support Windows version down to 7. I run a Linux host and compile into a directory shared between VMs to ensure I haven't broken cross-compilation

I'll test your branch now and see if I can make any progress on it.

Update: it works, but needed some tweaking to compile and it doesn't work on Windows 10

@benjarmstrong
Copy link
Contributor

No luck getting it to work on Windows 10.

We use IAudioClient3 instead of IAudioClient2 where available (Win 10 onwards), because then we can use IAudioClient3::InitializeSharedAudioStream which allows us to initialize an audio stream while specifying the buffer size and thus we can obey Godot's audio latency engine setting. But according to the docs AUDCLNT_STREAMFLAGS_RATEADJUST can't be used with that method, which seemingly rules out having a variable buffer size AND sample rate conversion.

Furthermore when activating the IAudioClient3 interface IsFormatSupported will always show the closest available sample rate as the system's native sample rate (at least on my system). IAudioClient2 is a lot more forgiving.

Weirdly IAudioClient3::InitializeSharedAudioStream supports the AUDCLNT_STREAMFLAGS_AUTOCONVERTPCM and AUDCLNT_STREAMFLAGS_SRC_DEFAULT_QUALITY parameters suggesting sample rate conversion is possible.

I'm gonna sleep on it.

@ellenhp
Copy link
Contributor

ellenhp commented Jan 23, 2022

Godot already resamples all audio that goes through it in the AudioStreamPlayback classes, so I'm not sure it makes sense to spend too much time trying to come up with a way to coax WASAPI into resampling cleanly. If IAudioClient3 doesn't support resampling then we can probably just adopt its sample rate for everything and call it good enough, IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants