Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add low latency initialisation support on windows by directly interacting with WASAPI #6088

Merged
merged 19 commits into from
Dec 26, 2023

Conversation

peppy
Copy link
Member

@peppy peppy commented Dec 25, 2023

This reduces latency from perceivable (25-40ms) to non-perceivable (5-15ms). With this change, the latency on windows actually becomes limited by the update thread rate.

Pushing this as a draft for those interested in testing it ahead of time. I hope to clean up the single static usage.

If you wish to test in the mean time, please checkout this PR, build osu! after running UseLocalFramework.ps1.

@peppy peppy marked this pull request as draft December 25, 2023 05:17
@peppy peppy force-pushed the wasapi-low-latency branch 4 times, most recently from 135c201 to 1fa2610 Compare December 25, 2023 05:58
@peppy peppy added the next-release Pull requests which are almost there label Dec 25, 2023
@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 25, 2023
@peppy peppy marked this pull request as ready for review December 25, 2023 06:47
@peppy
Copy link
Member Author

peppy commented Dec 25, 2023

From a code perspective, this is now ready for checking. Note that I haven't tested things still work after refactoring just yet.

/// If a global mixer is being used, this will be the BASS handle for it.
/// If non-null, all game mixers should be added to this mixer.
/// </summary>
internal readonly IBindable<int?> GlobalMixerHandle = new Bindable<int?>();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get this may be a bit messy, but I don't want to over-complicate things. In the future we probably still want to revisit having a global mixer in all cases (#4915) at which point this will become the norm, not the "exception" for windows-only.

I could have added the global mixer in this PR but I felt that's probably a bad move to limit potential breakage.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a global parent mixer is probably fine by itself, but what I worry about more is the changes in flags that the presence of this global mixer forces (BassFlags.Decode, BassFlags.MixerChanBuffer | BassFlags.MixerChanNoRampin).

I'd probably expect more commentary in xmldoc here, such that if the global mixer is present, then it basically takes over the duties of playback and all child mixers do not play on their own but rather expose audio data which the global pulls from as required. It seems like a major paradigm shift that isn't really documented anywhere (in fact I'm attempting to back-reason this currently using bass docs, but I'm not really completely sure if I read the intent of setting the aforementioned flags correctly).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I'm not really completely sure if I read the intent of setting the aforementioned flags correctly

the flags are required and it's all just bass nuances. i can document it but at the end of the day all i'll be doing is saying something like "this is done this way because it works". ie. without the decode flag it just breaks everything.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought there was some more nuance to it other than "make bass work in this configuration". If that is the case then I guess I can just not care.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of this stuff was trial and error in an isolated project until shit just worked 😅 💦

Co-authored-by: Bastian Pedersen <bastian.tangedal@gmail.com>
internal static bool InitDevice(int deviceId)
#region BASS Initialisation

// TODO: All this bass init stuff should probably not be in this class.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether this is something to be actioned at this time or left for later but I most definitely agree. Should be in its own class, preferably behind a [SupportedOSPlatform("windows")] attribute.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could move it now, but was planning to leave for a follow-up effort, because I guarantee that will increase review overhead ten-fold.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll look away for now then.

osu.Framework/Threading/AudioThread.cs Outdated Show resolved Hide resolved
/// If a global mixer is being used, this will be the BASS handle for it.
/// If non-null, all game mixers should be added to this mixer.
/// </summary>
internal readonly IBindable<int?> GlobalMixerHandle = new Bindable<int?>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a global parent mixer is probably fine by itself, but what I worry about more is the changes in flags that the presence of this global mixer forces (BassFlags.Decode, BassFlags.MixerChanBuffer | BassFlags.MixerChanNoRampin).

I'd probably expect more commentary in xmldoc here, such that if the global mixer is present, then it basically takes over the duties of playback and all child mixers do not play on their own but rather expose audio data which the global pulls from as required. It seems like a major paradigm shift that isn't really documented anywhere (in fact I'm attempting to back-reason this currently using bass docs, but I'm not really completely sure if I read the intent of setting the aforementioned flags correctly).

