Fix possible memory leaks in sound handler, simplify API

client/HeroMovementController.cpp

@@ -312,7 +312,7 @@ void HeroMovementController::updateMovementSound(const CGHeroInstance * h, int3
 			ENGINE->sound().stopSound(currentMovementSoundChannel);
 
 		if(!currentMovementSoundName.empty())
-			currentMovementSoundChannel = ENGINE->sound().playSound(currentMovementSoundName, -1, true);
+			currentMovementSoundChannel = ENGINE->sound().playSoundLooped(currentMovementSoundName);
 		else
 			currentMovementSoundChannel = -1;
 	}

client/battle/BattleAnimationClasses.cpp

@@ -357,7 +357,7 @@ bool MovementAnimation::init()
 
 	if (moveSoundHandler == -1)
 	{
-		moveSoundHandler = ENGINE->sound().playSound(stack->unitType()->sounds.move, -1);
+		moveSoundHandler = ENGINE->sound().playSoundLooped(stack->unitType()->sounds.move);
 	}
 
 	Point begPosition = owner.stacksController->getStackPositionAtHex(prevHex, stack);

client/media/CSoundHandler.cpp

@@ -52,6 +52,11 @@ CSoundHandler::CSoundHandler():
 	}
 }
 
+void CSoundHandler::MixChunkDeleter::operator()(Mix_Chunk * ptr)
+{
+	Mix_FreeChunk(ptr);
+}
+
 CSoundHandler::~CSoundHandler()
 {
 	if(isInitialized())
@@ -59,36 +64,28 @@ CSoundHandler::~CSoundHandler()
 		Mix_ChannelFinished(nullptr);
 		Mix_HaltChannel(-1);
 
-		for(auto & chunk : soundChunks)
-		{
-			if(chunk.second.first)
-				Mix_FreeChunk(chunk.second.first);
-		}
-
-		for(auto & chunk : soundChunksRaw)
-		{
-			if(chunk.second.first)
-				Mix_FreeChunk(chunk.second.first);
-		}
+		soundChunks.clear();
+		uncachedPlayingChunks.clear();
 	}
 }
 
+Mix_Chunk * CSoundHandler::getSoundChunkCached(const AudioPath & sound)
+{
+	if (soundChunks.find(sound) == soundChunks.end())
+		soundChunks[sound].first = getSoundChunk(sound);
+
+	return soundChunks[sound].first.get();
+}
+
 // Allocate an SDL chunk and cache it.
-Mix_Chunk * CSoundHandler::GetSoundChunk(const AudioPath & sound, bool cache)
+CSoundHandler::MixChunkPtr CSoundHandler::getSoundChunk(const AudioPath & sound)
 {
 	try
 	{
-		if(cache && soundChunks.find(sound) != soundChunks.end())
-			return soundChunks[sound].first;
-
 		auto data = CResourceHandler::get()->load(sound.addPrefix("SOUNDS/"))->readAll();
 		SDL_RWops * ops = SDL_RWFromMem(data.first.get(), data.second);
 		Mix_Chunk * chunk = Mix_LoadWAV_RW(ops, 1); // will free ops
-
-		if(cache)
-			soundChunks.insert({sound, std::make_pair(chunk, std::move(data.first))});
-
-		return chunk;
+		return MixChunkPtr(chunk);
 	}
 	catch(std::exception & e)
 	{
@@ -97,22 +94,15 @@ Mix_Chunk * CSoundHandler::GetSoundChunk(const AudioPath & sound, bool cache)
 	}
 }
 
