Fix Ogg/Mp3 import loop point not having single-sample precision

[bs-mwoerner]

Fixes #87427. Possibly also contributes to addressing godotengine/godot-proposals#84.

This is another suggestion for how to address the limited loop point precision in the Audio Stream Importer dialog for Ogg and Mp3 and supplements the ideas in #87554 and #87860. It adds a drop down that switches the unit used for the Offset field between Seconds, Samples, and Beats.

Advantages are:

• The default behavior is unchanged (seconds with millisecond resolution).
• Additionally, it is possible to specify loop points that don't map to whole milliseconds with sample precision or beat precision in combination with a BPM number.
• The loop point position is retained across reimports with sample precision.
• Just changing the displayed unit is non-destructive and will not immediately modify the underlying value unless the value is actually changed.
• No changes have been made to the serialization itself. Instead, the precision is kept by serializing the sample count as a 64 bit integer instead of the seconds as a limited-precision floating point representation. (Serialization and deserialization via the .import file does not retain the full float precision across the conversion to and from a string.)
  • I've changed the loop_offset import option to be named loop_offset_samples to differentiate what method was used for the import. The dialog and the resource importers work with either option but prefer loop_offset_samples if it is present.
• No changes have been made to the actual loop code in the audio streams or the players. These still use seconds at 64 bits resolution. That's not an issue there, because these settings are serialized at full precision in the binary resource.

Issues are:

• This still requires the AudioStream class to expose the sample rate so that the dialog can do the necessary conversions.
• If the BPM number is changed from the default 120, this is not retained across reimports. Currently, any number other than 0 is interpreted as the BPM option being enabled (with 0 being interpreted as 120 BPM and the option being disabled). The actual loop point will be retained, but the displayed "Beats" number will be relative to 120 BPM until the BPM number is reset to what it was during the previous import.
• The display unit (Seconds/Samples/Beats) is not serialized, as it's a merely cosmetic setting, so it didn't feel right to make it part of the import options. As a consequence, the dialog will always display seconds initially and only at millisecond precision (the original precision is retained, however, as can be checked by switching the unit to "Samples").

[AThousandShips]

Did you discuss integrating their PRs into this? Didn't see any discussion on that on the other PRs, wanting to make sure this has been communicated properly, that's the foundation of collaborative work 🙂

[bs-mwoerner]

Did you discuss integrating their PRs into this? Didn't see any discussion on that on the other PRs, wanting to make sure this has been communicated properly, that's the foundation of collaborative work 🙂

I've named them as co-authors in the commit and am in the process of writing comments on those other PRs to ask them to have a look at this one. Not sure if thats the best/most appropriate way to handle this, but I didn't want to just push commits onto their own repositories. I've only had one collaborative PR before, though, so I'd welcome some hints on how one would usually handle these things. 🙂 I'm building on other people's work here and would like this to be understood as a contribution to the discussion. If this PR is giving a different impression, then I did it wrong. 😅

[AThousandShips]

No worries, was mainly that it's best to discuss collaborative work before carrying it out, to not step on anyone's toes or make anyone feel left out or supplanted 🙂 my suggestion would be to simply discuss things with the others and decide together

I haven't compared the code here between the commits, so not saying this is anything of the sort, but for example some might be upset if someone else fixes a relatively small part of their work and makes a new PR, rather than discussing it with them

Simply put in my view the best way to avoid any confusion or conflict is to discuss first, but to make clear, not any kind of criticism of the way you acted here

[bs-mwoerner]

Yes, that makes a lot of sense. I suppose it's a bit of an edge case, because while I didn't consciously copy any code, the ideas are of course similar, since they come from the same discussion. @RustyRoboticsBV, if you prefer, I'd be happy to merge these changes into your PR. I'd just like you to have a chance to look at them first. I'd say the most relevant differences are:

