Dispose SampleChannel in Sample if it is not disposed yet

[hwsmm]

• Requires https://github.com/ppy/osu-framework/pull/6401

This PR fixes an issue where SampleChannel never gets disposed until the parent Sample is disposed.

SampleChannel.IsAlive can be false if Playing is not true, even if it is not disposed yet. Mixer removes the channel from itself after playing to the end, so if we don't dispose this item in ItemRemoved, it's forever lost.

It can be proven with the below patch. In Samples test scene, you can only see createCount going up continuously, but disposeCount going up only when you exit the scene, and SampleChannels that were already removed both from Mixer and from Sample at that point are already cleared by GC without calling Dispose. If you run osu! with this log patch, you can see that SampleChannelBass got disposed never gets printed without this PR.

This issue also can get fixed by adding a finalizer in SampleChannel, but I think this way is better considering how many SampleChannels are created during runtime.

--- a/osu.Framework/Audio/Sample/SampleChannelBass.cs
+++ b/osu.Framework/Audio/Sample/SampleChannelBass.cs
@@ -212,11 +212,15 @@ private void ensureChannel() => EnqueueAction(() =>
             if (!hasChannel)
                 return;

+            Logging.Logger.Log($"SampleChannelBass got created {++createCount} times so far");
+
             setLoopFlag(Looping);

             relativeFrequencyHandler.SetChannel(channel);
         });

+        private static int createCount = 0;
+
         #region Mixing

         private BassAudioMixer bassMixer => (BassAudioMixer)Mixer.AsNonNull();
@@ -231,6 +235,8 @@ private void ensureChannel() => EnqueueAction(() =>

         #endregion

+        private static int disposeCount = 0;
+
         protected override void Dispose(bool disposing)
         {
             if (IsDisposed)
@@ -238,6 +244,7 @@ protected override void Dispose(bool disposing)

             if (hasChannel)
             {
+                Logging.Logger.Log($"SampleChannelBass got disposed {++disposeCount} times so far");
                 bassMixer.StreamFree(this);
                 channel = 0;
             }

[peppy]

Thanks for this contribution. It seems that you are definitely correct in finding an issue here.

Reading through the changed, it's very hard to review due to the amount of logic changes. Checking your commit history in the branch, the majority are in https://github.com/ppy/osu-framework/pull/6397/commits/602ee1ec2bb386717728b2d5ca45fab8b123f4fc, suggesting the changes were made just to make tests work.

Can you confirm this is the case and your proposed fix is just 934b935? If so, I'd want to fix tests in a simpler way so we don't need the added logic. As a result I haven't reviewed the logic changes just yet.

[hwsmm]

Sorry for making the confusing commit. I was doing this at 4AM, and I found some logic issues while fixing tests.

I wanted to sleep so I ended up putting them all in one. I'll explain things in the commit, and will separate commits later.

SampleChannel userRequestedStop was added because calling Stop made playing to false, which in turn made IsAlive to false, but SampleChannel can be restarted if it haven't reached the end, so I added a condition there to make Sample dispose the channel later.

[peppy]

If you found other logic issues, I'd recommend fixing those in a separate PR. If you're okay with that, let's keep this PR to only fixing disposal.

[peppy]

I see you split things out, but please see https://github.com/ppy/osu-framework/pull/6397#issuecomment-2437250631. This PR should only fix disposal.

[hwsmm]

I am currently writing a separate PR which this PR is based on. I just pushed things first here...

It's now here: https://github.com/ppy/osu-framework/pull/6401