BLADERUNNER: Cleanup of audio code

Separated audio cache.
Fixed bug in the audio cache where still used sound might get freed.
Fixes crashes when engine is unloading which were caused
by a race condition between the timer code and engine teardown code.

13 files changed, 238 insertions, 151 deletions

diff --git a/engines/bladerunner/aud_stream.cpp b/engines/bladerunner/aud_stream.cpp
index 9cdd35d190..bb35702d10 100644
--- a/engines/bladerunner/aud_stream.cpp
+++ b/engines/bladerunner/aud_stream.cpp
@@ -22,7 +22,7 @@
 
 #include "bladerunner/aud_stream.h"
 
-#include "bladerunner/audio_player.h"
+#include "bladerunner/audio_cache.h"
 
 #include "common/util.h"
 
@@ -36,12 +36,12 @@ AudStream::AudStream(byte *data, int overrideFrequency) {
 	init(data);
 }
 
-AudStream::AudStream(AudioCache *cache, int32 hash) {
+AudStream::AudStream(AudioCache *cache, int32 hash, int overrideFrequency) {
 	assert(cache != nullptr);
 
 	_cache = cache;
 	_hash  = hash;
-	_overrideFrequency = -1;
+	_overrideFrequency = overrideFrequency;
 
 	_cache->incRef(_hash);
 
@@ -128,7 +128,7 @@ bool AudStream::rewind() {
 }
 
 int AudStream::getLength() const {
-	int bytesPerSecond = _frequency;
+	int bytesPerSecond = _overrideFrequency > 0 ? _overrideFrequency : _frequency;
 	if (_flags & 1) { // 16 bit
 		bytesPerSecond *= 2;
 	}
diff --git a/engines/bladerunner/aud_stream.h b/engines/bladerunner/aud_stream.h
index 0316e25d29..1d65022ad0 100644
--- a/engines/bladerunner/aud_stream.h
+++ b/engines/bladerunner/aud_stream.h
@@ -53,7 +53,7 @@ class AudStream : public Audio::RewindableAudioStream {
 
 public:
 	AudStream(byte *data, int overrideFrequency = -1);
-	AudStream(AudioCache *cache, int32 hash);
+	AudStream(AudioCache *cache, int32 hash, int overrideFrequency = -1);
 	~AudStream();
 
 	int readBuffer(int16 *buffer, const int numSamples);
diff --git a/engines/bladerunner/audio_cache.cpp b/engines/bladerunner/audio_cache.cpp
new file mode 100644
index 0000000000..734eeae4c7
--- /dev/null
+++ b/engines/bladerunner/audio_cache.cpp
@@ -0,0 +1,130 @@
+/* ScummVM - Graphic Adventure Engine
+ *
+ * ScummVM is the legal property of its developers, whose names
+ * are too numerous to list here. Please refer to the COPYRIGHT
+ * file distributed with this source distribution.
+ *
+ * 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 "bladerunner/audio_cache.h"
+
+#include "bladerunner/bladerunner.h"
+
+namespace BladeRunner {
+
+AudioCache::AudioCache(BladeRunnerEngine *vm) :
+	_vm(vm),
+	_totalSize(0),
+	_maxSize(2457600),
+	_accessCounter(0) {}
+
+AudioCache::~AudioCache() {
+	for (uint i = 0; i != _cacheItems.size(); ++i) {
+		free(_cacheItems[i].data);
+	}
+}
+
+bool AudioCache::canAllocate(uint32 size) const {
+	Common::StackLock lock(_mutex);
+
+	return _maxSize - _totalSize >= size;
+}
+
+bool AudioCache::dropOldest() {
+	Common::StackLock lock(_mutex);
+
+	if (_cacheItems.size() == 0)
+		return false;
+
+	int oldest = -1;
+	for (uint i = 1; i != _cacheItems.size(); ++i) {
+		if (_cacheItems[i].refs == 0) {
+			if (oldest == -1 || _cacheItems[i].lastAccess < _cacheItems[oldest].lastAccess) {
+				oldest = i;
+			}
+		}
+	}
+
+	if (oldest == -1) {
+		return false;
+	}
+
+	memset(_cacheItems[oldest].data, 0x00, _cacheItems[oldest].size);
+	free(_cacheItems[oldest].data);
+	_totalSize -= _cacheItems[oldest].size;
+	_cacheItems.remove_at(oldest);
+	return true;
+}
+
+byte *AudioCache::findByHash(int32 hash) {
+	Common::StackLock lock(_mutex);
+
+	for (uint i = 0; i != _cacheItems.size(); ++i) {
+		if (_cacheItems[i].hash == hash) {
+			_cacheItems[i].lastAccess = _accessCounter++;
+			return _cacheItems[i].data;
+		}
+	}
+
+	return nullptr;
+}
+
+void  AudioCache::storeByHash(int32 hash, Common::SeekableReadStream *stream) {
+	Common::StackLock lock(_mutex);
+
+	uint32 size = stream->size();
+	byte *data = (byte *)malloc(size);
+	stream->read(data, size);
+
+	cacheItem item = {
+		hash,
+		0,
+		_accessCounter++,
+		data,
+		size
+	};
+
+	_cacheItems.push_back(item);
+	_totalSize += size;
+}
+
+void AudioCache::incRef(int32 hash) {
+	Common::StackLock lock(_mutex);
+
+	for (uint i = 0; i != _cacheItems.size(); ++i) {
+		if (_cacheItems[i].hash == hash) {
+			_cacheItems[i].refs++;
+			return;
+		}
+	}
+	assert(false && "AudioCache::incRef: hash not found");
+}
+
+void AudioCache::decRef(int32 hash) {
+	Common::StackLock lock(_mutex);
+
+	for (uint i = 0; i != _cacheItems.size(); ++i) {
+		if (_cacheItems[i].hash == hash) {
+			assert(_cacheItems[i].refs > 0);
+			_cacheItems[i].refs--;
+			return;
+		}
+	}
+	assert(false && "AudioCache::decRef: hash not found");
+}
+
+} // End of namespace BladeRunner
diff --git a/engines/bladerunner/audio_cache.h b/engines/bladerunner/audio_cache.h
new file mode 100644
index 0000000000..8357689174
--- /dev/null
+++ b/engines/bladerunner/audio_cache.h
@@ -0,0 +1,70 @@
+/* ScummVM - Graphic Adventure Engine
+ *
+ * ScummVM is the legal property of its developers, whose names
+ * are too numerous to list here. Please refer to the COPYRIGHT
+ * file distributed with this source distribution.
+ *
+ * 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 BLADERUNNER_AUDIO_CACHE_H
+#define BLADERUNNER_AUDIO_CACHE_H
+
+#include "common/array.h"
+#include "common/mutex.h"
+
+namespace BladeRunner {
+
+class BladeRunnerEngine;
+class AudioCache;
+
+/*
+ * This is a poor imitation of Bladerunner's resource cache
+ */
+class AudioCache {
+	struct cacheItem {
+		int32   hash;
+		int     refs;
+		uint    lastAccess;
+		byte   *data;
+		uint32  size;
+	};
+
+	BladeRunnerEngine *_vm;
+
+	Common::Mutex            _mutex;
+	Common::Array<cacheItem> _cacheItems;
+
+	uint32 _totalSize;
+	uint32 _maxSize;
+	uint32 _accessCounter;
+
+public:
+	AudioCache(BladeRunnerEngine *vm);
+	~AudioCache();
+
+	bool  canAllocate(uint32 size) const;
+	bool  dropOldest();
+	byte *findByHash(int32 hash);
+	void  storeByHash(int32 hash, Common::SeekableReadStream *stream);
+
+	void  incRef(int32 hash);
+	void  decRef(int32 hash);
+};
+
+} // End of namespace BladeRunner
+
+#endif
diff --git a/engines/bladerunner/audio_player.cpp b/engines/bladerunner/audio_player.cpp
index 4493a02f59..df92be2d42 100644
--- a/engines/bladerunner/audio_player.cpp
+++ b/engines/bladerunner/audio_player.cpp
@@ -24,6 +24,7 @@
 
 #include "bladerunner/archive.h"
 #include "bladerunner/aud_stream.h"
+#include "bladerunner/audio_cache.h"
 #include "bladerunner/audio_mixer.h"
 #include "bladerunner/bladerunner.h"
 
@@ -37,99 +38,10 @@ namespace Common {
 
 namespace BladeRunner {
 
-AudioCache::~AudioCache() {
-	for (uint i = 0; i != _cacheItems.size(); ++i) {
-		free(_cacheItems[i].data);
-	}
-}
-
-bool AudioCache::canAllocate(uint32 size) const {
-	Common::StackLock lock(_mutex);
-
-	return _maxSize - _totalSize >= size;
-}
-
-bool AudioCache::dropOldest() {
-	Common::StackLock lock(_mutex);
-
-	if (_cacheItems.size() == 0)
-		return false;
-
-	uint oldest = 0;
-	for (uint i = 1; i != _cacheItems.size(); ++i) {
-		if (_cacheItems[i].refs == 0 && _cacheItems[i].lastAccess < _cacheItems[oldest].lastAccess)
-			oldest = i;
-	}
-
-	free(_cacheItems[oldest].data);
-	_totalSize -= _cacheItems[oldest].size;
-	_cacheItems.remove_at(oldest);
-	return true;
-}
-
-byte *AudioCache::findByHash(int32 hash) {
-	Common::StackLock lock(_mutex);
-
-	for (uint i = 0; i != _cacheItems.size(); ++i) {
-		if (_cacheItems[i].hash == hash) {
-			_cacheItems[i].lastAccess = _accessCounter++;
-			return _cacheItems[i].data;
-		}
-	}
-
-	return nullptr;
-}
-
-void AudioCache::storeByHash(int32 hash, Common::SeekableReadStream *stream) {
-	Common::StackLock lock(_mutex);
-
-	uint32 size = stream->size();
-	byte *data = (byte *)malloc(size);
-	stream->read(data, size);
-
-	cacheItem item = {
-		hash,
-		0,
-		_accessCounter++,
-		data,
-		size
-	};
-
-	_cacheItems.push_back(item);
-	_totalSize += size;
-}
-
-void AudioCache::incRef(int32 hash) {
-	Common::StackLock lock(_mutex);
-
-	for (uint i = 0; i != _cacheItems.size(); ++i) {
-		if (_cacheItems[i].hash == hash) {
-			_cacheItems[i].refs++;
-			return;
-		}
-	}
-	assert(false && "AudioCache::incRef: hash not found");
-}
-
-void AudioCache::decRef(int32 hash) {
-	Common::StackLock lock(_mutex);
-
-	for (uint i = 0; i != _cacheItems.size(); ++i) {
-		if (_cacheItems[i].hash == hash) {
-			assert(_cacheItems[i].refs > 0);
-			_cacheItems[i].refs--;
-			return;
-		}
-	}
-	assert(false && "AudioCache::decRef: hash not found");
-}
-
 AudioPlayer::AudioPlayer(BladeRunnerEngine *vm) {
 	_vm = vm;
-	_cache = new AudioCache();
 
 	for (int i = 0; i != 6; ++i) {
-		_tracks[i].hash = 0;
 		_tracks[i].priority = 0;
 		_tracks[i].isActive = false;
 		_tracks[i].channel = -1;
@@ -141,12 +53,11 @@ AudioPlayer::AudioPlayer(BladeRunnerEngine *vm) {
 
 AudioPlayer::~AudioPlayer() {
 	stopAll();
-	delete _cache;
 }
 
 void AudioPlayer::stopAll() {
 	for (int i = 0; i != kTracks; ++i) {
-		stop(i, false);
+		stop(i, true);
 	}
 	for (int i = 0; i != kTracks; ++i) {
 		while (isActive(i)) {
@@ -254,24 +165,24 @@ int AudioPlayer::playAud(const Common::String &name, int volume, int panFrom, in
 
 	/* Load audio resource and store in cache. Playback will happen directly from there. */
 	int32 hash = MIXArchive::getHash(name);
-	if (!_cache->findByHash(hash)) {
+	if (!_vm->_audioCache->findByHash(hash)) {
 		Common::SeekableReadStream *r = _vm->getResourceStream(name);
 		if (!r) {
 			return -1;
 		}
 
 		int32 size = r->size();
-		while (!_cache->canAllocate(size)) {
-			if (!_cache->dropOldest()) {
+		while (!_vm->_audioCache->canAllocate(size)) {
+			if (!_vm->_audioCache->dropOldest()) {
 				delete r;
 				return -1;
 			}
 		}
-		_cache->storeByHash(hash, r);
+		_vm->_audioCache->storeByHash(hash, r);
 		delete r;
 	}
 
-	AudStream *audioStream = new AudStream(_cache, hash);
+	AudStream *audioStream = new AudStream(_vm->_audioCache, hash);
 
 	int actualVolume = volume;
 	if (!(flags & kAudioPlayerOverrideVolume)) {
@@ -290,7 +201,6 @@ int AudioPlayer::playAud(const Common::String &name, int volume, int panFrom, in
 
 	if (channel == -1) {
 		delete audioStream;
-		_cache->decRef(hash);
 		return -1;
 	}
 
@@ -301,7 +211,6 @@ int AudioPlayer::playAud(const Common::String &name, int volume, int panFrom, in
 	_tracks[track].isActive = true;
 	_tracks[track].channel  = channel;
 	_tracks[track].priority = priority;
-	_tracks[track].hash     = hash;
 	_tracks[track].volume   = actualVolume;
 	_tracks[track].stream   = audioStream;
 
diff --git a/engines/bladerunner/audio_player.h b/engines/bladerunner/audio_player.h
index 34059e33f3..580bdbcb79 100644
--- a/engines/bladerunner/audio_player.h
+++ b/engines/bladerunner/audio_player.h
@@ -35,43 +35,6 @@ namespace BladeRunner {
 class BladeRunnerEngine;
 class AudioCache;
 
-/*
- * This is a poor imitation of Bladerunner's resource cache
- */
-class AudioCache {
-	struct cacheItem {
-		int32   hash;
-		int     refs;
-		uint    lastAccess;
-		byte   *data;
-		uint32  size;
-	};
-
-	Common::Mutex            _mutex;
-	Common::Array<cacheItem> _cacheItems;
-
-	uint32 _totalSize;
-	uint32 _maxSize;
-	uint32 _accessCounter;
-
-public:
-	AudioCache() :
-		_totalSize(0),
-		_maxSize(2457600),
-		_accessCounter(0) {
-	}
-	~AudioCache();
-
-	bool  canAllocate(uint32 size) const;
-	bool  dropOldest();
-	byte *findByHash(int32 hash);
-	void  storeByHash(int32 hash, Common::SeekableReadStream *stream);
-
-	void  incRef(int32 hash);
-	void  decRef(int32 hash);
-};
-
-
 enum AudioPlayerFlags {
 	kAudioPlayerLoop = 1,
 	kAudioPlayerOverrideVolume = 2
@@ -84,7 +47,6 @@ class AudioPlayer {
 		bool                isActive;
 		int                 channel;
 		int                 priority;
-		int32               hash;
 		int                 volume;
 		int                 pan;
 		Audio::AudioStream *stream;
@@ -93,7 +55,6 @@ class AudioPlayer {
 	BladeRunnerEngine *_vm;
 
 	Common::Mutex _mutex;
-	AudioCache   *_cache;
 	Track         _tracks[kTracks];
 	int           _sfxVolume;
 
diff --git a/engines/bladerunner/audio_speech.cpp b/engines/bladerunner/audio_speech.cpp
index 6963b7071a..aa8b0f419b 100644
--- a/engines/bladerunner/audio_speech.cpp
+++ b/engines/bladerunner/audio_speech.cpp
@@ -28,7 +28,6 @@
 #include "bladerunner/audio_player.h"
 #include "bladerunner/bladerunner.h"
 
-#include "common/debug.h"
 #include "common/str.h"
 
 namespace BladeRunner {
@@ -55,10 +54,22 @@ AudioSpeech::AudioSpeech(BladeRunnerEngine *vm) {
 }
 
 AudioSpeech::~AudioSpeech() {
+	stopSpeech();
+	while (isPlaying()) {
+		// wait for the mixer to finish
+	}
+
 	delete[] _data;
 }
 
 bool AudioSpeech::playSpeech(const Common::String &name, int pan) {
+	if (isPlaying()) {
+		stopSpeech();
+	}
+
+	// Audio cache is not usable as hash function is producing collision for speech lines.
+	// It was not used in the original game either
+
 	Common::ScopedPtr<Common::SeekableReadStream> r(_vm->getResourceStream(name));
 
 	if (!r) {
diff --git a/engines/bladerunner/bladerunner.cpp b/engines/bladerunner/bladerunner.cpp
index a12e37565b..5c49281256 100644
--- a/engines/bladerunner/bladerunner.cpp
+++ b/engines/bladerunner/bladerunner.cpp
@@ -25,6 +25,7 @@
 #include "bladerunner/actor.h"
 #include "bladerunner/actor_dialogue_queue.h"
 #include "bladerunner/ambient_sounds.h"
+#include "bladerunner/audio_cache.h"
 #include "bladerunner/audio_mixer.h"
 #include "bladerunner/audio_player.h"
 #include "bladerunner/audio_speech.h"
@@ -156,7 +157,7 @@ BladeRunnerEngine::BladeRunnerEngine(OSystem *syst, const ADGameDescription *des
 	_lights                  = nullptr;
 	_obstacles               = nullptr;
 	_sceneScript             = nullptr;
-	_time                = nullptr;
+	_time                    = nullptr;
 	_gameInfo                = nullptr;
 	_waypoints               = nullptr;
 	_gameVars                = nullptr;
@@ -164,6 +165,7 @@ BladeRunnerEngine::BladeRunnerEngine(OSystem *syst, const ADGameDescription *des
 	_sceneObjects            = nullptr;
 	_gameFlags               = nullptr;
 	_items                   = nullptr;
+	_audioCache              = nullptr;
 	_audioMixer              = nullptr;
 	_audioPlayer             = nullptr;
 	_music                   = nullptr;
@@ -379,7 +381,7 @@ bool BladeRunnerEngine::startup(bool hasSavegames) {
 		return false;
 	}
 
-	// TODO: Allocate audio cache
+	_audioCache = new AudioCache(this);
 
 	if (hasSavegames) {
 		if (!loadSplash()) {
@@ -664,6 +666,9 @@ void BladeRunnerEngine::shutdown() {
 	delete _audioMixer;
 	_audioMixer = nullptr;
 
+	delete _audioCache;
+	_audioCache = nullptr;
+
 	if (isArchiveOpen("MUSIC.MIX")) {
 		closeArchive("MUSIC.MIX");
 	}
diff --git a/engines/bladerunner/bladerunner.h b/engines/bladerunner/bladerunner.h
index f52f808b25..e1469dbbe1 100644
--- a/engines/bladerunner/bladerunner.h
+++ b/engines/bladerunner/bladerunner.h
@@ -60,6 +60,7 @@ class ActorDialogueQueue;
 class ScreenEffects;
 class AIScripts;
 class AmbientSounds;
+class AudioCache;
 class AudioMixer;
 class AudioPlayer;
 class AudioSpeech;
@@ -118,6 +119,7 @@ public:
 	ScreenEffects      *_screenEffects;
 	AIScripts          *_aiScripts;
 	AmbientSounds      *_ambientSounds;
+	AudioCache         *_audioCache;
 	AudioMixer         *_audioMixer;
 	AudioPlayer        *_audioPlayer;
 	AudioSpeech        *_audioSpeech;
@@ -277,7 +279,6 @@ public:
 	bool isSubtitlesEnabled();
 	void setSubtitlesEnabled(bool newVal);
 
-
 	Common::SeekableReadStream *getResourceStream(const Common::String &name);
 
 	bool playerHasControl();
diff --git a/engines/bladerunner/module.mk b/engines/bladerunner/module.mk
index 7f800e5354..e52f8e5bb1 100644
--- a/engines/bladerunner/module.mk
+++ b/engines/bladerunner/module.mk
@@ -10,6 +10,7 @@ MODULE_OBJS = \
 	ambient_sounds.o \
 	archive.o \
 	aud_stream.o \
+	audio_cache.o \
 	audio_mixer.o \
 	audio_player.o \
 	audio_speech.o \
diff --git a/engines/bladerunner/music.cpp b/engines/bladerunner/music.cpp
index 61326b6bb1..42c0b62759 100644
--- a/engines/bladerunner/music.cpp
+++ b/engines/bladerunner/music.cpp
@@ -45,7 +45,7 @@ Music::Music(BladeRunnerEngine *vm) {
 }
 
 Music::~Music() {
-	stop(1);
+	stop(0);
 	while (isPlaying()) {
 		// wait for the mixer to finish
 	}
diff --git a/engines/bladerunner/savefile.cpp b/engines/bladerunner/savefile.cpp
index 33a91b8b73..929bd8b008 100644
--- a/engines/bladerunner/savefile.cpp
+++ b/engines/bladerunner/savefile.cpp
@@ -31,9 +31,6 @@
 
 #include "graphics/thumbnail.h"
 
-
-
-#include "common/debug.h"
 namespace BladeRunner {
 
 SaveStateList SaveFileManager::list(const Common::String &target) {
diff --git a/engines/bladerunner/script/script.cpp b/engines/bladerunner/script/script.cpp
index 4c205d5d4c..14f34c4946 100644
--- a/engines/bladerunner/script/script.cpp
+++ b/engines/bladerunner/script/script.cpp
@@ -335,9 +335,11 @@ void ScriptBase::Actor_Says_With_Pause(int actorId, int sentenceId, float pause,
 			Sound_Play(Random_Query(319, 327), 40, 0, 0, 50);
 		}
 	}
-	if(pause > 0.0f && !_vm->_speechSkipped) {
+
+	if (pause > 0.0f && !_vm->_speechSkipped) {
 		Delay(pause * 1000);
 	}
+
 	Player_Gains_Control();
 }