Fix OGG audio loop offset pop.

Co-authored-by: MJacred <loesch.benny92@gmx.de>
Co-authored-by: Ellen Poe <ellen.h.poe@gmail.com>
Co-authored-by: Michael Wörner <mwoerner@semw-software.com>

5 files changed, 107 insertions, 102 deletions

diff --git a/modules/ogg/ogg_packet_sequence.cpp b/modules/ogg/ogg_packet_sequence.cpp
index fb29493ca9..1100367f03 100644
--- a/modules/ogg/ogg_packet_sequence.cpp
+++ b/modules/ogg/ogg_packet_sequence.cpp
@@ -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.
+		packet_cursor++;
+	}
 
 	return true;
 }
@@ -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();
 }
diff --git a/modules/ogg/ogg_packet_sequence.h b/modules/ogg/ogg_packet_sequence.h
index 7085504500..922a16560c 100644
--- a/modules/ogg/ogg_packet_sequence.h
+++ b/modules/ogg/ogg_packet_sequence.h
@@ -120,6 +120,13 @@ public:
 	// 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();
 };
diff --git a/modules/vorbis/audio_stream_ogg_vorbis.cpp b/modules/vorbis/audio_stream_ogg_vorbis.cpp
index b155dfc5d9..8a265ffaf3 100644
--- a/modules/vorbis/audio_stream_ogg_vorbis.cpp
+++ b/modules/vorbis/audio_stream_ogg_vorbis.cpp
@@ -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();
@@ -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;
 		}
 	}
 }
diff --git a/modules/vorbis/audio_stream_ogg_vorbis.h b/modules/vorbis/audio_stream_ogg_vorbis.h
index 41ce942eec..6abaeaaebd 100644
--- a/modules/vorbis/audio_stream_ogg_vorbis.h
+++ b/modules/vorbis/audio_stream_ogg_vorbis.h
@@ -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.
diff --git a/modules/vorbis/resource_importer_ogg_vorbis.cpp b/modules/vorbis/resource_importer_ogg_vorbis.cpp
index b42cd20589..a8c92f06f6 100644
--- a/modules/vorbis/resource_importer_ogg_vorbis.cpp
+++ b/modules/vorbis/resource_importer_ogg_vorbis.cpp
@@ -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"];
@@ -184,7 +184,7 @@ 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) {
@@ -192,6 +192,7 @@ Ref<AudioStreamOggVorbis> ResourceImporterOggVorbis::load_from_buffer(const Vect
 			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) {
@@ -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) {