This is a possible memory leak that appears related to channels holding on to instruments and their samples [when a sequence is involved].
Test case
In the test Lua script below I:
create an instrument, load a sample into a synth, add the synth as a voice to the instrument
assign the instrument to the tracks of a sequence, play the sequence
create a channel and add the instrument as source to it
in the sequence finish callback remove the instrument as source from the channel, set references to the instrument and channel to nil
Here's the script:
import "CoreLibs/object"
local pdSound <const> = playdate.sound
local pdInstrument <const> = pdSound.instrument
local pdSynth <const> = pdSound.synth
local pdSample <const> = pdSound.sample
-- Set up an instrument, synth, and sample for testing
local instrument <const> = pdInstrument.new()
local sample <const>, err = pdSample.new("sample.wav")
local synth <const> = pdSynth.new(sample)
instrument:addVoice(synth)
-- Create a sound sequence from a MIDI file and assign the instrument to all of its (really, one) tracks
local seq <const> = pdSound.sequence.new("test.mid")
for idx = 1, seq:getTrackCount() do
seq:getTrackAtIndex(idx):setInstrument(instrument)
end
-- Store the relevant references in a table for the sequence finishCallback to use
sndTable = {
instr = instrument,
channel = pdSound.channel.new()
}
-- Should show no playing sources
printTable(pdSound.playingSources())
-- Add the instrument as a source to the new channel. The problem seems to be here.
-- Leave this in, and sample memory is not properly freed.
-- Comment this out, and things appear to be as expected
sndTable.channel:addSource(sndTable.instr)
print("Playing sequence")
seq:play(function()
-- On playback completion, remove the instrument as source from the channel
-- ... or so we would expect
sndTable.channel:removeSource(sndTable.instr)
sndTable.instr = nil
sndTable.channel = nil
sndTable = nil
print("Sequence finished, everything cleared")
-- Should show no playing sources? Still shows one playing instrument
printTable(pdSound.playingSources())
end)
-- Should show one playing instrument
printTable(pdSound.playingSources())
function playdate.update()
end
Unexpected result
Two things happen:
the memory for the sample (and other objects, samples being the most notable) remains listed in the malloc log, as we can see below (active only, sorted by size, line 10 being where the sample is loaded in the script) for an active total shown of 444.58KB:
the final call to printTable(pdSound.playingSources()) at the very end continues to show the instrument as a playing source, where I would expect an empty table as after the first call. Full output:
{
}
Playing sequence
{
playdate.sound.instrument: 00000220B3D0AEB8,
}
11:25:33: Loading: OK
Sequence finished, everything cleared
{
playdate.sound.instrument: 00000220B3D0AEB8,
}
Narrowing it down
If I comment out line 33, which reads sndTable.channel:addSource(sndTable.instr), then the sample entry is gone from the active malloc log entries and indeed the active total in the bottom right is much lower (99.11 KB), which is closer to what I would expect:
but the output of printTable(pdSound.playingSources()) remains the same. I would have expected calls to channel:removeSource() to both allow memory to be released and remove the instrument entry from the playing sources, but that doesn't seem to be the case.
Repro tools
I've placed the test script, sample and MIDI file I used to reproduce this in this shared folder, in the hope that they'll be useful.
Wanted to add, since I double checked just now but didn't include that case in the test script, that this doesn't seem to happen if instead of a sequence + instrument we add a samplePlayer as a channel source. In that case removing the source gives the expected result.
What's going on here is in creating a new channel automatically adds it to the sound engine. (In Lua, but not in C, confusingly.) So there's still a reference to the channel after the variable is cleared, and that's holding a reference to the instrument. To remove the channel from the sound engine when you're done with it you have to call channel:remove() yourself.
I agree that if the channel was added automatically it should be removed automatically, too.. but the API is a but inconsistent here. Fileplayers and sampleplayers will continue to play after they've scoped out of Lua--the underlying C object is reference counted so that its ownership can be passed over to the sound lib when we collect the Lua wrapper. As I recall, we do that because people wanted to be able to fire off one-shot sounds without having to hold on to a reference so they don't stop playing as soon as the scope exits and the collector runs. For some reason, instruments and synths are different, their __gc functions stop them when they're collected.
I think what I'll do is add retain/release to channels so we can pass ownership over when the Lua object is collected, and set a flag that tells the sound lib to remove the channel when its sources are finished. I'm not sure if this will make it into 2.7 after all, but you've got the channel:remove() workaround in the meantime, and using that won't cause any trouble when this automatic removal scheme does show up.
Thanks for looking into it. That makes sense for creating/removing channels, but in normal usage I would want to keep the channel around, and just remove the instrument reference from it to recover the memory allocated for the samples, possibly adding other sources to it at a later time. In the test script, I clear the channel variable as an extra test to attempt to force that memory to be collected (forgot all about channel:remove(), my bad), but expected channel:removeSource() to take care of the instrument reference. That's where the main issue lies for me, am I misunderstanding what channel:removeSource() should do?
So I think the workaround for me would be to create non-permanent channels with the same properties that I have currently set on the main parent channel, assign instruments as sources to these additional channels, propagate any changes normally meant for the main channel to these new channels, and then remove them altogether with channel:remove() instead of relying on channel:removeSource() to swap sources out. A bit more complex, but doable, and does free the sample memory as you suggested.
I still would expect channel:removeSource() to get rid of the instrument and sample references and playdate.sound.playingSources() to return an empty table afterwards, and the fact that they don't still reads like a bug to me. Perhaps @dave you can clarify where this assumption is incorrect?
Found the fix, at last. I'm really surprised this wasn't causing a crash! It was confusing the instrument object with its lua wrapper, meaning when it tried to remove the instrument from the channel it was using a bogus pointer. After fixing that there's nothing listed after Sequence finished, everything cleared and I don't see the sample in the malloc log. Thank you for finding this! I've got the fix scheduled for 2.7.0, which should be out soon.
