From b7a85ee58ea185d876170af0bfc6708d00fa59c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 14 Aug 2023 11:00:51 +0200 Subject: [PATCH 1/3] Add failing test for `DrawableAudioWrapper` dropping adjustments --- .../Audio/DrawableAudioWrapperTest.cs | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 osu.Framework.Tests/Audio/DrawableAudioWrapperTest.cs diff --git a/osu.Framework.Tests/Audio/DrawableAudioWrapperTest.cs b/osu.Framework.Tests/Audio/DrawableAudioWrapperTest.cs new file mode 100644 index 0000000000..034249b1f8 --- /dev/null +++ b/osu.Framework.Tests/Audio/DrawableAudioWrapperTest.cs @@ -0,0 +1,48 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System.Threading; +using NUnit.Framework; +using osu.Framework.Audio; +using osu.Framework.Audio.Track; +using osu.Framework.Bindables; +using osu.Framework.Graphics.Audio; +using osu.Framework.Testing; +using osu.Framework.Tests.Visual; + +namespace osu.Framework.Tests.Audio +{ + [HeadlessTest] + public partial class DrawableAudioWrapperTest : FrameworkTestScene + { + [Test] + public void TestAdjustmentsPreservedOnOwnedComponentAfterRemoval() + { + TrackVirtual track = null!; + SlowDisposingDrawableAudioWrapper wrapper = null!; + + AddStep("add slow disposing component", () => Child = wrapper = new SlowDisposingDrawableAudioWrapper(track = new TrackVirtual(1000))); + AddStep("mute wrapper", () => wrapper.AddAdjustment(AdjustableProperty.Volume, new BindableDouble())); + AddStep("expire wrapper", () => wrapper.Expire()); + AddAssert("component still muted", () => track.AggregateVolume.Value, () => Is.EqualTo(0)); + AddStep("allow disposal to complete", () => wrapper.AllowDisposal.Set()); + } + + private partial class SlowDisposingDrawableAudioWrapper : DrawableAudioWrapper + { + public ManualResetEvent AllowDisposal { get; private set; } = new ManualResetEvent(false); + + public SlowDisposingDrawableAudioWrapper(IAdjustableAudioComponent component) + : base(component) + { + } + + protected override void Dispose(bool isDisposing) + { + AllowDisposal.WaitOne(10_000); + + base.Dispose(isDisposing); + } + } + } +} From 3feec7466f8370f40bf8897d10906f602a140985 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 14 Aug 2023 11:03:03 +0200 Subject: [PATCH 2/3] Remove unnecessary unbind of adjustments in `DrawableAudioWrapper` An innocent change to `DrawableAudioWrapper` (#5956) caused a rather serious game-side regression (ppy/osu#24519), wherein tracks would start playing on top of each other in song select. The immediate cause of the failure was that tracks were faded out to zero volume via audio adjustments. When audio adjustments are unbound, `AggregateBindable` logic causes the aggregate volume to be recomputed, and if there are no sources in the aggregate left, it will revert to the default initial value of 1. In normal usages this is perfectly fine and how the aggregate bindable system should work, but in rare cases where the async disposal thread was under pressure, such as with a weak online connection and many requests queued, it appears that there was a significant delay between the bindable unbind and the disposal of the drawable track. This in turn led to the track-to-be-disposed playing with full volume after being removed from the hierarchy, for an indeterminate amount of time, until disposal completion. At this point, several approaches were being considered. - The first approach would entail jury-rigging the `UnbindAllBindables()` path to somehow bypass the "recalculate aggregate on source removed" logic. This would generally be quite ugly, but work. - The second would consist of stopping all playback of drawable audio components once they're removed from the hierarchy. Applying this to `DrawableTrack` itself to fix this particular case is easy, but gets less easy with other various considerations (what about `DrawableSample`? what about if the drawable's _ancestor_ gets removed from the hierarchy rather than it getting removed from its immediate parent? etc.) - Another attempt had `UnbindAllBindables()` create a local `AudioAdjustments`, using the aggregate values of `adjustments` at the point of unbind, and binding the `component` to that, therefore "freezing" the audio adjustments at that point. As it turns out, that _also_ has issues, because `UnbindAllBindables()` can be called on a drawable multiple times (once synchronously via an expiry flow, and the second time from the async disposal queue and `Dispose()` itself) - and at the point of the second call, the aggregate values may be garbage again. Looking in blame, the directly offending `UnbindAdjustments()` call was added in 3a21eb96934d05048b6e9a57502574f03b312f08, to support `DrawableAudioWrapper` in scenarios where it doesn't own the `IAdjustableAudioComponent` it is operating on, and that is what the `UnbindAdjustments()` call was presumably for - namely, to unapply adjustments applied to the component by the wrapper, once the wrapper stops operating on the component. However, if the underlying component is to be disposed along with the drawable audio wrapper, therefore making the wrapper the de-facto owner of the component, there's no need to do any adjustment unbinding. It so turns out that the game-side failure is a case where this adjustment unbind can be skipped, and so it is done to fix the failure. --- osu.Framework/Graphics/Audio/DrawableAudioWrapper.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/osu.Framework/Graphics/Audio/DrawableAudioWrapper.cs b/osu.Framework/Graphics/Audio/DrawableAudioWrapper.cs index 68affe8d6a..74612941fd 100644 --- a/osu.Framework/Graphics/Audio/DrawableAudioWrapper.cs +++ b/osu.Framework/Graphics/Audio/DrawableAudioWrapper.cs @@ -142,7 +142,8 @@ internal override void UnbindAllBindables() { base.UnbindAllBindables(); - component?.UnbindAdjustments(adjustments); + if (!disposeUnderlyingComponentOnDispose) + component?.UnbindAdjustments(adjustments); } protected override void Dispose(bool isDisposing) From a3f76c93893ac2271881af7cb46de848f7974c46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 14 Aug 2023 11:18:19 +0200 Subject: [PATCH 3/3] Add test covering `DrawableAudioWrapper` behaviour for non-owned audio components --- .../Audio/DrawableAudioWrapperTest.cs | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/osu.Framework.Tests/Audio/DrawableAudioWrapperTest.cs b/osu.Framework.Tests/Audio/DrawableAudioWrapperTest.cs index 034249b1f8..ae87f5ef46 100644 --- a/osu.Framework.Tests/Audio/DrawableAudioWrapperTest.cs +++ b/osu.Framework.Tests/Audio/DrawableAudioWrapperTest.cs @@ -21,19 +21,34 @@ public void TestAdjustmentsPreservedOnOwnedComponentAfterRemoval() TrackVirtual track = null!; SlowDisposingDrawableAudioWrapper wrapper = null!; - AddStep("add slow disposing component", () => Child = wrapper = new SlowDisposingDrawableAudioWrapper(track = new TrackVirtual(1000))); + AddStep("add slow disposing wrapper for owned track", + () => Child = wrapper = new SlowDisposingDrawableAudioWrapper(track = new TrackVirtual(1000))); AddStep("mute wrapper", () => wrapper.AddAdjustment(AdjustableProperty.Volume, new BindableDouble())); AddStep("expire wrapper", () => wrapper.Expire()); AddAssert("component still muted", () => track.AggregateVolume.Value, () => Is.EqualTo(0)); AddStep("allow disposal to complete", () => wrapper.AllowDisposal.Set()); } + [Test] + public void TestAdjustmentsRevertedOnOwnedComponentAfterRemoval() + { + TrackVirtual track = null!; + SlowDisposingDrawableAudioWrapper wrapper = null!; + + AddStep("add slow disposing wrapper for non-owned track", + () => Child = wrapper = new SlowDisposingDrawableAudioWrapper(track = new TrackVirtual(1000), false)); + AddStep("mute wrapper", () => wrapper.AddAdjustment(AdjustableProperty.Volume, new BindableDouble())); + AddStep("expire wrapper", () => wrapper.Expire()); + AddAssert("component unmuted", () => track.AggregateVolume.Value, () => Is.EqualTo(1)); + AddStep("allow disposal to complete", () => wrapper.AllowDisposal.Set()); + } + private partial class SlowDisposingDrawableAudioWrapper : DrawableAudioWrapper { public ManualResetEvent AllowDisposal { get; private set; } = new ManualResetEvent(false); - public SlowDisposingDrawableAudioWrapper(IAdjustableAudioComponent component) - : base(component) + public SlowDisposingDrawableAudioWrapper(IAdjustableAudioComponent component, bool disposeUnderlyingComponentOnDispose = true) + : base(component, disposeUnderlyingComponentOnDispose) { }