Improvements for audio normalization

[hwsmm]

You may want to merge master into your branch before reviewing this. I merged master into my branch because I didn't want to play with framework version..

Changes:

• Use TrackLoudness from osu!framework (requires Add TrackLoudness (BASSloud) ppy/osu-framework#6281)
• Use bindings for normalized track volume, instead of mixer filters
• Hitobjects gets normalized in line with track if beatmap hitsounds option is enabled

[smallketchup82]

I believe you did the hard work of merging master into my branch given the 182 hidden items and 12 participants, so I probably wont bother with that.

I've already skimmed the code and it looks solid, so I'll probably merge after I do tests on it.

[smallketchup82]

Okay, after having looked through things.

1. I can't seem to edit your branch to get it in sync with my pulling from master (I keep getting an error 403 despite it saying that maintainers are allowed to edit this pull request? no idea)
2. Getting my environment prepared to test this is likely going to take a while (since I have my dependencies set up in a very particular way to allow for seamless switching between testing ManagedBass, osu, and osu-framework)
3. Expect a review sometime this week (though maybe next week since im pretty busy with exams this week). I've already noticed some stuff from looking at the diff. Specifically, some of the xmldoc needs to be updated in line with your changes (see https://github.com/hwsmm/osu/blob/49aa227c929e1dd1dbcd70cd514fd531584545e7/osu.Game/Audio/AudioNormalization.cs#L93)

[hwsmm]

I guess it's now okay? I just fixed the xmldoc you mentioned, too,

[smallketchup82]

I've tested and it works fine, apart from these I have no other gripes myself. I'll probably merge this once everything is addressed, then have smoogi re-review.

[smallketchup82]

I think it'd be better to lower the global volume reduction. In my testing, turning off normalization would blast your ears since 100% volume with 0.8 is much louder than 100% with -14 LUFS.

I'll probably do my own testing to find a proper value for it and consult with the core team & @nekodex about it. You can dismiss this message since its more of a note for myself. Though, nekodex, if you have any thoughts on this then do let me know

[smallketchup82]

Why do we only retain the relationship between tracks and hitsounds if beatmap hitsounds are turned on? Unless I'm mistaken and this does something entirely different, I think we'd want the volume offset to apply to skin hitsounds, beatmap hitsounds, and all other gameplay samples (combo sound, slider tick, etc).

Though I want to mention @nekodex and get his opinion on this.

[hwsmm]

I assumed that most players expect consistent hitsounds over maps if they don't have beatmap hitsounds enabled. I guess I should have discussed about this earlier..

[hwsmm]

Refactored the code a bit so that MusicController won't access the bindable directly, and applied your suggestions, too.

Edit: There are some things I probably have missed, but it's 3am here... so I'll fix them later.

[smallketchup82]

So I've written a git diff which removes the normalizeSampleIfTrue function, applying the volume offset regardless of whether that value is on or off.

In its place, I've written a barebones implementation of a configuration setting to toggle normalization on or off. Since it might be a little unclear, I created the storedVolume parameter to store the normalization values for the current beatmap, in case the user wants to turn on normalization. It allows for turning on and off the setting and being able to hear the normalization get enabled or disabled in realtime.

There's definitely ways to improve it (and probably shuffle things around to remove the multiple if statement checks for the config value in SetTrackNormalizationVolume). Since this is mostly a rough draft, I'm hoping you can take it and improve it to your liking. I haven't asked the team whether they want this or not, but it should be easy to revert later on if it's decided that it's unneeded.

I'll be honest and say that the main reason I wrote this was because the hitsounds can get really quiet depending on the skin, beatmap, etc. Some users might want to turn off normalization to make use of the higher volume ceiling. Although, maybe instead of allowing users to disable normalization, it could be wise to change this setting to be something like "Volume boost" which changes the target level from -14 to something louder, like -10, to give users a higher volume ceiling without needing to disable normalization.

This is probably something I'd want @nekodex to give his opinion on as well.

diff --git a/osu.Game/Audio/AudioNormalizationManager.cs b/osu.Game/Audio/AudioNormalizationManager.cs
index 5e6b7e5e4a..129e1d1cdb 100644
--- a/osu.Game/Audio/AudioNormalizationManager.cs
+++ b/osu.Game/Audio/AudioNormalizationManager.cs
@@ -21,7 +21,12 @@ public class AudioNormalizationManager
         /// </summary>
         public const double FALLBACK_VOLUME = 0.8;

-        private readonly Bindable<bool> beatmapHitsoundBind;
+        private readonly Bindable<bool> audioNormalizationSetting;
+
+        /// <summary>
+        /// Store the volume before normalization is disabled to restore it later if needed.
+        /// </summary>
+        private double storedVolume;

         /// <summary>
         /// Default <see cref="ITrackStore"/> will bind to this bindable.
@@ -44,23 +49,22 @@ public AudioNormalizationManager(AudioManager audioManager, OsuConfigManager con
         {
             audioManager.Tracks.AddAdjustment(AdjustableProperty.Volume, TrackNormalizeVolume);

-            beatmapHitsoundBind = config.GetBindable<bool>(OsuSetting.BeatmapHitsounds);
-
-            normalizeSampleIfTrue(beatmapHitsoundBind.Value);
-            beatmapHitsoundBind.BindValueChanged(change => normalizeSampleIfTrue(change.NewValue));
-        }
-
-        private void normalizeSampleIfTrue(bool value)
-        {
-            if (value)
+            audioNormalizationSetting = config.GetBindable<bool>(OsuSetting.AudioNormalization);
+            audioNormalizationSetting.BindValueChanged(change =>
             {
-                SampleNormalizeVolume.BindTo(TrackNormalizeVolume);
-            }
-            else
-            {
-                SampleNormalizeVolume.UnbindFrom(TrackNormalizeVolume);
-                SampleNormalizeVolume.Value = 1.0;
-            }
+                if (change.NewValue)
+                {
+                    TrackNormalizeVolume.Value = storedVolume;
+                    SampleNormalizeVolume.BindTo(TrackNormalizeVolume);
+                }
+                else
+                {
+                    storedVolume = TrackNormalizeVolume.Value;
+                    TrackNormalizeVolume.Value = FALLBACK_VOLUME;
+                    SampleNormalizeVolume.UnbindFrom(TrackNormalizeVolume);
+                    SampleNormalizeVolume.Value = 1.0;
+                }
+            }, true);
         }

         /// <summary>
@@ -75,17 +79,21 @@ public void SetTrackNormalizationVolume(AudioNormalization? audioNormalization,
             if (volume == null)
             {
                 volume = FALLBACK_VOLUME;
-                Logger.Log($"Normalization status: {Math.Round((double)volume * 100)}% (fallback)");
+                if (audioNormalizationSetting.Value) Logger.Log($"Normalization status: {Math.Round((double)volume * 100)}% (fallback)");
             }
-            else
-            {
+            else if (audioNormalizationSetting.Value)
                 Logger.Log($"Normalization status: {Math.Round((double)volume * 100)}%");
-            }

             if (volume == TrackNormalizeVolume.Value)
                 return;

-            action(TrackNormalizeVolume, (double)volume);
+            if (audioNormalizationSetting.Value)
+                action(TrackNormalizeVolume, (double)volume);
+            else
+            {
+                storedVolume = (double)volume;
+                action(TrackNormalizeVolume, FALLBACK_VOLUME);
+            }
         }
     }
 }
