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

WAV importer: Use cubic interpolation on resampler #89071

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

DeeJayLSP
Copy link
Contributor

@DeeJayLSP DeeJayLSP commented Mar 1, 2024

I found this while playing around with ResourceImporterWAV's resampling procedure.

After importing one of Kenney's Interface Sounds (specifically confirm2, converted to WAV) with a mix rate at 32000Hz, I noticed a significant distortion on the sound from the original.

I thought that was a side effect of downsampling, and decided to play around with the resampling code (because why not, maybe I could accidentally find a way to improve it). At one point however, I found a cubic interpolation function online that was different than the one in the importer, and decided to replace the one in resampling with it out of curiosity.

With this new cubic interpolation, the distortion became much less noticeable after reimporting. There is still a bit of distortion, the quality isn't anywhere close to libsamplerate's sinc interpolators, which would be the ideal approach for an importer, but it's still better than before.

In the end, I accidentally found a way of improving it. Since it's just 3 lines of code (+ one comment) I decided to submit.

Due to simplicity, I think this is cherrypickable for 3.x.

Edit: after finding the same algorithm was already used in AudioStreamPlaybackResampled, I decided to copypaste it into ResourceImporterWAV.

Edit 2: The algorithm used in AudioStreamPlaybackResampled was an adaptation from Math::cubic_interpolate() to be used with AudioFrames. Changed to use that instead.

In few words: all this PR does is replacing the resample interpolation in ResourceImporterWAV with Math::cubic_interpolate(), which yields much better results.

@DeeJayLSP DeeJayLSP requested a review from a team as a code owner March 1, 2024 23:42
@Calinou Calinou added this to the 4.3 milestone Mar 1, 2024
@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Mar 2, 2024

One last thing:
I also tried to resample the file to 36000Hz with this change out of curiosity, and the distortion was back.

But then I resampled the same way with libsamplerate's best sinc interpolation, and the result had a similar distortion too (when playing inside Godot), therefore not caused by this change but by Godot's own AudioStreamPlaybackWAV resample (because playing 36000Hz within AudioStreamPlaybackResampled sounds fine, see #58216).

I assume this is the most precise we can have.

@DeeJayLSP DeeJayLSP changed the title ResourceImporterWAV: fix/enhance resampler WAV importer: use cubic hermite on resampler Mar 2, 2024
@DeeJayLSP DeeJayLSP changed the title WAV importer: use cubic hermite on resampler WAV importer: use cubic interpolation on resampler Mar 4, 2024
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During our recent discussion in RocketChat, we discovered that the new code we were considering is identical to Math::cubic_interpolate(), which is already present in our codebase. This has led us to reconsider the use of libresample.

I propose the following test plan:

  1. Record samples with the current code and after the implementation of the new code.
  2. Provide alternative methods for verifying the audio, either as a recording or as part of a project.

This will allow us to compare the output and verify whether the new piece of code are indeed better.

We currently do not have an active audio team. However, contributions from anyone in the community would be greatly appreciated. If you're interested in helping out with this issue, please feel free to get involved.

@AThousandShips AThousandShips changed the title WAV importer: use cubic interpolation on resampler WAV importer: Use cubic interpolation on resampler Mar 4, 2024
@akien-mga akien-mga merged commit b811e9a into godotengine:master Mar 4, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@DeeJayLSP DeeJayLSP deleted the cubicres branch March 4, 2024 13:35
@DeeJayLSP
Copy link
Contributor Author

Just realized this could be simplified by passing frac directly to Math::cubic_interpolate() without having to declare mu.

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.

5 participants