• I'm supporting three units instead of just seconds and sample count. While I always complained about not being able to enter a sample count myself, the unit I start with is usually a beat number, and since the dialog already contains a "BPM" input, I thought I could make life even easier for myself by not even having to convert the beats to samples.
• I'm doing automatic conversions when the unit changes, so a value of "1" in "Seconds" mode will turn into a "44100" (or whatever the samplerate is) in "Samples" mode and a "2" in "Beats" mode (for 120 BPM).
• I'm mapping all that to a new internal sample count field that stores the current value while the dialog is open and only updates it when there's a significant change to the input field. This is so that no precision is lost unnecessarily when converting between units.
• Instead of serializing to value and unit (or value and use samples flag), I'm just serializing to a sample value (as an integer). Without the unit, I'll have to default back to seconds when opening the dialog again, but since my "unit" is a purely visual option, I thought it may not be correct to include it in the "import options" when it has no effect on the actual import. To be compatible with previous versions, I've changed the name of the option so that the dialog and the importers know what format they're dealing with. (The dialog now converts an existing seconds offset to a sample count internally during initialization.) The integer should now retain the full precision for a later reimport.
• The get_sample_rate() methods are mostly the same as in your PR, although I couldn't resist changing their return type from float to int. It's true that both the Ogg and the Mp3 stream store the sample rate as a float internally, but both of them originally get it as an int from their underlying libraries and I'm just generally suspicious of functions that insist on returning values that are inherently integral as a floating point value...

Actually, a question to the room at large: Is renaming an import option a breaking change (when the previous option is still supported but no longer listed)?

[RustyRoboticsBV]

My toes aren't stepped on, no worries!

Conceptually I'm a big fan of these changes (I was tinkering on something very similar actually, so we're pretty much on the same page here). I'm going to properly check this PR out tomorrow and see if I can find any issues. But at a glance, I very much support this PR. I'll leave mine open for now until this gets merged.

[RustyRoboticsBV]

Ok, I've given the new importer a spin, and it's looking good! A couple of thoughts:

I noticed that the loop_offset field still has a step value of 0.001 when in seconds mode. I'd prefer if this was changed to 0.00001, so that we can target specific samples in seconds mode. This isn't strictly necessary, but I think the user experience would be a little nicer this way.

I also noticed a small bug with the way the loop_offset field detects if its value change was large enough: it sometimes ignores value changes of 0.001 seconds. I think the problem is at line 486; I vaguely remember that the >= operator can sometimes return false unexpectedly on floats (due to float imprecision shenanigans). I suggest adding a small bias here to avoid this (if (ABS(current_loop_offset - new_loop_offset) >= loop_offset->get_step() * 0.9) seemed to work consistently for me).

Lastly, I found a bug where the advanced editor doesn't pass the loop point to the playback stream until the loop_offset is changed at least once. If the stream is then played to its end, it loops from the beginning instead of the desired loop point. If the loop_offset field is assigned a new value, this makes the problem go away until the window is closed again. This seems to happen because we now only set the stream's loop offset if we detect a large-enough value difference; adding something like stream->call("set_loop_offset", (double)_loop_offset_samples / stream->get_sampling_rate()); in the edit method fixes this.

Oh, and the class DB is complaining in the console about AudioOggVorbis::get_sampling_rate. I think it gets added twice? I don't have any experience with class DB stuff, so I dunno what that means.

But overall: great work!

[bs-mwoerner]

Thanks for the detailed feedback!

I noticed that the loop_offset field still has a step value of 0.001 when in seconds mode. I'd prefer if this was changed to 0.00001, so that we can target specific samples in seconds mode.

Yes, I feel that's still a bit of a shortcoming, too. I left it at 0.001 just for the sake of consistency with the previous user experience, but one thing I really liked about MJacred's PR was that you could just paste whatever seconds value you had in the clipboard from where you did your calculations while still having the option to step in millisecond intervals if you wanted to. The way it is now, you have to use samples if you need more than millisecond precision. Not sure if it's feasible to ask for a change to a basic GUI control for a PR like this. Just changing the step value to something finer would be an alternative that doesn't require changes outside of the dialog itself.

I also noticed a small bug with the way the loop_offset field detects if its value change was large enough

Good catch! I had a feeling that piece of code was a bit dodgy when I committed it. Looking at it now, maybe I should do it the other way around anyway: Convert the internal value to the unit, then round the result according to the step value, then check if that number matches what's already in the field. If so, don't update the internal value. That way, we could use a very generous epsilon of half a step without fear of clamping too eagerly.

Lastly, I found a bug where the advanced editor doesn't pass the loop point to the playback stream until the loop_offset is changed at least once.

Ah, yes, the initial change notifications are probably not enough, because the stream needs to be initialized once, even though the internal and the displayed value initially already match. Makes sense, thanks!

Oh, and the class DB is complaining in the console about AudioOggVorbis::get_sampling_rate.

Do you get that on the very latest commit? I've spend some time last night updating the ClassDB statements until the code checks were happy and for me, these errors are gone now. Maybe these issues are already fixed if you pull again?

[lyuma]

