Simplify love.audio's Pool class

Move most logic back to Source. Pool now simply contains a list of playing
sources and a mutex. Also gets rid of the pause/pauseAtomic duplication in
Source, since it can now request a lock from Pool itself.

--HG--
branch : minor

src/common/config.h

@@ -86,6 +86,16 @@
 #	define LOVE_UNUSED(x) (void)sizeof(x)
 #endif
 
+
+// Warn on unused return values
+#ifdef __GNUC__
+#	define LOVE_WARN_UNUSED __attribute__((warn_unused_result))
+#elif _MSC_VER
+#	define LOVE_WARN_UNUSED _Check_return_
+#else
+#	define LOVE_WARN_UNUSED
+#endif
+
 #ifndef LOVE_BUILD
 #	define LOVE_BUILD
 #	define LOVE_BUILD_STANDALONE

src/modules/audio/openal/Audio.cpp

@@ -240,7 +240,7 @@ bool Audio::play(love::audio::Source *source)
 
 bool Audio::play(const std::vector<love::audio::Source*> &sources)
 {
-	return pool->play(sources);
+	return Source::play(sources);
 }
 
 void Audio::stop(love::audio::Source *source)
@@ -250,12 +250,12 @@ void Audio::stop(love::audio::Source *source)
 
 void Audio::stop(const std::vector<love::audio::Source*> &sources)
 {
-	return pool->stop(sources);
+	return Source::stop(sources);
 }
 
 void Audio::stop()
 {
-	pool->stop();
+	return Source::stop(pool);
 }
 
 void Audio::pause(love::audio::Source *source)
@@ -265,12 +265,12 @@ void Audio::pause(love::audio::Source *source)
 
 void Audio::pause(const std::vector<love::audio::Source*> &sources)
 {
-	return pool->pause(sources);
+	return Source::pause(sources);
 }
 
 std::vector<love::audio::Source*> Audio::pause()
 {
-	return pool->pause();
+	return Source::pause(pool);
 }
 
 void Audio::setVolume(float volume)

src/modules/audio/openal/Pool.cpp

@@ -73,7 +73,7 @@ Pool::Pool()
 
 Pool::~Pool()
 {
-	stop();
+	Source::stop(this);
 
 	// Free all sources.
 	alDeleteSources(totalSources, sources);
@@ -103,20 +103,9 @@ void Pool::update()
 {
 	thread::Lock lock(mutex);
 
-	std::map<Source *, ALuint>::iterator i = playing.begin();
+	for (const auto &i : playing)
-
+		if (!i.first->update())
-	while (i != playing.end())
+			releaseSource(i.first);
-	{
-		if (!i->first->update())
-		{
-			i->first->stopAtomic();
-			i->first->release();
-			available.push(i->second);
-			playing.erase(i++);
-		}
-		else
-			i++;
-	}
 }
 
 int Pool::getSourceCount() const
@@ -129,19 +118,14 @@ int Pool::getMaxSources() const
 	return totalSources;
 }
 
-bool Pool::assignSource(Source *source, ALuint &out, char *wasPlaying)
+bool Pool::assignSource(Source *source, ALuint &out, char &wasPlaying)
 {
 	out = 0;
 
 	if (findSource(source, out))
-	{
+		return wasPlaying = true;
-		if (wasPlaying)
-			*wasPlaying = true;
-		return true;
-	}
 
-	if (wasPlaying)
+	wasPlaying = false;
-		*wasPlaying = false;
 
 	if (available.empty())
 		return false;
@@ -154,179 +138,48 @@ bool Pool::assignSource(Source *source, ALuint &out, char *wasPlaying)
 	return true;
 }
 
