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.

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

Make AudioDriverOpenSL's input callback thread-safe #92969

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 3 additions & 2 deletions drivers/coreaudio/audio_driver_coreaudio.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ OSStatus AudioDriverCoreAudio::input_callback(void *inRefCon,
ad->input_buffer_write(sample);
}
}
ad->input_buffer_end_write();
} else {
ERR_PRINT("AudioUnitRender failed, code: " + itos(result));
}
Expand Down Expand Up @@ -633,8 +634,8 @@ void AudioDriverCoreAudio::_set_device(const String &output_device, bool input)

if (input) {
// Reset audio input to keep synchronization.
input_position = 0;
input_size = 0;
input_read = SizePosition(0, 0);
input_write = SizePosition(0, 0);
}
}
}
Expand Down
5 changes: 3 additions & 2 deletions drivers/pulseaudio/audio_driver_pulseaudio.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -269,8 +269,8 @@ Error AudioDriverPulseAudio::init_output_device() {
samples_out.resize(pa_buffer_size);

// Reset audio input to keep synchronization.
input_position = 0;
input_size = 0;
input_read = SizePosition(0, 0);
input_write = SizePosition(0, 0);

return OK;
}
Expand Down Expand Up @@ -546,6 +546,7 @@ void AudioDriverPulseAudio::thread_func(void *p_udata) {
ad->input_buffer_write(sample);
}
}
ad->input_buffer_end_write();

