godotengine / godotengine/godot
Data races due to lack of synchronization with audio thread
- 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
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