-bool Pool::play(Source *source)
+bool Pool::releaseSource(Source *source, bool stop)
 {
-	thread::Lock lock(mutex);
+	ALuint s;
-	ALuint out;
 
-	char wasPlaying;
+	if (findSource(source, s))
-	if (!assignSource(source, out, &wasPlaying))
-		return false;
-
-	if (!wasPlaying)
-		return source->playAtomic(out);
-	else
-	{
-		source->resumeAtomic();
-		return true;
-	}
-}
-
-bool Pool::play(const std::vector<love::audio::Source*> &sources)
-{
-	thread::Lock lock(mutex);
-
-	std::vector<ALuint> ids(sources.size());
-	// NOTE: not bool, because std::vector<bool> is implemented as a bitvector
-	// which means no pointers can be created.
-	std::vector<char> wasPlaying(sources.size());
-
-	for (size_t i = 0; i < sources.size(); i++)
-	{
-		Source *source = (Source*) sources[i];
-		if (!assignSource(source, ids[i], &wasPlaying[i]))
-		{
-			// Now we need to release the resources we had already allocated
-			for (size_t j = 0; j < sources.size(); j++)
-				if (!wasPlaying[j])
-					release((Source*) sources[j]);
-			return false;
-		}
-	}
-
-	return Source::playAtomic(sources, ids, wasPlaying);
-}
-
-void Pool::stop()
-{
-	thread::Lock lock(mutex);
-	for (const auto &i : playing)
-	{
-		i.first->stopAtomic();
-		i.first->release();
-		available.push(i.second);
-	}
-
-	playing.clear();
-}
-
-void Pool::stop(Source *source)
-{
-	thread::Lock lock(mutex);
-	removeSource(source);
-}
-
-void Pool::stop(const std::vector<love::audio::Source*> &sources)
-{
-	thread::Lock lock(mutex);
-	Source::stopAtomic(sources);
-}
-
-std::vector<love::audio::Source*> Pool::pause()
-{
-	thread::Lock lock(mutex);
-
-	std::vector<love::audio::Source*> werePlaying;
-	werePlaying.reserve(playing.size());
-
-	for (const auto &i : playing)
-	{
-		if (!i.first->isPlaying())
-			continue;
-		werePlaying.push_back(i.first);
-		i.first->pauseAtomic();
-	}
-
-	return werePlaying;
-}
-
-void Pool::pause(Source *source)
-{
-	thread::Lock lock(mutex);
-	ALuint out;
-	if (findSource(source, out))
-		source->pauseAtomic();
-}
-
-void Pool::pause(const std::vector<love::audio::Source*> &sources)
-{
-	thread::Lock lock(mutex);
-	Source::pauseAtomic(sources);
-}
-
-void Pool::release(Source *source)
-{
-	ALuint s = findi(source);
-
-	if (s != 0)
 	{
+		if (stop)
+			source->stopAtomic();
+		source->release();
 		available.push(s);
 		playing.erase(source);
-	}
-}
-
-void Pool::seek(Source *source, float offset, void *unit)
-{
-	thread::Lock lock(mutex);
-	return source->seekAtomic(offset, unit);
-}
-
-float Pool::tell(Source *source, void *unit)
-{
-	thread::Lock lock(mutex);
-	return source->tellAtomic(unit);
-}
-
-double Pool::getDuration(Source *source, void *unit)
-{
-	thread::Lock lock(mutex);
-	return source->getDurationAtomic(unit);
-}
-
-bool Pool::queue(Source *source, void *data, ALsizei length)
-{
-	thread::Lock lock(mutex);
-	return source->queueAtomic(data, length);
-}
-
-ALuint Pool::findi(const Source *source) const
-{
-	std::map<Source *, ALuint>::const_iterator i = playing.find((Source *)source);
-
-	if (i != playing.end())
-		return i->second;
-
-	return 0;
-}
-
-bool Pool::findSource(Source *source, ALuint &out)
-{
-	std::map<Source *, ALuint>::const_iterator i = playing.find((Source *)source);
-
-	bool found = i != playing.end();
-
-	if (found)
-		out = i->second;
-
-	return found;
-}
-
-bool Pool::removeSource(Source *source)
-{
-	std::map<Source *, ALuint>::iterator i = playing.find((Source *)source);
-
-	if (i != playing.end())
-	{
-		source->stopAtomic();
-		available.push(i->second);
-		playing.erase(i++);
-		source->release();
 		return true;
 	}
 
 	return false;
 }
 
+bool Pool::findSource(Source *source, ALuint &out)
+{
+	std::map<Source *, ALuint>::const_iterator i = playing.find(source);
+
+	if (i == playing.end())
+		return false;
+
+	out = i->second;
+	return true;
+}
+
+thread::Lock Pool::lock()
+{
+	return thread::Lock(mutex);
+}
+
+std::vector<love::audio::Source*> Pool::getPlayingSources()
+{
+	std::vector<love::audio::Source*> sources;
+	sources.reserve(playing.size());
+	for (auto &i : playing)
+		sources.push_back(i.first);
+	return sources;
+}
+
 } // openal
 } // audio
 } // love

src/modules/audio/openal/Pool.h

@@ -83,33 +83,20 @@ public:
 	int getSourceCount() const;
 	int getMaxSources() const;
 
