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

Remove allocations during bass device sync polling operation #6124

Merged
merged 3 commits into from
Jan 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions osu.Framework.Tests/Audio/AudioManagerWithDeviceLoss.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using ManagedBass;
using osu.Framework.Audio;
Expand Down Expand Up @@ -40,12 +40,20 @@ protected override bool InitBass(int device)
}
}

protected override IEnumerable<DeviceInfo> EnumerateAllDevices()
protected override bool CheckForDeviceChanges(ImmutableArray<DeviceInfo> previousDevices)
{
var devices = base.EnumerateAllDevices();
if (simulateLoss)
return true;

return base.CheckForDeviceChanges(previousDevices);
}

protected override ImmutableArray<DeviceInfo> GetAllDevices()
{
var devices = base.GetAllDevices();

if (simulateLoss)
devices = devices.Take(BASS_INTERNAL_DEVICE_COUNT);
devices = devices.Take(BASS_INTERNAL_DEVICE_COUNT).ToImmutableArray();

return devices;
}
Expand Down
67 changes: 46 additions & 21 deletions osu.Framework/Audio/AudioManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -137,15 +137,14 @@ public class AudioManager : AudioCollectionManager<AudioComponent>
Bass.CurrentDevice != Bass.DefaultDevice;

// Mutated by multiple threads, must be thread safe.
private ImmutableList<DeviceInfo> audioDevices = ImmutableList<DeviceInfo>.Empty;
private ImmutableArray<DeviceInfo> audioDevices = ImmutableArray<DeviceInfo>.Empty;
private ImmutableList<string> audioDeviceNames = ImmutableList<string>.Empty;

private Scheduler scheduler => thread.Scheduler;

private Scheduler eventScheduler => EventScheduler ?? scheduler;

private readonly CancellationTokenSource cancelSource = new CancellationTokenSource();
private readonly DeviceInfoUpdateComparer updateComparer = new DeviceInfoUpdateComparer();

/// <summary>
/// The scheduler used for invoking publicly exposed delegate events.
Expand Down Expand Up @@ -208,7 +207,8 @@ public AudioManager(AudioThread audioThread, ResourceStore<byte[]> trackStore, R
{
try
{
syncAudioDevices();
if (CheckForDeviceChanges(audioDevices))
syncAudioDevices();
Thread.Sleep(1000);
}
catch
Expand Down Expand Up @@ -427,17 +427,10 @@ protected virtual bool InitBass(int device)

private void syncAudioDevices()
{
// audioDevices are updated if:
// - A new device is added
// - An existing device is Enabled/Disabled or set as Default
var updatedAudioDevices = EnumerateAllDevices().ToImmutableList();
if (audioDevices.SequenceEqual(updatedAudioDevices, updateComparer))
return;

audioDevices = updatedAudioDevices;
audioDevices = GetAllDevices();

// Bass should always be providing "No sound" and "Default" device.
Trace.Assert(audioDevices.Count >= BASS_INTERNAL_DEVICE_COUNT, "Bass did not provide any audio devices.");
Trace.Assert(audioDevices.Length >= BASS_INTERNAL_DEVICE_COUNT, "Bass did not provide any audio devices.");

var oldDeviceNames = audioDeviceNames;
var newDeviceNames = audioDeviceNames = audioDevices.Skip(BASS_INTERNAL_DEVICE_COUNT).Where(d => d.IsEnabled).Select(d => d.Name).ToImmutableList();
Expand All @@ -459,11 +452,50 @@ private void syncAudioDevices()
}
}

protected virtual IEnumerable<DeviceInfo> EnumerateAllDevices()
/// <summary>
/// Check whether any audio device changes have occurred.
///
/// Changes supported are:
/// - A new device is added
/// - An existing device is Enabled/Disabled or set as Default
/// </summary>
/// <remarks>
/// This method is optimised to incur the lowest overhead possible.
/// </remarks>
/// <param name="previousDevices">The previous audio devices array.</param>
/// <returns>Whether a change was detected.</returns>
protected virtual bool CheckForDeviceChanges(ImmutableArray<DeviceInfo> previousDevices)
{
int deviceCount = Bass.DeviceCount;

if (previousDevices.Length != deviceCount)
return true;

for (int i = 0; i < deviceCount; i++)
yield return Bass.GetDeviceInfo(i);
{
var prevInfo = previousDevices[i];

Bass.GetDeviceInfo(i, out var info);

if (info.IsEnabled != prevInfo.IsEnabled)
return true;

if (info.IsDefault != prevInfo.IsDefault)
return true;
}

return false;
}

protected virtual ImmutableArray<DeviceInfo> GetAllDevices()
{
int deviceCount = Bass.DeviceCount;

var devices = ImmutableArray.CreateBuilder<DeviceInfo>(deviceCount);
for (int i = 0; i < deviceCount; i++)
devices.Add(Bass.GetDeviceInfo(i));

return devices.MoveToImmutable();
}

// The current device is considered valid if it is enabled, initialized, and not a fallback device.
Expand All @@ -479,12 +511,5 @@ public override string ToString()
string deviceName = audioDevices.ElementAtOrDefault(Bass.CurrentDevice).Name;
return $@"{GetType().ReadableName()} ({deviceName ?? "Unknown"})";
}

private class DeviceInfoUpdateComparer : IEqualityComparer<DeviceInfo>
{
public bool Equals(DeviceInfo x, DeviceInfo y) => x.IsEnabled == y.IsEnabled && x.IsDefault == y.IsDefault;

public int GetHashCode(DeviceInfo obj) => obj.Name.GetHashCode();
}
}
}
4 changes: 3 additions & 1 deletion osu.Framework/Input/Bindings/KeyCombination.cs
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,9 @@ public static KeyCombination FromInputState(InputState state, Vector2? scrollDel

Debug.Assert(!keys.Contains(InputKey.None)); // Having None in pressed keys will break IsPressed
keys.Sort();
return new KeyCombination(keys.ToImmutable());

// Can't use `MoveToImmutable` here as we don't provide accurate capacity.
return new KeyCombination(keys.ToImmutableList());
Comment on lines +648 to +649
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure what this change is doing in here...

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Also fixed a couple of incorrect calls on array building. Check underlying implementation for reasoning (one allocates, other doesn't).

See 7ce8d47.

I can move to another PR if you want, but it was one line change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

1705049523
1705049533
1705049546

In this particular case it looks to me that the difference should be zero and both will allocate, but /shrug. (The .MoveToImmutable() stuff seems valid.)

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Oh yeah, I meant to revert this case. As per the comment, we can't use MoveTo due to capacity not being set (a hidden gotcha).

This will still allocate as the call I incorrectly "reverted" to is synonymous with the original.

}
}

Expand Down
2 changes: 1 addition & 1 deletion osu.Framework/Platform/SDL2Window_Windowing.cs
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ private static ImmutableArray<Display> getSDLDisplays()
Logger.Log($"Failed to retrieve SDL display at index ({i})", level: LogLevel.Error);
}

return builder.ToImmutable();
return builder.MoveToImmutable();
Copy link
Collaborator

Choose a reason for hiding this comment

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

...or this one either, for that matter...

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

See above.

}

private static bool tryGetDisplayFromSDL(int displayIndex, [NotNullWhen(true)] out Display? display)
Expand Down
Loading