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

Sound played with SDL_AudioStream via WASAPI has wrong sample rate (regression/old bug) #6326

Closed
leonstyhre opened this issue Oct 4, 2022 · 16 comments
Assignees
Milestone

Comments

@leonstyhre
Copy link

Hi!

It seems like an old bug has returned, in both 2.0.22 and 2.24.0 the sample rate conversion is incorrect when using WASAPI. Using my application (https://es-de.org) it's easy to reproduce this issue as any video will have audio playing too slowly.

If you would like to test it, then this is the download to the 2.0.0-alpha release which uses SDL 2.24.0:

https://gitlab.com/es-de/emulationstation-de/-/packages/9556946

Replacing the SDL2.dll file with the 2.0.20 version immediately fixes the problem without any other changes to the application. As well, changing the code to use 48000 Hz sample rate instead of 41000 Hz also makes the audio play at the correct speed. No other platforms I have tested (primarily Linux and macOS) have the same problem.

I didn't discover this problem in 2.0.22 as I jumped straight from 2.0.20 to 2.24.0 for my application.

See the following issue, the problem seems to be identical:

#4946

Thanks!

@slouken slouken added this to the 2.26.0 milestone Oct 4, 2022
@icculus
Copy link
Collaborator

icculus commented Oct 4, 2022

Hmm, I can see the difference.

I downloaded the 2.0.0-alpha, copied over a ROM of Castlevania II for the NES, used the Scraper feature to download a preview video thing, and let it play on the gamelist screen.

It sounds right (to me, at least) but the video is getting ahead of the audio pretty quickly (within a few seconds, Simon's whip is pretty clearly noticeably behind the sound effect), which it doesn't do if I drop in an SDL that lets WASAPI handle the resampling, so presumably we're resampling a few too many samples at a time for some reason here.

The fastest fix, for you, is either rebuild SDL with this #if 1 changed to #if 0, which will force WASAPI to handle resampling instead of SDL. We turned this to 1 in 2.0.22, because some people said they heard distortion in WASAPI's resampler, but I do not believe this was a widespread problem.

#if 1 /* we're getting reports that WASAPI's resampler introduces distortions, so it's disabled for now. --ryan. */

Alternately, somewhere before you call SDL_Init(), add this line to your code, which will force SDL to use DirectSound instead of WASAPI, which provides its own resampler, too.

#ifdef __WIN32__
SDL_SetHint("SDL_AUDIODRIVER", "directsound");
#endif

...but this is obviously just a temporary solution until we sort this out and you should plan to remove this in a later build.

@leonstyhre
Copy link
Author

Yes the speed difference is subtle and it's most easily detected by the video getting out of sync with the audio. As a test I had the same game video playing on Linux and Windows via ES-DE on two different computers at the same time and it's then very obvious that they are drifting apart and that the pitch is different.

If I change this line in AudioManager.cpp in ES-DE:
sRequestedAudioFormat.freq = 44100;
To this:
sRequestedAudioFormat.freq = 48000;

Then the audio is playing properly, at least as far as I could tell during my brief side-by-side testing. That's seemingly the same problem as I reported in the previous issue linked above. Btw, this is not a workaround I'm willing to make in my application as I'm not sure what weird side effects it could have. And if WASAPI is not used on a certain machine it will also screw up the audio.

Anyway, thanks for the proposals for workarounds, I'll do either of these two for the next alpha release as a temporary solution. But just to confirm that I didn't misunderstand, you do intend to fix this resampling issue for the 2.26.0 release?

@slouken
Copy link
Collaborator

slouken commented Oct 4, 2022

Yes, it will be fixed for the 2.26.0 release. Thanks for the repro case!

@leonstyhre
Copy link
Author

Great, thanks for confirming that! I'll probably ship an SDL2.dll with WASAPI handling the resampling in the meanwhile, it will be some months before I'll have the final 2.0.0 release ready anyway.

@icculus
Copy link
Collaborator

icculus commented Oct 5, 2022

Interesting discovery: I built libsamplerate into a Windows build of SDL and enabled that instead of our internal resampler, and it has the exact same problem...so the resampler itself is probably not the issue, as libsamplerate is considered to be pretty solid in general, but more likely some issue in how we're feeding audio to WASAPI for playback. I'm still looking deeper.

@leonstyhre
Copy link
Author

That's a very interesting find, looking forward to hearing what the root cause is when you've figured it out!

@leonstyhre
Copy link
Author

I can also confirm that compiling 2.24.0 with #if 0 set as you recommended works perfect, audio now plays at the correct speed and I don't hear any distortions on my test machines at least.

@icculus
Copy link
Collaborator

icculus commented Nov 5, 2022

Quick non-update: I won't be able to test this on Windows until Monday, but I assume the sampling rate fix in 78f9710 will not fix this bug.

Right now I'm leaning towards just turning the WASAPI resampler back on, at least for 2.26.0, honestly. I have to go look at what the complaint was that made us disable it.

@leonstyhre
Copy link
Author

Thanks for the update! Unfortunately I can confirm that 78f9710 did not fix the issue. I have never had any issues with the WASAPI resampler though (such as distorations) and I have not heard of it being an issue for any of the users of my application. So maybe that problem only occurs under very specific circumstances?

@icculus
Copy link
Collaborator

icculus commented Nov 11, 2022

Okay, I spent some time on this, and I think turning WASAPI's internal resampler back on is the right move.

We turned it off because of the axe-clang sound in vkQuake causing a weird effect in #5538, but after doing that, we discovered this happens with WASAPI's resampler, our resampler, and to a lesser degree with libsamplerate, too.

Since it didn't fix that issue, and it's causing this one and probably others, let's just flip it back.

@slouken, agree?

@slouken
Copy link
Collaborator

slouken commented Nov 11, 2022

I believe people also reported resampling distortion with WASAPI in DOTA 2, but the e-mails have been deleted so I can't remember for sure.

If the issue really is in how we're feeding data to WASAPI, then that workaround seems fine, but we should document it well. :)

@icculus
Copy link
Collaborator

icculus commented Nov 11, 2022

Ok, I've turned it back on in 85aa9b8, which I'm satisfied with for 2.26.0, but I'm leaving this bug open because I'd like to understand what we're messing up in the WASAPI code and/or the resampling code.

@icculus icculus modified the milestones: 2.26.0, 3.0 Nov 11, 2022
@icculus
Copy link
Collaborator

icculus commented Nov 11, 2022

In any case, @leonstyhre's specific problem is resolved by this change, but a proper fix (or at least a proper understanding) is still pending, but it'll be safe for EmulationStation to upgrade to 2.26.0.

@leonstyhre
Copy link
Author

Thanks a lot for this! And yes I agree that it would be good to understand the root cause (and even better if it could be solved), but at least the WASAPI audio is now working correctly again. And as mentioned earlier on, I have never experienced any issues like distortions and such with this resampling enabled. Not saying it doesn't exist, but it's never been a problem in ES-DE.

@icculus
Copy link
Collaborator

icculus commented Oct 4, 2023

For SDL2, we're just going to leave the WASAPI resampler in place; for SDL3, we're probably safe to rely on our own (rewritten) resampler, but I don't really see a good reason to disable the WASAPI resampler, either, so I'm going to leave it alone and call this done until there's a specific need to change it.

@icculus icculus closed this as completed Oct 4, 2023
@leonstyhre
Copy link
Author

Thanks for the update! And to follow up on my previous comment from almost a year ago, I still have not experienced or heard of any issues related to WASAPI so as far as I can tell it's working perfectly fine.

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

No branches or pull requests

3 participants