Fix OGG audio loop offset pop

[ghost]

This fixes #75762, fixes #59348 and parts of #64775

Fixed the mixing function so it doesn't skip the last ogg packet of the file, and also made it so the burning algorithm only burns the needed samples from the last packet, and not the whole packet.

There are still some issues, if you download the example project from the issue, and you try the audio-pop-no-intro.ogg file, you'll see that there's still a pop, this is because for some reason, the engine is missing 38 samples, and not at the end of the file like the error originally was.

Another problem that became aparent is the decimal precision of the loop offset variable, in a song of mine which has a loop point at 31.46, the loop offset becomes 31.459991 or something like that, which introduces a tiny pop (this does not happen in Godot 3). If I manually change that value to 31.4600010, the pop disappears and the song plays like it does in Godot 3. So that's another problem somewhere else.

THIS DOES NOT FIX THE WAV POP.

However, it seems to have fixed the mp3 pop I was getting, so I guess it could have been a simple floating point precision error, but more testing is probably needed.

I'll keep looking into it.

If you know why the 38 samples from the 0 seconds offset loop could be going missing please let me know.

Thanks.

[AThousandShips]

Thank you for contributing, some style checks

[AThousandShips]

What happens below if the packet isn't loaded correctly?

[AThousandShips]

Please update your commit message to be more brief and descriptive, like the PR title, you can add the description in the body of the message

I'll keep looking into it.

If you're not done with this please mark it as draft, top right under the reviews

[ghost]

Thanks, I fixed the comments, I was using them to find my changes and forgot to remove the name, also thanks for pointing out the commented return as well.

I will make this into a draft. Sorry for the inconvenience.

[AThousandShips]

I will make this into a draft. Sorry for the inconvenience.

No problem!

[ghost]

Oh no, I renamed the commits and somehow an older commit made it's way into this, uh... is there a way I can fix this?

[AThousandShips]

See here

[ghost]

All fixed, thank you again!

[ghost]

Hello again, I fixed the issue with the offset being at 0, it didn't have to do with the offset being at 0 at all, I was just checking if the samples left in the last packet were greater than the needed samples by the mixer, instead of checking if they were greater than 0 like I should have been, I think this is ready for review.

Edit: I'm leaving the decimal precision out of the question because I don't know if that's part of the audio system, or a general thing, I honestly can't think of a reason on why it changes behavior between godot 3 and 4.

[ghost]

Alright, everything should be correctly styled now, and I also realized that loop_offset was a float even though the getter returns a double and the seek function receives a double as well, so I changed it to double and that also fixed the decimal point precision issue, which means ogg now fully functions the same as Godot 3 (Hopefully)

Thanks for taking the time to review this.

[AThousandShips]

Does it now completely fix the issue you referenced?

[ghost]

It completely fixes the OGG part, however, the issue also talks about WAV and MP3 having pops as well. This pull request doesn't touch any of that.

[MJacred]

First of all: Thank you so much for tackling this!

---

EDIT: ok… as both files had the same sample count it struck me as really weird. So I took that file with the audio pop, imported it into Audacity and exported it as-is… The pop was gone. Will test with the rest of the files and leave a second comment with the result!
What is even weirder: Audacity could loop it perfectly inside itself, so it might do some auto-corrections on import??

---

I tested with couple of files (all loop offsets > 0 and at full seconds). And I sometimes still get pops…

I have 3 files that are exactly 92 secs. They work fine.
Then I tested two files with 67.333 secs (and same sample count). The loop offset was exactly 6 secs for both. And only one popped.
Thus, I cut that popping file in Audacity to a total length of 5.5 secs. While the loop offset changed, it was still at 1.5 secs. No audio pop.
Therefore, I reckon there is still something wrong with the sample calculation. Especially because…

I got sometimes the warning

modules/vorbis/audio_stream_ogg_vorbis.cpp:340 - Burning negative samples doesn't make sense. Check seek algorithm.

when

• I double-click the file in the Godot Editor and it opens the importer dialog,
• I seek with the mouse manually
• the file loops <<< this happened every time for that file I cut in Audacity, but not for the others

And I had some file drop issues in the editor (Failed to open [filename].ogg, no further info). But that's something else

[ghost]

The burning negative samples warning is something that has me weirded out as well.

I was sure that to calculate the samples to burn I could substract the previous page granule from the desired granule position, which would give us the amount of "granule" to burn in the current page. Then, we'd substract the previous granule position from the current page granule position, which would give us the granule size of the page, and then use that size to compare it to the burned samples.

Doing all of that actually gets rid of the warning, so it never tries to burn negative samples, however, it seems that calculating samples_to_burn that way is wrong, because it leads to popping.

And that doesn't make any sense to me. But doing the current calculation seems to work with the caveat of giving the warning when manually seeking through the file.

I could go in and try again with the desired granule - last granule formula. Maybe I did something wrong the first time I tried it.

This is my first time working with audio, so trying to navigate all the calculations is a bit tricky,

Thanks for testing this out, I will await your next comment!

EDIT:
Ah! I found out what's going on, the samples_in_page value is missing 1024 samples, because the first packet is returning 0 samples_out for some reason.

