Remove effect prioritisation to fix segfaults

[smoogipoo]

• Closes Periodic AccessViolation somewhere in BASS #6202

I intend for this to be a somewhat temporary change because this entire thing needs to be rewritten... As far as I can tell we're not relying on effect priorities in osu! (cc @nekodex ).

We've had issues with FXSetPriority segfaulting the test runner in the past, but it has not been reported to be doing the same in-game: ppy/osu#27338

I haven't been able to repro this in osu!framework, but, under pipewire the game should segfault when entering PlayerLoader with the following diff applied:

diff --git a/osu.Framework/Audio/Mixing/Bass/BassAudioMixer.cs b/osu.Framework/Audio/Mixing/Bass/BassAudioMixer.cs
index 2605a84008..1ff9a69fbc 100644
--- a/osu.Framework/Audio/Mixing/Bass/BassAudioMixer.cs
+++ b/osu.Framework/Audio/Mixing/Bass/BassAudioMixer.cs
@@ -14,6 +14,7 @@
 using osu.Framework.Extensions.EnumExtensions;
 using osu.Framework.Extensions.ObjectExtensions;
 using osu.Framework.Statistics;
+using osu.Framework.Utils;

 namespace osu.Framework.Audio.Mixing.Bass
 {
@@ -272,6 +273,14 @@ protected override void UpdateState()
                 removeChannelFromBassMix(channel);
             }

+            foreach (var effect in ActiveEffects)
+            {
+                if (effect.Handle == 0)
+                    continue;
+
+                ManagedBass.Bass.FXSetPriority(effect.Handle, RNG.Next(0, 10));
+            }
+
             FrameStatistics.Add(StatisticsCounterType.MixChannels, activeChannels.Count);
             base.UpdateState();
         }

I considered the alternative of instead removing and re-adding the effects. This causes a short but noticeable glitch when player load finishes.

What's the cause of all of this? I don't know. Likely some sort of x86-register dependent behaviour in BASS (wouldn't be the first time). It's a bit too deep for the time being.

[bdach]

Incidentally... ppy/osu#27466 (comment)

That's windows too by the way.

[smoogipoo]

Ooh. Well there you go, straight from the horse's (Windows error description's) mouth 😄 I forgot about that.

[nekodex]

Hmm, while I don't believe we're using effect priorities in osu! (yet), I should mention that this change may result in unexpected behaviour whenever the order of effects are changed on a mixer.

Not all audio effects are transitive (e.g. compressor->reverb does not result in the same output as reverb->compressor), so if someone using framework reorders effects expecting the output to change and it doesn't (due to this PR), it may lead to some confusion.

I almost think the alternative of a "short glitch" would be preferable, as at least the resulting behaviour (effects processing in the correct order) would be as expected from a user's perspective.

Ultimately this is probably an edge-case that likely no-one will hit, but I could see it potentially happening for a DAW/DJ application for instance, or as a stretch, a game with dynamic effects applying and mutating realtime in response to... stuff.

[smoogipoo]

For now I'm banking on mixers being unused by anyone other than us, so the important part for me at this point is whether that will have an effect on our existing usages. Effect prioritisation is pretty awkward right now, along with the whole replace-effect-at-index-to-update thing...

(not to mention in going through the code I found potential out-of-range indexing 🙈, it's all so bad)

I'm also considering that we're still lacking nested mixers. With nested mixers, wouldn't effect prioritisation either become a non-issue or become an explicit detail for special cases like you mentioned (due to performance, if anything)?

Edit: Also wanted to mention that I'll be spending a weekend or two rewriting mixers because of this.

[nekodex]

Yeah I agree, the effect handling in framework is a bit... not great... at the moment.

For the time being, removing FXSetPriority is probably fine because:

• We're not even setting effect.Priority anywhere yet, let alone changing it.
• We're only using low-pass/high-pass filters in osu! currently, and filters are effectively transitive meaning we're not gonna notice any issues if they're run out of order anyway. That may change in the future though if/when we start using other effects.

I'm not sure I see how nested mixers alleviates anything though? With each mixer having its own list of effects, we're just nesting the problem at that point? 😅

We could probably workaround this in the future by setting priority at effect creation time and then never changing it. Likely that'll be enough for our needs anyway.

Regardless, surely the correct solution here is to get the segfault in BASS_FXSetPriority fixed rather than trying to work around it? ...funnily enough, I came across this surprisingly relevant thread while briefly digging into this 😜 https://www.un4seen.com/forum/?topic=19592.0

[smoogipoo]

Hmm, indeed it it would be nesting the problem... For that particular case I was imagining a structure like this:

Mixer
{
    Effects = { Compressor },
    Children =
    {
        Mixer
        {
            Effects = { Reverb },
            Children =
            {
                /* Tracks, samples, etc */
            }
        }
    }
}

(i.e. reverb applied first, then compressor second, not sure if correct but it's an example)

Beyond performance concerns, do you imagine changing priority to be a more common operation, or for it to only be global like my example there?

[nekodex]

Honestly? I'm not sure we'll need effect priority changing for osu! itself at all. I imagine we'd be defining effect priority once (e.g. in a BackgroundDependencyLoader method) and then leaving it at that.

I can't really think of any scenarios for osu! off the top of my head where we'd need to alter effect priorities after the fact.