-	bool play(Source *source);
-	void stop();
-	void stop(Source *source);
-	std::vector<love::audio::Source*> pause();
-	void pause(Source *source);
-	void seek(Source *source, float offset, void *unit);
-	float tell(Source *source, void *unit);
-	double getDuration(Source *source, void *unit);
-	bool queue(Source *source, void *data, ALsizei length);
-
-	bool play(const std::vector<love::audio::Source*> &sources);
-	void stop(const std::vector<love::audio::Source*> &sources);
-	void pause(const std::vector<love::audio::Source*> &sources);
-
 private:
 
+	friend class Source;
+	LOVE_WARN_UNUSED thread::Lock lock();
+	std::vector<love::audio::Source*> getPlayingSources();
+
 	/**
 	 * Makes the specified OpenAL source available for use.
 	 * @param source The OpenAL source.
 	 **/
-	void release(Source *source);
+	bool releaseSource(Source *source, bool stop = true);
 
-	ALuint findi(const Source *source) const;
+	bool assignSource(Source *source, ALuint &out, char &wasPlaying);
-
-	bool assignSource(Source *source, ALuint &out, char *wasPlaying = nullptr);
 	bool findSource(Source *source, ALuint &out);
-	bool removeSource(Source *source);
 
 	// Maximum possible number of OpenAL sources the pool attempts to generate.
 	static const int MAX_SOURCES = 64;

src/modules/audio/openal/Source.cpp

@@ -30,6 +30,8 @@
 
 #define audiomodule() (Module::getInstance<Audio>(Module::M_AUDIO))
 
