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

Fix AudioStream cubic resampling #51082

Merged
merged 2 commits into from
Aug 6, 2021

Conversation

ellenhp
Copy link
Contributor

@ellenhp ellenhp commented Jul 30, 2021

Reverts b2264cb and fixes Godot's cubic resampling algorithm. Lots of context for this is in #23544 and #49131.

Fixes #49131.

This is implemented with some help from the math on this page: https://www.paulinternet.nl/?page=bicubic

I'm not really a math person. :)

@fire
Copy link
Member

fire commented Jul 30, 2021

Reviewing with Ellen offline.

@ellenhp ellenhp marked this pull request as ready for review July 31, 2021 06:19
@ellenhp ellenhp requested a review from a team as a code owner July 31, 2021 06:19
@Zylann
Copy link
Contributor

Zylann commented Jul 31, 2021

Comments here #23544 (comment)

@ellenhp
Copy link
Contributor Author

ellenhp commented Jul 31, 2021

Summarizing discussion, I think the issue Zylann is running into is related to the WASAPI driver, not AudioStreamPlaybackResampled bugs. I'll probably try and come up with a testing matrix and use OBS to record what everything sounds like so people can review without trying every possible test case.

@ellenhp
Copy link
Contributor Author

ellenhp commented Aug 1, 2021

Okay, I have a demo of this now! Took a while fiddling with OBS but I did it.

The audio files I used are in this zip:

soundfiles.zip

My system's sample rate is 44100, so keep that in mind here. The other thing is that the github video player seems to start muted so turn on the audio for these :)


This is Godot 3.2.3 with the project mix rate set to 44100. Everything sounds fine, nothing is unusual about it.

godot3_official_44100.mp4

This is Godot 3.2.3 with the project mix rate set to 48000. There's a fairly audible ringing artifact on the low frequency beat sound. This is what I fixed with #46086

godot3_official_48000.mp4

Fast-forward to today, with a build from 23bf04a (sample rate 44100). Theoretically there's an audible low-pass effect (#49131) from the new resampling algorithm, but I can't hear it with these audio files.

godot4_head_44100.mp4

And 48000. Should also have a low-pass.

godot4_head_48000.mp4

So what I did in this PR is reverted to the old cubic algorithm and adjusted the coefficients to match a derivation I found online (linked above). Here's how that sounds at 44100 (theoretically a no-op).

godot4_fixcubic_44100.mp4

And here's how it sounds at 48000, which caused issues prior to the coefficient adjustment.

godot4_fixcubic_48000.mp4

No ringing! At least that I can hear.

@akien-mga akien-mga changed the title Fix cubic resampling Fix AudioStream cubic resampling Aug 6, 2021
@akien-mga
Copy link
Member

akien-mga commented Aug 6, 2021

I've tested the changes locally with the files provided in #51082 (comment), and my bad ears and cheap earplugs didn't let me notice much difference (on Linux with Pulseaudio, system sampling rate seems to be 48000 Hz). No regression either though, and since cubic resampling seems to be the better option, it definitely makes sense to go back to it with a fixed implementation.

I can confirm that the new implementation does not reintroduce the ringing from #23544.

I suggest we merge this, include it in the 3.4 betas, and we'll see if audiophiles find new issues or can confirm that the old issues have been fixed.

@akien-mga akien-mga merged commit d4e7a1a into godotengine:master Aug 6, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.4. (Will be included in upcoming 3.4 beta 3.)

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Aug 6, 2021
@akien-mga
Copy link
Member

Cherry-picked for 3.3.3.

@ellenhp ellenhp deleted the fix_cubic_resampling branch August 18, 2021 15:07
@CystalCore CystalCore mentioned this pull request Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.OGG files are played back with rolled-off high frequencies (low-pass)
4 participants