godotengine / godotengine/godot

Data races due to lack of synchronization with audio thread

Open
#122,000 3 comments 1 reaction 0 assignees View on GitHub
bug topic:audio
Dominant language
C++
Stars
117k
Forks
26.8k
PR merge metrics
PR metrics pending

Description

### Tested versions

Discovered the issue on 4.7, present on master branch as well. This seems to be a legacy core problem.

### System information

Windows 11 (system info is irrelevant)

### Issue description

Currently in Godot audio code there are violations of thread safety.

There are numerous places throughout the audio mixing classes, where reading and writing to data fields, is done concurrently between main thread and audio thread without any synchronization.

Relevant public documentation ([Create an AudioStreamPlayback](https://docs.godotengine.org/en/stable/engine_details/engine_api/custom_audiostreams.html#create-an-audiostreamplayback)) mentions avoiding i/o and dynamic allocations, but nothing about synchronizing data reads and writes with audio thread from public interface.
Native Godot code follows the suit, while it could provide examples of how to handle said synchronization.

The most obvious example of the issue - tracking current playback position:
```cpp
// audio_stream_wav.h

class AudioStreamPlaybackWAV : public AudioStreamPlaybackResampled
{
...
private:
int64_t offset = 0;

protected:
// called from audio thread
virtual int _mix_internal(AudioFrame *p_buffer, int p_frames) override;

public:
// public interface, called from non-audio thread
virtual double get_playback_position() const override;
virtual void seek(double p_time) override;
...
}
```

```cpp
// audio_stream_wav.cpp

int AudioStreamPlaybackWAV::_mix_internal(AudioFrame *p_buffer, int p_frames) {
// ...

// reading and writing to the `offset` variable
}

double AudioStreamPlaybackWAV::get_playback_position() const {
// data race
return double(offset) / base->mix_rate;
}

void AudioStreamPlaybackWAV::seek(double p_time) {
// ...

// data race
offset = int64_t(p_time * base->mix_rate);
}

```

...the same example prevalent in all the audio steram playback classes.

Not using `atomic`/`SafeNumeric` where it it should had been used, is just one example of lacking synchronization.

There might be different issues, like calling thread-unsafe public interfaces from within mixing callback. But it's hard to reason about unless a function thread context is documented.

#### Addressing this issue can be done in stages
- Document synchronization requirements and function thread context - this should prevent exacerbating the issue.
- Minimal fix: synchronize internal counters and flags within audio mixing classes (i.e. use `atomic`/`SafeNumeric`) and some `AudioServer` counters accessed from outside of audio thread - removes UB, now we can reason about the execution, and address other issues and bugs with more confidence.
- Review `AudioServer` internals: make sure all the public API and internal synchronisations are thread-safe

Doing an audio mixing callstack audit should help uncover all the places where data races might occur.

Ideally, it should be made really obvious which functions are processed by audio thread and in which circumstances.
This might be against the documentation policy, but even a single line of annotation at a function declaration - like `// called from audio thread` - could prevent future mistakes. It would be easier to see annotation in code, than having to over to Godot documentation website, searching for what you're allowed to call from which context, though both could be in place.

### Steps to reproduce

This is a code analysis/review bug, a structural problem visible in the source code.,

### Minimal reproduction project (MRP)

Not applicable.

Contributor guide

Open the contributing guide

Research direction

Start with audio_stream_wav.h and audio_stream_wav.cpp, tracing AudioStreamPlaybackWAV::_mix_internal(), get_playback_position(), and seek() across audio and non-audio thread contexts. Then review the audio mixing classes and AudioServer internals for similar unsynchronized counters, flags, and public interfaces. Done should include documented thread-context requirements and a reviewed, synchronized scope agreed for the staged fixes.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, godot
Domain
audio-video-rtc
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.