MusicChunkPtr: managed MusicChunk pointer

Make all uses of MusicChunk safe.

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 \

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<Mutex> 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<Mutex> protect(mutex);
 
-	if (chunk->other != nullptr) {
+	assert(!chunk->other || !chunk->other->other);
-		assert(chunk->other->other == nullptr);
-		buffer.Free(chunk->other);
-	}
 
 	buffer.Free(chunk);
 }

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

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

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);
+}

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 <memory>
+
+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<MusicChunk, MusicChunkDeleter>;
+
+#endif

src/MusicPipe.cxx

@@ -38,7 +38,7 @@ MusicPipe::Contains(const MusicChunk *chunk) const noexcept
 
 #endif
 
-MusicChunk *
+MusicChunkPtr
 MusicPipe::Shift() noexcept
 {
 	const std::lock_guard<Mutex> protect(mutex);
@@ -69,20 +69,17 @@ MusicPipe::Shift() noexcept
 #endif
 	}
 
-	return chunk;
+	return MusicChunkPtr(chunk, MusicChunkDeleter(buffer));
 }
 
 void
 MusicPipe::Clear() noexcept
 {
-	MusicChunk *chunk;
+	while (Shift()) {}
-
-	while ((chunk = Shift()) != nullptr)
-		buffer.Return(chunk);
 }
 
 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;
+	auto *c = chunk.release();
-	*tail_r = chunk;
+	c->next = nullptr;
-	tail_r = &chunk->next;
+	*tail_r = c;
+	tail_r = &c->next;
 
 	++size;
 }

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 <assert.h>
 
-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.

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);
+	auto chunk = std::move(current_chunk);
-	if (chunk->IsEmpty())
+	if (!chunk->IsEmpty())
-		dc.pipe->GetBuffer().Return(chunk);
+		dc.pipe->Push(std::move(chunk));
-	else
-		dc.pipe->Push(chunk);
 
 	const std::lock_guard<Mutex> 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) {
+		current_chunk.reset();
-			dc.pipe->GetBuffer().Return(current_chunk);
-			current_chunk = nullptr;
-		}
 
 		dc.pipe->Clear();
 

src/decoder/Bridge.hxx

@@ -22,6 +22,7 @@
 
 #include "Client.hxx"
 #include "ReplayGainInfo.hxx"
+#include "MusicChunkPtr.hxx"
 
 #include <exception>
 #include <memory>
@@ -89,7 +90,7 @@ public:
 	std::unique_ptr<Tag> decoder_tag;
 
 	/** the chunk currently being written to */
-	MusicChunk *current_chunk = nullptr;
+	MusicChunkPtr current_chunk;
 
 	ReplayGainInfo replay_gain_info;
 

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();
+		const auto shifted = pipe->Shift();
-		assert(shifted == chunk);
+		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 */
+		/* chunk is automatically returned to the buffer by
-		pipe->GetBuffer().Return(shifted);
+		   ~MusicChunkPtr() */
 	}
 
 	return 0;

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;

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

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,
+	   DetachedSong &song, MusicChunkPtr chunk,
-	   MusicBuffer &buffer,
 	   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()) {
+	if (chunk->IsEmpty())
-		buffer.Return(chunk);
 		return;
-	}
 
 	{
 		const std::lock_guard<Mutex> lock(pc.mutex);
@@ -772,9 +769,10 @@ play_chunk(PlayerControl &pc,
 
 	/* send the chunk to the audio outputs */
 
-	pc.outputs.Play(chunk);
+	const double chunk_length(chunk->length);
-	pc.total_play_time += (double)chunk->length /
+
-		format.GetTimeToSize();
+	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.reset();
-				other_chunk = nullptr;
 			}
 
-			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 */