Dispose SampleChannel in Sample if it is not disposed yet

[hwsmm]

• Requires Fix SampleChannelBass reporting wrong state #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 602ee1e, 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 #6397 (comment). 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: #6401

[peppy]

Still need to remove some commits from here

[hwsmm]

If #6401 gets merged, then we will have only 4 commits for this, I think?

I am not sure if there is a rule for this, but I think this way is better as I won't need to merge master after the required PR gets merged. You can still find an exclusive diff for this PR here: hwsmm/osu-framework@hwsmm:osu-framework:fix-smpbass...clean-samplech

[peppy]

The whole point is to make both PRs exclusive. I want to merge the disposal fix as a priority.

[hwsmm]

It's required because using SampleChannelBass.Stop would dispose the channel in the next update without it. Keeping a correct value for IsAlive is important for this to work well. If you are fine without it, I can separate two PRs.

I think I should have made intentions more clear. I'm sorry about that..