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

Fix OGG audio loop offset pop #80452

Merged
merged 1 commit into from Oct 13, 2023
Merged

Fix OGG audio loop offset pop #80452

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
18 changes: 17 additions & 1 deletion modules/ogg/ogg_packet_sequence.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,9 @@ bool OggPacketSequencePlayback::next_ogg_packet(ogg_packet **p_packet) const {

*p_packet = packet;

packet_cursor++;
if (!packet->e_o_s) { // Added this so it doesn't try to go to the next packet if it's the last packet of the file.
Copy link
Contributor

@nikitalita nikitalita Dec 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@nikitalita nikitalita Dec 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

packet_cursor++;
}

return true;
}
Expand Down Expand Up @@ -216,6 +218,20 @@ bool OggPacketSequencePlayback::seek_page(int64_t p_granule_pos) {
return true;
}

int64_t OggPacketSequencePlayback::get_page_number() const {
return page_cursor;
}

bool OggPacketSequencePlayback::set_page_number(int64_t p_page_number) {
if (p_page_number >= 0 && p_page_number < ogg_packet_sequence->page_data.size()) {
page_cursor = p_page_number;
packet_cursor = 0;
packetno = 0;
return true;
}
return false;
}

OggPacketSequencePlayback::OggPacketSequencePlayback() {
packet = new ogg_packet();
}
Expand Down
7 changes: 7 additions & 0 deletions modules/ogg/ogg_packet_sequence.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,13 @@ class OggPacketSequencePlayback : public RefCounted {
// Returns true on success, false on failure.
bool seek_page(int64_t p_granule_pos);

// Gets the current page number.
int64_t get_page_number() const;

// Moves to a specific page in the stream.
// Returns true on success, false if the page number is out of bounds.
bool set_page_number(int64_t p_page_number);

OggPacketSequencePlayback();
virtual ~OggPacketSequencePlayback();
};
Expand Down
161 changes: 67 additions & 94 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 @@ -264,11 +264,10 @@ void AudioStreamPlaybackOggVorbis::seek(double p_time) {
return;
}

vorbis_synthesis_restart(&dsp_state);

if (p_time >= vorbis_stream->get_length()) {
p_time = 0;
}

frames_mixed = uint32_t(vorbis_data->get_sampling_rate() * p_time);

const int64_t desired_sample = p_time * get_stream_sampling_rate();
Expand All @@ -278,107 +277,81 @@ void AudioStreamPlaybackOggVorbis::seek(double p_time) {
return;
}

