Make `AudioDriverOpenSL`'s input callback thread-safe #92969

kus04e4ek · 2024-06-10T10:18:41Z

Fixes: Audio input gets muted after a while on android #86428 (Should probably mention that this problem is as old as support for the microphone on Android, there could be other similar issues this PR closes. See Make AudioDriverOpenSL's input callback thread-safe #92969 (comment)).
Superseded by Make AudioStreamPlaybackMicrophone::_mix_internal be able to run at the same time as input callback for AudioDriverOpenSL #93154. Still decided to keep it just in case

Tested using this project: AudioTest.zip.

Turns out that not making the input callback thread-safe caused weird issues and input_buffer was not getting referenced (see #92969 (comment)), so now it's thread-safe. However, the copied implementation from AudioDriverCoreAudio causes audio issues (you can hear them if you compile only the first commit), as it seems that input and output callbacks run at the same time on Android. That was a problem because there was only one mutex that these two threads needed to share, but the output callback never waits for this mutex to unlock (to make it as fast as possible, this also matches the AudioDriverCoreAudio implementation).

However, there needed to be at least something to account for them running at the same time, so I decided to add another mutex that output and input callback both wait for. But why doesn't adding another mutex slow down output callback a lot? First of all, it's because this mutex is only used by the input callback, so it doesn't impact speed that much, the other mutex is pretty much used all over Godot and can be used by GDScript. Second of all, output callback only waits for the new mutex mid-executing, if it waited for the other mutex, it would have been waiting for it before starting execution. You can look at the implementation in the second commit, that's why there are two commits. This solution also doesn't impact any other audio driver.

To minimize locking even more, pushed the third commit. It's also a little bit of a different solution to the issue, ~~but to function properly it needs the first two commits, that's why I decided to keep it in this PR, can create the new one, if required~~. Now it locks on the output callback only when there's an underrun in the input buffer. See #92969 (comment) and #93154.

The same input mutex can be created for AudioDriverCoreAudio as well, but I decided to leave it for the other PR.
I could only come up with this solution, so if anyone has other ideas, I would love to hear them ^^

P.S.: There are also other solutions for locks, but I don't know much about threads, is Mutex the best lock here or should I use something else? (I'll also ask in Rocket.Chat)

EDIT: ~~This might be a bug that wasn't present since the begging and something in the Vector implementation just broke.~~ See #92969 (comment).

platform/android/audio_driver_opensl.cpp

kus04e4ek · 2024-06-10T10:23:55Z

platform/android/audio_driver_opensl.cpp

-	if (active) {
-		mutex.lock();
-	}
+	output_mutex.lock();


if (active) is not necessary and could create issues(?), no other AudioDriver checks for it

kus04e4ek · 2024-06-10T10:24:24Z

servers/audio/audio_stream.cpp

