From 9c9666da9d8c4b8b7a618ef5c5a9826400538681 Mon Sep 17 00:00:00 2001 From: Casey Langen Date: Wed, 29 Jun 2016 01:56:01 -0700 Subject: [PATCH] Fixed a deadlock that could occur if an IOutput blocks until buffers are freed in Stop(). --- src/contrib/coreaudioout/CoreAudioOut.cpp | 14 ++--- src/core/audio/GaplessTransport.cpp | 64 +++++++++++++++-------- src/core/audio/GaplessTransport.h | 7 ++- 3 files changed, 55 insertions(+), 30 deletions(-) diff --git a/src/contrib/coreaudioout/CoreAudioOut.cpp b/src/contrib/coreaudioout/CoreAudioOut.cpp index 694e2bec3..ce29179dd 100644 --- a/src/contrib/coreaudioout/CoreAudioOut.cpp +++ b/src/contrib/coreaudioout/CoreAudioOut.cpp @@ -69,14 +69,16 @@ size_t countBuffersWithProvider( } void CoreAudioOut::NotifyBufferCompleted(BufferContext *context) { - boost::recursive_mutex::scoped_lock lock(this->mutex); + { + boost::recursive_mutex::scoped_lock lock(this->mutex); - auto it = this->buffers.begin(); - while (it != this->buffers.end()) { - if (*it == context) { - this->buffers.erase(it); + auto it = this->buffers.begin(); + while (it != this->buffers.end()) { + if (*it == context) { + this->buffers.erase(it); + } + ++it; } - ++it; } context->provider->OnBufferProcessed(context->buffer); diff --git a/src/core/audio/GaplessTransport.cpp b/src/core/audio/GaplessTransport.cpp index fa1379fe1..32e3d0609 100644 --- a/src/core/audio/GaplessTransport.cpp +++ b/src/core/audio/GaplessTransport.cpp @@ -46,7 +46,7 @@ static std::string TAG = "Transport"; #define RESET_NEXT_PLAYER() \ delete this->nextPlayer; \ - this->nextPlayer = NULL; + this->nextPlayer = nullptr; #define DEFER(x, y) \ { \ @@ -65,7 +65,7 @@ static void deletePlayer(Player* p) { GaplessTransport::GaplessTransport() : volume(1.0) , state(PlaybackStopped) -, nextPlayer(NULL) +, nextPlayer(nullptr) , nextCanStart(false) { this->output = Player::CreateDefaultOutput(); } @@ -103,43 +103,50 @@ void GaplessTransport::Start(const std::string& url) { void GaplessTransport::StartWithPlayer(Player* newPlayer) { if (newPlayer) { + newPlayer->PlaybackStarted.connect(this, &GaplessTransport::OnPlaybackStarted); + newPlayer->PlaybackAlmostEnded.connect(this, &GaplessTransport::OnPlaybackAlmostEnded); + newPlayer->PlaybackFinished.connect(this, &GaplessTransport::OnPlaybackFinished); + newPlayer->PlaybackError.connect(this, &GaplessTransport::OnPlaybackError); + + bool playingNext = false; + { boost::recursive_mutex::scoped_lock lock(this->stateMutex); - bool playingNext = (newPlayer == nextPlayer); + playingNext = (newPlayer == nextPlayer); if (newPlayer != nextPlayer) { delete nextPlayer; } - this->nextPlayer = NULL; - - /* first argument suppresses the "Stop" event from getting triggered, - the second param is used for gapless playback -- we won't stop the output - and will allow pending buffers to finish */ - this->Stop(true, !playingNext); - this->SetNextCanStart(false); - - newPlayer->PlaybackStarted.connect(this, &GaplessTransport::OnPlaybackStarted); - newPlayer->PlaybackAlmostEnded.connect(this, &GaplessTransport::OnPlaybackAlmostEnded); - newPlayer->PlaybackFinished.connect(this, &GaplessTransport::OnPlaybackFinished); - newPlayer->PlaybackError.connect(this, &GaplessTransport::OnPlaybackError); - - musik::debug::info(TAG, "play()"); - + this->nextPlayer = nullptr; this->active.push_back(newPlayer); - this->output->Resume(); - newPlayer->Play(); } + /* first argument suppresses the "Stop" event from getting triggered, + the second param is used for gapless playback -- we won't stop the output + and will allow pending buffers to finish if we're not automatically + playing the next track. note we do this outside of critical section so + outputs *can* stop buffers immediately, and not to worry about causing a + deadlock. */ + this->StopInternal(true, !playingNext, newPlayer); + this->SetNextCanStart(false); + this->output->Resume(); + newPlayer->Play(); + musik::debug::info(TAG, "play()"); + this->RaiseStreamEvent(ITransport::StreamScheduled, newPlayer); } } void GaplessTransport::Stop() { - this->Stop(false, true); + this->StopInternal(false, true); } -void GaplessTransport::Stop(bool suppressStopEvent, bool stopOutput) { +void GaplessTransport::StopInternal( + bool suppressStopEvent, + bool stopOutput, + Player* exclude) +{ musik::debug::info(TAG, "stop"); /* if we stop the output, we kill all of the Players immediately. @@ -151,7 +158,18 @@ void GaplessTransport::Stop(bool suppressStopEvent, bool stopOutput) { { boost::recursive_mutex::scoped_lock lock(this->stateMutex); RESET_NEXT_PLAYER(); - std::swap(toDelete, this->active); + + /* move all but the excluded player to the toDelete set. */ + auto it = this->active.begin(); + while (it != this->active.end()) { + if (*it != exclude) { + toDelete.push_back(*it); + it = this->active.erase(it); + } + else { + ++it; + } + } } /* delete these in the background to avoid deadlock in some cases diff --git a/src/core/audio/GaplessTransport.h b/src/core/audio/GaplessTransport.h index e88b31068..9d4370146 100644 --- a/src/core/audio/GaplessTransport.h +++ b/src/core/audio/GaplessTransport.h @@ -67,7 +67,12 @@ namespace musik { namespace core { namespace audio { private: void StartWithPlayer(Player* player); - void Stop(bool suppressStopEvent, bool stopOutput); + + void StopInternal( + bool suppressStopEvent, + bool stopOutput, + Player* exclude = nullptr); + void RemoveActive(Player* player); void DeletePlayers(std::list players); void SetNextCanStart(bool nextCanStart);