search

godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.95k stars 21.15k forks source link

[Godot 4] Audio pop on looping with loop offset > 0 #64775

Open MJacred opened 2 years ago

MJacred commented 2 years ago

Affects ogg vorbis & wav quite noticeable. As well as mp3, but to a lesser degree (see here)

Godot version

4.0 Alpha 1, 4.0 Alpha 14, 4.0 Beta 17, and https://github.com/godotengine/godot/pull/64488, https://github.com/godotengine/godot/pull/52626, https://github.com/godotengine/godot/pull/71780

System information

Issue description

NOTE: Looping only works without audio pop, if there is no loop point (i.e. offset == 0 seconds).

What doesn't work

in MRP, you'll find 2 projects: one for 3.5, the other for 4 Alpha 14. And example audio file is in both projects, trimmed down to the necessary parts: end of file and loop point at 2 seconds

waveform looks good in Audacity (defaults to ALSA): left part shows end of file and the loop point, right side shows the same, but "merged" waveform

For testing purpose, I also tried manually editing the sample at the loop point down a pixel or two so it's perfectly on the 0 line, but that made no difference

Steps to reproduce

Open MRPs and play game/scene (has an audio player which seeks towards end of file, shortly before looping). Or use the Editor's preview functionality

For some audio files, the pop only appears on my good headphone (HyperX HX-HSCA-RD Cloud Alpha), but not on my ~10-year-old cheap 10 EUR / USD Logitech stereo speakers who cannot reproduce high quality audio.
For the MRP, I used one file where it's audible on both, headphone and speakers.

Audio file is stereo 44100Hz 32-bit float.
I also tried setting format to 16-bit PCM in Audacity, but after exporting I see no difference in the meta data using exiftool and mediafire, except that that the file size increased by 21 kB… And the pop did not disappear. Maybe the conversion does not work/export in Audacity…

Minimal reproduction project

The godot 4 zip was created using using this PR

godot_3.5.zip godot_4.zip

more testing files for PR: https://github.com/godotengine/godot/pull/80452

  1. cooking_S00 loops at second 9.0
  2. sweep_S00 loops at second 1.0
  3. sweep_S00 low loops at second 1.0 (no pop, for comparison sake with 2.)
    • it's basically 2. re-exported with Audacity with Quality setting 5

or all bundled: all_bundled.tar.gz

Calinou commented 2 years ago

Can you reproduce this with WAV or MP3 sounds?

MJacred commented 2 years ago

tested in: 4.0 Alpha 14

MP3 works like a charm.

MP3 export used in Audacity: mp3-1

Comparing to OGG, I can only fetch

-> Maybe something with the bit format? But looking as WAV has same issue with 16-bit format…

:no_entry_sign: WAV also has audio pop

image

MJacred commented 2 years ago

Found out how to change bit rate of OGG export in Audacity.

Exported OGG with Bit rate 192 kb/s and Overall bit rate 187 kb/s. But pop did not disappear

EDIT: to get more test data, I tested 7 files where audio pops occurred using ogg. All looped without audio pop, once converted to mp3

MJacred commented 2 years ago

@Calinou: Can this issue be in milestone 4.0? Having to use mp3 instead of ogg almost doubles the file size (one example file: 2.1 MB -> 3.7 MB)…

EDIT: thanks!

MJacred commented 1 year ago

Re-tested with MRP in Godot 4.0 beta 17 (and just to be sure, I re-imported audio file and in the scene removed and re-added the audio file to the stream) and audio pop is still there.

@ellenhp, could you take a look, please? I tested with https://github.com/godotengine/godot/pull/71780, but it didn't help.
Looping only works without audio pop (tested in 4.0 beta 17 and your PR with the affected audio file attached in the MRP), if there is no loop point (i.e. offset == 0 seconds).

ellenhp commented 1 year ago

Have you double-checked that you're looping at a zero-crossing? Loops in Godot are gapless meaning we don't do any fading between the end of one playback and start of another, so it's essential that your loop points be at zero crossings, otherwise you'll get pops.

edit: I'm not entirely sure why this would not affect mp3's but I suspect minimp3 isn't doing sample-accurate seeking and does a fadein/out to compensate.

MJacred commented 1 year ago

As shown in the image in the description, the end and the loop offset are not perfectly at 0. I manually moved them there (for testing purpose), but it didn't help.

Odd is: if the loop offset equals 0, there's no pop.