+using love::thread::Lock;
+
 namespace love
 {
 namespace audio
@@ -234,8 +236,7 @@ Source::Source(const Source &s)
 
 Source::~Source()
 {
-	if (valid)
+	stop();
-		pool->stop(this);
 
 	if (sourceType != TYPE_STATIC)
 		alDeleteBuffers(MAX_BUFFERS, streamBuffers);
@@ -257,19 +258,34 @@ love::audio::Source *Source::clone()
 
 bool Source::play()
 {
-	valid = pool->play(this);
+	Lock l = pool->lock();
-	return valid;
+	ALuint out;
+
+	char wasPlaying;
+	if (!pool->assignSource(this, out, wasPlaying))
+		return valid = false;
+
+	if (!wasPlaying)
+		return valid = playAtomic(out);
+
+	resumeAtomic();
+	return valid = true;
 }
 
 void Source::stop()
 {
-	if (valid)
+	if (!valid)
-		pool->stop(this);
+		return;
+
+	Lock l = pool->lock();
+	pool->releaseSource(this);
 }
 
 void Source::pause()
 {
-	pool->pause(this);
+	Lock l = pool->lock();
+	if (pool->isPlaying(this))
+		pauseAtomic();
 }
 
 bool Source::isPlaying() const
@@ -343,7 +359,7 @@ bool Source::update()
 				return true;
 			}
 			return false;
-		case TYPE_QUEUE:
+		case TYPE_QUEUE: 
 		{
 			ALint processed;
 			ALuint buffers[MAX_BUFFERS];
@@ -409,11 +425,13 @@ float Source::getVolume() const
 	return volume;
 }
 
-void Source::seekAtomic(float offset, void *unit)
+void Source::seek(float offset, Source::Unit unit)
 {
+	Lock l = pool->lock();
+
 	float offsetSamples, offsetSeconds;
 
-	switch (*((Source::Unit *) unit))
+	switch (unit)
 	{
 	case Source::UNIT_SAMPLES:
 		offsetSamples = offset;
@@ -492,16 +510,13 @@ void Source::seekAtomic(float offset, void *unit)
 	this->offsetSeconds = offsetSeconds;
 }
 
-void Source::seek(float offset, Source::Unit unit)
+float Source::tell(Source::Unit unit)
 {
-	return pool->seek(this, offset, &unit);
+	Lock l = pool->lock();
-}
 
-float Source::tellAtomic(void *unit) const
-{
 	float offset = 0.0f;
 
-	switch (*((Source::Unit *) unit))
+	switch (unit)
 	{
 	case Source::UNIT_SAMPLES:
 		if (valid)
@@ -519,14 +534,9 @@ float Source::tellAtomic(void *unit) const
 	return offset;
 }
 
-float Source::tell(Source::Unit unit)
+double Source::getDuration(Unit unit)
 {
-	return pool->tell(this, &unit);
+	Lock l = pool->lock();
-}
-
-double Source::getDurationAtomic(void *vunit)
-{
-	Unit unit = *(Unit *) vunit;
 
 	switch (sourceType)
 	{
@@ -564,11 +574,6 @@ double Source::getDurationAtomic(void *vunit)
 	return 0.0;
 }
 
-double Source::getDuration(Unit unit)
-{
-	return pool->getDuration(this, &unit);
-}
-
 void Source::setPosition(float *v)
 {
 	if (channels > 1)
@@ -716,11 +721,8 @@ bool Source::queue(void *data, size_t length, int dataSampleRate, int dataBitDep
 	if (length == 0)
 		return true;
 
-	return pool->queue(this, data, (ALsizei)length);
+	Lock l = pool->lock();
-}
 
-bool Source::queueAtomic(void *data, ALsizei length)
-{
 	if (valid)
 	{
 		ALuint buffer = unusedBufferPeek();
@@ -887,10 +889,6 @@ bool Source::playAtomic(ALuint source)
 	if (sourceType != TYPE_STREAM)
 		offsetSamples = offsetSeconds = 0;
 
-	//this is set to success state afterwards anyway, but setting it here
-	//to true preemptively avoids race condition with update bug
-	valid = true;
-
 	return success;
 }
 
@@ -919,26 +917,40 @@ void Source::resumeAtomic()
 	}
 }
 
-bool Source::playAtomic(const std::vector<love::audio::Source*> &sources, const std::vector<ALuint> &ids, const std::vector<char> &wasPlaying)
+bool Source::play(const std::vector<love::audio::Source*> &sources)
 {
 	if (sources.size() == 0)
 		return true;
 
+	Pool *pool = ((Source*) sources[0])->pool;
+	Lock l = pool->lock();
+
+	// NOTE: not bool, because std::vector<bool> is implemented as a bitvector
+	// which means no bool references can be created.
+	std::vector<char> wasPlaying(sources.size());
+	std::vector<ALuint> ids(sources.size());
+
+	for (size_t i = 0; i < sources.size(); i++)
+	{
+		if (!pool->assignSource((Source*) sources[i], ids[i], wasPlaying[i]))
+		{
+			for (size_t j = 0; j < i; j++)
+				if (!wasPlaying[j])
+					pool->releaseSource((Source*) sources[j], false);
+			return false;
+		}
+	}
+
 	std::vector<ALuint> toPlay;
 	toPlay.reserve(sources.size());
 	for (size_t i = 0; i < sources.size(); i++)
 	{
-		Source *source = (Source*) sources[i];
-		// Paused sources have wasPlaying set to true, so do this first
-		if (!source->isPlaying())
-			toPlay.push_back(ids[i]);
-
-		// If it wasn't playing (nor paused), we have just allocated a new
-		// source
 		if (wasPlaying[i])
 			continue;
+		Source *source = (Source*) sources[i];
 		source->source = ids[i];
 		source->prepareAtomic();
+		toPlay.push_back(ids[i]);
 	}
 
 	alGetError();
@@ -957,11 +969,14 @@ bool Source::playAtomic(const std::vector<love::audio::Source*> &sources, const
 	return success;
 }
 
-void Source::stopAtomic(const std::vector<love::audio::Source*> &sources)
+void Source::stop(const std::vector<love::audio::Source*> &sources)
 {
 	if (sources.size() == 0)
 		return;
 
+	Pool *pool = ((Source*) sources[0])->pool;
+	Lock l = pool->lock();
+
 	std::vector<ALuint> sourceIds;
 	sourceIds.reserve(sources.size());
 	for (auto &_source : sources)
@@ -978,14 +993,17 @@ void Source::stopAtomic(const std::vector<love::audio::Source*> &sources)
 		Source *source = (Source*) _source;
 		if (source->valid)
 			source->teardownAtomic();
+		pool->releaseSource(source, false);
 	}
 }
 
-void Source::pauseAtomic(const std::vector<love::audio::Source*> &sources)
+void Source::pause(const std::vector<love::audio::Source*> &sources)
 {
 	if (sources.size() == 0)
 		return;
 
+	Lock l = ((Source*) sources[0])->pool->lock();
+
 	std::vector<ALuint> sourceIds;
 	sourceIds.reserve(sources.size());
 	for (auto &_source : sources)
@@ -998,6 +1016,26 @@ void Source::pauseAtomic(const std::vector<love::audio::Source*> &sources)
 	alSourcePausev((ALsizei) sources.size(), &sourceIds[0]);
 }
 
+std::vector<love::audio::Source*> Source::pause(Pool *pool)
+{
+	Lock l = pool->lock();
+	std::vector<love::audio::Source*> sources = pool->getPlayingSources();
+
+	auto newend = std::remove_if(sources.begin(), sources.end(), [](love::audio::Source* s) {
+		return !s->isPlaying();
+	});
+	sources.erase(newend, sources.end());
+
+	pause(sources);
+	return sources;
+}
+
+void Source::stop(Pool *pool)
+{
+	Lock l = pool->lock();
+	stop(pool->getPlayingSources());
+}
+
 void Source::reset()
 {
 	alSourcei(source, AL_BUFFER, AL_NONE);

src/modules/audio/openal/Source.h

@@ -114,11 +114,8 @@ public:
 	virtual float getPitch() const;
 	virtual void setVolume(float volume);
 	virtual float getVolume() const;
-	virtual void seekAtomic(float offset, void *unit);
 	virtual void seek(float offset, Unit unit);
-	virtual float tellAtomic(void *unit) const;
 	virtual float tell(Unit unit);
-	virtual double getDurationAtomic(void *unit);
 	virtual double getDuration(Unit unit);
 	virtual void setPosition(float *v);
 	virtual void getPosition(float *v) const;
@@ -157,7 +154,6 @@ public:
 
 	virtual int getFreeBufferCount() const;
 	virtual bool queue(void *data, size_t length, int dataSampleRate, int dataBitDepth, int dataChannels);
-	virtual bool queueAtomic(void *data, ALsizei length);
 
 	void prepareAtomic();
 	void teardownAtomic();
@@ -167,9 +163,12 @@ public:
 	void pauseAtomic();
 	void resumeAtomic();
 
-	static bool playAtomic(const std::vector<love::audio::Source*> &sources, const std::vector<ALuint> &ids, const std::vector<char> &wasPlaying);
+	static bool play(const std::vector<love::audio::Source*> &sources);
-	static void stopAtomic(const std::vector<love::audio::Source*> &sources);
+	static void stop(const std::vector<love::audio::Source*> &sources);
-	static void pauseAtomic(const std::vector<love::audio::Source*> &sources);
+	static void pause(const std::vector<love::audio::Source*> &sources);
+
+	static std::vector<love::audio::Source*> pause(Pool *pool);
+	static void stop(Pool *pool);
 
 private:
 

src/modules/thread/threads.cpp

@@ -37,9 +37,16 @@ Lock::Lock(Mutex &m)
 	mutex->lock();
 }
 
+Lock::Lock(Lock &&other)
+{
+	mutex = other.mutex;
+	other.mutex = nullptr;
+}
+
 Lock::~Lock()
 {
-	mutex->unlock();
+	if (mutex)
+		mutex->unlock();
 }
 
 EmptyLock::EmptyLock()

src/modules/thread/threads.h

@@ -57,6 +57,7 @@ class Lock
 public:
 	Lock(Mutex *m);
 	Lock(Mutex &m);
+	Lock(Lock &&other);
 	~Lock();
 
 private:

src/scripts/nogame.lua.h

@@ -27,7 +27,7 @@ const unsigned char nogame_lua[] =
 
 	0x2d, 0x2d, 0x5b, 0x5b, 0x0a,
 	0x43, 0x6f, 0x70, 0x79, 0x72, 0x69, 0x67, 0x68, 0x74, 0x20, 0x28, 0x63, 0x29, 0x20, 0x32, 0x30, 0x30, 0x36, 
-	0x2d, 0x32, 0x30, 0x31, 0x36, 0x20, 0x4c, 0x4f, 0x56, 0x45, 0x20, 0x44, 0x65, 0x76, 0x65, 0x6c, 0x6f, 0x70, 
+	0x2d, 0x32, 0x30, 0x31, 0x37, 0x20, 0x4c, 0x4f, 0x56, 0x45, 0x20, 0x44, 0x65, 0x76, 0x65, 0x6c, 0x6f, 0x70, 
 	0x6d, 0x65, 0x6e, 0x74, 0x20, 0x54, 0x65, 0x61, 0x6d, 0x0a,
 	0x54, 0x68, 0x69, 0x73, 0x20, 0x73, 0x6f, 0x66, 0x74, 0x77, 0x61, 0x72, 0x65, 0x20, 0x69, 0x73, 0x20, 0x70, 
 	0x72, 0x6f, 0x76, 0x69, 0x64, 0x65, 0x64, 0x20, 0x27, 0x61, 0x73, 0x2d, 0x69, 0x73, 0x27, 0x2c, 0x20, 0x77,