@peppy
Copy link
Member Author

peppy commented Dec 25, 2023

I've added some more commentary in the xmldoc, see what you think.

bdach added a commit to bdach/osu that referenced this pull request Dec 25, 2023
…pleted" sync

This fixes an issue identified with the WASAPI implementation in
ppy/osu-framework#6088. It has no real effect
on current `master`, but fixes a deadlock that occurs with the
aforementioned framework branch when one lets a preview track play out
to the end - at this point all audio will stop and an attempt to perform
any synchronous BASS operation (playing another track, seeking) will
result in a deadlock.

It isn't terribly clear as to why this is happening precisely, but
there does not appear to be any need to stop and seek at that point,
so this feels like a decent workaround even if the actual issue is
upstream (and will unblock pushing out WASAPI support to users).
@bdach
Copy link
Collaborator

bdach commented Dec 25, 2023

Switching default output (either by audio settings, or by disconnecting / reconnecting a device) does not work by default for me.

I have a rough version that makes it sorta work:

diff --git a/osu.Framework/Audio/AudioManager.cs b/osu.Framework/Audio/AudioManager.cs
index 07e63484e..fd8f87f40 100644
--- a/osu.Framework/Audio/AudioManager.cs
+++ b/osu.Framework/Audio/AudioManager.cs
@@ -162,7 +162,8 @@ public AudioManager(AudioThread audioThread, ResourceStore<byte[]> trackStore, R
 
             thread.RegisterManager(this);
 
-            AudioDevice.ValueChanged += onDeviceChanged;
+            AudioDevice.ValueChanged += _ => onDeviceChanged();
+            GlobalMixerHandle.ValueChanged += _ => onDeviceChanged();
 
             AddItem(TrackMixer = createAudioMixer(null, nameof(TrackMixer)));
             AddItem(SampleMixer = createAudioMixer(null, nameof(SampleMixer)));
@@ -221,9 +222,9 @@ protected override void Dispose(bool disposing)
             base.Dispose(disposing);
         }
 
-        private void onDeviceChanged(ValueChangedEvent<string> args)
+        private void onDeviceChanged()
         {
-            scheduler.Add(() => setAudioDevice(args.NewValue));
+            scheduler.Add(() => setAudioDevice(AudioDevice.Value));
         }
 
         private void onDevicesChanged()
diff --git a/osu.Framework/Threading/AudioThread.cs b/osu.Framework/Threading/AudioThread.cs
index 439cf4d16..7e02937ca 100644
--- a/osu.Framework/Threading/AudioThread.cs
+++ b/osu.Framework/Threading/AudioThread.cs
@@ -121,6 +121,7 @@ protected override void OnExit()
         // TODO: All this bass init stuff should probably not be in this class.
 
         private WasapiProcedure? wasapiProcedure;
+        private WasapiNotifyProcedure? wasapiNotifyProcedure;
 
         /// <summary>
         /// If a global mixer is being used, this will be the BASS handle for it.
@@ -212,7 +213,11 @@ private void attemptWasapiInitialisation()
 
             // To keep things in a sane state let's only keep one device initialised via wasapi.
             freeWasapi();
+            initWasapi(wasapiDevice);
+        }
 
+        private void initWasapi(int wasapiDevice)
+        {
             // This is intentionally initialised inline and stored to a field.
             // If we don't do this, it gets GC'd away.
             wasapiProcedure = (buffer, length, _) =>
@@ -222,13 +227,30 @@ private void attemptWasapiInitialisation()
 
                 return Bass.ChannelGetData(globalMixerHandle.Value!.Value, buffer, length);
             };
+            wasapiNotifyProcedure = (notify, device, _) =>
+            {
+                if (notify == WasapiNotificationType.DefaultOutput)
+                {
+                    freeWasapi();
+                    initWasapi(device);
+                }
+            };
+
+            bool initialised = BassWasapi.Init(wasapiDevice, Procedure: wasapiProcedure, Buffer: 0.02f, Period: 0.005f);
 
