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.

Sign up for GitHub

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

Jump to bottom

Fix for: ogg importer dialog rounds loop offset values to 3 digits, introducing a sampling imprecision that can lead to an audible pop on loop. #87554

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+79 −12
Open

Fix for: ogg importer dialog rounds loop offset values to 3 digits, introducing a sampling imprecision that can lead to an audible pop on loop. #87554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 24 additions & 8 deletions editor/import/audio_stream_import_settings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,7 @@ void AudioStreamImportSettingsDialog::edit(const String &p_path, const String &p
beats_enabled->set_pressed(beats > 0);
loop->set_pressed(config_file->get_value("params", "loop", false));
loop_offset->set_value(config_file->get_value("params", "loop_offset", 0));
loop_use_samples->set_pressed(config_file->get_value("params", "loop_use_samples", 0));
bar_beats_edit->set_value(config_file->get_value("params", "bar_beats", 4));

List<String> keys;
Expand All @@ -453,6 +454,7 @@ void AudioStreamImportSettingsDialog::edit(const String &p_path, const String &p
bar_beats_edit->set_value(4);
loop->set_pressed(false);
loop_offset->set_value(0);
loop_use_samples->set_pressed(false);
}

_preview_zoom_reset();
Expand All @@ -471,13 +473,20 @@ void AudioStreamImportSettingsDialog::_settings_changed() {

updating_settings = true;
stream->call("set_loop", loop->is_pressed());
stream->call("set_loop_offset", loop_offset->get_value());
if (loop->is_pressed()) {
loop_offset->set_editable(true);

if (loop_use_samples->is_pressed()) {
stream->call("set_loop_offset", stream->sample_to_seconds(loop_offset->get_value()));
loop_offset->set_suffix("");
} else {
loop_offset->set_editable(false);
stream->call("set_loop_offset", loop_offset->get_value());
loop_offset->set_suffix("sec");
}

bool loop_is_pressed = loop->is_pressed();
loop_offset_label->set_visible(loop_is_pressed);
loop_offset->set_visible(loop_is_pressed);
loop_use_samples->set_visible(loop_is_pressed);

if (bpm_enabled->is_pressed()) {
stream->call("set_bpm", bpm_edit->get_value());
beats_enabled->set_disabled(false);
Expand Down Expand Up @@ -518,6 +527,7 @@ void AudioStreamImportSettingsDialog::_settings_changed() {
void AudioStreamImportSettingsDialog::_reimport() {
params["loop"] = loop->is_pressed();
params["loop_offset"] = loop_offset->get_value();
params["loop_use_samples"] = loop_use_samples->is_pressed();
params["bpm"] = bpm_enabled->is_pressed() ? double(bpm_edit->get_value()) : double(0);
params["beat_count"] = (bpm_enabled->is_pressed() && beats_enabled->is_pressed()) ? int(beats_edit->get_value()) : int(0);
params["bar_beats"] = (bpm_enabled->is_pressed()) ? int(bar_beats_edit->get_value()) : int(4);
Expand All @@ -540,14 +550,20 @@ AudioStreamImportSettingsDialog::AudioStreamImportSettingsDialog() {
loop->connect("toggled", callable_mp(this, &AudioStreamImportSettingsDialog::_settings_changed).unbind(1));
loop_hb->add_child(loop);
loop_hb->add_spacer();
loop_hb->add_child(memnew(Label(TTR("Offset:"))));
loop_offset_label = memnew(Label(TTR("Offset:")));
loop_hb->add_child(loop_offset_label);
loop_offset = memnew(SpinBox);
loop_offset->set_max(10000);
loop_offset->set_step(0.001);
loop_offset->set_suffix("sec");
loop_offset->set_custom_minimum_size(loop_offset->get_custom_minimum_size() + Size2(128, 0));
loop_offset->set_max(999999999);
loop_offset->set_step(0.00001);
loop_offset->set_tooltip_text(TTR("Loop offset (from beginning). Note that if BPM is set, this setting will be ignored."));
loop_offset->connect("value_changed", callable_mp(this, &AudioStreamImportSettingsDialog::_settings_changed).unbind(1));
loop_hb->add_child(loop_offset);
loop_use_samples = memnew(CheckBox);
loop_use_samples->set_text(TTR("Use Samples?"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
loop_use_samples->set_text(TTR("Use Samples?"));
loop_use_samples->set_text(TTR("Use Samples"));

Other check boxes here don't use a question mark, nor do they seem to do so elsewhere

loop_use_samples->set_tooltip_text(TTR("When enabled, the loop offset will be defined in samples instead of in seconds."));
loop_use_samples->connect("toggled", callable_mp(this, &AudioStreamImportSettingsDialog::_settings_changed).unbind(1));
loop_hb->add_child(loop_use_samples);
main_vbox->add_margin_child(TTR("Loop:"), loop_hb);

HBoxContainer *interactive_hb = memnew(HBoxContainer);
Expand Down
2 changes: 2 additions & 0 deletions editor/import/audio_stream_import_settings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,9 @@ class AudioStreamImportSettingsDialog : public ConfirmationDialog {
Label *bar_beats_label = nullptr;
SpinBox *bar_beats_edit = nullptr;
CheckBox *loop = nullptr;
Label *loop_offset_label = nullptr;
SpinBox *loop_offset = nullptr;
CheckBox *loop_use_samples = nullptr;
ColorRect *color_rect = nullptr;
Ref<AudioStream> stream;
AudioStreamPlayer *_player = nullptr;
Expand Down
10 changes: 7 additions & 3 deletions modules/minimp3/audio_stream_mp3.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ int AudioStreamPlaybackMP3::_mix_internal(AudioFrame *p_buffer, int p_frames) {

bool beat_loop = use_loop && mp3_stream->get_bpm() > 0 && mp3_stream->get_beat_count() > 0;
if (beat_loop) {
beat_length_frames = mp3_stream->get_beat_count() * mp3_stream->sample_rate * 60 / mp3_stream->get_bpm();
beat_length_frames = mp3_stream->get_beat_count() * mp3_stream->get_sample_rate() * 60 / mp3_stream->get_bpm();
}

while (todo && active) {
Expand Down Expand Up @@ -125,7 +125,7 @@ int AudioStreamPlaybackMP3::get_loop_count() const {
}

double AudioStreamPlaybackMP3::get_playback_position() const {
return double(frames_mixed) / mp3_stream->sample_rate;
return double(frames_mixed) / mp3_stream->get_sample_rate();
}

void AudioStreamPlaybackMP3::seek(double p_time) {
Expand All @@ -137,7 +137,7 @@ void AudioStreamPlaybackMP3::seek(double p_time) {
p_time = 0;
}

frames_mixed = uint32_t(mp3_stream->sample_rate * p_time);
frames_mixed = uint32_t(mp3_stream->get_sample_rate() * p_time);
mp3dec_ex_seek(mp3d, (uint64_t)frames_mixed * mp3_stream->channels);
}

Expand Down Expand Up @@ -253,6 +253,10 @@ bool AudioStreamMP3::is_monophonic() const {
return false;
}

float AudioStreamMP3::get_sample_rate() const {
return sample_rate;
}

void AudioStreamMP3::get_parameter_list(List<Parameter> *r_parameters) {
r_parameters->push_back(Parameter(PropertyInfo(Variant::BOOL, "looping", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_DEFAULT | PROPERTY_USAGE_CHECKABLE), Variant()));
}
Expand Down
2 changes: 2 additions & 0 deletions modules/minimp3/audio_stream_mp3.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ class AudioStreamMP3 : public AudioStream {

virtual bool is_monophonic() const override;

virtual float get_sample_rate() const override;

virtual void get_parameter_list(List<Parameter> *r_parameters) override;

AudioStreamMP3();
Expand Down
3 changes: 3 additions & 0 deletions modules/minimp3/doc_classes/ResourceImporterMP3.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,5 +33,8 @@
Only has an effect if [member loop] is [code]true[/code].
A more convenient editor for [member loop_offset] is provided in the [b]Advanced Import Settings[/b] dialog, as it lets you preview your changes without having to reimport the audio.
</member>
<member name="loop_use_samples" type="bool" setter="" getter="" default="false">
If enabled, [member loop_offset] will be interpreted as being defined in samples, instead of in seconds. This allows for greater precision, but will often not be needed. Use this if you notice a pop whenever your audio loops.
</member>
</members>
</class>
9 changes: 8 additions & 1 deletion modules/minimp3/resource_importer_mp3.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ String ResourceImporterMP3::get_preset_name(int p_idx) const {
void ResourceImporterMP3::get_import_options(const String &p_path, List<ImportOption> *r_options, int p_preset) const {
r_options->push_back(ImportOption(PropertyInfo(Variant::BOOL, "loop"), false));
r_options->push_back(ImportOption(PropertyInfo(Variant::FLOAT, "loop_offset"), 0));
r_options->push_back(ImportOption(PropertyInfo(Variant::BOOL, "loop_use_samples"), false));
r_options->push_back(ImportOption(PropertyInfo(Variant::FLOAT, "bpm", PROPERTY_HINT_RANGE, "0,400,0.01,or_greater"), 0));
r_options->push_back(ImportOption(PropertyInfo(Variant::INT, "beat_count", PROPERTY_HINT_RANGE, "0,512,or_greater"), 0));
r_options->push_back(ImportOption(PropertyInfo(Variant::INT, "bar_beats", PROPERTY_HINT_RANGE, "2,32,or_greater"), 4));
Expand Down Expand Up @@ -117,7 +118,8 @@ Ref<AudioStreamMP3> ResourceImporterMP3::import_mp3(const String &p_path) {

Error ResourceImporterMP3::import(const String &p_source_file, const String &p_save_path, const HashMap<StringName, Variant> &p_options, List<String> *r_platform_variants, List<String> *r_gen_files, Variant *r_metadata) {
bool loop = p_options["loop"];
float loop_offset = p_options["loop_offset"];
double loop_offset = p_options["loop_offset"];
bool loop_use_samples = p_options["loop_use_samples"];
double bpm = p_options["bpm"];
float beat_count = p_options["beat_count"];
float bar_beats = p_options["bar_beats"];
Expand All @@ -126,6 +128,11 @@ Error ResourceImporterMP3::import(const String &p_source_file, const String &p_s
if (mp3_stream.is_null()) {
return ERR_CANT_OPEN;
}

if (loop_use_samples) {
loop_offset = mp3_stream->sample_to_seconds(loop_offset);
}

mp3_stream->set_loop(loop);
mp3_stream->set_loop_offset(loop_offset);
mp3_stream->set_bpm(bpm);
Expand Down
4 changes: 4 additions & 0 deletions modules/vorbis/audio_stream_ogg_vorbis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,10 @@ bool AudioStreamOggVorbis::is_monophonic() const {
return false;
}

float AudioStreamOggVorbis::get_sample_rate() const {
return get_packet_sequence()->get_sampling_rate();
}

void AudioStreamOggVorbis::get_parameter_list(List<Parameter> *r_parameters) {
r_parameters->push_back(Parameter(PropertyInfo(Variant::BOOL, "looping", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_DEFAULT | PROPERTY_USAGE_CHECKABLE), Variant()));
}
Expand Down
2 changes: 2 additions & 0 deletions modules/vorbis/audio_stream_ogg_vorbis.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,8 @@ class AudioStreamOggVorbis : public AudioStream {

virtual bool is_monophonic() const override;

virtual float get_sample_rate() const override;

virtual void get_parameter_list(List<Parameter> *r_parameters) override;

AudioStreamOggVorbis();
Expand Down
3 changes: 3 additions & 0 deletions modules/vorbis/doc_classes/ResourceImporterOggVorbis.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,5 +49,8 @@
Only has an effect if [member loop] is [code]true[/code].
A more convenient editor for [member loop_offset] is provided in the [b]Advanced Import Settings[/b] dialog, as it lets you preview your changes without having to reimport the audio.
</member>
<member name="loop_use_samples" type="bool" setter="" getter="" default="false">
If enabled, [member loop_offset] will be interpreted as being defined in samples, instead of in seconds. This allows for greater precision, but will often not be needed. Use this if you notice a pop whenever your audio loops.
</member>
</members>
</class>
6 changes: 6 additions & 0 deletions modules/vorbis/resource_importer_ogg_vorbis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ String ResourceImporterOggVorbis::get_preset_name(int p_idx) const {
void ResourceImporterOggVorbis::get_import_options(const String &p_path, List<ImportOption> *r_options, int p_preset) const {
r_options->push_back(ImportOption(PropertyInfo(Variant::BOOL, "loop"), false));
r_options->push_back(ImportOption(PropertyInfo(Variant::FLOAT, "loop_offset"), 0));
r_options->push_back(ImportOption(PropertyInfo(Variant::BOOL, "loop_use_samples"), false));
r_options->push_back(ImportOption(PropertyInfo(Variant::FLOAT, "bpm", PROPERTY_HINT_RANGE, "0,400,0.01,or_greater"), 0));
r_options->push_back(ImportOption(PropertyInfo(Variant::INT, "beat_count", PROPERTY_HINT_RANGE, "0,512,or_greater"), 0));
r_options->push_back(ImportOption(PropertyInfo(Variant::INT, "bar_beats", PROPERTY_HINT_RANGE, "2,32,or_greater"), 4));
Expand All @@ -98,6 +99,7 @@ void ResourceImporterOggVorbis::show_advanced_options(const String &p_path) {
Error ResourceImporterOggVorbis::import(const String &p_source_file, const String &p_save_path, const HashMap<StringName, Variant> &p_options, List<String> *r_platform_variants, List<String> *r_gen_files, Variant *r_metadata) {
bool loop = p_options["loop"];
double loop_offset = p_options["loop_offset"];
bool loop_use_samples = p_options["loop_use_samples"];
double bpm = p_options["bpm"];
int beat_count = p_options["beat_count"];
int bar_beats = p_options["bar_beats"];
Expand All @@ -107,6 +109,10 @@ Error ResourceImporterOggVorbis::import(const String &p_source_file, const Strin
return ERR_CANT_OPEN;
}

if (loop_use_samples) {
loop_offset = ogg_vorbis_stream->sample_to_seconds(loop_offset);
}

ogg_vorbis_stream->set_loop(loop);
ogg_vorbis_stream->set_loop_offset(loop_offset);
ogg_vorbis_stream->set_bpm(bpm);
Expand Down
15 changes: 15 additions & 0 deletions servers/audio/audio_stream.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,21 @@ bool AudioStream::is_monophonic() const {
return ret;
}

float AudioStream::get_sample_rate() const {
float ret = 0;
GDVIRTUAL_CALL(_get_sample_rate, ret);
return ret;
}

double AudioStream::sample_to_seconds(int p_sample_index) const {
float sample_rate = get_sample_rate();
if (sample_rate == 0) {
WARN_PRINT("The AudioStream sample rate was 0. Assumed 44,100 Hz instead.");
sample_rate = 44100;
}
return p_sample_index / sample_rate;
}

double AudioStream::get_bpm() const {
double ret = 0;
GDVIRTUAL_CALL(_get_bpm, ret);
Expand Down
3 changes: 3 additions & 0 deletions servers/audio/audio_stream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ class AudioStream : public Resource {
GDVIRTUAL0RC(String, _get_stream_name)
GDVIRTUAL0RC(double, _get_length)
GDVIRTUAL0RC(bool, _is_monophonic)
GDVIRTUAL0RC(float, _get_sample_rate)
GDVIRTUAL0RC(double, _get_bpm)
GDVIRTUAL0RC(bool, _has_loop)
GDVIRTUAL0RC(int, _get_bar_beats)
Expand All @@ -144,6 +145,8 @@ class AudioStream : public Resource {

virtual double get_length() const;
virtual bool is_monophonic() const;
virtual float get_sample_rate() const;
double sample_to_seconds(int p_sample_index) const;

void tag_used(float p_offset);
uint64_t get_tagged_frame() const;
Expand Down
Loading