Not sure if it's feasible to ask for a change to a basic GUI control for a PR like this.

When it comes to one of my biggest pet peeves with Godot, I would be thrilled if someone were to propose a solution to the underlying control.

Inspector rounding being tied to step size is highly frustrating to me. If I paste or type in a value manually, the control should not round. The fact that Godot rounds displayed data and manually-input data is wrong, except possibly in the case of step=1 to ensure values are integers

the other thing is it would be nice for the control to display the value in full precision regardless of step size, but if it is == equal to the rounded value, we should make sure it displays the rounded value. This way, a user using the slider will never see the extra precision but it can still be typed in manually and displayed.

If you'd like, I can type this into its own proposal

[bs-mwoerner]

I've now updated the commit with these fixes:

• The step size is now 0.00001 as suggested, which is enough to address single samples at 48.000 Hz.
• The loop offset is now initialized correctly upon reopening the dialog.
• The comparison should now be floating point safe. Turns out I already rounded when doing the conversion, so it's just a matter of adding any epsilon not greater than the step size (and for some reason I had picked exactly the step size. Living on the edge.) The condition is now
  if (ABS(current_loop_offset - new_loop_offset) >= loop_offset->get_step() * 0.5)

If you'd like, I can type this into its own proposal

I think that'd be great! Then we can separate these topics and later integrate the updated control into this dialog to make things a bit nicer. Blender seems to use three different precisions: display precision, value precision, and step precision. Different input fields use different values, but for a scale factor, for instance, if the field contains the value 1.23456, then

• The field shows "1.235" when it doesn't have the input focus.
• The field shows "1.23456" when it has the input focus.
• The number changes to "1.245" when the increment button is clicked (shown as "1.24456" when the field is focused).

It's possible that the actual value is never rounded in Blender, so that second precision may just always be the max precision of the data type.

[RustyRoboticsBV]

Do you get that on the very latest commit? I've spend some time last night updating the ClassDB statements until the code checks were happy and for me, these errors are gone now. Maybe these issues are already fixed if you pull again?

Yeah, turns out it was an older version. It's indeed gone on the latest version.

[Calinou]

Suggested change

	<method name="_get_sampling_rate" qualifiers="virtual const">
	<return type="int" />
	<description>
	</description>
	</method>
	<method name="_get_sampling_rate" qualifiers="virtual const">
	<return type="int" />
	<description>
	Return the sampling rate of the audio stream in samples per second.
	</description>
	</method>

[Mickeon]

Although #86958 has yet to be merged, if you want to be consistent:

Suggested change

	<method name="_get_sampling_rate" qualifiers="virtual const">
	<return type="int" />
	<description>
	</description>
	</method>
	<method name="_get_sampling_rate" qualifiers="virtual const">
	<return type="int" />
	<description>
	Overridable method. Should return the sampling rate of this audio stream, in samples per second.
	</description>
	</method>

[reduz]

Heyo, I am most likely going to favor my own solution for this:
#86517

As you can't really rely on sample precision for looping these types of files properly given their nature. This also does a small cross fade for the loop, so its much easier for users to do the actual looping without having to use sample level precision.

[reduz]

To clarify, it is entirely intended that OGG/MP3 have one type of looping, and WAV has another. Both file types are used for different purposes and there is no interest in unification.

[bs-mwoerner]

Oh, I hadn't checked for previous pull requests on this topic, sorry about that. Yes, that should already solve most of these cases. I'm not quite sure how we'd then handle variable-tempo music, but that may be a bit of a non-standard case (although I do have one of those in my project). In any case, the previous workaround of baking parts of the loop into the file until the loop point aligns with a millisecond would still work for those.

If there are no objections, I'll close this in favor of #86517.

[MJacred]

@bs-mwoerner: Sorry that I couldn't get to your PR sooner. Personally, I'm in favour of saving loop point as samples (int), and giving the user an option to calculate it from seconds (incorporating my PR change to increase the accuracy a bit + setting step to that accuracy (6 places after the dot, iirc)). EDIT: using your dropdown logic seems to fit that purpose

Regarding @reduz' PR, as I stated here, I don't see how his code affects non-BPM looping. If there is some code that enables it for "normal" looping, then I can't find it.

This

godot/modules/vorbis/audio_stream_ogg_vorbis.cpp

Line 120 in 89b20b8

loop_fade_remaining = 0;

is the only line I can find that triggers this fading. And it's limited to BPM, because this line

godot/modules/vorbis/audio_stream_ogg_vorbis.cpp

