Osu: Glitching audio when restarting beatmap through pause overlay

[marcuscastelo]

Describe the bug: When playing a beatmap, if you pause the game or lose, after clicking on retry or clicking on quit and then playing the same beatmap again, the game starts to glitch a lot.
PS: The game works as expected with continue button at pause and quick reload (unless on the pause overlay).

I've tried to solve this issue, but no luck in past hours. I believe the problem is within the classes FailOverlay, PauseOverlay, or it's base classes (such as GameplayMenuOverlay). When there is no (FailOverlay/PauseOverlay).Show(), the bug isn't present, which is weird, since it would indicate that the problem is with Hide() messing up something Show() has done, but the continue button works just fine with Show and Hide of overlays, so I really don't know where to look.
Actually, I've started looking at osu!lazer code for the first time today, so maybe I'm totally wrong.

Screenshots or videos showing encountered issue:
Demonstration Video: https://youtu.be/otgRI_P8bb4

osu!lazer version: 2019.1029.0

Logs: https://pastebin.com/c7BnafTB (Equivalent to demonstration video)

I'm using majaro linux, with dotnet 3.0.

[marcuscastelo]

I've done some more tests, the problem resides in that line:

at osu.Game.Screens.Play.GameplayClockContainer class.

If I comment this line, the game works perfectly, without any visual glitches anymore. Unfortunately, though, no sound is reproduced.

OBS: adjustableClock is of type DecoupleableInterpolatingFramedClock, which is from framework, so it seems to be a Framework bug (maybe compatibility issues with manjaro?)

OBS2: I've noticed that when you click on retry button from ScoreResultsPage, the bug never happens (no clue so far why)

[bdach]

Bisection turned up garbage, but I'm reasonably confident this regressed in #6688 and I'm noticing it on Windows too, so most likely not framework-related, not platform-related and not an issue with the line @marcuscastelo has pointed out itself, although it is clock-related, so it was definitely a good trail.

I'm way out of my depth here audio-wise, but I think this only happens when restarting through the pause/fail overlay because of the frequency adjustments applied on pause, which seem to carry through pauseFreqAdjust in the container. When I subscribed and logged value changed events, I saw that bindable transform from 1 to 0 on pause:
https://github.com/ppy/osu/blob/fd13f0bc550036ba921d1e839412406470be98eb/osu.Game/Screens/Play/GameplayClockContainer.cs#L152
then stay at 0 in StopUsingBeatmapClock and then never change again, even though Start is supposed to reset it:
https://github.com/ppy/osu/blob/fd13f0bc550036ba921d1e839412406470be98eb/osu.Game/Screens/Play/GameplayClockContainer.cs#L130
Setting it again to 1 in StopUsingBeatmapClock seems to fix it, however hacky that might be. Not entirely sure why that's necessary, though - possibly since Restart is ran as a task and then scheduled, this becomes a race? Not entirely sure why this was working before either, needs more investigation (probably from someone more competent in these matters).

[SpiritsUnite]

I ran into this as well, and did some investigation,

It looks like a new GamplayClockContainer is created for each playthrough of a beatmap:

https://github.com/ppy/osu/blob/cacb036c79048b79af32a4f355a9295caa07b867/osu.Game/Screens/Play/Player.cs#L132

So, when replaying the map, a new instance of GamplayClockContainer is created, and, I think the reason that pauseFreqAdjust stays at 0 even though Start is called is that that pauseFreqAdjust is from the first GamplayClockContainer instance, whereas Start is being called on a different instance. I'm thinking that this is the reason the audio is glitching out - pauseFreqAdjusts from previous playthroughs are still in effect.

As for why it didn't work before #6688, what I'm seeing is that when the user restarts or quits from a map, the previous GamplayClockContainer is disposed of. Looking at its Dispose method,

https://github.com/ppy/osu/blob/fd13f0bc550036ba921d1e839412406470be98eb/osu.Game/Screens/Play/GameplayClockContainer.cs#L198-L202

we can see that the frequency adjustments are removed from sourceClock - this is why it worked before #6688. After #6688, StopUsingBeatmapClock changes sourceClock to be something else beforehand, so the pause frequency adjustments are removed from the wrong clock. I'm not really familiar with the code yet so I may have gotten some details wrong, but I think that's the gist of the problem. I think we could fix this by removing the pause frequency adjustment in StopUsingBeatmapClock as well.

[bdach]

Good catch! I agree that moving out the frequency adjustment removal to StopUsingBeatmapClock seems to be a correct fix here (probably as correct as can be, really).

Would be also nice to have a test to check if all adjustments are removed upon restart so that this doesn't regress again, but I don't know how difficult writing something like that would be right now.

[peppy]

I'll grab this one, already made some failing tests for it. @SpiritsUnite great job discovering the underlying issue.