read_bytes += bytes;
ret = pa_stream_drop(ad->pa_rec_str);
Expand Down
5 changes: 3 additions & 2 deletions drivers/wasapi/audio_driver_wasapi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -483,8 +483,8 @@ Error AudioDriverWASAPI::init_output_device(bool p_reinit) {
// Sample rate is independent of channels (ref: https://stackoverflow.com/questions/11048825/audio-sample-frequency-rely-on-channels)
samples_in.resize(buffer_frames * channels);

input_position = 0;
input_size = 0;
input_read = SizePosition(0, 0);
input_write = SizePosition(0, 0);

print_verbose("WASAPI: detected " + itos(audio_output.channels) + " channels");
print_verbose("WASAPI: audio buffer frames: " + itos(buffer_frames) + " calculated latency: " + itos(buffer_frames * 1000 / mix_rate) + "ms");
Expand Down Expand Up @@ -859,6 +859,7 @@ void AudioDriverWASAPI::thread_func(void *p_udata) {
ad->input_buffer_write(l);
ad->input_buffer_write(r);
}
ad->input_buffer_end_write();

read_frames += num_frames_available;

Expand Down
23 changes: 15 additions & 8 deletions platform/android/audio_driver_opensl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ void AudioDriverOpenSL::_buffer_callback(
if (pause) {
mix = false;
} else {
mix = mutex.try_lock();
mix = output_mutex.try_lock();
}

if (mix) {
Expand All @@ -57,7 +57,7 @@ void AudioDriverOpenSL::_buffer_callback(
}

if (mix) {
mutex.unlock();
output_mutex.unlock();
}

const int32_t *src_buff = mixdown_buffer;
Expand Down Expand Up @@ -184,11 +184,14 @@ void AudioDriverOpenSL::start() {
}

void AudioDriverOpenSL::_record_buffer_callback(SLAndroidSimpleBufferQueueItf queueItf) {
input_lock();
for (int i = 0; i < rec_buffer.size(); i++) {
int32_t sample = rec_buffer[i] << 16;
input_buffer_write(sample);
input_buffer_write(sample); // call twice to convert to Stereo
}
input_buffer_end_write();
input_unlock();

SLresult res = (*recordBufferQueueItf)->Enqueue(recordBufferQueueItf, rec_buffer.ptrw(), rec_buffer.size() * sizeof(int16_t));
ERR_FAIL_COND(res != SL_RESULT_SUCCESS);
Expand Down Expand Up @@ -292,6 +295,14 @@ Error AudioDriverOpenSL::input_stop() {
return OK;
}

void AudioDriverOpenSL::input_lock() {
input_mutex.lock();
}

void AudioDriverOpenSL::input_unlock() {
input_mutex.unlock();
}

int AudioDriverOpenSL::get_mix_rate() const {
return 44100; // hardcoded for Android, as selected by SL_SAMPLINGRATE_44_1
}
Expand All @@ -301,15 +312,11 @@ AudioDriver::SpeakerMode AudioDriverOpenSL::get_speaker_mode() const {
}

void AudioDriverOpenSL::lock() {
if (active) {
mutex.lock();
}
output_mutex.lock();
Comment on lines -304 to +315
Copy link
Contributor Author

@kus04e4ek kus04e4ek Jun 10, 2024

Choose a reason for hiding this comment

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

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

}

void AudioDriverOpenSL::unlock() {
if (active) {
mutex.unlock();
}
output_mutex.unlock();
}

void AudioDriverOpenSL::finish() {
Expand Down
6 changes: 5 additions & 1 deletion platform/android/audio_driver_opensl.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@

class AudioDriverOpenSL : public AudioDriver {
bool active = false;
Mutex mutex;
Mutex output_mutex;
Mutex input_mutex;

enum {
BUFFER_COUNT = 2
Expand Down Expand Up @@ -103,6 +104,9 @@ class AudioDriverOpenSL : public AudioDriver {
virtual Error input_start() override;
virtual Error input_stop() override;

virtual void input_lock() override;
virtual void input_unlock() override;

void set_pause(bool p_pause);

AudioDriverOpenSL();
Expand Down
1 change: 1 addition & 0 deletions platform/web/audio_driver_web.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ void AudioDriverWeb::_audio_driver_capture(int p_from, int p_samples) {
for (int i = read_pos; i < read_pos + to_read; i++) {
input_buffer_write(int32_t(input_rb[i] * 32768.f) * (1U << 16));
}
input_buffer_end_write();
}

Error AudioDriverWeb::init() {
Expand Down
54 changes: 33 additions & 21 deletions servers/audio/audio_stream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -322,53 +322,65 @@ AudioStreamMicrophone::AudioStreamMicrophone() {
}

int AudioStreamPlaybackMicrophone::_mix_internal(AudioFrame *p_buffer, int p_frames) {
AudioDriver::get_singleton()->lock();

Vector<int32_t> buf = AudioDriver::get_singleton()->get_input_buffer();
unsigned int input_size = AudioDriver::get_singleton()->get_input_size();
int mix_rate = AudioDriver::get_singleton()->get_mix_rate();
const LocalVector<int32_t> &buf = AudioDriver::get_singleton()->get_input_buffer();
AudioDriver::SizePosition size_position = AudioDriver::get_singleton()->get_input_size_position();
unsigned int mix_rate = AudioDriver::get_singleton()->get_mix_rate();
unsigned int playback_delay = MIN(((50 * mix_rate) / 1000) * 2, buf.size() >> 1);
#ifdef DEBUG_ENABLED
unsigned int input_position = AudioDriver::get_singleton()->get_input_position();
#endif

int mixed_frames = p_frames;

if (playback_delay > input_size) {
if (playback_delay > size_position.size) {
for (int i = 0; i < p_frames; i++) {
p_buffer[i] = AudioFrame(0.0f, 0.0f);
}
input_ofs = 0;
} else {
for (int i = 0; i < p_frames; i++) {
if (input_size > input_ofs && (int)input_ofs < buf.size()) {
bool was_locked = false;

int current_frame = 0;
for (; current_frame < p_frames; current_frame++) {
if (size_position.size > input_ofs && input_ofs < buf.size()) {
float l = (buf[input_ofs++] >> 16) / 32768.f;
if ((int)input_ofs >= buf.size()) {
if (input_ofs >= buf.size()) {
input_ofs = 0;
}
float r = (buf[input_ofs++] >> 16) / 32768.f;
if ((int)input_ofs >= buf.size()) {
if (input_ofs >= buf.size()) {
input_ofs = 0;
}

p_buffer[i] = AudioFrame(l, r);
p_buffer[current_frame] = AudioFrame(l, r);
} else {
if (mixed_frames == p_frames) {
mixed_frames = i;
if (!was_locked) {
// 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;

// Process the same frame on the next loop iteration.
current_frame--;
} else {
break;
}
p_buffer[i] = AudioFrame(0.0f, 0.0f);
}
}

mixed_frames = current_frame;
for (; current_frame < p_frames; current_frame++) {
p_buffer[current_frame] = AudioFrame(0.0f, 0.0f);
}
}

#ifdef DEBUG_ENABLED
if (input_ofs > input_position && (int)(input_ofs - input_position) < (p_frames * 2)) {
print_verbose(String(get_class_name()) + " buffer underrun: input_position=" + itos(input_position) + " input_ofs=" + itos(input_ofs) + " input_size=" + itos(input_size));
if (mixed_frames != p_frames) {
ERR_PRINT(vformat("Buffer underrun: size_position.size = %d, input_ofs = %d, buf.size() = %d.", size_position.size, input_ofs, buf.size()));
} else if (input_ofs > size_position.position && (int)(input_ofs - size_position.position) < (p_frames * 2)) {
print_verbose(String(get_class_name()) + " buffer underrun: size_position.position=" + itos(size_position.position) + " size_position.size=" + itos(size_position.size) + " input_ofs=" + itos(input_ofs));
}
#endif

AudioDriver::get_singleton()->unlock();

return mixed_frames;
}

Expand Down
22 changes: 13 additions & 9 deletions servers/audio_server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -98,24 +98,28 @@ double AudioDriver::get_time_to_next_mix() {
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 (input_write.position < input_buffer.size()) {
input_buffer[input_write.position++] = sample;
if (input_write.position >= input_buffer.size()) {
input_write.position = 0;
}
if ((int)input_size < input_buffer.size()) {
input_size++;
if (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;
}

int AudioDriver::_get_configured_mix_rate() {
StringName audio_driver_setting = "audio/driver/mix_rate";
int mix_rate = GLOBAL_GET(audio_driver_setting);
Expand Down
25 changes: 19 additions & 6 deletions servers/audio_server.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,26 @@ class AudioDriver {
SafeNumeric<uint64_t> prof_time;
#endif

public:
struct SizePosition {
unsigned int size;
unsigned int position;

SizePosition(unsigned int p_size = 0, unsigned int p_position = 0) noexcept :
Copy link
Contributor Author

Choose a reason for hiding this comment

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

noexcept to fix compilation error with g++

size(p_size), position(p_position) {}
};
Comment on lines +60 to +66
Copy link
Contributor Author

@kus04e4ek kus04e4ek Jun 11, 2024

Choose a reason for hiding this comment

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

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


protected:
Vector<int32_t> input_buffer;
unsigned int input_position = 0;
unsigned int input_size = 0;
LocalVector<int32_t> input_buffer;
std::atomic<SizePosition> input_read;
SizePosition input_write;
Copy link
Contributor Author

@kus04e4ek kus04e4ek Jun 11, 2024

Choose a reason for hiding this comment

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

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


void audio_server_process(int p_frames, int32_t *p_buffer, bool p_update_mix_time = true);
void update_mix_time(int p_frames);

void input_buffer_init(int driver_buffer_frames);
void input_buffer_write(int32_t sample);
void input_buffer_end_write();

int _get_configured_mix_rate();

Expand Down Expand Up @@ -111,6 +122,9 @@ class AudioDriver {
virtual Error input_start() { return FAILED; }
virtual Error input_stop() { return FAILED; }

virtual void input_lock() { lock(); }
virtual void input_unlock() { unlock(); }

virtual PackedStringArray get_input_device_list();
virtual String get_input_device() { return "Default"; }
virtual void set_input_device(const String &p_name) {}
Expand All @@ -120,9 +134,8 @@ class AudioDriver {
SpeakerMode get_speaker_mode_by_total_channels(int p_channels) const;
int get_total_channels_by_speaker_mode(SpeakerMode) const;

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 LocalVector<int32_t> &get_input_buffer() { return input_buffer; }
SizePosition get_input_size_position() { return input_read; }

#ifdef DEBUG_ENABLED
uint64_t get_profiling_time() const { return prof_time.get(); }
Expand Down
Loading