-
Notifications
You must be signed in to change notification settings - Fork 451
feat: implement audio backend specific audio managers #6628
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
base: master
Are you sure you want to change the base?
feat: implement audio backend specific audio managers #6628
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very quick pass
osu.Framework/Game_Audio.cs
Outdated
|
|
||
| if (wasapiSupported) | ||
| { | ||
| // Give preference to WASAPI if available. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except as touched on, the offsets are wrong with WASAPI and need further consideration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be an inaccurate comment. Apologies for the confusion.
| // Give preference to WASAPI if available. | |
| // Candidate WASAPI if available. |
| private Scheduler eventScheduler => EventScheduler ?? Scheduler; | ||
|
|
||
| // Mutated by multiple threads, must be thread safe. | ||
| private ImmutableList<DeviceInfo> audioDevices = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of thing is beyond dodge. The private here should be a backing for the public version, not a separate field which is used to store "old" and "new" in a local method (do that local to the method instead).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main purpose of this field is to reverse lookup from device ID to index. BASS uses indexes to refer to devices, but since this class provides device IDs as a public interface, a way to get the index from the ID is needed. Also, BASS's indexes are for the list of all devices, including those that are not available, while this class provides a filtered list of only available devices as a public interface.
Perhaps the name of this field can be changed to make it clearer.
| // WASAPI device indices don't match normal BASS devices. | ||
| // Each device is listed multiple times with each supported channel/frequency pair. | ||
| // | ||
| // Working backwards to find the correct device is how bass does things internally (see BassWasapi.GetBassDevice). | ||
| if (Bass.CurrentDevice > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this handled in the new code? It doesn't look to be present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section is no longer needed.
In the previous code, the framework always referred to BASS's device list as the one to provide. Since BASS and BASS add-ons each have their own device lists, the traditional WASAPI support needed to re-match what was requested in BASS's device list to WASAPI's device list. This is what this section was doing. While this worked out by chance for WASAPI, at least ASIO provides a completely different unit of list.
In the new code, due to the abstraction, the device list is provided directly according to the add-on actually being used, so such matching is unnecessary.
|
I haven't considered the new code splitting, structure, DI usage or anything. That's going to take hours / days of consideration. Hard to say if any of this abstraction is required when wasapi was already working with the old structure. |
…n id that it should be used
This problem is fixed @ 1d0f324. |
This problem is also fixed @ 07b4727. |
Working on #705
This PR changes the audio management system to use a more modular approach. The main changes include:
osu.Framework.Audio.AudioManagerclass, which was a monolithic audio manager.osu.Framework.Audio.Manager.IAudioManagerinterface and a newosu.Framework.Audio.Manager.AudioManagerabstract class, which are no longer dependent on BASS, will allow for easier work on other audio backends in the future (cc: Implement SDL3 Audio backend #6002).osu.Framework.Audio.Manager.Bass.BassAudioManagerabstract class, which will contain BASS-specific audio management logic.osu.Framework.Audio.Manager.Bass.BassPrimitiveAudioManagerclass, which is a concrete implementation of audio management using BASS, so it can be used as a drop-in replacement for the oldosu.Framework.Audio.AudioManagerclass. WASAPI support is completely removed in this class.osu.Framework.Audio.Manager.Bass.BassWasapiAudioManagerclass, which is a concrete implementation of audio management using BASS with WASAPI support.And the following tweaks:
osu.Framework.Threading.AudioThreadand moved them toBassPrimitiveAudioManagerandBassWasapiAudioManager.OSU_AUDIO_BACKENDandOSU_AUDIO_DEVICE.Notable breaking changes
osu.Framework.Audio.AudioManagerclass is no longer available, and new audio manager classes have some design differences.SetStoremethod to set the resource store after construction.osu.Framework.Audio.Manager.IGlobalMixerProvideinterface to provide the global mixer handle.Known issues
Switching devices later using WASAPI causes the sound to stop playing. I've probably made a design mistake regarding global mixer handling.When a debugger breakpoint occurs while using WASAPI, buffer updates stall, causing the last data in the buffer to loop indefinitely. This may surprise or frustrate you.