From cf348f9fae824843e2fe2a4911308aaa901b076c Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 26 Apr 2019 18:34:16 +0200 Subject: [PATCH] decoder/Control: pass std::unique_lock<> to Cond::wait() --- src/decoder/Bridge.cxx | 8 ++--- src/decoder/Control.cxx | 19 ++++++----- src/decoder/Control.hxx | 26 ++++++++------- src/decoder/Thread.cxx | 4 +-- src/player/Thread.cxx | 73 +++++++++++++++++++++-------------------- 5 files changed, 68 insertions(+), 62 deletions(-) diff --git a/src/decoder/Bridge.cxx b/src/decoder/Bridge.cxx index c39d75036..28e0d0104 100644 --- a/src/decoder/Bridge.cxx +++ b/src/decoder/Bridge.cxx @@ -74,10 +74,10 @@ DecoderBridge::CheckCancelRead() const noexcept * one. */ static DecoderCommand -need_chunks(DecoderControl &dc) noexcept +NeedChunks(DecoderControl &dc, std::unique_lock &lock) noexcept { if (dc.command == DecoderCommand::NONE) - dc.Wait(); + dc.Wait(lock); return dc.command; } @@ -85,8 +85,8 @@ need_chunks(DecoderControl &dc) noexcept static DecoderCommand LockNeedChunks(DecoderControl &dc) noexcept { - const std::lock_guard protect(dc.mutex); - return need_chunks(dc); + std::unique_lock lock(dc.mutex); + return NeedChunks(dc, lock); } MusicChunk * diff --git a/src/decoder/Control.cxx b/src/decoder/Control.cxx index 8fc3334b2..75473d5e2 100644 --- a/src/decoder/Control.cxx +++ b/src/decoder/Control.cxx @@ -39,12 +39,12 @@ DecoderControl::~DecoderControl() noexcept } void -DecoderControl::WaitForDecoder() noexcept +DecoderControl::WaitForDecoder(std::unique_lock &lock) noexcept { assert(!client_is_waiting); client_is_waiting = true; - client_cond.wait(mutex); + client_cond.wait(lock); assert(client_is_waiting); client_is_waiting = false; @@ -88,7 +88,8 @@ DecoderControl::IsCurrentSong(const DetachedSong &_song) const noexcept } void -DecoderControl::Start(std::unique_ptr _song, +DecoderControl::Start(std::unique_lock &lock, + std::unique_ptr _song, SongTime _start_time, SongTime _end_time, MusicBuffer &_buffer, std::shared_ptr _pipe) noexcept @@ -103,25 +104,25 @@ DecoderControl::Start(std::unique_ptr _song, pipe = std::move(_pipe); ClearError(); - SynchronousCommandLocked(DecoderCommand::START); + SynchronousCommandLocked(lock, DecoderCommand::START); } void -DecoderControl::Stop() noexcept +DecoderControl::Stop(std::unique_lock &lock) noexcept { if (command != DecoderCommand::NONE) /* Attempt to cancel the current command. If it's too late and the decoder thread is already executing the old command, we'll call STOP again in this function (see below). */ - SynchronousCommandLocked(DecoderCommand::STOP); + SynchronousCommandLocked(lock, DecoderCommand::STOP); if (state != DecoderState::STOP && state != DecoderState::ERROR) - SynchronousCommandLocked(DecoderCommand::STOP); + SynchronousCommandLocked(lock, DecoderCommand::STOP); } void -DecoderControl::Seek(SongTime t) +DecoderControl::Seek(std::unique_lock &lock, SongTime t) { assert(state != DecoderState::START); assert(state != DecoderState::ERROR); @@ -145,7 +146,7 @@ DecoderControl::Seek(SongTime t) seek_time = t; seek_error = false; - SynchronousCommandLocked(DecoderCommand::SEEK); + SynchronousCommandLocked(lock, DecoderCommand::SEEK); if (seek_error) throw std::runtime_error("Decoder failed to seek"); diff --git a/src/decoder/Control.hxx b/src/decoder/Control.hxx index 91806b6a2..f6a45a2b4 100644 --- a/src/decoder/Control.hxx +++ b/src/decoder/Control.hxx @@ -207,8 +207,8 @@ public: * is only valid in the decoder thread. The object must be locked * prior to calling this function. */ - void Wait() noexcept { - cond.wait(mutex); + void Wait(std::unique_lock &lock) noexcept { + cond.wait(lock); } /** @@ -218,7 +218,7 @@ public: * * Caller must hold the lock. */ - void WaitForDecoder() noexcept; + void WaitForDecoder(std::unique_lock &lock) noexcept; bool IsIdle() const noexcept { return state == DecoderState::STOP || @@ -318,9 +318,9 @@ private: * To be called from the client thread. Caller must lock the * object. */ - void WaitCommandLocked() noexcept { + void WaitCommandLocked(std::unique_lock &lock) noexcept { while (command != DecoderCommand::NONE) - WaitForDecoder(); + WaitForDecoder(lock); } /** @@ -330,10 +330,11 @@ private: * To be called from the client thread. Caller must lock the * object. */ - void SynchronousCommandLocked(DecoderCommand cmd) noexcept { + void SynchronousCommandLocked(std::unique_lock &lock, + DecoderCommand cmd) noexcept { command = cmd; Signal(); - WaitCommandLocked(); + WaitCommandLocked(lock); } /** @@ -344,9 +345,9 @@ private: * object. */ void LockSynchronousCommand(DecoderCommand cmd) noexcept { - const std::lock_guard protect(mutex); + std::unique_lock lock(mutex); ClearError(); - SynchronousCommandLocked(cmd); + SynchronousCommandLocked(lock, cmd); } void LockAsynchronousCommand(DecoderCommand cmd) noexcept { @@ -382,7 +383,8 @@ public: * @param pipe the pipe which receives the decoded chunks (owned by * the caller) */ - void Start(std::unique_ptr song, + void Start(std::unique_lock &lock, + std::unique_ptr song, SongTime start_time, SongTime end_time, MusicBuffer &buffer, std::shared_ptr pipe) noexcept; @@ -390,14 +392,14 @@ public: /** * Caller must lock the object. */ - void Stop() noexcept; + void Stop(std::unique_lock &lock) noexcept; /** * Throws #std::runtime_error on error. * * Caller must lock the object. */ - void Seek(SongTime t); + void Seek(std::unique_lock &lock, SongTime t); void Quit() noexcept; diff --git a/src/decoder/Thread.cxx b/src/decoder/Thread.cxx index cc2aa3d9d..91a4344d2 100644 --- a/src/decoder/Thread.cxx +++ b/src/decoder/Thread.cxx @@ -487,7 +487,7 @@ DecoderControl::RunThread() noexcept { SetThreadName("decoder"); - const std::lock_guard protect(mutex); + std::unique_lock lock(mutex); do { assert(state == DecoderState::STOP || @@ -528,7 +528,7 @@ DecoderControl::RunThread() noexcept break; case DecoderCommand::NONE: - Wait(); + Wait(lock); break; } } while (command != DecoderCommand::NONE || !quit); diff --git a/src/player/Thread.cxx b/src/player/Thread.cxx index 3e30fdf32..8d424e53e 100644 --- a/src/player/Thread.cxx +++ b/src/player/Thread.cxx @@ -222,7 +222,8 @@ private: * * Caller must lock the mutex. */ - void StartDecoder(std::shared_ptr pipe) noexcept; + void StartDecoder(std::unique_lock &lock, + std::shared_ptr pipe) noexcept; /** * The decoder has acknowledged the "START" command (see @@ -235,14 +236,14 @@ private: * @return false if the decoder has failed, true on success * (though the decoder startup may or may not yet be finished) */ - bool CheckDecoderStartup() noexcept; + bool CheckDecoderStartup(std::unique_lock &lock) noexcept; /** * Stop the decoder and clears (and frees) its music pipe. * * Caller must lock the mutex. */ - void StopDecoder() noexcept; + void StopDecoder(std::unique_lock &lock) noexcept; /** * Is the decoder still busy on the same song as the player? @@ -275,7 +276,8 @@ private: * * @return false if the decoder has failed */ - bool SeekDecoder(SongTime seek_time) noexcept; + bool SeekDecoder(std::unique_lock &lock, + SongTime seek_time) noexcept; /** * This is the handler for the #PlayerCommand::SEEK command. @@ -284,7 +286,7 @@ private: * * @return false if the decoder has failed */ - bool SeekDecoder() noexcept; + bool SeekDecoder(std::unique_lock &lock) noexcept; void CancelPendingSeek() noexcept { pending_seek = SongTime::zero(); @@ -342,7 +344,7 @@ private: * * @return false to stop playback */ - bool ProcessCommand() noexcept; + bool ProcessCommand(std::unique_lock &lock) noexcept; /** * This is called at the border between two songs: the audio output @@ -363,7 +365,8 @@ public: }; void -Player::StartDecoder(std::shared_ptr _pipe) noexcept +Player::StartDecoder(std::unique_lock &lock, + std::shared_ptr _pipe) noexcept { assert(queued || pc.command == PlayerCommand::SEEK); assert(pc.next_song != nullptr); @@ -373,17 +376,17 @@ Player::StartDecoder(std::shared_ptr _pipe) noexcept SongTime start_time = pc.next_song->GetStartTime() + pc.seek_time; - dc.Start(std::make_unique(*pc.next_song), + dc.Start(lock, std::make_unique(*pc.next_song), start_time, pc.next_song->GetEndTime(), buffer, std::move(_pipe)); } void -Player::StopDecoder() noexcept +Player::StopDecoder(std::unique_lock &lock) noexcept { const PlayerControl::ScopeOccupied occupied(pc); - dc.Stop(); + dc.Stop(lock); if (dc.pipe != nullptr) { /* clear and free the decoder pipe */ @@ -500,8 +503,8 @@ Player::OpenOutput() noexcept return true; } -bool -Player::CheckDecoderStartup() noexcept +inline bool +Player::CheckDecoderStartup(std::unique_lock &lock) noexcept { assert(decoder_starting); @@ -534,7 +537,7 @@ Player::CheckDecoderStartup() noexcept if (pending_seek > SongTime::zero()) { assert(pc.seeking); - bool success = SeekDecoder(pending_seek); + bool success = SeekDecoder(lock, pending_seek); pc.seeking = false; pc.ClientSignal(); if (!success) @@ -562,14 +565,14 @@ Player::CheckDecoderStartup() noexcept } else { /* the decoder is not yet ready; wait some more */ - dc.WaitForDecoder(); + dc.WaitForDecoder(lock); return true; } } bool -Player::SeekDecoder(SongTime seek_time) noexcept +Player::SeekDecoder(std::unique_lock &lock, SongTime seek_time) noexcept { assert(song); assert(!decoder_starting); @@ -583,7 +586,7 @@ Player::SeekDecoder(SongTime seek_time) noexcept try { const PlayerControl::ScopeOccupied occupied(pc); - dc.Seek(song->GetStartTime() + seek_time); + dc.Seek(lock, song->GetStartTime() + seek_time); } catch (...) { /* decoder failure */ pc.SetError(PlayerError::DECODER, std::current_exception()); @@ -595,7 +598,7 @@ Player::SeekDecoder(SongTime seek_time) noexcept } inline bool -Player::SeekDecoder() noexcept +Player::SeekDecoder(std::unique_lock &lock) noexcept { assert(pc.next_song != nullptr); @@ -612,14 +615,14 @@ Player::SeekDecoder() noexcept /* the decoder is already decoding the "next" song - stop it and start the previous song again */ - StopDecoder(); + StopDecoder(lock); /* clear music chunks which might still reside in the pipe */ pipe->Clear(); /* re-start the decoder */ - StartDecoder(pipe); + StartDecoder(lock, pipe); ActivateDecoder(); pc.seeking = true; @@ -650,7 +653,7 @@ Player::SeekDecoder() noexcept } else { /* send the SEEK command */ - if (!SeekDecoder(pc.seek_time)) { + if (!SeekDecoder(lock, pc.seek_time)) { pc.CommandFinished(); return false; } @@ -668,7 +671,7 @@ Player::SeekDecoder() noexcept } inline bool -Player::ProcessCommand() noexcept +Player::ProcessCommand(std::unique_lock &lock) noexcept { switch (pc.command) { case PlayerCommand::NONE: @@ -697,7 +700,7 @@ Player::ProcessCommand() noexcept pc.CommandFinished(); if (dc.IsIdle()) - StartDecoder(std::make_shared()); + StartDecoder(lock, std::make_shared()); break; @@ -720,7 +723,7 @@ Player::ProcessCommand() noexcept break; case PlayerCommand::SEEK: - return SeekDecoder(); + return SeekDecoder(lock); case PlayerCommand::CANCEL: if (pc.next_song == nullptr) @@ -732,7 +735,7 @@ Player::ProcessCommand() noexcept if (IsDecoderAtNextSong()) /* the decoder is already decoding the song - stop it and reset the position */ - StopDecoder(); + StopDecoder(lock); pc.next_song.reset(); queued = false; @@ -865,7 +868,7 @@ Player::PlayNextChunk() noexcept } else { /* there are not enough decoded chunks yet */ - const std::lock_guard lock(pc.mutex); + std::unique_lock lock(pc.mutex); if (dc.IsIdle()) { /* the decoder isn't running, abort @@ -874,7 +877,7 @@ Player::PlayNextChunk() noexcept } else { /* wait for the decoder */ dc.Signal(); - dc.WaitForDecoder(); + dc.WaitForDecoder(lock); return true; } @@ -960,20 +963,20 @@ Player::Run() noexcept { pipe = std::make_shared(); - const std::lock_guard lock(pc.mutex); + std::unique_lock lock(pc.mutex); - StartDecoder(pipe); + StartDecoder(lock, pipe); ActivateDecoder(); pc.state = PlayerState::PLAY; pc.CommandFinished(); - while (ProcessCommand()) { + while (ProcessCommand(lock)) { if (decoder_starting) { /* wait until the decoder is initialized completely */ - if (!CheckDecoderStartup()) + if (!CheckDecoderStartup(lock)) break; continue; @@ -988,7 +991,7 @@ Player::Run() noexcept !dc.IsIdle() && !buffer.IsFull()) { /* not enough decoded buffer space yet */ - dc.WaitForDecoder(); + dc.WaitForDecoder(lock); continue; } else { /* buffering is complete */ @@ -1002,7 +1005,7 @@ Player::Run() noexcept assert(dc.pipe == nullptr || dc.pipe == pipe); - StartDecoder(std::make_shared()); + StartDecoder(lock, std::make_shared()); } if (/* no cross-fading if MPD is going to pause at the @@ -1052,7 +1055,7 @@ Player::Run() noexcept // TODO: eliminate this kludge dc.Signal(); - dc.WaitForDecoder(); + dc.WaitForDecoder(lock); } else if (IsDecoderAtNextSong()) { /* at the beginning of a new song */ @@ -1079,12 +1082,12 @@ Player::Run() noexcept // TODO: eliminate this kludge dc.Signal(); - dc.WaitForDecoder(); + dc.WaitForDecoder(lock); } } CancelPendingSeek(); - StopDecoder(); + StopDecoder(lock); pipe.reset();