diff --git a/osu.Game/Configuration/OsuConfigManager.cs b/osu.Game/Configuration/OsuConfigManager.cs
index f4a4c553d8..30aac469ff 100644
--- a/osu.Game/Configuration/OsuConfigManager.cs
+++ b/osu.Game/Configuration/OsuConfigManager.cs
@@ -109,6 +109,8 @@ protected override void InitialiseDefaults()

             SetDefault(OsuSetting.AudioOffset, 0, -500.0, 500.0, 1);

+            SetDefault(OsuSetting.AudioNormalization, true);
+
             // Input
             SetDefault(OsuSetting.MenuCursorSize, 1.0f, 0.5f, 2f, 0.01f);
             SetDefault(OsuSetting.GameplayCursorSize, 1.0f, 0.1f, 2f, 0.01f);
@@ -360,6 +362,7 @@ public enum OsuSetting
         /// This is added to the audio track's current time. Higher values will cause gameplay to occur earlier, relative to the audio track.
         /// </summary>
         AudioOffset,
+        AudioNormalization,

         VolumeInactive,
         MenuMusic,
diff --git a/osu.Game/Localisation/AudioSettingsStrings.cs b/osu.Game/Localisation/AudioSettingsStrings.cs
index 89db60d8a6..138970e38e 100644
--- a/osu.Game/Localisation/AudioSettingsStrings.cs
+++ b/osu.Game/Localisation/AudioSettingsStrings.cs
@@ -64,6 +64,11 @@ public static class AudioSettingsStrings
         /// </summary>
         public static LocalisableString AudioOffset => new TranslatableString(getKey(@"audio_offset"), @"Audio offset");

+        /// <summary>
+        /// "Normalize Audio"
+        /// </summary>
+        public static LocalisableString AudioNormalization => new TranslatableString(getKey(@"audio_normalization"), @"Normalize Audio");
+
         /// <summary>
         /// "Play a few beatmaps to receive a suggested offset!"
         /// </summary>
diff --git a/osu.Game/Overlays/Settings/Sections/Audio/VolumeSettings.cs b/osu.Game/Overlays/Settings/Sections/Audio/VolumeSettings.cs
index 2bb5fa983f..7d73dbd199 100644
--- a/osu.Game/Overlays/Settings/Sections/Audio/VolumeSettings.cs
+++ b/osu.Game/Overlays/Settings/Sections/Audio/VolumeSettings.cs
@@ -49,6 +49,11 @@ private void load(AudioManager audio, OsuConfigManager config)
                     KeyboardStep = 0.01f,
                     DisplayAsPercentage = true
                 },
+                new SettingsCheckbox
+                {
+                    LabelText = AudioSettingsStrings.AudioNormalization,
+                    Current = config.GetBindable<bool>(OsuSetting.AudioNormalization)
+                }
             };
         }

[hwsmm]

Added 'Normalize Audio' setting with your diff, and I refactored it once again. I guess it's now in an acceptable state?

[smallketchup82]

The code itself looks fine to me now. I'll merge this since further review from me probably wouldn't be productive. Better to leave the rest to the core team.

In regards to #1 (comment), it seems like the current implementation applies the normalization regardless of whether beatmap hitsounds are turned on or off. If the core team wants to change that, that can be addressed then.

I'd like for you to be subscribed to the original PR, as having your input on things would be useful.