Skip to content

Commit

Permalink
Improve sound sample thread-safety and add assertions
Browse files Browse the repository at this point in the history 
Lock the sound lock when accessing any of the sound samples, voices and channels.

Add assertions to ensure that sound sample IDs are within range and that samples are loaded.

Rename parameter `FromEditor` to `ForceLoad` to make usage clearer.

Closes ddnet#8262.
  • Loading branch information
@Robyt3
Robyt3 committed Dec 26, 2024
1 parent e823874 commit 8f8ab84
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 69 deletions.
121 changes: 65 additions & 56 deletions src/engine/client/sound.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ int CSound::Init()

// Initialize sample indices. We always need them to load sounds in
// the editor even if sound is disabled or failed to be enabled.
const CLockScope LockScope(m_SoundLock);
m_FirstFreeSampleIndex = 0;
for(size_t i = 0; i < std::size(m_aSamples) - 1; ++i)
{
Expand Down Expand Up @@ -235,10 +236,10 @@ int CSound::Init()
#endif
m_pMixBuffer = (int *)calloc(m_MaxFrames * 2, sizeof(int));

SDL_PauseAudioDevice(m_Device, 0);

m_SoundEnabled = true;
Update();

SDL_PauseAudioDevice(m_Device, 0);
return 0;
}

Expand All @@ -258,19 +259,28 @@ void CSound::UpdateVolume()

void CSound::Shutdown()
{
for(unsigned SampleId = 0; SampleId < NUM_SAMPLES; SampleId++)
{
UnloadSample(SampleId);
}
StopAll();

// Stop sound callback before freeing sample data
SDL_CloseAudioDevice(m_Device);
SDL_QuitSubSystem(SDL_INIT_AUDIO);
m_Device = 0;

const CLockScope LockScope(m_SoundLock);
for(auto &Sample : m_aSamples)
{
free(Sample.m_pData);
Sample.m_pData = nullptr;
}

free(m_pMixBuffer);
m_pMixBuffer = nullptr;
m_SoundEnabled = false;
}