Note that Godot 3.5 and Audacity have no issue with this file as-is.

ellenhp commented 1 year ago

Let me check to see if there's a loop crossfade in 3.5.

ellenhp commented 1 year ago

No crossfade, but the wav code hasn't been touched much between 3.x and 4.x so could you double-check that a wav file with identical loop points has different behavior between those versions? I know you checked oggs but I want to make sure that you checked wavs before I go digging because that'll help me prioritize either looking in the AudioServer or in the new ogg/vorbis code in 4.x.

MJacred commented 1 year ago

I re-tested in Godot 3.5 and your PR

… and I replaced the MRP in the description: it now includes ogg vorbis and wav and both with intro and without

godot 4.0 (your PR)

note: wav tested in inspector; ogg vorbis tested in importer

no intro && with offset == 0

with intro && with offset == 2 seconds

godot 3.5

note: ogg vorbis and wav tested in inspector

no intro && with offset == 0 and with intro && offset == 2 seconds

Notes

I opened the wav files in Audacity and it looped them without any small pop

ellenhp commented 1 year ago

Thank you. :) I wouldn't expect #71780 to fix this but it shouldn't break it either so this testing will be helpful. It might have something to do with the lookahead buffer I implemented way back when, or the way that looping is detected by the audio server. I'll do my best to find time to look into this.

ellenhp commented 1 year ago

I've had some time to think about this, and I honestly think it's more likely that the new seek code for oggs isn't sample-accurate.

ellenhp commented 1 year ago

Confirmed that the seek algorithm isn't sample accurate. The sample burning code is also wrong, which I discovered by holding down left mouse button and scrubbing across the entire track in the import view.

I mentioned this back when we were discussing replacement of stb_vorbis, but I really want to be using vorbisfile instead of building our own container format like we did for 4.0. That would fix a bunch of problems, fix this bug, and reintroduce the ability to parse oggs at runtime. At the cost of a few reallocs on the audio thread. I'm absolutely convinced that it won't be a problem though, especially because 4.0 already reduced the long-tail mixing latency dramatically from 3.x (by over half, the last time I profiled). We can afford a realloc or two and I will stand by that until profiling results tell me otherwise. 512 samples is 11 milliseconds which is a really, really long time for a modern computer. We do hundreds, probably thousands of mallocs on the main thread and still manage to hit 17ms frame deadlines for a 60fps experience. I really think it's time to reexamine the "no allocation on the audio thread" thing. We should make decisions like this based on measurement IMO, not fixed rules. Vorbisfile was designed for exactly this purpose.

MJacred commented 1 year ago

@ellenhp, from the other issue I saw that your hands are full, currently. But did you have time to consult with the others regarding the preparation steps, namely using vorbisfile and not a custom container format + the "no allocation on the audio thread" thing?

ghost commented 1 year ago

Hi, I'm having the same issue, however the mp3 version also introduces a small "pop" less noticeable than the one introduced with wav or ogg.

Are you sure mp3 was fine for you? Or was it just a very small "pop"?

MJacred commented 1 year ago

@strellydev: I don't remember hearing any pops for mp3, but I might have been too tired at that point to hear very faint pops. @ellenhp expected that also mp3 should be affected and she suspects minimp3 isn't doing sample-accurate seeking and does a fadein/out to compensate. (source). That would mean that this fadein/out does not work well enough in your case. Therefore, yes, if you can hear a pop in your mp3 file in Godot 4 but not in Godot 3, then mp3 is affected as well.

ghost commented 1 year ago

I see, thanks for your answer, I missed that sentence in the conversation.

I'll try taking a look in the code to see if I can figure out how it works and how to fix it maybe.

ghost commented 1 year ago

Confirmed that the seek algorithm isn't sample accurate. The sample burning code is also wrong, which I discovered by holding down left mouse button and scrubbing across the entire track in the import view.

I mentioned this back when we were discussing replacement of stb_vorbis, but I really want to be using vorbisfile instead of building our own container format like we did for 4.0. That would fix a bunch of problems, fix this bug, and reintroduce the ability to parse oggs at runtime. At the cost of a few reallocs on the audio thread. I'm absolutely convinced that it won't be a problem though, especially because 4.0 already reduced the long-tail mixing latency dramatically from 3.x (by over half, the last time I profiled). We can afford a realloc or two and I will stand by that until profiling results tell me otherwise. 512 samples is 11 milliseconds which is a really, really long time for a modern computer. We do hundreds, probably thousands of mallocs on the main thread and still manage to hit 17ms frame deadlines for a 60fps experience. I really think it's time to reexamine the "no allocation on the audio thread" thing. We should make decisions like this based on measurement IMO, not fixed rules. Vorbisfile was designed for exactly this purpose.