-Mix_Chunk * CSoundHandler::GetSoundChunk(std::pair<std::unique_ptr<ui8[]>, si64> & data, bool cache)
+CSoundHandler::MixChunkPtr CSoundHandler::getSoundChunk(std::pair<std::unique_ptr<ui8[]>, si64> & data)
 {
 	try
 	{
 		std::vector<ui8> startBytes = std::vector<ui8>(data.first.get(), data.first.get() + std::min(static_cast<si64>(100), data.second));
 
-		if(cache && soundChunksRaw.find(startBytes) != soundChunksRaw.end())
-			return soundChunksRaw[startBytes].first;
-
 		SDL_RWops * ops = SDL_RWFromMem(data.first.get(), data.second);
 		Mix_Chunk * chunk = Mix_LoadWAV_RW(ops, 1); // will free ops
-
-		if(cache)
-			soundChunksRaw.insert({startBytes, std::make_pair(chunk, std::move(data.first))});
-
-		return chunk;
+		return MixChunkPtr(chunk);
 	}
 	catch(std::exception & e)
 	{
@@ -174,36 +164,53 @@ uint32_t CSoundHandler::getSoundDurationMilliseconds(const AudioPath & sound)
 }
 
 // Plays a sound, and return its channel so we can fade it out later
-int CSoundHandler::playSound(soundBase::soundID soundID, int repeats)
+int CSoundHandler::playSound(soundBase::soundID soundID)
 {
 	assert(soundID < soundBase::sound_after_last);
 	auto sound = AudioPath::builtin(soundsList[soundID]);
 	logGlobal->trace("Attempt to play sound %d with file name %s with cache", soundID, sound.getOriginalName());
 
-	return playSound(sound, repeats, true);
+	return playSoundImpl(sound, 0, true);
+}
+
+int CSoundHandler::playSoundLooped(const AudioPath & sound)
+{
+	return playSoundImpl(sound, -1, true);
 }
 
-int CSoundHandler::playSound(const AudioPath & sound, int repeats, bool cache)
+int CSoundHandler::playSound(const AudioPath & sound)
+{
+	return playSoundImpl(sound, 0, false);
+}
+
+int CSoundHandler::playSoundImpl(const AudioPath & sound, int repeats, bool useCache)
 {
 	if(!isInitialized() || sound.empty())
 		return -1;
 
 	int channel;
-	Mix_Chunk * chunk = GetSoundChunk(sound, cache);
+	MixChunkPtr chunkPtr = getSoundChunk(sound);
+	Mix_Chunk * chunk = nullptr;
+	if (!useCache)
+	{
+		chunkPtr = getSoundChunk(sound);
+		chunk = chunkPtr.get();
+	}
+	else
+		chunk = getSoundChunkCached(sound);
 
 	if(chunk)
 	{
-		channel = Mix_PlayChannel(-1, chunk, repeats);
+		channel = Mix_PlayChannel(-1, chunk, 0);
 		if(channel == -1)
 		{
 			logGlobal->error("Unable to play sound file %s , error %s", sound.getOriginalName(), Mix_GetError());
-			if(!cache)
-				Mix_FreeChunk(chunk);
 		}
-		else if(cache)
-			initCallback(channel);
 		else
-			initCallback(channel, [chunk](){ Mix_FreeChunk(chunk);});
+		{
+			storeChunk(channel, std::move(chunkPtr));
+			initCallback(channel);
+		}
 	}
 	else
 		channel = -1;
@@ -211,22 +218,28 @@ int CSoundHandler::playSound(const AudioPath & sound, int repeats, bool cache)
 	return channel;
 }
 
-int CSoundHandler::playSound(std::pair<std::unique_ptr<ui8[]>, si64> & data, int repeats, bool cache)
+void CSoundHandler::storeChunk(int channel, MixChunkPtr chunk)
+{
+	std::scoped_lock lockGuard(mutexCallbacks);
+	uncachedPlayingChunks[channel] = std::move(chunk);
+}
+
+int CSoundHandler::playSound(std::pair<std::unique_ptr<ui8[]>, si64> & data)
 {
 	int channel = -1;
-	if(Mix_Chunk * chunk = GetSoundChunk(data, cache))
+	auto chunk = getSoundChunk(data);
+	if(chunk)
 	{
-		channel = Mix_PlayChannel(-1, chunk, repeats);
+		channel = Mix_PlayChannel(-1, chunk.get(), 0);
 		if(channel == -1)
 		{
 			logGlobal->error("Unable to play sound, error %s", Mix_GetError());
-			if(!cache)
-				Mix_FreeChunk(chunk);
 		}
-		else if(cache)
-			initCallback(channel);
 		else
-			initCallback(channel, [chunk](){ Mix_FreeChunk(chunk);});
+		{
+			storeChunk(channel, std::move(chunk));
+			initCallback(channel);
+		}
 	}
 	return channel;
 }
@@ -386,7 +399,7 @@ void CSoundHandler::ambientUpdateChannels(std::map<AudioPath, int> soundsArg)
 
 		if(!vstd::contains(ambientChannels, soundId))
 		{
-			int channel = playSound(soundId, -1);
+			int channel = playSoundLooped(soundId);
 			int channelVolume = ambientDistToVolume(distance);
 			channelVolumes[channel] = channelVolume;
 

client/media/CSoundHandler.h

@@ -19,16 +19,29 @@ struct Mix_Chunk;
 class CSoundHandler final : public CAudioBase, public ISoundPlayer
 {
 private:
+	struct MixChunkDeleter
+	{
+		void operator ()(Mix_Chunk *);
+	};
+	using MixChunkPtr = std::unique_ptr<Mix_Chunk, MixChunkDeleter>;
+
 	//update volume on configuration change
 	SettingsListener listener;
 	void onVolumeChange(const JsonNode & volumeNode);
 
-	using CachedChunk = std::pair<Mix_Chunk *, std::unique_ptr<ui8[]>>;
+	using CachedChunk = std::pair<MixChunkPtr, std::unique_ptr<ui8[]>>;
+
+	/// List of all permanently cached sound chunks
 	std::map<AudioPath, CachedChunk> soundChunks;
-	std::map<std::vector<ui8>, CachedChunk> soundChunksRaw;
 
-	Mix_Chunk * GetSoundChunk(const AudioPath & sound, bool cache);
-	Mix_Chunk * GetSoundChunk(std::pair<std::unique_ptr<ui8[]>, si64> & data, bool cache);
+	/// List of all currently playing chunks that are currently playing
+	/// and should be deallocated once channel playback is over
+	/// indexed by channel ID
+	std::map<int, MixChunkPtr> uncachedPlayingChunks;
+
+	MixChunkPtr getSoundChunk(const AudioPath & sound);
+	Mix_Chunk * getSoundChunkCached(const AudioPath & sound);
+	MixChunkPtr getSoundChunk(std::pair<std::unique_ptr<ui8[]>, si64> & data);
 
 	/// have entry for every currently active channel
 	/// vector will be empty if callback was not set
@@ -51,6 +64,8 @@ private:
 
 	void initCallback(int channel, const std::function<void()> & function);
 	void initCallback(int channel);
+	void storeChunk(int channel, MixChunkPtr chunk);
+	int playSoundImpl(const AudioPath & sound, int repeats, bool useCache);
 
 public:
 	CSoundHandler();
@@ -62,9 +77,10 @@ public:
 
 	// Sounds
 	uint32_t getSoundDurationMilliseconds(const AudioPath & sound) final;
-	int playSound(soundBase::soundID soundID, int repeats = 0) final;
-	int playSound(const AudioPath & sound, int repeats = 0, bool cache = false) final;
-	int playSound(std::pair<std::unique_ptr<ui8[]>, si64> & data, int repeats = 0, bool cache = false) final;
+	int playSound(soundBase::soundID soundID) final;
+	int playSound(const AudioPath & sound) final;
+	int playSoundLooped(const AudioPath & sound) final;
+	int playSound(std::pair<std::unique_ptr<ui8[]>, si64> & data) final;
 	int playSoundFromSet(std::vector<soundBase::soundID> & sound_vec) final;
 	void stopSound(int handler) final;
 	void pauseSound(int handler) final;

client/media/ISoundPlayer.h

@@ -17,9 +17,10 @@ class ISoundPlayer
 public:
 	virtual ~ISoundPlayer() = default;
 
-	virtual int playSound(soundBase::soundID soundID, int repeats = 0) = 0;
-	virtual int playSound(const AudioPath & sound, int repeats = 0, bool cache = false) = 0;
-	virtual int playSound(std::pair<std::unique_ptr<ui8[]>, si64> & data, int repeats = 0, bool cache = false) = 0;
+	virtual int playSound(soundBase::soundID soundID) = 0;
+	virtual int playSound(const AudioPath & sound) = 0;
+	virtual int playSoundLooped(const AudioPath & sound) = 0;
+	virtual int playSound(std::pair<std::unique_ptr<ui8[]>, si64> & data) = 0;
 	virtual int playSoundFromSet(std::vector<soundBase::soundID> & sound_vec) = 0;
 	virtual void stopSound(int handler) = 0;
 	virtual void pauseSound(int handler) = 0;