CSample *CSound::AllocSample()
{
const CLockScope LockScope(m_SoundLock);
if(m_FirstFreeSampleIndex == SAMPLE_INDEX_FULL)
return nullptr;

Expand All @@ -289,7 +299,7 @@ CSample *CSound::AllocSample()

void CSound::RateConvert(CSample &Sample) const
{
dbg_assert(Sample.m_pData != nullptr, "Sample is not loaded");
dbg_assert(Sample.IsLoaded(), "Sample not loaded");
// make sure that we need to convert this sound
if(Sample.m_Rate == m_MixingRate)
return;
Expand Down Expand Up @@ -514,9 +524,6 @@ int CSound::LoadOpus(const char *pFilename, int StorageType)
if(!m_SoundEnabled)
return -1;

if(!m_pStorage)
return -1;

CSample *pSample = AllocSample();
if(!pSample)
{
Expand Down Expand Up @@ -554,9 +561,6 @@ int CSound::LoadWV(const char *pFilename, int StorageType)
if(!m_SoundEnabled)
return -1;

if(!m_pStorage)
return -1;

CSample *pSample = AllocSample();
if(!pSample)
{
Expand Down Expand Up @@ -588,13 +592,10 @@ int CSound::LoadWV(const char *pFilename, int StorageType)
return pSample->m_Index;
}

int CSound::LoadOpusFromMem(const void *pData, unsigned DataSize, bool FromEditor = false)
int CSound::LoadOpusFromMem(const void *pData, unsigned DataSize, bool ForceLoad = false)
{
// no need to load sound when we are running with no sound
if(!m_SoundEnabled && !FromEditor)
return -1;

if(!pData)
if(!m_SoundEnabled && !ForceLoad)
return -1;

CSample *pSample = AllocSample();
Expand All @@ -611,13 +612,10 @@ int CSound::LoadOpusFromMem(const void *pData, unsigned DataSize, bool FromEdito
return pSample->m_Index;
}

int CSound::LoadWVFromMem(const void *pData, unsigned DataSize, bool FromEditor = false)
int CSound::LoadWVFromMem(const void *pData, unsigned DataSize, bool ForceLoad = false)
{
// no need to load sound when we are running with no sound
if(!m_SoundEnabled && !FromEditor)
return -1;

if(!pData)
if(!m_SoundEnabled && !ForceLoad)
return -1;

CSample *pSample = AllocSample();
Expand All @@ -636,12 +634,13 @@ int CSound::LoadWVFromMem(const void *pData, unsigned DataSize, bool FromEditor

void CSound::UnloadSample(int SampleId)
{
if(SampleId == -1 || SampleId >= NUM_SAMPLES)
if(SampleId == -1)
return;

Stop(SampleId);

// Free data
const CLockScope LockScope(m_SoundLock);
CSample &Sample = m_aSamples[SampleId];
free(Sample.m_pData);
Sample.m_pData = nullptr;
Expand All @@ -656,18 +655,19 @@ void CSound::UnloadSample(int SampleId)

float CSound::GetSampleTotalTime(int SampleId)
{
if(SampleId == -1 || SampleId >= NUM_SAMPLES)
return 0.0f;
dbg_assert(SampleId >= 0 && SampleId < NUM_SAMPLES, "SampleId invalid");

const CLockScope LockScope(m_SoundLock);
dbg_assert(m_aSamples[SampleId].IsLoaded(), "Sample not loaded");
return m_aSamples[SampleId].TotalTime();
}

float CSound::GetSampleCurrentTime(int SampleId)
{
if(SampleId == -1 || SampleId >= NUM_SAMPLES)
return 0.0f;
dbg_assert(SampleId >= 0 && SampleId < NUM_SAMPLES, "SampleId invalid");

const CLockScope LockScope(m_SoundLock);
dbg_assert(m_aSamples[SampleId].IsLoaded(), "Sample not loaded");
CSample *pSample = &m_aSamples[SampleId];
for(auto &Voice : m_aVoices)
{
Expand All @@ -682,10 +682,10 @@ float CSound::GetSampleCurrentTime(int SampleId)

void CSound::SetSampleCurrentTime(int SampleId, float Time)
{
if(SampleId == -1 || SampleId >= NUM_SAMPLES)
return;
dbg_assert(SampleId >= 0 && SampleId < NUM_SAMPLES, "SampleId invalid");

const CLockScope LockScope(m_SoundLock);
dbg_assert(m_aSamples[SampleId].IsLoaded(), "Sample not loaded");
CSample *pSample = &m_aSamples[SampleId];
for(auto &Voice : m_aVoices)
{
Expand All @@ -701,6 +701,9 @@ void CSound::SetSampleCurrentTime(int SampleId, float Time)

void CSound::SetChannel(int ChannelId, float Vol, float Pan)
{
dbg_assert(ChannelId >= 0 && ChannelId < NUM_CHANNELS, "ChannelId invalid");

const CLockScope LockScope(m_SoundLock);
m_aChannels[ChannelId].m_Vol = (int)(Vol * 255.0f);
m_aChannels[ChannelId].m_Pan = (int)(Pan * 255.0f); // TODO: this is only on and off right now
}
Expand Down Expand Up @@ -836,36 +839,34 @@ ISound::CVoiceHandle CSound::Play(int ChannelId, int SampleId, int Flags, float
break;
}
}
if(VoiceId == -1)
{
return CreateVoiceHandle(-1, -1);
}

// voice found, use it
int Age = -1;
if(VoiceId != -1)
m_aVoices[VoiceId].m_pSample = &m_aSamples[SampleId];
m_aVoices[VoiceId].m_pChannel = &m_aChannels[ChannelId];
if(Flags & FLAG_LOOP)
{
m_aVoices[VoiceId].m_pSample = &m_aSamples[SampleId];
m_aVoices[VoiceId].m_pChannel = &m_aChannels[ChannelId];
if(Flags & FLAG_LOOP)
{
m_aVoices[VoiceId].m_Tick = m_aSamples[SampleId].m_PausedAt;
}
else if(Flags & FLAG_PREVIEW)
{
m_aVoices[VoiceId].m_Tick = m_aSamples[SampleId].m_PausedAt;
m_aSamples[SampleId].m_PausedAt = 0;
}
else
{
m_aVoices[VoiceId].m_Tick = 0;
}
m_aVoices[VoiceId].m_Vol = (int)(clamp(Volume, 0.0f, 1.0f) * 255.0f);
m_aVoices[VoiceId].m_Flags = Flags;
m_aVoices[VoiceId].m_Position = Position;
m_aVoices[VoiceId].m_Falloff = 0.0f;
m_aVoices[VoiceId].m_Shape = ISound::SHAPE_CIRCLE;
m_aVoices[VoiceId].m_Circle.m_Radius = 1500;
Age = m_aVoices[VoiceId].m_Age;
m_aVoices[VoiceId].m_Tick = m_aSamples[SampleId].m_PausedAt;
}

return CreateVoiceHandle(VoiceId, Age);
else if(Flags & FLAG_PREVIEW)
{
m_aVoices[VoiceId].m_Tick = m_aSamples[SampleId].m_PausedAt;
m_aSamples[SampleId].m_PausedAt = 0;
}
else
{
m_aVoices[VoiceId].m_Tick = 0;
}
m_aVoices[VoiceId].m_Vol = (int)(clamp(Volume, 0.0f, 1.0f) * 255.0f);
m_aVoices[VoiceId].m_Flags = Flags;
m_aVoices[VoiceId].m_Position = Position;
m_aVoices[VoiceId].m_Falloff = 0.0f;
m_aVoices[VoiceId].m_Shape = ISound::SHAPE_CIRCLE;
m_aVoices[VoiceId].m_Circle.m_Radius = 1500;
return CreateVoiceHandle(VoiceId, m_aVoices[VoiceId].m_Age);
}

ISound::CVoiceHandle CSound::PlayAt(int ChannelId, int SampleId, int Flags, float Volume, vec2 Position)
Expand All @@ -880,9 +881,12 @@ ISound::CVoiceHandle CSound::Play(int ChannelId, int SampleId, int Flags, float

void CSound::Pause(int SampleId)
{
dbg_assert(SampleId >= 0 && SampleId < NUM_SAMPLES, "SampleId invalid");

// TODO: a nice fade out
const CLockScope LockScope(m_SoundLock);
CSample *pSample = &m_aSamples[SampleId];
dbg_assert(m_aSamples[SampleId].IsLoaded(), "Sample not loaded");
for(auto &Voice : m_aVoices)
{
if(Voice.m_pSample == pSample)
Expand All @@ -895,9 +899,12 @@ void CSound::Pause(int SampleId)

void CSound::Stop(int SampleId)
{
dbg_assert(SampleId >= 0 && SampleId < NUM_SAMPLES, "SampleId invalid");

// TODO: a nice fade out
const CLockScope LockScope(m_SoundLock);
CSample *pSample = &m_aSamples[SampleId];
dbg_assert(m_aSamples[SampleId].IsLoaded(), "Sample not loaded");
for(auto &Voice : m_aVoices)
{
if(Voice.m_pSample == pSample)
Expand Down Expand Up @@ -945,8 +952,10 @@ void CSound::StopVoice(CVoiceHandle Voice)

bool CSound::IsPlaying(int SampleId)
{
dbg_assert(SampleId >= 0 && SampleId < NUM_SAMPLES, "SampleId invalid");
const CLockScope LockScope(m_SoundLock);
const CSample *pSample = &m_aSamples[SampleId];
dbg_assert(m_aSamples[SampleId].IsLoaded(), "Sample not loaded");
return std::any_of(std::begin(m_aVoices), std::end(m_aVoices), [pSample](const auto &Voice) { return Voice.m_pSample == pSample; });
}

Expand Down
27 changes: 16 additions & 11 deletions src/engine/client/sound.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ struct CSample
{
return m_NumFrames / (float)m_Rate;
}

bool IsLoaded() const
{
return m_pData != nullptr;
}
};

struct CChannel
Expand Down Expand Up @@ -68,12 +73,12 @@ class CSound : public IEngineSound
SDL_AudioDeviceID m_Device = 0;
CLock m_SoundLock;

CSample m_aSamples[NUM_SAMPLES] = {{0}};
int m_FirstFreeSampleIndex = 0;
CSample m_aSamples[NUM_SAMPLES] GUARDED_BY(m_SoundLock) = {{0}};
int m_FirstFreeSampleIndex GUARDED_BY(m_SoundLock) = 0;

CVoice m_aVoices[NUM_VOICES] = {{0}};
CChannel m_aChannels[NUM_CHANNELS] = {{255, 0}};
int m_NextVoice = 0;
CVoice m_aVoices[NUM_VOICES] GUARDED_BY(m_SoundLock) = {{0}};
CChannel m_aChannels[NUM_CHANNELS] GUARDED_BY(m_SoundLock) = {{255, 0}};
int m_NextVoice GUARDED_BY(m_SoundLock) = 0;
uint32_t m_MaxFrames = 0;

// This is not an std::atomic<vec2> as this would require linking with
Expand All @@ -88,7 +93,7 @@ class CSound : public IEngineSound

int *m_pMixBuffer = nullptr;

CSample *AllocSample();
CSample *AllocSample() REQUIRES(!m_SoundLock);
void RateConvert(CSample &Sample) const;

bool DecodeOpus(CSample &Sample, const void *pData, unsigned DataSize) const;
Expand All @@ -97,23 +102,23 @@ class CSound : public IEngineSound
void UpdateVolume();

public:
int Init() override;
int Init() override REQUIRES(!m_SoundLock);
int Update() override;
void Shutdown() override REQUIRES(!m_SoundLock);

bool IsSoundEnabled() override { return m_SoundEnabled; }

int LoadOpus(const char *pFilename, int StorageType = IStorage::TYPE_ALL) override REQUIRES(!m_SoundLock);
int LoadWV(const char *pFilename, int StorageType = IStorage::TYPE_ALL) override REQUIRES(!m_SoundLock);
int LoadOpusFromMem(const void *pData, unsigned DataSize, bool FromEditor) override REQUIRES(!m_SoundLock);
int LoadWVFromMem(const void *pData, unsigned DataSize, bool FromEditor) override REQUIRES(!m_SoundLock);
int LoadOpusFromMem(const void *pData, unsigned DataSize, bool ForceLoad) override REQUIRES(!m_SoundLock);
int LoadWVFromMem(const void *pData, unsigned DataSize, bool ForceLoad) override REQUIRES(!m_SoundLock);
void UnloadSample(int SampleId) override REQUIRES(!m_SoundLock);

float GetSampleTotalTime(int SampleId) override; // in s
float GetSampleTotalTime(int SampleId) override REQUIRES(!m_SoundLock); // in s
float GetSampleCurrentTime(int SampleId) override REQUIRES(!m_SoundLock); // in s
void SetSampleCurrentTime(int SampleId, float Time) override REQUIRES(!m_SoundLock);

void SetChannel(int ChannelId, float Vol, float Pan) override;
void SetChannel(int ChannelId, float Vol, float Pan) override REQUIRES(!m_SoundLock);
void SetListenerPosition(vec2 Position) override;

void SetVoiceVolume(CVoiceHandle Voice, float Volume) override REQUIRES(!m_SoundLock);
Expand Down
4 changes: 2 additions & 2 deletions src/engine/sound.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ class ISound : public IInterface

virtual int LoadOpus(const char *pFilename, int StorageType = IStorage::TYPE_ALL) = 0;
virtual int LoadWV(const char *pFilename, int StorageType = IStorage::TYPE_ALL) = 0;
virtual int LoadOpusFromMem(const void *pData, unsigned DataSize, bool FromEditor = false) = 0;
virtual int LoadWVFromMem(const void *pData, unsigned DataSize, bool FromEditor = false) = 0;
virtual int LoadOpusFromMem(const void *pData, unsigned DataSize, bool ForceLoad = false) = 0;
virtual int LoadWVFromMem(const void *pData, unsigned DataSize, bool ForceLoad = false) = 0;
virtual void UnloadSample(int SampleId) = 0;

virtual float GetSampleTotalTime(int SampleId) = 0; // in s
Expand Down

0 comments on commit 8f8ab84

Please sign in to comment.