-            if (BassWasapi.Init(wasapiDevice, Procedure: wasapiProcedure, Buffer: 0.02f, Period: 0.005f))
+            if (!initialised)
             {
-                BassWasapi.GetInfo(out var wasapiInfo);
-                globalMixerHandle.Value = BassMix.CreateMixerStream(wasapiInfo.Frequency, wasapiInfo.Channels, BassFlags.MixerNonStop | BassFlags.Decode | BassFlags.Float);
-                BassWasapi.Start();
+                if (Bass.LastError == Errors.Already)
+                    BassWasapi.CurrentDevice = wasapiDevice;
+                else
+                    return;
             }
+
+            BassWasapi.GetInfo(out var wasapiInfo);
+            globalMixerHandle.Value = BassMix.CreateMixerStream(wasapiInfo.Frequency, wasapiInfo.Channels, BassFlags.MixerNonStop | BassFlags.Decode | BassFlags.Float);
+            BassWasapi.Start();
+
+            BassWasapi.SetNotify(wasapiNotifyProcedure);
         }
 
         private void freeWasapi()
@@ -237,7 +259,8 @@ private void freeWasapi()
 
             // The mixer probably doesn't need to be recycled. Just keeping things sane for now.
             Bass.StreamFree(globalMixerHandle.Value.Value);
-            BassWasapi.Free();
+            BassWasapi.Stop();
+            //BassWasapi.Free();
             globalMixerHandle.Value = null;
         }

but:

  • I had to comment BassWasapi.Free() because it deadlocks if it's left as-is there and I'm not sure why or how to fix because it can't be called from there
  • the AudioManager stuff is a dirty hack just so that all the sub-mixers make their way onto the recycled mixer
  • sometimes it plays corrupted sound through multiple devices (reproduction: rapidly switch active device between two devices)
  • I even got it to crash once for whatever reason

so I dunno. I'm leaving it there, this may need more serious restructuring. Definitely think that the lack of usage of SetNotify() in this diff as written is an omission and I don't see how this could correctly handle output switches otherwise.

@bdach
Copy link
Collaborator

bdach commented Dec 26, 2023

I also got the following with my above broken patch once:

Application: osu!.exe
CoreCLR Version: 6.0.2023.32017
.NET Version: 6.0.20
Description: The application requested process termination through System.Environment.FailFast(string message).
Message: A callback was made on a garbage collected delegate of type 'ppy.ManagedBass.Wasapi!ManagedBass.Wasapi.WasapiProcedure::Invoke'.
Stack:

so if you're gonna use that for something, both of the bass callbacks should probably be created once ever.

@Susko3
Copy link
Member

Susko3 commented Dec 26, 2023

With this PR, opening the audio mixer visualiser (Ctrl+F9) makes the tracks and samples speed up really fast. Closing the mixer visualiser restores it back to normal. The "speed up" is tied to the audio thread update rate, eg. in single threaded, the audio goes faster as you cycle trough Vsync, 2x, 4x, 8x. Or if you unfocus the window.

In TestSceneAudioMixerVisualiser, only the global tracks and samples are sped up, it's fine on the added mixers. Unless you open the actual Ctrl+F9 mixer, then everything is sped up (as with all the other test scenes).

Loud noise warning when you test this!

@Morilli
Copy link

Morilli commented Dec 26, 2023

Fwiw when testing osu!-side with this framework PR (on windows), I do need to set a universal offset of around -45 now to match hitsounds to the audio when playing whereas I was previous using the default 0 just fine. Is that an intended side-effect of these changes?

@bdach
Copy link
Collaborator

bdach commented Dec 26, 2023

@Morilli generally yes we know

@bdach
Copy link
Collaborator

bdach commented Dec 26, 2023

With this PR, opening the audio mixer visualiser (Ctrl+F9) makes the tracks and samples speed up really fast

Fixed by 1d2f64c

@bdach bdach merged commit 396c9f8 into ppy:master Dec 26, 2023
21 checks passed
@peppy peppy deleted the wasapi-low-latency branch January 17, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants