diff --git a/stepmania/src/RageSound.cpp b/stepmania/src/RageSound.cpp index 7e7cbaaec6..87339445c6 100644 --- a/stepmania/src/RageSound.cpp +++ b/stepmania/src/RageSound.cpp @@ -57,10 +57,10 @@ RageSoundParams::RageSoundParams(): StopMode = M_AUTO; } -RageSound::RageSound() +RageSound::RageSound(): + m_Mutex( "RageSound" ) { ASSERT(SOUNDMAN); - LockMut(SOUNDMAN->lock); original = this; Sample = NULL; @@ -88,13 +88,24 @@ RageSound::~RageSound() } RageSound::RageSound(const RageSound &cpy): - RageSoundBase( cpy ) + RageSoundBase( cpy ), + m_Mutex( "RageSound" ) { ASSERT(SOUNDMAN); - LockMut(SOUNDMAN->lock); Sample = NULL; + *this = cpy; + + /* Register ourself, so we receive Update()s. We have a different ID than + * our parent. */ + ID = SOUNDMAN->RegisterSound( this ); +} + +RageSound &RageSound::operator=( const RageSound &cpy ) +{ + LockMut(cpy.m_Mutex); + original = cpy.original; m_Param = cpy.m_Param; decode_position = cpy.decode_position; @@ -103,19 +114,20 @@ RageSound::RageSound(const RageSound &cpy): playing_thread = 0; databuf.reserve(internal_buffer_size); + delete Sample; Sample = cpy.Sample->Copy(); /* Load() won't work on a copy if m_sFilePath is already set, so * copy this down here. */ m_sFilePath = cpy.m_sFilePath; - /* Register ourself, so we receive Update()s. We have a different ID than - * our parent. */ - ID = SOUNDMAN->RegisterSound( this ); + return *this; } void RageSound::Unload() { + LockMut(m_Mutex); + if(IsPlaying()) StopPlaying(); @@ -181,17 +193,8 @@ bool RageSound::Load(CString sSoundFilePath, int precache) return true; } -/* Read data at the rate we're playing it. We only do this to smooth out the rate - * we read data; the sound thread will always read more if it's needed. - * - * Actually, this isn't a good idea. The sound driver will read in small chunks, - * interleaving between files. For example, if four files are playing, and each - * is two chunks behind, it'll read a chunk from each file twice, instead of reading - * two chunks for each file at a time, which reduces the chance of underrun. */ void RageSound::Update(float delta) { - LockMut(SOUNDMAN->lock); - /* Erase old pos_map data. */ // CleanPosMap( pos_map ); } @@ -265,8 +268,6 @@ void RageSound::RateChange(char *buf, int &cnt, * actually read; 0 = EOF. */ int RageSound::FillBuf( int frames ) { - LockMut(SOUNDMAN->lock); - ASSERT(Sample); bool got_something = false; @@ -401,6 +402,13 @@ void FadeSound( Sint16 *buffer, int frames, float fStartVolume, float fEndVolume } } +/* RageSound::GetDataToPlay and RageSound::FillBuf are the main threaded API. These + * need to execute without blocking other threads from calling eg. GetPositionSeconds, + * since they may take some time to run. + Sample (r), databuf (r) + decode_position (r), databuf (r) + * + */ /* Retrieve audio data, for mixing. At the time of this call, the frameno at which the * sound will be played doesn't have to be known. Once committed, and the frameno * is known, call CommitPCMData. size is in bytes. @@ -414,9 +422,10 @@ bool RageSound::GetDataToPlay( int16_t *buffer, int size, int &sound_frame, int { int NumRewindsThisCall = 0; - LockMut(SOUNDMAN->lock); + /* We only update decode_position; only take a shared lock, so we don't block the main thread. */ +// LockMut(m_Mutex); - ASSERT(playing); + ASSERT_M( playing, ssprintf("%p", this) ); frames_stored = 0; sound_frame = decode_position; @@ -455,7 +464,7 @@ bool RageSound::GetDataToPlay( int16_t *buffer, int size, int &sound_frame, int return false; } - /* Rewind and start over. */ + /* Rewind and start over. XXX: this will take an exclusive lock */ SetPositionSeconds( m_Param.m_StartSecond ); /* Make sure we can get some data. If we can't, then we'll have @@ -517,8 +526,6 @@ void RageSound::CommitPlayingPosition( int64_t frameno, int pos, int got_frames * Be careful; this is called in a separate thread. */ int RageSound::GetPCM( char *buffer, int size, int64_t frameno ) { - LockMut(SOUNDMAN->lock); - ASSERT(playing); /* @@ -553,8 +560,6 @@ int RageSound::GetPCM( char *buffer, int size, int64_t frameno ) * playing, Stop is called. */ void RageSound::StartPlaying() { - LockMut(SOUNDMAN->lock); - // If no volume is set, use the default. if( m_Param.m_Volume == -1 ) m_Param.m_Volume = SOUNDMAN->GetMixVolume(); @@ -568,11 +573,16 @@ void RageSound::StartPlaying() GetLoadedFilePath().c_str(), m_Param.StartTime.Ago() ); /* Tell the sound manager to start mixing us. */ +// LOG->Trace("set playing true for %p (StartPlaying) (%s)", this, this->GetLoadedFilePath().c_str()); + playing = true; playing_thread = RageThread::GetCurrentThreadID(); SOUNDMAN->StartMixing(this); - SOUNDMAN->playing_sounds.insert( this ); + + SOUNDMAN->RegisterPlayingSound( this ); + +// LOG->Trace("StartPlaying %p finished (%s)", this, this->GetLoadedFilePath().c_str()); } void RageSound::StopPlaying() @@ -585,35 +595,47 @@ void RageSound::StopPlaying() /* Tell the sound driver to stop mixing this sound. */ SOUNDMAN->StopMixing(this); - SOUNDMAN->lock.Lock(); - SOUNDMAN->playing_sounds.erase( this ); - SOUNDMAN->lock.Unlock(); + SOUNDMAN->UnregisterPlayingSound( this ); + /* Lock the mutex after calling UnregisterPlayingSound. We must not make driver + * calls with our mutex locked (driver mutex < sound mutex). Nobody else will + * see our sound as not playing until we set playing = false. */ + m_Mutex.Lock(); + +// LOG->Trace("set playing false for %p (StopPlaying) (%s)", this, this->GetLoadedFilePath().c_str()); playing = false; playing_thread = 0; pos_map.Clear(); +// LOG->Trace("StopPlaying %p finished (%s)", this, this->GetLoadedFilePath().c_str()); + + m_Mutex.Unlock(); } /* This is similar to StopPlaying, except it's called by sound drivers when we're done * playing, rather than by users to as us to stop. (The only difference is that this * doesn't call SOUNDMAN->StopMixing; there's no reason to tell the sound driver to - * stop mixing, since they're the one telling us we're done.) */ + * stop mixing, since they're the one telling us we're done.) + * + * This is only called from the main thread. */ void RageSound::SoundIsFinishedPlaying() { if(!playing) return; + m_Mutex.Lock(); stopped_position = (int) GetPositionSecondsInternal(); - SOUNDMAN->lock.Lock(); - SOUNDMAN->playing_sounds.erase( this ); - SOUNDMAN->lock.Unlock(); + SOUNDMAN->UnregisterPlayingSound( this ); +// LOG->Trace("set playing false for %p (SoundIsFinishedPlaying) (%s)", this, this->GetLoadedFilePath().c_str()); playing = false; playing_thread = 0; pos_map.Clear(); +// LOG->Trace("SoundIsFinishedPlaying %p finished (%s)", this, this->GetLoadedFilePath().c_str()); + + m_Mutex.Unlock(); } RageSound *RageSound::Play( const RageSoundParams *params ) @@ -645,7 +667,7 @@ float RageSound::GetLengthSeconds() /* Get the position in frames. */ int64_t RageSound::GetPositionSecondsInternal( bool *approximate ) const { - LockMut(SOUNDMAN->lock); + LockMut(m_Mutex); if( approximate ) *approximate = false; @@ -667,9 +689,6 @@ int64_t RageSound::GetPositionSecondsInternal( bool *approximate ) const /* Get our current hardware position. */ int64_t cur_frame = SOUNDMAN->GetPosition(this); - /* Before using pos_map, flush any incoming positions. */ - SOUNDMAN->FlushPosMapQueue(); - return pos_map.Search( cur_frame, approximate ); } @@ -684,7 +703,7 @@ int64_t RageSound::GetPositionSecondsInternal( bool *approximate ) const float RageSound::GetPositionSeconds( bool *approximate, RageTimer *Timestamp ) const { - LockMut(SOUNDMAN->lock); + LockMut(m_Mutex); if( Timestamp ) { @@ -715,10 +734,7 @@ int RageSound::GetSampleRate() const bool RageSound::SetPositionFrames( int frames ) { - /* This can take a while. Only lock the sound buffer if we're actually playing. */ - LockMutex L(SOUNDMAN->lock); - if(!playing) - L.Unlock(); + LockMut(m_Mutex); { /* "decode_position" records the number of frames we've output to the @@ -788,6 +804,16 @@ float RageSound::GetVolume() const return m_Param.m_Volume; } +void RageSound::LockSound() +{ + m_Mutex.Lock(); +} + +void RageSound::UnlockSound() +{ + m_Mutex.Unlock(); +} + float RageSound::GetPlaybackRate() const { return float(m_Param.speed_input_samples) / m_Param.speed_output_samples; diff --git a/stepmania/src/RageSound.h b/stepmania/src/RageSound.h index e37d6a90e5..ec6d40cd3f 100644 --- a/stepmania/src/RageSound.h +++ b/stepmania/src/RageSound.h @@ -70,6 +70,7 @@ public: RageSound(); ~RageSound(); RageSound(const RageSound &cpy); + RageSound &operator=( const RageSound &cpy ); /* If cache == true (1), we'll preload the entire file into memory if * it's small enough. If this is done, a large number of copies of the @@ -107,6 +108,10 @@ public: bool IsPlaying() const { return playing; } unsigned GetPlayingThread() const { return playing_thread; } + /* Lock and unlock this sound. */ + void LockSound(); + void UnlockSound(); + float GetPlaybackRate() const; RageTimer GetStartTime() const; float GetVolume() const; @@ -119,6 +124,8 @@ private: * this is ourself. */ RageSound *original; + mutable RageMutex m_Mutex; + SoundReader *Sample; CircBuf databuf; int FillBuf(int bytes); diff --git a/stepmania/src/RageSoundManager.cpp b/stepmania/src/RageSoundManager.cpp index 32f3193ae0..ca96485d41 100644 --- a/stepmania/src/RageSoundManager.cpp +++ b/stepmania/src/RageSoundManager.cpp @@ -14,10 +14,27 @@ #include "arch/Sound/RageSoundDriver.h" #include "SDL_audio.h" +/* + * This mutex is locked before Update() deletes old sounds from owned_sounds. Lock + * this mutex if you want to ensure that sounds remain valid. (Other threads may + * still delete them; this only guarantees that RageSoundManager won't.) + * + * The lock ordering requirements are: + * g_DeletionMutex before RageSound::Lock + * RageSound::Lock before g_SoundManMutex + * RageSound::Lock must not be locked when calling driver calls (since the driver + * may lock a mutex and then make RageSound calls back) + * + * (This is important: you must not make RageSound calls that might lock while holding + * g_SoundManMutex, but you can do so while holding g_DeletionMutex.) + */ + +static RageMutex g_DeletionMutex("SoundDeletionMutex"); +static RageMutex g_SoundManMutex("SoundMan"); + RageSoundManager *SOUNDMAN = NULL; -RageSoundManager::RageSoundManager(CString drivers): - lock("RageSoundManager") +RageSoundManager::RageSoundManager(CString drivers) { /* needs to be done first */ SOUNDMAN = this; @@ -39,12 +56,14 @@ RageSoundManager::RageSoundManager(CString drivers): RageSoundManager::~RageSoundManager() { - lock.Lock(); + g_SoundManMutex.Lock(); /* lock for access to owned_sounds */ + set sounds = owned_sounds; + g_SoundManMutex.Unlock(); /* finished with owned_sounds */ + /* Clear any sounds that we own and havn't freed yet. */ - set::iterator j = owned_sounds.begin(); - while(j != owned_sounds.end()) + set::iterator j = sounds.begin(); + while(j != sounds.end()) delete *(j++); - lock.Unlock(); /* Don't lock while deleting the driver (the decoder thread might deadlock). */ delete driver; @@ -67,11 +86,12 @@ int64_t RageSoundManager::GetPosition( const RageSoundBase *snd ) const void RageSoundManager::Update(float delta) { - LockMut(lock); - FlushPosMapQueue(); + g_DeletionMutex.Lock(); + /* Scan the owned_sounds list for sounds that are no longer playing, and delete them. */ + g_SoundManMutex.Lock(); /* lock for access to owned_sounds */ set::iterator it; set ToDelete; for( it = owned_sounds.begin(); it != owned_sounds.end(); ++it ) @@ -79,14 +99,18 @@ void RageSoundManager::Update(float delta) ToDelete.insert( *it ); for( it = ToDelete.begin(); it != ToDelete.end(); ++it ) - { - delete *it; owned_sounds.erase( *it ); - } + g_SoundManMutex.Unlock(); /* finished with owned_sounds */ - for(set::iterator i = all_sounds.begin(); - i != all_sounds.end(); ++i) - (*i)->Update(delta); + /* We can safely delete sounds while holding g_DeletionMutex, but not while + * holding g_SoundManMutex (see the mutex ordering at the top of the file). */ + for( it = ToDelete.begin(); it != ToDelete.end(); ++it ) + delete *it; + g_DeletionMutex.Unlock(); + +// for(set::iterator i = all_sounds.begin(); +// i != all_sounds.end(); ++i) +// (*i)->Update(delta); driver->Update(delta); } @@ -94,7 +118,7 @@ void RageSoundManager::Update(float delta) /* Register the given sound, and return a unique ID. */ int RageSoundManager::RegisterSound( RageSound *p ) { - LockMut(lock); + LockMut(g_SoundManMutex); /* lock for access to all_sounds and iID */ all_sounds.insert( p ); @@ -104,8 +128,23 @@ int RageSoundManager::RegisterSound( RageSound *p ) void RageSoundManager::UnregisterSound( RageSound *p ) { - LockMut(lock); + g_SoundManMutex.Lock(); /* lock for access to all_sounds */ all_sounds.erase( p ); + g_SoundManMutex.Unlock(); /* finished with all_sounds */ +} + +void RageSoundManager::RegisterPlayingSound( RageSound *p ) +{ + g_SoundManMutex.Lock(); /* lock for access to playing_sounds */ + SOUNDMAN->playing_sounds.insert( p ); + g_SoundManMutex.Unlock(); /* finished with playing_sounds */ +} + +void RageSoundManager::UnregisterPlayingSound( RageSound *p ) +{ + g_SoundManMutex.Lock(); /* lock for access to playing_sounds */ + SOUNDMAN->playing_sounds.erase( p ); + g_SoundManMutex.Unlock(); /* finished with playing_sounds */ } void RageSoundManager::CommitPlayingPosition( int ID, int64_t frameno, int pos, int got_frames ) @@ -120,27 +159,36 @@ void RageSoundManager::CommitPlayingPosition( int ID, int64_t frameno, int pos, pos_map_queue.write( &p, 1 ); } +RageSound *RageSoundManager::GetSoundByID( int ID ) +{ + LockMut(g_SoundManMutex); /* lock for access to all_sounds */ + + /* Find the sound with p.ID. */ + set::iterator it; + for( it = all_sounds.begin(); it != all_sounds.end(); ++it ) + if( (*it)->GetID() == ID ) + return *it; + return NULL; +} + +/* This is only called by RageSoundManager::Update. */ void RageSoundManager::FlushPosMapQueue() { - LockMut(SOUNDMAN->lock); queued_pos_map_t p; + /* We don't need to lock to access pos_map_queue. */ while( pos_map_queue.read( &p, 1 ) ) { - /* Find the sound with p.ID. */ - set::iterator it; - for( it = all_sounds.begin(); it != all_sounds.end(); ++it ) - if( (*it)->GetID() == p.ID ) - break; + RageSound *pSound = GetSoundByID( p.ID ); /* If we can't find the ID, the sound was probably deleted before we got here. */ - if( it == all_sounds.end() ) + if( pSound == NULL ) { LOG->Trace("ignored unknown (stale?) commit ID %i", p.ID); continue; } - (*it)->CommitPlayingPosition( p.frameno, p.pos, p.got_frames ); + pSound->CommitPlayingPosition( p.frameno, p.pos, p.got_frames ); } } @@ -156,8 +204,6 @@ int RageSoundManager::GetDriverSampleRate( int rate ) const RageSound *RageSoundManager::PlaySound( RageSound &snd, const RageSoundParams *params ) { - LockMut(lock); - RageSound *sound_to_play; if(!snd.IsPlaying()) sound_to_play = &snd; @@ -166,7 +212,9 @@ RageSound *RageSoundManager::PlaySound( RageSound &snd, const RageSoundParams *p sound_to_play = new RageSound(snd); /* We're responsible for freeing it. */ + g_SoundManMutex.Lock(); /* lock for access to owned_sounds */ owned_sounds.insert(sound_to_play); + g_SoundManMutex.Unlock(); /* finished with owned_sounds */ } if( params ) @@ -180,45 +228,83 @@ RageSound *RageSoundManager::PlaySound( RageSound &snd, const RageSoundParams *p return sound_to_play; } +/* Stop playing all playing sounds derived from the same parent as snd. */ void RageSoundManager::StopPlayingAllCopiesOfSound(RageSound &snd) { - LockMut(lock); + g_DeletionMutex.Lock(); - /* Stop playing all playing sounds derived from the same parent as snd. */ vector snds; - GetCopies(snd, snds); - for(vector::iterator i = snds.begin(); i != snds.end(); i++) + GetCopies( snd, snds ); + + vector::iterator it; + for( it = snds.begin(); it != snds.end(); ++it ) { - if((*i)->IsPlaying()) - (*i)->StopPlaying(); + if( (*it)->IsPlaying() ) + (*it)->StopPlaying(); } + + g_DeletionMutex.Unlock(); +} + +/* XXX: If this is ever called from a thread, it should take a bLockSounds parameter, + * like GetCopies. */ +set RageSoundManager::GetPlayingSounds() const +{ + LockMut(g_SoundManMutex); /* lock for access to playing_sounds */ + return playing_sounds; } void RageSoundManager::StopPlayingSoundsForThisThread() { /* Lock to make sure sounds don't become invalidated below before we get to them. */ - LockMut(lock); + g_DeletionMutex.Lock(); set Sounds = GetPlayingSounds(); - for( set::iterator it = Sounds.begin(); it != Sounds.end(); ++it ) + set::iterator it; + for( it = Sounds.begin(); it != Sounds.end(); ++it ) { if( (*it)->GetPlayingThread() != RageThread::GetCurrentThreadID() ) continue; (*it)->Stop(); } + g_DeletionMutex.Unlock(); } -void RageSoundManager::GetCopies(RageSound &snd, vector &snds) +/* + * If bLockSounds is true, all returned sounds will be locked; you must call Unlock() + * on all returned sounds when you're done. This is used for thread safety: without + * it, if this is called in a separate thread, returned copies might stop playing + * and be deleted by RageSoundManager::Update before you're done with them. + * + * If bLockSounds is false, sounds are not locked. This is only safe to use in the same + * thread as RageSoundManager::Update (the gameplay thread). + */ +void RageSoundManager::GetCopies( RageSound &snd, vector &snds, bool bLockSounds ) { - LockMut(lock); + snds.clear(); + /* Locking this means that Update() will not delete sounds. g_SoundManMutex does that, + * too, but we can't hold g_SoundManMutex when we lock sounds. */ + g_DeletionMutex.Lock(); + + g_SoundManMutex.Lock(); /* lock for access to all_sounds */ + set sounds = all_sounds; + g_SoundManMutex.Unlock(); /* finished with owned_sounds */ + RageSound *parent = snd.GetOriginal(); - snds.clear(); - for(set::iterator i = playing_sounds.begin(); - i != playing_sounds.end(); i++) - if((*i)->GetOriginal() == parent) - snds.push_back(*i); + set::iterator it; + for( it = sounds.begin(); it != sounds.end(); ++it ) + { + if( (*it)->GetOriginal() != parent ) + continue; + if( bLockSounds ) + (*it)->LockSound(); + + snds.push_back( *it ); + } + + g_DeletionMutex.Unlock(); } /* Don't hold the lock when we don't have to. We call this function from other @@ -234,17 +320,17 @@ void RageSoundManager::PlayOnce( CString sPath ) /* We're responsible for freeing it. Add it to owned_sounds *after* we start * playing, so RageSoundManager::Update doesn't free it before we actually start * it. */ - LockMut(lock); - lock.Lock(); + g_SoundManMutex.Lock(); /* lock for access to owned_sounds */ owned_sounds.insert(snd); - lock.Unlock(); + g_SoundManMutex.Unlock(); /* finished with owned_sounds */ } void RageSoundManager::SetPrefs(float MixVol) { - LockMut(lock); - + g_SoundManMutex.Lock(); /* lock for access to MixVolume */ MixVolume = MixVol; + g_SoundManMutex.Unlock(); /* finished with MixVolume */ + driver->VolumeChanged(); } diff --git a/stepmania/src/RageSoundManager.h b/stepmania/src/RageSoundManager.h index f74036c31e..994a9a3139 100644 --- a/stepmania/src/RageSoundManager.h +++ b/stepmania/src/RageSoundManager.h @@ -4,7 +4,6 @@ #include #include #include "SDL_utils.h" -#include "RageThreads.h" #include "RageUtil_CircularBuffer.h" class RageSound; @@ -17,7 +16,11 @@ class RageSoundManager /* Set of sounds that we've taken over (and are responsible for deleting * when they're finished playing): */ set owned_sounds; + set playing_sounds; + /* A list of all sounds that currently exist. */ + set all_sounds; + RageSoundDriver *driver; /* Prefs: */ @@ -31,8 +34,6 @@ class RageSoundManager CircBuf pos_map_queue; public: - RageMutex lock; - RageSoundManager(CString drivers); ~RageSoundManager(); @@ -45,11 +46,12 @@ public: int64_t GetPosition( const RageSoundBase *snd ) const; /* used by RageSound */ int RegisterSound( RageSound *p ); /* used by RageSound */ void UnregisterSound( RageSound *p ); /* used by RageSound */ + void RegisterPlayingSound( RageSound *p ); /* used by RageSound */ + void UnregisterPlayingSound( RageSound *p ); /* used by RageSound */ void CommitPlayingPosition( int ID, int64_t frameno, int pos, int got_bytes ); /* used by drivers */ - void FlushPosMapQueue(); /* used by RageSound */ float GetPlayLatency() const; int GetDriverSampleRate( int rate ) const; - const set &GetPlayingSounds() const { return playing_sounds; } + set GetPlayingSounds() const; void PlayOnce( CString sPath ); @@ -60,15 +62,13 @@ public: * before exiting a thread. */ void StopPlayingSoundsForThisThread(); - /* A list of all sounds that currently exist. RageSound adds and removes - * itself to this. */ - set all_sounds; - - /* RageSound adds and removes itself to this. */ - set playing_sounds; - void GetCopies(RageSound &snd, vector &snds); + void GetCopies( RageSound &snd, vector &snds, bool bLockSounds=false ); static void AttenuateBuf( Sint16 *buf, int samples, float vol ); + +private: + void FlushPosMapQueue(); + RageSound *GetSoundByID( int ID ); }; /* This inputs and outputs 16-bit 44khz stereo input. */