Music Module: Looking for memory leaks and/or better approaches

Help and Feedback Code Review
#1

The code will be attached to the bottom of the post. I think it’s important for the context to be first, so even a dropdown would not suffice in having the context be first.

I’m currently creating a music module for my game that’s intended to completely manage the playing and loading of music. I noticed that sounds (especially music) consume a large amount of client memory so I started changing the music system to allow developers to control the loading and unloading of music (presence in DataModel) as well as its playback.

The point of this Code Review thread is that I am greatly unsatisfied with the API and have concerns that there’s memory leaks. The API can be grazed over at a cursory glance while my memory leak concerns stem from how I handle tweens. Soundtracks aren’t treated as objects so developers have to constantly pass names when they want to interact with a track which is… somewhat ugly.

I’ve thought about moving my system to an object-oriented approach where tracks would be managed as an object. Methods such as playing and pausing would exist on the object and then the main player’s version of those functions would check which song the developer wants to interact with before calling the relevant function on the track object.

Would appreciate any replies about if my code leaks memory or if there’s a better way to approach this music player. Below is the exact code I currently have including framework calls (irrelevant to know though) and unfinished documentation (assuming I change how I do this).

Music.lua 
--- Controls music loading and playback from the game's data.
-- Memory conscious; unloaded tracks are removed from the DataModel.
-- @author colbert2677

local ContentProvider = game:GetService("ContentProvider")
local SoundService = game:GetService("SoundService")
local TweenService = game:GetService("TweenService")

local MUSIC_SOUND_GROUP = SoundService:WaitForChild("BGM")
local DEFAULT_TWEEN_INFO = TweenInfo.new(1, Enum.EasingStyle.Linear, Enum.EasingDirection.Out)
local MAX_LOADED_TRACKS = 6

local Soundtrack
local Promise

local Butler = {
	LoadedTracks = table.create(MAX_LOADED_TRACKS),
}

--- Get a track from the loaded tracks table by name
-- Cases of nil returns need to be handled in each method
-- @param trackName name of the track to fetch
-- @return table of track data or nil
function Butler:_GetLoadedTrack(trackName: string)
	local trackData = self.LoadedTracks[trackName]
	
	if not trackData then
		local issueMessage = string.format("Attempted to interact with track \"%s\" (not a loaded track)")
		self:DebugLog(issueMessage, "warning")
		return nil
	end

	return trackData
end

--- Force cancel the active tween given a track data table
-- @param trackData a table of track data
function Butler:_CancelTweenIn(trackData: table)
	local tween = trackData.Tween
	if not tween then return end

	tween:Cancel() -- Stop tween
	tween:Destroy() -- Disconnect connections

	trackData.Tween = nil
end

--- Get a list and number of tracks loaded by the Butler
-- @return an array of the names of loaded tracks
function Butler:GetLoadedTracks()
	local trackNameTable = table.create(MAX_LOADED_TRACKS)

	for trackName, _ in pairs(self.LoadedTracks) do
		table.insert(trackNameTable, trackName)
	end

	return trackNameTable
end

--- Identical to GetLoadedTracks but scoped to playing tracks
-- @see GetLoadedTracks
function Butler:GetPlayingTracks()
	local trackNameTable = table.create(MAX_LOADED_TRACKS)

	for trackName, trackData in pairs(self.LoadedTracks) do
		if trackData.Sound.IsPlaying == true then
			table.insert(trackNameTable, trackName)
		end
	end

	return trackNameTable
end

