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

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

Open
Tracked by #76797
MJacred opened this issue Aug 23, 2022 · 29 comments · May be fixed by #83341
Open
Tracked by #76797

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

MJacred opened this issue Aug 23, 2022 · 29 comments · May be fixed by #83341

Comments

@MJacred
Copy link
Contributor

MJacred commented Aug 23, 2022

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 #64488, #52626, #71780

System information

  • Distribution: Ubuntu 20.04.1 (OS: X11)
  • Graphics Card: Nvidia GeForce GTX 970 (driver: nvidia 510.73.05)
  • Audio Drivers: PulseAudio, ALSA
  • CPU Name: Intel(R) Core(TM) i7-10700KF CPU @ 3.80GHz

Issue description

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

What doesn't work

  • Godot 4
  • looping works fine in Godot 3.5 and Audacity

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: #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
Copy link
Member

Calinou commented Aug 23, 2022

Can you reproduce this with WAV or MP3 sounds?

@MJacred
Copy link
Contributor Author

MJacred commented Aug 23, 2022

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

  • exiftool
    • Nominal Bitrate: 500 kbps
  • mediafino
    • Bit rate: 500 kb/s
    • Stream size: 516 KiB

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

🚫 WAV also has audio pop

image

@MJacred MJacred changed the title [Godot 4] Audio pop on looping (ogg vorbis) [Godot 4] Audio pop on looping (ogg vorbis & wav; mp3 is fine) Aug 23, 2022
@MJacred
Copy link
Contributor Author

MJacred commented Aug 23, 2022

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 MJacred changed the title [Godot 4] Audio pop on looping (ogg vorbis & wav; mp3 is fine) [Godot 4] Audio pop on looping (affects ogg vorbis & wav; mp3 is fine) Aug 23, 2022
@MJacred
Copy link
Contributor Author

MJacred commented Sep 1, 2022

@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!

@Calinou Calinou added this to the 4.0 milestone Sep 1, 2022
@akien-mga akien-mga modified the milestones: 4.0, 4.1 Jan 31, 2023
@MJacred
Copy link
Contributor Author

MJacred commented Feb 7, 2023

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 #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).

@MJacred MJacred changed the title [Godot 4] Audio pop on looping (affects ogg vorbis & wav; mp3 is fine) [Godot 4] Audio pop on looping with loop offset > 0 (affects ogg vorbis & wav; mp3 is fine) Feb 7, 2023
@ellenhp
Copy link
Contributor

ellenhp commented Feb 7, 2023

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
Copy link
Contributor Author

MJacred commented Feb 7, 2023

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
Copy link
Contributor

ellenhp commented Feb 7, 2023

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

@ellenhp
Copy link
Contributor

ellenhp commented Feb 8, 2023

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
Copy link
Contributor Author

MJacred commented Feb 8, 2023

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

  • ogg: no audio pop (speaker and headphones were fine) ✔️
  • wav: no audio pop with speaker; small (and stays small) audio pop with head phones when system volume > -24dB ❌

with intro && with offset == 2 seconds

  • ogg: audio pop with speaker and headphones ❌
  • wav: no audio pop with speaker; small (and stays small) audio pop with head phones when system volume > -24dB ❌

godot 3.5

note: ogg vorbis and wav tested in inspector

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

  • ogg: always fine with both speaker and headphones
  • wav: fine with speaker, but small pop in headphones

Notes

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

@ellenhp
Copy link
Contributor

ellenhp commented Feb 8, 2023

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
Copy link
Contributor

ellenhp commented Feb 12, 2023

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
Copy link
Contributor

ellenhp commented Feb 12, 2023

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
Copy link
Contributor Author

MJacred commented Apr 18, 2023

@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
Copy link

ghost commented Apr 21, 2023

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
Copy link
Contributor Author

MJacred commented Apr 21, 2023

@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.

@MJacred MJacred changed the title [Godot 4] Audio pop on looping with loop offset > 0 (affects ogg vorbis & wav; mp3 is fine) [Godot 4] Audio pop on looping with loop offset > 0 Apr 21, 2023
@ghost
Copy link

ghost commented Apr 21, 2023

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
Copy link

ghost commented Apr 26, 2023

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
Copy link
Contributor

fkeyzuwu commented Aug 6, 2023

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
Copy link
Member

Calinou commented Aug 7, 2023

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
Copy link
Contributor

fkeyzuwu commented Aug 7, 2023

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
Copy link

ghost commented Aug 7, 2023

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
Copy link

ghost commented Aug 9, 2023

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
Copy link

ghost commented Aug 9, 2023

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
Copy link

ghost commented Aug 9, 2023

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
Copy link
Contributor

ellenhp commented Oct 13, 2023

Should be fixed by #80452

@ellenhp ellenhp closed this as completed Oct 13, 2023
@ellenhp
Copy link
Contributor

ellenhp commented Oct 14, 2023

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.

@ellenhp ellenhp reopened this Oct 14, 2023
@bs-mwoerner bs-mwoerner linked a pull request Oct 14, 2023 that will close this issue
@bs-mwoerner
Copy link
Contributor

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
Copy link
Contributor Author

MJacred commented Oct 16, 2023

@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

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

Successfully merging a pull request may close this issue.

8 participants