Line 81 in 89b20b8

if (beat_length_frames >= 0) {

prevents the if-clause from being triggered for "normal" looping (because beat_length_frames equals -1).

If the fading @reduz mentioned is done in AudioServer (or if I'm blind), then nevermind this objection. But I'm still in favour of samples.

[bs-mwoerner]

No worries and thanks for taking a look! As I understand it, the idea is that accurate looping is only required for music and most music uses a fixed tempo. For these cases, you can identify the loop point with sample accuracy if you combine the bpm value and the beat count. Since these are stored as two separate values, the limited serialization precision for floats should not be a factor. For example, a loop point at beat 8 at 134 bpm would be at 3.58208955... seconds, which you can't enter into the field and which would be truncated during saving, but you can enter 134 and 8 separately, which can both be safely stored and loaded and which will later be combined to give the precise value.

The fade, I think, is only intended to happen when you make use of the option to loop before the end of the file (by specifying a corresponding bpm and beat number). The stream would then play a bit beyond the end of the loop while fading back into the start of the loop.

If the file was produced to be loopable, then most likely the file end is the loop end and no fade is done. The only problem is with files that don't have a constant bpm (sound effects or variable tempo music). For these cases, you'd either have to edit the file to align the loop point with a position that can be entered into the dialog or set the loop point manually in a script after the import.

I still like your full-precision Range modification, although this probably only makes sense in combination with some modification that makes sure that the value survives until a possible reimport (such as internally switching to integer, not sure if there are other ways).

[MJacred]

@bs-mwoerner: That's how I understood his PR as well, that's why I requested a change of its title here. Someone who wants to loop (with a loop offset), but does not want to use BPM logic, won't benefit from that PR at all, and therefore it's no replacement for this PR.

I still like your full-precision Range modification, although this probably only makes sense in combination with some modification that makes sure that the value survives until a possible reimport (such as internally switching to integer, not sure if there are other ways).

Indeed, saving as sample (int), the range modification, and double (de-)serializing should be discussed further. The latter two can be handled in a different place (rocket chat / godot-proposal / in my PR).

[reduz]

@MJacred Well, if you are looping non-music (like SFX) then there is not much of a point in having sample precise looping as long as the stream does a quick fade.

Looping audio files to sample precision to avoid clicks is a horrible experience, only really justified if you are doing it for instrument samples and the likes, which is not something you will do with MP3/OGG. WAV is far more suited for it.

[reduz]

As said, will make the fade also work with non BPM looping btw.

[MJacred]

@reduz:

Looping audio files to sample precision to avoid clicks is a horrible experience

Ok, I don't have that kind of experience so I can't comment on that. I know that Audacity can pull off seamless looping with loop offset, but I have no idea what they do under the hood to make it work (which may not be feasible for game engines or may be exactly what you do).
My main reason for pushing for samples is because in the end we derive a sample number from the loop offset double anyway. But if you also add the fade for non BPM looping, then there is no necessity for saving as samples anymore.

I'll gladly test looping for non BPM looping.

Building on that, I'd refactor #87860 to just "Use double for loop_offset consistently" and increase steps from 0.001 to 0.0001 (that precision should still survive). Or do you want to incorporate that into your #86517? And perhaps it is possible to preserve more decimal places for double when (de-)serializing?

[bs-mwoerner]

I've played around with this a bit and think using approximate loop points combined with the fade sounds the same as sample-accurate looping, so supporting sample counts should not be necessary.

Here's a loop using sample counts and no fade:

SamplesWithoutFade.mp4

I can convert this into the bpm system by

• Baking a bit of the start of the loop into the file (so that the fade has something to work with).
• Converting the previous length of the file into an average tempo and beat number.
• Using an approximate (milli)second value for the loop start.

This is the loop with bpm fade:

BeatsWithFade.mp4

The timing being off by a millisecond or two is not noticeable and the fade glosses over any discontinuities in the wave form, so I think this is just fine. For constant tempo files, you don't even have to work out the average tempo.

[Zireael07]

Why close?

[bs-mwoerner]

Why close?

Oh, sorry, I thought the rationale was clear from the recent comments or I would have added an explanation: reduz's existing PR #86517 fades over cracks around loop points, so if that's implemented (especially if made to work with non-BPM looping), there's probably no good reason to change the internal data structures to accommodate sample-accurate loop points. And since there weren't any unresolved objections as far as I can tell, I thought it was time to close this PR in favor of #86517.