+	if (mixed_frames != p_frames) {
+		ERR_PRINT(vformat("Buffer underrun: input_size = %d, input_ofs = %d, buf.size() = %d.", input_size, input_ofs, buf.size()));


It was useful for debugging, decided to keep it

platform/android/audio_driver_opensl.cpp

kus04e4ek · 2024-06-11T08:30:42Z

servers/audio_server.cpp

 void AudioDriver::input_buffer_init(int driver_buffer_frames) {
 	const int input_buffer_channels = 2;
 	input_buffer.resize(driver_buffer_frames * input_buffer_channels * 4);
-	input_position = 0;
-	input_size = 0;
+	input_read = SizePosition(0, 0);
+	input_write = SizePosition(0, 0);
 }

 void AudioDriver::input_buffer_write(int32_t sample) {
-	if ((int)input_position < input_buffer.size()) {
-		input_buffer.write[input_position++] = sample;
-		if ((int)input_position >= input_buffer.size()) {
-			input_position = 0;
+	if ((int)input_write.position < input_buffer.size()) {
+		input_buffer.write[input_write.position++] = sample;
+		if ((int)input_write.position >= input_buffer.size()) {
+			input_write.position = 0;
 		}
-		if ((int)input_size < input_buffer.size()) {
-			input_size++;
+		if ((int)input_write.size < input_buffer.size()) {
+			input_write.size++;
 		}
 	} else {
-		WARN_PRINT("input_buffer_write: Invalid input_position=" + itos(input_position) + " input_buffer.size()=" + itos(input_buffer.size()));
+		WARN_PRINT("input_buffer_write: Invalid input_write.position=" + itos(input_write.position) + " input_buffer.size()=" + itos(input_buffer.size()));
 	}
 }

+void AudioDriver::input_buffer_end_write() {
+	input_read = input_write;
+}


Alternatively:

void AudioDriver::input_buffer_init(int driver_buffer_frames) { const int input_buffer_channels = 2; input_buffer.resize(driver_buffer_frames * input_buffer_channels * 4); input_read = SizePosition(0, 0); } void AudioDriver::input_buffer_write(int32_t sample) { SizePosition input_write = input_read; if ((int)input_write.position < input_buffer.size()) { input_buffer.write[input_write.position++] = sample; if ((int)input_write.position >= input_buffer.size()) { input_write.position = 0; } if ((int)input_write.size < input_buffer.size()) { input_write.size++; } input_read = input_write; } else { WARN_PRINT("input_buffer_write: Invalid input_write.position=" + itos(input_write.position) + " input_buffer.size()=" + itos(input_buffer.size())); } }

Decided to go with this solution, as it seems more performant(?), because of not assigning a value to the std::atomic multiple times.

kus04e4ek · 2024-06-11T08:34:53Z

servers/audio_server.h

-	unsigned int input_position = 0;
-	unsigned int input_size = 0;
+	std::atomic<SizePosition> input_read;
+	SizePosition input_write;


input_write doesn't have to be std::atomic, mirrors behaviour from the previous implementation

kus04e4ek · 2024-06-11T08:38:12Z

servers/audio_server.h

-	Vector<int32_t> get_input_buffer() { return input_buffer; }
-	unsigned int get_input_position() { return input_position; }
-	unsigned int get_input_size() { return input_size; }
+	const Vector<int32_t> &get_input_buffer() { return input_buffer; }


That's what was causing issues in the first place, for some reason in some rare occasions Vector was not copied properly, returning Vector with nullptr pointer. And just in general there shouldn't be any issues with not copying it, there should be plenty of space in the input buffer

More about it here: #93125.

kus04e4ek · 2024-06-11T08:54:09Z

servers/audio_server.h

+	struct SizePosition {
+		unsigned int size;
+		unsigned int position;
+
+		SizePosition(unsigned int p_size = 0, unsigned int p_position = 0) noexcept :
+				size(p_size), position(p_position) {}
+	};


Created a whole class to fit it in one std::atomic, it's important that this values change together. Also it's just a 64-bit structure, so lock-free std::atomic will probably work on most devices (worked on my phone).
EDIT: It's actually not as important to change them together anymore, mostly for debugging puproses now

kus04e4ek · 2024-06-11T08:54:27Z

servers/audio_server.h

+		unsigned int size;
+		unsigned int position;
+
+		SizePosition(unsigned int p_size = 0, unsigned int p_position = 0) noexcept :


noexcept to fix compilation error with g++

servers/audio/audio_stream.cpp

kus04e4ek · 2024-06-11T09:21:52Z

servers/audio/audio_stream.cpp

+					// Wait while the other thread writes to the input buffer.
+					AudioDriver::get_singleton()->input_lock();
+					AudioDriver::get_singleton()->input_unlock();
+
+					size_position = AudioDriver::get_singleton()->get_input_size_position();
+					was_locked = true;
+
+					// Write to the same frame on the next loop iteration.
+					current_frame--;


Doesn't seem that this code actually executes ever, maybe it should be removed and replaced with just printing an error? This would make the third commit fully independent from the first two

…e audio.

kus04e4ek requested review from a team as code owners June 10, 2024 10:18

AThousandShips added bug topic:audio labels Jun 10, 2024

AThousandShips added this to the 4.3 milestone Jun 10, 2024

AThousandShips added the platform:android label Jun 10, 2024

kus04e4ek commented Jun 10, 2024

View reviewed changes

AThousandShips reviewed Jun 10, 2024

View reviewed changes

platform/android/audio_driver_opensl.cpp Outdated Show resolved Hide resolved

kus04e4ek added 2 commits June 10, 2024 23:28

Make AudioDriverOpenSL's input callback thread-safe.

0734ff5

Add input lock to AudioDriver.

3101b48

kus04e4ek force-pushed the opensl branch from c5bca37 to 3101b48 Compare June 10, 2024 15:34

kus04e4ek requested a review from a team as a code owner June 11, 2024 08:22

kus04e4ek force-pushed the opensl branch 2 times, most recently from bc4749a to a69c143 Compare June 11, 2024 09:21

kus04e4ek commented Jun 11, 2024

View reviewed changes

kus04e4ek force-pushed the opensl branch 2 times, most recently from 0e60826 to 7501e75 Compare June 12, 2024 06:55

kus04e4ek mentioned this pull request Jun 13, 2024

Vector's constructor fails to create a reference to the other Vector when the other thread writes to that object #93125

Open

AThousandShips requested a review from RandomShaper June 13, 2024 10:43

Minimize locking from reading data from microphone when outputting th…

de4b0f3

…e audio.

kus04e4ek force-pushed the opensl branch from 7501e75 to de4b0f3 Compare June 13, 2024 11:07

kus04e4ek mentioned this pull request Jun 14, 2024

Make AudioStreamPlaybackMicrophone::_mix_internal be able to run at the same time as input callback for AudioDriverOpenSL #93154

Open

AThousandShips mentioned this pull request Jun 15, 2024

Fix audio input gets muted after a while on android #93200

Open

akien-mga modified the milestones: 4.3, 4.4 Jul 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `AudioDriverOpenSL`'s input callback thread-safe #92969

Make `AudioDriverOpenSL`'s input callback thread-safe #92969

kus04e4ek commented Jun 10, 2024 •

edited

Loading

kus04e4ek Jun 10, 2024 •

edited

Loading

kus04e4ek Jun 10, 2024

kus04e4ek Jun 11, 2024 •

edited

Loading

kus04e4ek Jun 11, 2024 •

edited

Loading

kus04e4ek Jun 11, 2024

This comment was marked as outdated.

kus04e4ek Jun 13, 2024

kus04e4ek Jun 11, 2024 •

edited

Loading

kus04e4ek Jun 11, 2024

kus04e4ek Jun 11, 2024 •

edited

Loading

kus04e4ek Jun 14, 2024

		if (mixed_frames != p_frames) {
		ERR_PRINT(vformat("Buffer underrun: input_size = %d, input_ofs = %d, buf.size() = %d.", input_size, input_ofs, buf.size()));

Make AudioDriverOpenSL's input callback thread-safe #92969

Are you sure you want to change the base?

Make AudioDriverOpenSL's input callback thread-safe #92969

Conversation

kus04e4ek commented Jun 10, 2024 • edited Loading

kus04e4ek Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

kus04e4ek Jun 10, 2024

Choose a reason for hiding this comment

kus04e4ek Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

kus04e4ek Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

kus04e4ek Jun 11, 2024

Choose a reason for hiding this comment

This comment was marked as outdated.

kus04e4ek Jun 13, 2024

Choose a reason for hiding this comment

kus04e4ek Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

kus04e4ek Jun 11, 2024

Choose a reason for hiding this comment

kus04e4ek Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

kus04e4ek Jun 14, 2024

Choose a reason for hiding this comment

Make `AudioDriverOpenSL`'s input callback thread-safe #92969

Make `AudioDriverOpenSL`'s input callback thread-safe #92969

kus04e4ek commented Jun 10, 2024 •

edited

Loading

kus04e4ek Jun 10, 2024 •

edited

Loading

kus04e4ek Jun 11, 2024 •

edited

Loading

kus04e4ek Jun 11, 2024 •

edited

Loading

kus04e4ek Jun 11, 2024 •

edited

Loading

kus04e4ek Jun 11, 2024 •

edited

Loading