Fix frames potentially getting added to spectator replay in wrong format

The way spectator currently works, the `Spectator` screen is responsible
for adding new frames to the replay, even when it has a child
(`SpectatorPlayer`) present.

There was a possibility that a new play had already started, and on
returning to the Spectator screen (to initialise the new play) there
would be a brief period where the Player instance is still reading from
the replay, the `userBeganPlaying` call had not yet finished
initialising the new target replay, and `userSentFrames` is run
(asynchronously), writing frames to the previous replay using the
incorrect ruleset instance).

To make this work, it doesn't `Schedule` frame addition to the replay
(making things a bit unsafe). Changing this itself isn't such a simple
one to do, so I instead opted to fix this via locking.

Closes https://github.com/ppy/osu/issues/10777.
This commit is contained in:
Dean Herbert 2020-11-11 13:39:42 +09:00
parent 6593aac3f2
commit 11cf04eed1
1 changed files with 48 additions and 32 deletions

View File

@ -3,6 +3,7 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using JetBrains.Annotations;
using osu.Framework.Allocation;
@ -61,7 +62,9 @@ public class Spectator : OsuScreen
[Resolved]
private RulesetStore rulesets { get; set; }
private Replay replay;
private Score score;
private readonly object scoreLock = new object();
private Container beatmapPanelContainer;
@ -198,23 +201,32 @@ private void beatmapUpdated(ValueChangedEvent<WeakReference<BeatmapSetInfo>> bea
private void userSentFrames(int userId, FrameDataBundle data)
{
// this is not scheduled as it handles propagation of frames even when in a child screen (at which point we are not alive).
// probably not the safest way to handle this.
if (userId != targetUser.Id)
return;
// this should never happen as the server sends the user's state on watching,
// but is here as a safety measure.
if (replay == null)
return;
foreach (var frame in data.Frames)
lock (scoreLock)
{
IConvertibleReplayFrame convertibleFrame = rulesetInstance.CreateConvertibleReplayFrame();
convertibleFrame.FromLegacy(frame, beatmap.Value.Beatmap);
// this should never happen as the server sends the user's state on watching,
// but is here as a safety measure.
if (score == null)
return;
var convertedFrame = (ReplayFrame)convertibleFrame;
convertedFrame.Time = frame.Time;
// rulesetInstance should be guaranteed to be in sync with the score via scoreLock.
Debug.Assert(rulesetInstance != null && rulesetInstance.RulesetInfo.Equals(score.ScoreInfo.Ruleset));
replay.Frames.Add(convertedFrame);
foreach (var frame in data.Frames)
{
IConvertibleReplayFrame convertibleFrame = rulesetInstance.CreateConvertibleReplayFrame();
convertibleFrame.FromLegacy(frame, beatmap.Value.Beatmap);
var convertedFrame = (ReplayFrame)convertibleFrame;
convertedFrame.Time = frame.Time;
score.Replay.Frames.Add(convertedFrame);
}
}
}
@ -247,10 +259,13 @@ private void userFinishedPlaying(int userId, SpectatorState state)
if (userId != targetUser.Id)
return;
if (replay != null)
lock (scoreLock)
{
replay.HasReceivedAllFrames = true;
replay = null;
if (score != null)
{
score.Replay.HasReceivedAllFrames = true;
score = null;
}
}
Schedule(clearDisplay);
@ -283,27 +298,28 @@ private void attemptStart()
return;
}
replay ??= new Replay { HasReceivedAllFrames = false };
var scoreInfo = new ScoreInfo
lock (scoreLock)
{
Beatmap = resolvedBeatmap,
User = targetUser,
Mods = state.Mods.Select(m => m.ToMod(resolvedRuleset)).ToArray(),
Ruleset = resolvedRuleset.RulesetInfo,
};
score = new Score
{
ScoreInfo = new ScoreInfo
{
Beatmap = resolvedBeatmap,
User = targetUser,
Mods = state.Mods.Select(m => m.ToMod(resolvedRuleset)).ToArray(),
Ruleset = resolvedRuleset.RulesetInfo,
},
Replay = new Replay { HasReceivedAllFrames = false },
};
ruleset.Value = resolvedRuleset.RulesetInfo;
rulesetInstance = resolvedRuleset;
ruleset.Value = resolvedRuleset.RulesetInfo;
rulesetInstance = resolvedRuleset;
beatmap.Value = beatmaps.GetWorkingBeatmap(resolvedBeatmap);
watchButton.Enabled.Value = true;
beatmap.Value = beatmaps.GetWorkingBeatmap(resolvedBeatmap);
watchButton.Enabled.Value = true;
this.Push(new SpectatorPlayerLoader(new Score
{
ScoreInfo = scoreInfo,
Replay = replay,
}));
this.Push(new SpectatorPlayerLoader(score));
}
}
private void showBeatmapPanel(SpectatorState state)