-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Add flow for re-sending replay frames that spectator did not receive at end of play #38163
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| // 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; | ||
| using System.Collections.Generic; | ||
| using MessagePack; | ||
|
|
||
| namespace osu.Game.Online.Spectator | ||
| { | ||
| /// <summary> | ||
| /// Sent by the server to retrieve frame bundles that the client sent but never reached the server, if any. | ||
| /// </summary> | ||
| /// <param name="ScoreTokenId">The ID of the score token associated with the score with missing bundles.</param> | ||
| /// <param name="FrameBundleSequenceNumbers">The sequence numbers of frame bundles that the server never received.</param> | ||
| [Serializable] | ||
| [MessagePackObject] | ||
| public record CompleteReplayRequest( | ||
| [property: Key(0)] long ScoreTokenId, | ||
| [property: Key(1)] IEnumerable<long> FrameBundleSequenceNumbers | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| // 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; | ||
| using System.Collections.Generic; | ||
| using MessagePack; | ||
|
|
||
| namespace osu.Game.Online.Spectator | ||
| { | ||
| /// <summary> | ||
| /// Response to <see cref="CompleteReplayRequest"/>. | ||
| /// </summary> | ||
| /// <param name="FrameBundles"> | ||
| /// The frame bundles requested by sequence number in the corresponding <see cref="CompleteReplayRequest"/>. | ||
| /// Can be blank (contain no frames) if the client does not have the frames available any more. | ||
| /// </param> | ||
| [Serializable] | ||
| [MessagePackObject] | ||
| public record CompleteReplayResponse( | ||
| [property: Key(0)] IEnumerable<FrameDataBundle> FrameBundles | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,17 +21,25 @@ public class FrameDataBundle | |
| [Key(1)] | ||
| public IList<LegacyReplayFrame> Frames { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// The sequence number of this frame bundle. | ||
| /// Used to determine ordering of frame bundles, and for server-side checks that server received all frame bundles it was supposed to. | ||
| /// </summary> | ||
| [Key(2)] | ||
| public long? SequenceNumber { get; set; } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already have Anyway, any reason this is in the bundle and not the header? I'm not sure what the differentiation we have as to what goes in either, but curious if you have reasoning for putting it here specifically.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No idea what this is, as best I can tell it's completely dead.
There is no reason. Putting it on the bundle makes the server side a touch less painful because it removes one level of property scraping but that's not a real reason. |
||
|
|
||
| public FrameDataBundle(ScoreInfo score, ScoreProcessor scoreProcessor, IList<LegacyReplayFrame> frames) | ||
| { | ||
| Frames = frames; | ||
| Header = new FrameHeader(score, scoreProcessor.GetScoreProcessorStatistics()); | ||
| } | ||
|
|
||
| [JsonConstructor] | ||
| public FrameDataBundle(FrameHeader header, IList<LegacyReplayFrame> frames) | ||
| public FrameDataBundle(FrameHeader header, IList<LegacyReplayFrame> frames, long? sequenceNumber) | ||
| { | ||
| Header = header; | ||
| Frames = frames; | ||
| SequenceNumber = sequenceNumber; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| using System.Collections.Generic; | ||
| using System.Diagnostics; | ||
| using System.Linq; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using JetBrains.Annotations; | ||
| using osu.Framework.Allocation; | ||
|
|
@@ -89,8 +90,10 @@ public abstract partial class SpectatorClient : Component, ISpectatorClient | |
| private Score? currentScore; | ||
| private long? currentScoreToken; | ||
| private ScoreProcessor? currentScoreProcessor; | ||
| private long currentFrameBundleSequenceNumber; | ||
|
|
||
| private readonly Queue<FrameDataBundle> pendingFrameBundles = new Queue<FrameDataBundle>(); | ||
| private readonly Dictionary<long, List<FrameDataBundle>> allFrameBundles = new Dictionary<long, List<FrameDataBundle>>(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wonder how big this will grow during a multi-hour marathon beatmap 🤔. Might be worth calculating on paper.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Messages can be up to 32 KB in size by default. Not yet sure how this translates to actual usage, will investigate.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Back of napkin estimations are not great. Listening to suggestions what to do about it. 316 of those 459 bytes are the actual replay frames, so even dropping the frame headers does not help here much.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd chunk the responses client side to a sane number, for sure. But also I wasn't even thinking about max message size (so good you were I guess?). Was more about client side memory usage for very long maps. But 128 kb per minute sounds not too bad.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't understand what this means. Please elaborate. Half of the appeal of this solution was that this was a single-shot request that doesn't need retrying. If suddenly I have to split this single-shot request into however many, each of which can fail (what happens when any one of them does?), this is going to get very complex very quick. I'd sooner entertain solutions like having the client send the |
||
|
|
||
| private readonly List<LegacyReplayFrame> pendingFrames = new List<LegacyReplayFrame>(); | ||
|
|
||
|
|
@@ -284,6 +287,8 @@ public void EndPlaying(GameplayState state) | |
| if (pendingFrames.Count > 0) | ||
| purgePendingFrames(); | ||
|
|
||
| currentState.LastFrameBundleSequenceNumber = currentFrameBundleSequenceNumber; | ||
|
|
||
| clearScoreState(); | ||
|
|
||
| if (state.HasPassed) | ||
|
|
@@ -305,6 +310,9 @@ private void setStateForScore(long? scoreToken, GameplayState state, Score score | |
| currentScore = score; | ||
| currentScoreToken = scoreToken; | ||
| currentScoreProcessor = state.ScoreProcessor; | ||
| currentFrameBundleSequenceNumber = 0; | ||
| if (scoreToken != null) | ||
| allFrameBundles[scoreToken.Value] = new List<FrameDataBundle>(); | ||
| } | ||
|
|
||
| private void clearScoreState() | ||
|
|
@@ -315,6 +323,22 @@ private void clearScoreState() | |
| currentScore = null; | ||
| currentScoreProcessor = null; | ||
| currentScoreToken = null; | ||
| currentFrameBundleSequenceNumber = 0; | ||
| } | ||
|
|
||
| public Task<CompleteReplayResponse> CompleteReplay(CompleteReplayRequest completeReplayRequest) | ||
| { | ||
| if (!allFrameBundles.Remove(completeReplayRequest.ScoreTokenId, out var frameBundlesForScoreToken)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You touched on this in the OP, but I fear that this is not enough in terms of cleanup logic. If a user reconnects after a long time being disconnected and the server has forgotten about the user's pending replay data, it's going to remain in the client dictionary until restart, correct?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Correct. I have no opinion on how to handle that as choosing an expiry mechanism is highly subjective. |
||
| return Task.FromResult(new CompleteReplayResponse([])); | ||
|
|
||
| var sequenceNumberSet = completeReplayRequest.FrameBundleSequenceNumbers.ToHashSet(); | ||
| if (sequenceNumberSet.Count == 0) | ||
| return Task.FromResult(new CompleteReplayResponse([])); | ||
|
|
||
| var bundles = frameBundlesForScoreToken.Where(b => b.SequenceNumber != null && sequenceNumberSet.Contains(b.SequenceNumber.Value)) | ||
| .OrderBy(b => b.SequenceNumber) | ||
| .ToArray(); | ||
| return Task.FromResult(new CompleteReplayResponse(bundles)); | ||
| } | ||
|
|
||
| public virtual void WatchUser(int userId) | ||
|
|
@@ -389,7 +413,16 @@ private void purgePendingFrames() | |
| Debug.Assert(currentScoreProcessor != null); | ||
|
|
||
| var frames = pendingFrames.ToArray(); | ||
| var bundle = new FrameDataBundle(currentScore.ScoreInfo, currentScoreProcessor, frames); | ||
| var bundle = new FrameDataBundle(currentScore.ScoreInfo, currentScoreProcessor, frames) | ||
| { | ||
| SequenceNumber = Interlocked.Increment(ref currentFrameBundleSequenceNumber) | ||
| }; | ||
|
|
||
| if (currentScoreToken != null) | ||
| { | ||
| Debug.Assert(allFrameBundles.ContainsKey(currentScoreToken.Value)); | ||
| allFrameBundles[currentScoreToken.Value].Add(bundle); | ||
| } | ||
|
|
||
| pendingFrames.Clear(); | ||
| lastPurgeTime = Time.Current; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I got in the habit of doing with bancho/stable is prefixing these kinds of calls with
ClientorServer, because I guarantee we are going to end up withRequestobjects being fired in both directions sooner or later.Thoughts?
ServerCompleteReplayRequestClientCompleteReplayResponseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the fact that no other operation between client and spectator server uses this convention, I do not have any particular opinions on this proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until now we haven't had any classes with
Request/Responsepairing, so I'm not sure it would suit there. Specifically important for whenRequest/Responseis involved IMHO, because until now the client is always theRequester when using these terms (aka web requests).But we could potentially prefix other classes where relevant.