godotengine
Make AudioStreamPlaybackWAV inherit from AudioStreamPlaybackResampled
Fixes https://github.com/godotengine/godot/issues/58216 by removing resampling-related code from AudioStreamPlaybackWAV and making it inherit from AudioStreamPlaybackResampled.
I only tested this at a basic level, not extensively. Requires testing with a wide variety of audio files, with all kinds of looping set up. Performance testing is needed, too. The cubic hermite resampling that AudioStreamPlaybackResampled does is extremely cheap in theory, but I'm not sure it's been battle tested by running several dozen instances of it at once.
This has the added bonus of bypassing the issue the ADPCM decoder has of not doing interpolation; before this patch, low sample rate ADPCM audio has nasty aliasing artifacts that you can even see with your eyes. By relying entirely on AudioStreamPlaybackResampled, that issue is avoided. I'm not sure if an issue is already open for this or not.
Not necessary to push right now, but next time you amend the PR, could you edit the commit title to be like the PR's title? It's much clearer with proper capitalization.
See https://github.com/godotengine/godot/blob/master/CONTRIBUTING.md#format-your-commit-messages-with-readability-in-mind
Sounds good! I just noticed that I left some fixed point stuff behind, so I'll have to make a new push anyway.
Interesting idea. I know historically Godot has relied on WAVs for very low overhead audio playback. But on modern processors where a couple extra operations with good cache locality can sometimes have literally zero impact on performance, I think this makes a lot of sense. Do backwards and ping-pong loops still work?
I checked earlier, backwards and ping-pong loops do still work.
While testing, I noticed that there's a pre-existing bug (i.e. not caused by this PR) where using backwards loop mode and setting the loop end offset beyond the end of the audio data can read out-of-bounds memory and produce corrupt audio, even on the master branch. Uh oh.
Interesting idea. I know historically Godot has relied on WAVs for very low overhead audio playback. But on modern processors where a couple extra operations with good cache locality can sometimes have literally zero impact on performance, I think this makes a lot of sense.
Yeah, that's the idea, but I'm not sure what the specific performance impact is in practice. It might be the case that AudioStreamPlaybackResampled needs optimization or something.
My intuition is that hermite resampling should be fast enough, because audio data moves very slowly in general. 48khz isn't hard to keep up with for a modern processor generally. That said, it requires 11 multiplies and 10 adds per sample in the current form. On a 1GHz mobile processor with an FPU capable of an operation every clock, that's 1 millisecond of processor time per second per sound (edit: per channel) if you keep the pipeline completely full and never have a cache miss or stall execution. That's an unrealistic condition, so in reality it will be more than 1ms. That's way more than I thought it would be before doing the math. I know lots of mobile processors have a lot of cores but I'm still a bit unsure now. I think I'll defer to Juan here because he has worked in audio both before and after computers got really fast.
It's not a lot, but it should be possible to cut that down to 7 multiplications and 10 additions. Using the h00, h01, etc. nomenclature from the wikipedia page (https://en.wikipedia.org/wiki/Cubic_Hermite_spline):
pre, a, b, post
a_tan = b - pre
b_tan = post - a
t_squared = t * t
h11 = t_squared * (t - 1)
z = t_squared - h11
h01 = z - h11
h00 = 1 - h01
h10 = t - z
output = a * h00 + b * h01 + (a_tan * h10 + b_tan * h11) * 0.5
Opened a PR for optimizing the resampling algorithm as described in the previous post: https://github.com/godotengine/godot/pull/83536
@wareya This PR broke after the implementation of QOA through #91014.
It can be fixed by replacing the entire is_qoa block with the one in this comment and removing the cache variables from QOA_State (not needed as resampling does not take them into consideration anymore).
Additionally, it's unnecessary to keep AudioStreamWAV::DATA_PAD and its offsets when fetching data, since interpolation isn't done by AudioStreamPlaybackWAV anymore.
@wareya I have rebased this PR on top of master and addressing my comments. You can see it here, feel free to copy.
One thing to be aware of however: any new audio files with less than 5120 frames (around 0.11s at 44.1kHz) imported between #95815 and #96768 with any loop mode enabled to the last frame will crash Godot as it will try to fetch an out-of-bounds index. After the latter PR, this can be fixed by reimporting as it is.
Also, it seems that the latency of Resampled's mix buffer carries over to the preview, creating a small gap at the beginning:
Although this is just a visual bug and should not affect actual playback.
Also a message to reviewers: I don't see how this breaks compatibility unless the crash mentioned above counts. No code in existing projects has to be changed.
I figured out why the preview had the start gap: you're supposed to call begin_resample() at the start() function.
I have updated my rebased reference to include it. Once again, feel free to copy it for this PR.
@wareya Are there still plans to update this PR? I was thinking about salvaging a few months ago, but I don't want to "steal" from someone who barely has commits in the engine.
Consider this me formally asking for permission to do so if there are no plans 🙂.
I want to revive this PR, but it might take a while for me to get around to it; I'm in a programming language design rabbit hole at the moment. If you get around to it first, then sure, you can "steal" it! Just have at least one commit as "authored by me" or whatever git's word for it is.