ogg_packet *packet;
if (!vorbis_data_playback->next_ogg_packet(&packet)) {
WARN_PRINT_ONCE("seeking beyond limits");
return;
// We want to start decoding before the page that we expect the sample to be in (the sample may
// be part of a partial packet across page boundaries). Otherwise, the decoder may not have
// synchronized before reaching the sample.
int64_t start_page_number = vorbis_data_playback->get_page_number() - 1;
if (start_page_number < 0) {
start_page_number = 0;
}

// The granule position of the page we're seeking through.
int64_t granule_pos = 0;

int headers_remaining = 0;
int samples_in_page = 0;
int err;
while (true) {
if (vorbis_synthesis_idheader(packet)) {
headers_remaining = 3;
}
if (!headers_remaining) {
err = vorbis_synthesis(&block, packet);
ERR_FAIL_COND_MSG(err != 0, "Error during vorbis synthesis " + itos(err));

err = vorbis_synthesis_blockin(&dsp_state, &block);
ERR_FAIL_COND_MSG(err != 0, "Error during vorbis block processing " + itos(err));

int samples_out = vorbis_synthesis_pcmout(&dsp_state, nullptr);
err = vorbis_synthesis_read(&dsp_state, samples_out);
ERR_FAIL_COND_MSG(err != 0, "Error during vorbis read updating " + itos(err));

samples_in_page += samples_out;

} else {
headers_remaining--;
}
if (packet->granulepos != -1 && headers_remaining == 0) {
// This indicates the end of the page.
granule_pos = packet->granulepos;
break;
}
if (packet->e_o_s) {
break;
}
if (!vorbis_data_playback->next_ogg_packet(&packet)) {
// We should get an e_o_s flag before this happens.
WARN_PRINT("Vorbis file ended without warning.");
break;
}
}
ogg_packet *packet;
int err;

int64_t samples_to_burn = samples_in_page - (granule_pos - desired_sample);
// We start at an unknown granule position.
int64_t granule_pos = -1;

if (samples_to_burn > samples_in_page) {
WARN_PRINT_ONCE("Burning more samples than we have in this page. Check seek algorithm.");
} else if (samples_to_burn < 0) {
WARN_PRINT_ONCE("Burning negative samples doesn't make sense. Check seek algorithm.");
}
// Decode data until we get to the desired sample or notice that we have read past it.
vorbis_data_playback->set_page_number(start_page_number);
vorbis_synthesis_restart(&dsp_state);

// Seek again, this time we'll burn a specific number of samples instead of all of them.
if (!vorbis_data_playback->seek_page(desired_sample)) {
WARN_PRINT("seek failed");
return;
}

if (!vorbis_data_playback->next_ogg_packet(&packet)) {
WARN_PRINT_ONCE("seeking beyond limits");
return;
}
vorbis_synthesis_restart(&dsp_state);
while (true) {
if (!vorbis_data_playback->next_ogg_packet(&packet)) {
WARN_PRINT_ONCE("Seeking beyond limits");
return;
}

while (true) {
if (vorbis_synthesis_idheader(packet)) {
headers_remaining = 3;
}
if (!headers_remaining) {
err = vorbis_synthesis(&block, packet);
ERR_FAIL_COND_MSG(err != 0, "Error during vorbis synthesis " + itos(err));

err = vorbis_synthesis_blockin(&dsp_state, &block);
ERR_FAIL_COND_MSG(err != 0, "Error during vorbis block processing " + itos(err));

int samples_out = vorbis_synthesis_pcmout(&dsp_state, nullptr);
int read_samples = samples_to_burn > samples_out ? samples_out : samples_to_burn;
err = vorbis_synthesis_read(&dsp_state, samples_out);
ERR_FAIL_COND_MSG(err != 0, "Error during vorbis read updating " + itos(err));
samples_to_burn -= read_samples;

if (samples_to_burn <= 0) {
break;
if (err != OV_ENOTAUDIO) {
ERR_FAIL_COND_MSG(err != 0, "Error during vorbis synthesis " + itos(err) + ".");

err = vorbis_synthesis_blockin(&dsp_state, &block);
ERR_FAIL_COND_MSG(err != 0, "Error during vorbis block processing " + itos(err) + ".");

int samples_out = vorbis_synthesis_pcmout(&dsp_state, nullptr);

if (granule_pos < 0) {
// We don't know where we are yet, so just keep on decoding.
err = vorbis_synthesis_read(&dsp_state, samples_out);
ERR_FAIL_COND_MSG(err != 0, "Error during vorbis read updating " + itos(err) + ".");
} else if (granule_pos + samples_out >= desired_sample) {
// Our sample is in this block. Skip the beginning of the block up to the sample, then
// return.
int skip_samples = (int)(desired_sample - granule_pos);
err = vorbis_synthesis_read(&dsp_state, skip_samples);
ERR_FAIL_COND_MSG(err != 0, "Error during vorbis read updating " + itos(err) + ".");
have_samples_left = skip_samples < samples_out;
have_packets_left = !packet->e_o_s;
return;
} else {
// Our sample is not in this block. Skip it.
err = vorbis_synthesis_read(&dsp_state, samples_out);
ERR_FAIL_COND_MSG(err != 0, "Error during vorbis read updating " + itos(err) + ".");
granule_pos += samples_out;
}
}
if (packet->granulepos != -1) {
// We found an update to our granule position.
granule_pos = packet->granulepos;
if (granule_pos > desired_sample) {
// We've read past our sample. We need to start on an earlier page.
if (start_page_number == 0) {
// We didn't find the sample even reading from the beginning.
have_samples_left = false;
have_packets_left = !packet->e_o_s;
return;
}
start_page_number--;
break;
}
}
if (packet->e_o_s) {
// We've reached the end of the stream and didn't find our sample.
have_samples_left = false;
have_packets_left = false;
return;
}
} else {
headers_remaining--;
}
if (packet->granulepos != -1 && headers_remaining == 0) {
// This indicates the end of the page.
break;
}
if (packet->e_o_s) {
break;
}
if (!vorbis_data_playback->next_ogg_packet(&packet)) {
// We should get an e_o_s flag before this happens.
WARN_PRINT("Vorbis file ended without warning.");
break;
}
}
}
Expand Down
4 changes: 2 additions & 2 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 @@ -107,9 +107,9 @@ class AudioStreamOggVorbis : public AudioStream {
friend class AudioStreamPlaybackOggVorbis;

int channels = 1;
float length = 0.0;
double length = 0.0;
bool loop = false;
float loop_offset = 0.0;
double loop_offset = 0.0;

// Performs a seek to the beginning of the stream, should not be called during playback!
// Also causes allocation and deallocation.
Expand Down
19 changes: 14 additions & 5 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 @@ -97,7 +97,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"];
float loop_offset = p_options["loop_offset"];
double loop_offset = p_options["loop_offset"];
double bpm = p_options["bpm"];
int beat_count = p_options["beat_count"];
int bar_beats = p_options["bar_beats"];
Expand Down Expand Up @@ -184,14 +184,15 @@ Ref<AudioStreamOggVorbis> ResourceImporterOggVorbis::load_from_buffer(const Vect
ERR_FAIL_COND_V_MSG(err != 0, Ref<AudioStreamOggVorbis>(), "Ogg stream error " + itos(err));
int desync_iters = 0;

Vector<Vector<uint8_t>> packet_data;
RBMap<uint64_t, Vector<Vector<uint8_t>>> sorted_packets;
int64_t granule_pos = 0;

while (true) {
err = ogg_stream_packetout(&stream_state, &packet);
if (err == -1) {
// According to the docs this is usually recoverable, but don't sit here spinning forever.
desync_iters++;
WARN_PRINT_ONCE("Desync during ogg import.");
ERR_FAIL_COND_V_MSG(desync_iters > 100, Ref<AudioStreamOggVorbis>(), "Packet sync issue during Ogg import");
continue;
} else if (err == 0) {
Expand All @@ -207,16 +208,24 @@ Ref<AudioStreamOggVorbis> ResourceImporterOggVorbis::load_from_buffer(const Vect
}
break;
}
granule_pos = packet.granulepos;
if (packet.granulepos > granule_pos) {
granule_pos = packet.granulepos;
}

PackedByteArray data;
data.resize(packet.bytes);
memcpy(data.ptrw(), packet.packet, packet.bytes);
packet_data.push_back(data);
sorted_packets[granule_pos].push_back(data);
packet_count++;
}
Vector<Vector<uint8_t>> packet_data;
for (const KeyValue<uint64_t, Vector<Vector<uint8_t>>> &pair : sorted_packets) {
for (const Vector<uint8_t> &packets : pair.value) {
packet_data.push_back(packets);
}
}
if (initialized_stream) {
ogg_packet_sequence->push_page(granule_pos, packet_data);
ogg_packet_sequence->push_page(ogg_page_granulepos(&page), packet_data);
}
}
if (initialized_stream) {
Expand Down
Loading