From 54d295c247deb1f543d922eb838d3eff2f7a89ce Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sat, 30 Dec 2017 18:00:40 +0100 Subject: [PATCH] MusicChunkPtr: managed MusicChunk pointer Make all uses of MusicChunk safe. --- Makefile.am | 1 + src/MusicBuffer.cxx | 9 +++----- src/MusicBuffer.hxx | 5 ++-- src/MusicChunk.hxx | 3 ++- src/MusicChunkPtr.cxx | 28 +++++++++++++++++++++++ src/MusicChunkPtr.hxx | 42 ++++++++++++++++++++++++++++++++++ src/MusicPipe.cxx | 18 +++++++-------- src/MusicPipe.hxx | 6 ++--- src/decoder/Bridge.cxx | 17 +++++--------- src/decoder/Bridge.hxx | 3 ++- src/output/MultipleOutputs.cxx | 13 +++++------ src/output/MultipleOutputs.hxx | 4 ++-- src/player/Outputs.hxx | 3 ++- src/player/Thread.cxx | 28 +++++++++++------------ 14 files changed, 120 insertions(+), 60 deletions(-) create mode 100644 src/MusicChunkPtr.cxx create mode 100644 src/MusicChunkPtr.hxx diff --git a/Makefile.am b/Makefile.am index 8a6beb43b..64ce2a13a 100644 --- a/Makefile.am +++ b/Makefile.am @@ -136,6 +136,7 @@ libmpd_a_SOURCES = \ src/MusicBuffer.cxx src/MusicBuffer.hxx \ src/MusicPipe.cxx src/MusicPipe.hxx \ src/MusicChunk.cxx src/MusicChunk.hxx \ + src/MusicChunkPtr.cxx src/MusicChunkPtr.hxx \ src/Mapper.cxx src/Mapper.hxx \ src/Partition.cxx src/Partition.hxx \ src/Permission.cxx src/Permission.hxx \ diff --git a/src/MusicBuffer.cxx b/src/MusicBuffer.cxx index 2d08f42f1..2555656df 100644 --- a/src/MusicBuffer.cxx +++ b/src/MusicBuffer.cxx @@ -27,11 +27,11 @@ MusicBuffer::MusicBuffer(unsigned num_chunks) noexcept :buffer(num_chunks) { } -MusicChunk * +MusicChunkPtr MusicBuffer::Allocate() noexcept { const std::lock_guard protect(mutex); - return buffer.Allocate(); + return MusicChunkPtr(buffer.Allocate(), MusicChunkDeleter(*this)); } void @@ -41,10 +41,7 @@ MusicBuffer::Return(MusicChunk *chunk) noexcept const std::lock_guard protect(mutex); - if (chunk->other != nullptr) { - assert(chunk->other->other == nullptr); - buffer.Free(chunk->other); - } + assert(!chunk->other || !chunk->other->other); buffer.Free(chunk); } diff --git a/src/MusicBuffer.hxx b/src/MusicBuffer.hxx index b3783a53f..3bae9a197 100644 --- a/src/MusicBuffer.hxx +++ b/src/MusicBuffer.hxx @@ -20,11 +20,10 @@ #ifndef MPD_MUSIC_BUFFER_HXX #define MPD_MUSIC_BUFFER_HXX +#include "MusicChunkPtr.hxx" #include "util/SliceBuffer.hxx" #include "thread/Mutex.hxx" -struct MusicChunk; - /** * An allocator for #MusicChunk objects. */ @@ -71,7 +70,7 @@ public: * @return an empty chunk or nullptr if there are no chunks * available */ - MusicChunk *Allocate() noexcept; + MusicChunkPtr Allocate() noexcept; /** * Returns a chunk to the buffer. It can be reused by diff --git a/src/MusicChunk.hxx b/src/MusicChunk.hxx index 7f3382896..1ff93a442 100644 --- a/src/MusicChunk.hxx +++ b/src/MusicChunk.hxx @@ -20,6 +20,7 @@ #ifndef MPD_MUSIC_CHUNK_HXX #define MPD_MUSIC_CHUNK_HXX +#include "MusicChunkPtr.hxx" #include "Chrono.hxx" #include "ReplayGainInfo.hxx" #include "util/WritableBuffer.hxx" @@ -50,7 +51,7 @@ struct MusicChunkInfo { * An optional chunk which should be mixed into this chunk. * This is used for cross-fading. */ - MusicChunk *other = nullptr; + MusicChunkPtr other; /** * An optional tag associated with this chunk (and the diff --git a/src/MusicChunkPtr.cxx b/src/MusicChunkPtr.cxx new file mode 100644 index 000000000..9a013a894 --- /dev/null +++ b/src/MusicChunkPtr.cxx @@ -0,0 +1,28 @@ +/* + * Copyright 2003-2018 The Music Player Daemon Project + * http://www.musicpd.org + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#include "config.h" +#include "MusicChunkPtr.hxx" +#include "MusicBuffer.hxx" + +void +MusicChunkDeleter::operator()(MusicChunk *chunk) noexcept +{ + buffer->Return(chunk); +} diff --git a/src/MusicChunkPtr.hxx b/src/MusicChunkPtr.hxx new file mode 100644 index 000000000..81c8ad30e --- /dev/null +++ b/src/MusicChunkPtr.hxx @@ -0,0 +1,42 @@ +/* + * Copyright 2003-2018 The Music Player Daemon Project + * http://www.musicpd.org + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#ifndef MPD_MUSIC_CHUNK_PTR_HXX +#define MPD_MUSIC_CHUNK_PTR_HXX + +#include "check.h" + +#include + +struct MusicChunk; +class MusicBuffer; + +class MusicChunkDeleter { + MusicBuffer *buffer; + +public: + MusicChunkDeleter() = default; + explicit MusicChunkDeleter(MusicBuffer &_buffer):buffer(&_buffer) {} + + void operator()(MusicChunk *chunk) noexcept; +}; + +using MusicChunkPtr = std::unique_ptr; + +#endif diff --git a/src/MusicPipe.cxx b/src/MusicPipe.cxx index 35a79edc5..7a78855b2 100644 --- a/src/MusicPipe.cxx +++ b/src/MusicPipe.cxx @@ -38,7 +38,7 @@ MusicPipe::Contains(const MusicChunk *chunk) const noexcept #endif -MusicChunk * +MusicChunkPtr MusicPipe::Shift() noexcept { const std::lock_guard protect(mutex); @@ -69,20 +69,17 @@ MusicPipe::Shift() noexcept #endif } - return chunk; + return MusicChunkPtr(chunk, MusicChunkDeleter(buffer)); } void MusicPipe::Clear() noexcept { - MusicChunk *chunk; - - while ((chunk = Shift()) != nullptr) - buffer.Return(chunk); + while (Shift()) {} } void -MusicPipe::Push(MusicChunk *chunk) noexcept +MusicPipe::Push(MusicChunkPtr chunk) noexcept { assert(!chunk->IsEmpty()); assert(chunk->length == 0 || chunk->audio_format.IsValid()); @@ -98,9 +95,10 @@ MusicPipe::Push(MusicChunk *chunk) noexcept audio_format = chunk->audio_format; #endif - chunk->next = nullptr; - *tail_r = chunk; - tail_r = &chunk->next; + auto *c = chunk.release(); + c->next = nullptr; + *tail_r = c; + tail_r = &c->next; ++size; } diff --git a/src/MusicPipe.hxx b/src/MusicPipe.hxx index fd0471fd4..57ae0dd7c 100644 --- a/src/MusicPipe.hxx +++ b/src/MusicPipe.hxx @@ -20,6 +20,7 @@ #ifndef MPD_PIPE_H #define MPD_PIPE_H +#include "MusicChunkPtr.hxx" #include "thread/Mutex.hxx" #include "Compiler.h" @@ -29,7 +30,6 @@ #include -struct MusicChunk; class MusicBuffer; /** @@ -108,7 +108,7 @@ public: /** * Removes the first chunk from the head, and returns it. */ - MusicChunk *Shift() noexcept; + MusicChunkPtr Shift() noexcept; /** * Clears the whole pipe and returns the chunks to the buffer. @@ -118,7 +118,7 @@ public: /** * Pushes a chunk to the tail of the pipe. */ - void Push(MusicChunk *chunk) noexcept; + void Push(MusicChunkPtr chunk) noexcept; /** * Returns the number of chunks currently in this pipe. diff --git a/src/decoder/Bridge.cxx b/src/decoder/Bridge.cxx index 87d2e3a8a..c8ae799b9 100644 --- a/src/decoder/Bridge.cxx +++ b/src/decoder/Bridge.cxx @@ -95,7 +95,7 @@ DecoderBridge::GetChunk() noexcept DecoderCommand cmd; if (current_chunk != nullptr) - return current_chunk; + return current_chunk.get(); do { current_chunk = dc.pipe->GetBuffer().Allocate(); @@ -104,7 +104,7 @@ DecoderBridge::GetChunk() noexcept if (replay_gain_serial != 0) current_chunk->replay_gain_info = replay_gain_info; - return current_chunk; + return current_chunk.get(); } cmd = LockNeedChunks(dc); @@ -121,11 +121,9 @@ DecoderBridge::FlushChunk() assert(!initial_seek_pending); assert(current_chunk != nullptr); - auto *chunk = std::exchange(current_chunk, nullptr); - if (chunk->IsEmpty()) - dc.pipe->GetBuffer().Return(chunk); - else - dc.pipe->Push(chunk); + auto chunk = std::move(current_chunk); + if (!chunk->IsEmpty()) + dc.pipe->Push(std::move(chunk)); const std::lock_guard protect(dc.mutex); if (dc.client_is_waiting) @@ -302,10 +300,7 @@ DecoderBridge::CommandFinished() /* delete frames from the old song position */ - if (current_chunk != nullptr) { - dc.pipe->GetBuffer().Return(current_chunk); - current_chunk = nullptr; - } + current_chunk.reset(); dc.pipe->Clear(); diff --git a/src/decoder/Bridge.hxx b/src/decoder/Bridge.hxx index 1750e444b..e19d28e27 100644 --- a/src/decoder/Bridge.hxx +++ b/src/decoder/Bridge.hxx @@ -22,6 +22,7 @@ #include "Client.hxx" #include "ReplayGainInfo.hxx" +#include "MusicChunkPtr.hxx" #include #include @@ -89,7 +90,7 @@ public: std::unique_ptr decoder_tag; /** the chunk currently being written to */ - MusicChunk *current_chunk = nullptr; + MusicChunkPtr current_chunk; ReplayGainInfo replay_gain_info; diff --git a/src/output/MultipleOutputs.cxx b/src/output/MultipleOutputs.cxx index 6e993ba3a..fec6baf21 100644 --- a/src/output/MultipleOutputs.cxx +++ b/src/output/MultipleOutputs.cxx @@ -205,7 +205,7 @@ MultipleOutputs::SetReplayGainMode(ReplayGainMode mode) noexcept } void -MultipleOutputs::Play(MusicChunk *chunk) +MultipleOutputs::Play(MusicChunkPtr chunk) { assert(pipe != nullptr); assert(chunk != nullptr); @@ -215,7 +215,7 @@ MultipleOutputs::Play(MusicChunk *chunk) /* TODO: obtain real error */ throw std::runtime_error("Failed to open audio output"); - pipe->Push(chunk); + pipe->Push(std::move(chunk)); for (auto *ao : outputs) ao->LockPlay(); @@ -314,7 +314,6 @@ MultipleOutputs::CheckPipe() noexcept { const MusicChunk *chunk; bool is_tail; - MusicChunk *shifted; bool locked[outputs.size()]; assert(pipe != nullptr); @@ -339,8 +338,8 @@ MultipleOutputs::CheckPipe() noexcept ClearTailChunk(chunk, locked); /* remove the chunk from the pipe */ - shifted = pipe->Shift(); - assert(shifted == chunk); + const auto shifted = pipe->Shift(); + assert(shifted.get() == chunk); if (is_tail) /* unlock all audio outputs which were locked @@ -349,8 +348,8 @@ MultipleOutputs::CheckPipe() noexcept if (locked[i]) outputs[i]->mutex.unlock(); - /* return the chunk to the buffer */ - pipe->GetBuffer().Return(shifted); + /* chunk is automatically returned to the buffer by + ~MusicChunkPtr() */ } return 0; diff --git a/src/output/MultipleOutputs.hxx b/src/output/MultipleOutputs.hxx index 3048a81f5..79f9becca 100644 --- a/src/output/MultipleOutputs.hxx +++ b/src/output/MultipleOutputs.hxx @@ -27,6 +27,7 @@ #define OUTPUT_ALL_H #include "Control.hxx" +#include "MusicChunkPtr.hxx" #include "player/Outputs.hxx" #include "AudioFormat.hxx" #include "ReplayGainMode.hxx" @@ -42,7 +43,6 @@ class MusicPipe; class EventLoop; class MixerListener; class AudioOutputClient; -struct MusicChunk; struct ReplayGainConfig; class MultipleOutputs final : public PlayerOutputs { @@ -185,7 +185,7 @@ private: MusicBuffer &_buffer) override; void Close() noexcept override; void Release() noexcept override; - void Play(MusicChunk *chunk) override; + void Play(MusicChunkPtr chunk) override; unsigned CheckPipe() noexcept override; void Pause() noexcept override; void Drain() noexcept override; diff --git a/src/player/Outputs.hxx b/src/player/Outputs.hxx index 9b896ce5d..71e5e43d8 100644 --- a/src/player/Outputs.hxx +++ b/src/player/Outputs.hxx @@ -20,6 +20,7 @@ #ifndef MPD_PLAYER_OUTPUT_INTERFACE_HXX #define MPD_PLAYER_OUTPUT_INTERFACE_HXX +#include "MusicChunkPtr.hxx" #include "Chrono.hxx" #include "Compiler.h" @@ -74,7 +75,7 @@ public: * * @param chunk the #MusicChunk object to be played */ - virtual void Play(MusicChunk *chunk) = 0; + virtual void Play(MusicChunkPtr chunk) = 0; /** * Checks if the output devices have drained their music pipe, and diff --git a/src/player/Thread.cxx b/src/player/Thread.cxx index b359ec4b8..e841ea673 100644 --- a/src/player/Thread.cxx +++ b/src/player/Thread.cxx @@ -751,8 +751,7 @@ update_song_tag(PlayerControl &pc, DetachedSong &song, */ static void play_chunk(PlayerControl &pc, - DetachedSong &song, MusicChunk *chunk, - MusicBuffer &buffer, + DetachedSong &song, MusicChunkPtr chunk, const AudioFormat format) { assert(chunk->CheckFormat(format)); @@ -760,10 +759,8 @@ play_chunk(PlayerControl &pc, if (chunk->tag != nullptr) update_song_tag(pc, song, *chunk->tag); - if (chunk->IsEmpty()) { - buffer.Return(chunk); + if (chunk->IsEmpty()) return; - } { const std::lock_guard lock(pc.mutex); @@ -772,9 +769,10 @@ play_chunk(PlayerControl &pc, /* send the chunk to the audio outputs */ - pc.outputs.Play(chunk); - pc.total_play_time += (double)chunk->length / - format.GetTimeToSize(); + const double chunk_length(chunk->length); + + pc.outputs.Play(std::move(chunk)); + pc.total_play_time += chunk_length / format.GetTimeToSize(); } inline bool @@ -796,7 +794,7 @@ Player::PlayNextChunk() noexcept xfade_state = CrossFadeState::ACTIVE; } - MusicChunk *chunk = nullptr; + MusicChunkPtr chunk; if (xfade_state == CrossFadeState::ACTIVE) { /* perform cross fade */ @@ -805,7 +803,7 @@ Player::PlayNextChunk() noexcept unsigned cross_fade_position = pipe->GetSize(); assert(cross_fade_position <= cross_fade_chunks); - MusicChunk *other_chunk = dc.pipe->Shift(); + auto other_chunk = dc.pipe->Shift(); if (other_chunk != nullptr) { chunk = pipe->Shift(); assert(chunk != nullptr); @@ -832,11 +830,10 @@ Player::PlayNextChunk() noexcept beginning of the new song, we can easily recover by throwing it away now */ - buffer.Return(other_chunk); - other_chunk = nullptr; + other_chunk.reset(); } - chunk->other = other_chunk; + chunk->other = std::move(other_chunk); } else { /* there are not enough decoded chunks yet */ @@ -872,11 +869,12 @@ Player::PlayNextChunk() noexcept /* play the current chunk */ try { - play_chunk(pc, *song, chunk, buffer, play_audio_format); + play_chunk(pc, *song, std::move(chunk), + play_audio_format); } catch (...) { LogError(std::current_exception()); - buffer.Return(chunk); + chunk.reset(); /* pause: the user may resume playback as soon as an audio output becomes available */