AudioStreamImportSettingsDialog: loop offset field only allows values with up to three decimals, causing a loop point imprecision that sometimes leads to an audible pop when looping

[RustyRoboticsBV]

Tested versions

Reproducable in: v4.1.1.stable.mono.official [bd6af8e], v4.3.dev2.official [3524346]

System information

Godot v4.3.dev2 - Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 1650 (NVIDIA; 30.0.14.7219) - AMD Ryzen 7 4800H with Radeon Graphics (16 Threads)

Issue description

Some of my ogg music files that use the loop offset variable have audible pops whenever they loop. I've done some testing, and the issue appears to be the following: the ogg importer only lets you set loop time offsets in up to three decimals of precision. With the default sample rate of 44,100 Hz, this creates a potential imprecision of 0.0009999999 * 44,100 ≈ 44 samples during a loop.

For example, I have an ogg file where the loop point needs to be at sample 651,323 for a seamless loop. With a sample rate of 44,100 Hz, that gets me a time offset value of 14.7692307692 seconds.
Unfortunately, the import window only allows a time offset of either 14.769s (sample 651,312) or 14.770s (sample 651,357), missing the mark by either 11 or 34 samples respectively.

The result is an audible pop in some songs, which I've confirmed by comparing a recording of the loop to the original file in Audacity. If I manually set the loop point at runtime through AudioStream.Set("loop_offset", <higher_precision_value>), higher-precision values are allowed and the issue disappears. All songs that I've tested seem to loop perfectly after doing this.

Editing the "default float step" editor setting did not fix the problem. Manually editing the .import file in notepad doesn't work either: the value still gets imported with the loop offset rounded to three decimals.

This problem was already mentioned by @strellydev in #80452 and #64775, but I felt it was worth creating a more targeted thread for this issue, as I suspect it's more a resource loading problem than a problem in the ogg implementation itself.

I'm going to investigate the issue and post my findings. If the problem is what I think it is, I propose increasing the precision of the loop offset field to at least five decimals.

Steps to reproduce

1. Add an ogg file to project.
2. Enable looping.
3. Enter a value in loop offset field. The value gets rounded to three decimals.
4. Depending on the music file, audio pops may occur on loop.

Minimal reproduction project (MRP)

N/A

[bs-mwoerner]

My practical advice and what I usually do is: Edit the audio files so that the loop points are at whole second offsets, although that's indeed a bit silly. If this is no longer an issue with the internal implementation after #80452, then a true fix could be as simple as allowing the loop point to be given in samples instead of seconds and then just convert that to the internal 64 bit floating point time value.

Ah, for WAV, the loop point is (only) specified in samples, so there's some inconsistency here anyway.

[RustyRoboticsBV]

Update: The culprit appears to be line 546 of audio_stream_import_settings.cpp: loop_offset->set_step(0.001);

Unfortunately, after changing it to a more precise step, loop offset values still get rounded. However, the decimal position that values get rounded on is now inconsistent, and seems to depend on the number of digits before the decimal point (higher numbers can have less decimals, lower numbers can have more). Smells like casting-induced floating-point imprecision to me, but it might also be that values get rounded somewhere else.

a true fix could be as simple as allowing the loop point to be given in samples instead of seconds and then just convert that to the internal 64 bit floating point time value.
Ah, for WAV, the loop point is (only) specified in samples, so there's some inconsistency here anyway.

I like this suggestion, though I'm a little concerned about breaking existing projects with this fix. Still, I'm going to try and implement it, see if I can get things working.

Perhaps I could look into implementing a toggle of some kind, which would allow users to switch between seconds and sample values? This would preserve existing projects, while still giving more precision when needed. On the plus side, I think it could probably be implemented without even touching AudioStreamOggVorbis, so the importer would handle all the conversions.

[bs-mwoerner]

I like this suggestion, though I'm a little concerned about breaking existing projects with this fix. Still, I'm going to try and implement it, see if I can get things working.

Yes, I was thinking about having the samples input as an additional UI option without changing anything internally, so that compatibility is not an issue. One would probably have to make sure that a seconds value is only applied if it is significantly different from the current internal value, or it would be easy to inadvertently overwrite the current value with a rounded version just by switching formats.

[RustyRoboticsBV]

Update: I think I've gotten things working! The imprecision-induced looping pop seems to be fixed now, for both ogg and mp3 (as the importer dialog is shared between the two).

I'm going to try and figure out how to make a pull request now.

[MJacred]

@RustyRoboticsBV: could you check #87860 to see if it fits your needs, please?

It would have have way fewer changes compared to #87554, which will most likely take quite some discussion with an alternate approach in the end (is what my gut tells me). It removes some internal rounding and provides you enough precision for your use case (provided the (de-)serialization process does not mess it up)

[RustyRoboticsBV]

@RustyRoboticsBV: could you check #87860 to see if it fits your needs, please?

Building your code as we speak!