--- Load a track with an id with optional modifiers
-- Requires a named identifier
-- @param trackName named identifier
-- @param trackId assetid for the track
-- @param trackSpeed PlaybackSpeed
-- @param trackVolume Volume
function Butler:LoadTrackById(trackName: string, trackId: number, trackSpeed: number, trackVolume: number)
	if #self:GetPlayingTracks() >= MAX_LOADED_TRACKS then
		local issueMessage = string.format("Cannot exceed %d loaded tracks (soft rejected id=%d)", MAX_LOADED_TRACKS, trackId)
		self:DebugLog(issueMessage, "warning")
		return
	elseif self.LoadedTracks[trackName] then
		local issueMessage = string.format("Track already loaded (soft rejected name=%s)", trackName)
		self:DebugLog(issueMessage, "warning")
		return
	end

	local sound = Instance.new("Sound")
	sound.Name = trackName
	sound.Looped = true
	sound.SoundId = "rbxassetid://" .. tostring(trackId)
	sound.PlaybackSpeed = trackSpeed or sound.PlaybackSpeed
	sound.Volume = 0
	sound.SoundGroup = MUSIC_SOUND_GROUP

	local soundData = table.create(4)
	soundData.Sound = sound
	soundData.OriginalVolume = trackVolume
	soundData.LastAction = "Loaded"

	self.LoadedTracks[trackName] = soundData

	Promise.try(function ()
		ContentProvider:PreloadAsync({sound})
	end)

	sound.Parent = MUSIC_SOUND_GROUP
end

--- Load a track from the preset Soundtrack module in Shared
-- trackName must be a valid key in the above module's table
-- @see LoadTrackById
function Butler:LoadTrackByName(trackName: string)
	local soundtrackData = Soundtrack[trackName]
	if not soundtrackData then
		local issueMessage = string.format("Attempted to load a track that does not exist (\"%s\")", trackName)
		self:DebugLog(issueMessage)
		return
	end

	local trackId = soundtrackData.SoundId
	local trackSpeed = soundtrackData.PlaybackSpeed
	local trackVolume = soundtrackData.Volume

	self:LoadTrackById(trackName, trackId, trackSpeed, trackVolume)
end

--- Prepare track for destruction
-- @param trackName name of a loaded track to unload
function Butler:UnloadTrack(trackName: string)
	local track = self.LoadedTracks[trackName]
	if not track then
		local issueMessage = string.format("Attempted to unload a track that does not exist (\"%s\")", trackName)
		self:DebugLog(issueMessage, "warning")
		return
	end

	self:CancelTweenIn(track)
	self:StopTrack(trackName)

	local tween = track.Tween
	if (tween) and not (tween.PlaybackState == Enum.PlaybackState.Completed or tween.PlaybackState == Enum.PlaybackState.Cancelled) then
		tween.Completed:Wait()
	end

	track.Sound:Destroy()
	track.Tween:Destroy()
	
	track = nil
	tween = nil

	self.LoadedTracks[trackName] = nil
end

function Butler:UnloadAllTracks()
	for trackName, _ in pairs(self.LoadedTracks) do
		Promise.try(function ()
			self:UnloadTrack(trackName)
		end)
	end
end

function Butler:PlayTrack(trackName: string)
	local track = self:_GetLoadedTrack(trackName)
	if not track then return end

	local sound = track.Sound

	self:_CancelTweenIn(track)

	-- Very bootleg in place of an if statement
	sound[sound.IsPaused and "Resume" or "Play"](sound)
	
	local tween = TweenService:Create(sound, DEFAULT_TWEEN_INFO, {Volume = track.OriginalVolume})
	track.Tween = tween
	tween:Play()
end

function Butler:PauseTrack(trackName: string)
	local track = self:_GetLoadedTrack(trackName)
	if not track then return end

	local sound = track.Sound

	self:_CancelTweenIn(track)

	local tween = TweenService:Create(sound, DEFAULT_TWEEN_INFO, {Volume = 0})
	tween.Completed:Connect(function (playbackState)
		if not (playbackState == Enum.PlaybackState.Completed) then return end
		sound:Pause()
	end)
	track.Tween = tween
	tween:Play()
end

function Butler:MuteTrack(trackName: string)
	local track = self:_GetLoadedTrack(trackName)
	if not track then return end

	local sound = track.Sound

	self:_CancelTweenIn(track)

	local tween = TweenService:Create(sound, DEFAULT_TWEEN_INFO, {Volume = 0})
	track.Tween = tween
	tween:Play()
end