So if the seeked sample is on the last 1024 samples of the page, it will think it has to burn negative samples, which will later cause a pop if it so happens that the loop point coincides with the warning happening. That could explain the pop you were having.

I do not understand why samples_out is 0 with the first packet, but if I set samples_out to 1024 when it's 0 in both seeking and burning loops,, the warning disappears and no pops are introduced.

However, hard coding an extra 1024 samples seems hacky, so I'll try to figure out why it's skipping the first one.

Cheers

[ghost]

Is there ANY chance that the bug could come from the third party ogg vorbis code?

Because when we do vorbis_synthesis_restart(), the dsp_state->pcm_returned is set to -1, which, later, when reading the first packet after the restart, it checks if pcm_returned is -1, and if it is, it sets pcm_returned to the same value as pcm_current, which basically makes it act as if those first 1024 samples were already read., thus, making samples_out be 0 when calling vorbis_synthesis_pcmout() later.

If I do the change shown in the image in the file thirdparty/libvorbis/block.c, everything works fine. I didn't find anything breaking, but I don't know if changing that would have an effect on a specific scenario, or whatever. It's hard to tell.

[MJacred]

@ellenhp: Before you do the final approve, please let me test through my files as well. I'll do so this night and report back.
I can already confirm that 3 popping files are playing just fine after the most recent changes.

I need some tea, there are several dozen files...

[ellenhp]

@MJacred Sure thing.

[MJacred]

@ellenhp: ok, I'm done.

So, my result is: Only one file was popping. But it also popped in Audacity. And only at a rather high volume output. Therefore a file issue.
I tested on both speakers (stereo) and audio jack headphones (yeah, sometimes it pops in one but not the other. Or at least the pops are more audible in one or the other).
Though I must say all my files have a loop offset > 0, so I didn't test looping to beginning.

The final verdict from my end: the looping is so smooth, you can slip on it.

Now I want to celebrate. All of us have earned some celebration 🥳 I'll sleep well tonight

---

[…] is exactly what I ended up doing

@bs-mwoerner: Haha. Kinda glad I narrowed down on the solution over time and didn't get lost. And thank you again for your contribution!

[ghost]

Imma dance

[ellenhp]

@akien-mga I think it's worth merging this despite the freeze. I'll let you make that call though.

@strellydev Thank you for your hard work tracking down this bug, isolating the problem areas and coming up with partial fixes. Also for your patience with git and all the nit comments I left.
@MJacred Thank you for the test files, help tracking down the issue and ultimately proposing a working solution.
@bs-mwoerner Thank you for getting this across the finish line.

I'm so glad this is fixed. I feel like I can breathe again. It's been a source of anxiety for me for years.

[AThousandShips]

Unless this is considered an enhancement or a new feature it's unaffected by freeze, unless it's a risky fix that we want to test for longer I don't see any problem merging for 4.2

[akien-mga]

Amazing teamwork everyone!

[akien-mga]

Thanks everyone, and congrats @strellydev and @bs-mwoerner for your first merged Godot contribution 🎉

[ellenhp]

@akien-mga just to make sure you don't miss it for release notes: there are importer changes here so users will (potentially? needs testing) have to re-import ogg files to get the full effect of this fix.

[nikitalita]

This change can cause an infinite loop since it causes the function to NEVER return false. After we reach EOS, the packet_cursor is never incremented, so the packet_cursor always stays pointed to the last packet. This causes the function to keep setting p_packet to the last packet and returning false forever.

[bs-mwoerner]

That may be a bit unintuitive yes. Normally, callers check the e_o_s flag and are supposed to stop reading at the end of the stream. If they don't, then they would have previously gotten false and logged an error, whereas now, they get true and the last packet again. This doesn't make a difference as long as callers correctly check the e_o_s flag, but you might argue that the previous way of erroring out when trying to read past the end of the stream was the safer route.

I think this change wasn't directly related to the fix but rather one of a few modifications made in an attempt to narrow down the original problem.

[nikitalita]

Returning a boolean value instead of an error makes it look like one of those classic reader functions that you put in the condition of a while loop and keep going until it returns false; the fact that it doesn't behave that way is very confusing, and I ended up banging my head against the wall for a couple of hours trying to figure out why I kept reading after the EOS.

In either case, I've tested removing this check with the example projects for the issues that this PR is purported to have fixed, and they work fine.

[ellenhp]

Can you provide an ogg/vorbis file that causes this issue? Here or in the comments of #85996

[nikitalita]

It's not really a problem with any particular ogg file, it's an issue with this being part of the public API and it having confusing behavior. It's also semantically incorrect, since there's no case where this function will actually return false.

When something other than godot uses this function (e.g a custom module) and expects the next_ogg_packet() to return false when there's no more packets to be read (e.g. while (playback->next_ogg_packet(&pkt)), this can cause an infinite loop.

[ellenhp]

Okay, you mentioned an infinite loop so I assumed it was a reproducible case rather than an API issue. Sorry for misunderstanding.

[YuriSizov]

Unfortunately cannot be cleanly cherry-picked because it's based on the codebase which includes #78084. A dedicated PR for 4.1 is welcome, if we want to fix this issue in that branch.