Would this fix #66452

fkeyzuwu commented 1 year ago

not entirely related, but in general looping with an mp3 file is not recommended afaik since mp3 files have dead header information at the beginning of the file(which is audible) so looping mp3s will always sound a bit off because of it

Calinou commented 1 year ago

not entirely related, but in general looping with an mp3 file is not recommended afaik since mp3 files have dead header information at the beginning of the file(which is audible) so looping mp3s will always sound a bit off because of it

Couldn't Godot ignore the header information when looping?

fkeyzuwu commented 1 year ago

not entirely related, but in general looping with an mp3 file is not recommended afaik since mp3 files have dead header information at the beginning of the file(which is audible) so looping mp3s will always sound a bit off because of it

Couldn't Godot ignore the header information when looping?

maybe it can, depends on how its coded i guess. in unity for example if im not mistaken it doesnt ignore the header.

ghost commented 1 year ago

I've been looking at this on and off, I've tried removing the lookahead buffer, I also fixed the ogg seek algorithm errors that pop up when dragging the cursor along the song and when selecting the last possible ogg package, I've tried messing with the server, I've tried messing with the search algorithm, I changed the burning samples calculation so it's more (in my opinion) accurate, and nothing, the pop keeps happening. The only idea I have left is that maybe it's not actually mixing the end of the file when looping and the pop happens because the end is being skipped or something, I have no idea if this is the case, but it's the only thing I can think of, after seeing that when looping, the frames_mixed variable isn't the actual value of the last granule as (I assume) it should be.

But anyway, hope that this maybe sparks some ideas into someone elses brain maybe. I just wish the code was commentated.

ghost commented 1 year ago

Good news, I was able to fix the OGG pop.

The code was indeed skipping the last ogg packet.

There was some other small problems with the seek algorithm and some other things, but the skip was the main thing.

The fix however means that the warning for burning samples fix I did doesn't work, so sometimes the "burning negative samples" warning appears again.

I will create a pull request for the ogg fix.

ghost commented 1 year ago

Bad news,

I was making sure everything was working nicely with many different loops and... For some ungodly reason when the loop is at 0 (using the audio pop no intro file from the sample project) it still skips some samples, like in this example, it skips 38 samples for no reason, even though it's reaching the end of the file correctly and mixes the very last samples from the very last packet.

I don't even know where those 38 samples could be going or where it's skipping them x_x...

Another thing, with a song I made that has a loop at the 31.46 second mark, it works, however, it seems like the decimal precision of the engine is messing up, because there's still a TINY pop, and when I look at the value of the offset in the debug menu of visual studio, it's at 31.4599991 or something like that, which introduces the small pop, if I manually change that value to be 31.4600010, there's no pop. So that seems to be an issue from somewhere else.

Which means the only mistery left is the 38 random samples from the loop at 0.

I will create the pull request and point it out to see if someone can see where the problem could be.

Cheers.

ghost commented 1 year ago

Good news (again)

I fixed the offset at 0 pop, it was actually still missing some at the end because I was checking if the samples left in the last packet were greater than the samples needed by the mixing function, instead of checking if they were greater than 0 like I should have.

Hooray! Now the only thing left for ogg to work properly is for the decimal precision to work! :^)

Edit: In the end the decimal precision problem was also fixed, so OGG should be completely fixed now.

ellenhp commented 1 year ago

Should be fixed by #80452

ellenhp commented 1 year ago

Actually I just noticed this affects (affected?) WAVs as well. We haven't touched those yet so I'm going to reopen out of an abundance of caution. Pops while looping an MP3 with a loop offset are expected due to format limitations, but with WAV I would expect it to work perfectly and if it doesn't that's a bug.

bs-mwoerner commented 1 year ago

I created a pull request that should address the WAV clicks, which was actually an entirely different issue from what caused the OGG clicks. Seems to work for me now, but I'd appreciate if you could test this fix with your own files.

MJacred commented 1 year ago

@bs-mwoerner: thanks! I'll check it out this week (I think I'll find some time tomorrow night)

EDIT: tested and gave feedback in that PR