function Butler:UnmuteTrack(trackName: string)
	local track = self:_GetLoadedTrack(trackName)
	if not track then return end

	local sound = track.Sound

	self:_CancelTweenIn(track)

	local tween = TweenService:Create(sound, DEFAULT_TWEEN_INFO, {Volume = track.OriginalVolume})
	track.Tween = tween
	tween:Play()
end

function Butler:StopTrack(trackName: string)
	local track = self:_GetLoadedTrack(trackName)
	if not track then return end

	local sound = track.Sound

	self:_CancelTweenIn(track)

	local tween = TweenService:Create(sound, DEFAULT_TWEEN_INFO, {Volume = 0})
	tween.Completed:Connect(function (playbackState)
		if not (playbackState == Enum.PlaybackState.Completed) then return end
		sound:Stop()
	end)
	track.Tween = tween
	tween:Play()
end

function Butler:Start()
	-- Left intentionally blank.
end

function Butler:Init()
	Soundtrack = self.Shared.Soundtrack
	Promise = self.Shared.Promise
end

return Butler

(The code looks long but the bottom half with all the functions are functionally identical. Some just use different values in the tween goals or have Completed connections.

2 Likes
#2

I assume _GetLoadedTrack() is a private function while GetLoadedTrack() is a public function.

if _GetLoadedTrack() is a private function, instead of making it a method try making it a local function?

Do local function GetLoadedTrack() for private function while function Butler:GetLoadedTrack() for public.

#3

I would use Quenty’s Maid api , and create a Maid for the tracks, and tweens which handles the :Destroy() methods for Instances, and any other connections Instances have. Then when the player leaves, or stops playing music you would just do Maid:DoCleaning() , or Maid:Destroy() which will destroy the Maid, but also run the :DoCleaning() method.

1 Like
#4

@MrMouse2405 Thank you for the post however this does not contribute any sort of improvement upon what I’m currently doing other than moving GetLoadedTrack from being an indice in the module’s main table to being an upvalue for the module. I am primarily looking for areas where a memory leak may be caused or for an overall better approach to my system, not for trivial syntax changes.

@OnlyRoids Appreciate the response. I find the Maid pattern a little too much for a simple system like this, I’m trying to see if I can clean up my instances without an abstraction on the process for one system (if I had multiple systems that used Maid, it’d be a different story altogether).

There are only two object dependencies, the sound and the tween. The sound is appropriately removed from tables and destroyed though I’m not sure about the tweens. The tweens might be fine but in the case where I destroy the tween from a Completed connection that’s where I’m uncertain. For example, take the following code:

tween.Completed:Connect(function ()
    tween:Destroy()
end)

I have no idea if that garbage collects properly, including after if I’ve removed all references to the tween (which are from the creator function where the variable is cleaned when the function finishes and the scope closes, a connection which is destroyed and removal from my data dictionary).

I’ll definitely keep the Maid pattern in mind though. Thankfully the Instance that the tween is running on is provided as a property so I’ll just have to deal with a few hanging references until the items are cleaned from the Maid. In the case that I’m using a Maid I’ll probably be switching to an object-oriented approach completely.

#5

It will, since :Destroy() implicitly disconnects all connections. But, if you had a scenario like this

local function foo()
	local tween = TweenService:Create(...)
	tween.Completed:Connect(function() end)
end

then yes this would leak memory, since the connection keeps the object alive, so you would have to destroy it or disconnect the connection explicitly if you wanted to be 100% sure.

1 Like
#6

Oh perfect thank you. Garbage collection and memory leaks tend to trouble me no matter how many resources I read up on them but I think I’m getting around.

Knowing this I could probably explicitly destroy all Completed connections right at the top before the guard clause checking for the tween’s PlaybackState. That resolves the cancellation method and pretty much every memory leak concern.

Next thing is looking at this weird and ugly API I’ve got down.

#7

Update: the music module is now being used in a production-level environment, ugly API included. I will most likely be moving to stateful objects but not full on OOP (something either lightweight or bootleg) to better handle the API or I’ll just write it to be better.

Since the memory leaks were the other concern, I’ve gone ahead and marked the post that resolves my concern about memory leaks best as the solution. I will trifle with the API on my own time.