Skip to content

Bail from score submission if audio playback rate is too far from reality #26138

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 26, 2023
Merged

Bail from score submission if audio playback rate is too far from reality #26138

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
225528d
Bail from score submission if audio playback rate is too far from rea…
peppy Dec 26, 2023
d70fddb
Fix elapsed time being counted twice on first frame
peppy Dec 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions osu.Game/Screens/Play/MasterGameplayClockContainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using osu.Framework.Audio.Track;
using osu.Framework.Bindables;
using osu.Framework.Graphics;
using osu.Framework.Logging;
using osu.Framework.Timing;
using osu.Game.Beatmaps;
using osu.Game.Beatmaps.ControlPoints;
Expand Down Expand Up @@ -39,6 +40,14 @@ public partial class MasterGameplayClockContainer : GameplayClockContainer, IBea
Precision = 0.1,
};

/// <summary>
/// Whether the audio playback is within acceptable ranges.
/// Will become false if audio playback is not going as expected.
/// </summary>
public IBindable<bool> PlaybackRateValid => playbackRateValid;

private readonly Bindable<bool> playbackRateValid = new Bindable<bool>(true);

private readonly WorkingBeatmap beatmap;

private Track track;
Expand Down Expand Up @@ -128,6 +137,7 @@ public override void Seek(double time)
{
// Safety in case the clock is seeked while stopped.
LastStopTime = null;
elapsedValidationTime = null;

base.Seek(time);
}
Expand Down Expand Up @@ -197,6 +207,51 @@ public void StopUsingBeatmapClock()
addAdjustmentsToTrack();
}

protected override void Update()
{
base.Update();
checkPlaybackValidity();
}

#region Clock validation (ensure things are running correctly for local gameplay)

private double elapsedGameplayClockTime;
private double? elapsedValidationTime;
private int playbackDiscrepancyCount;

private const int allowed_playback_discrepancies = 5;

private void checkPlaybackValidity()
{
if (GameplayClock.IsRunning)
{
elapsedGameplayClockTime += GameplayClock.ElapsedFrameTime;

elapsedValidationTime ??= elapsedGameplayClockTime;
elapsedValidationTime += GameplayClock.Rate * Time.Elapsed;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work? If elapsedValidationTime is null in a frame, doesn't this cause said frame to be double-counted?

I'd expect something closer to

                if (elapsedValidationTime == null)
                    elapsedValidationTime = elapsedGameplayClockTime;
                else
                    elapsedValidationTime += GameplayClock.Rate * Time.Elapsed;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that looks better. Good catch.


if (Math.Abs(elapsedGameplayClockTime - elapsedValidationTime!.Value) > 300)
{
if (playbackDiscrepancyCount++ > allowed_playback_discrepancies)
{
if (playbackRateValid.Value)
{
playbackRateValid.Value = false;
Logger.Log("System audio playback is not working as expected. Some online functionality will not work.\n\nPlease check your audio drivers.", level: LogLevel.Important);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be a localizable notification, as this is a user-facing string.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i argue this is not important for now

https://github.com/search?q=repo%3Appy%2Fosu%20LogLevel.Important&type=code

Copy link
Member Author

@peppy peppy Dec 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's an edge case that only 1 in 10000000 will see, but sure, at some opint. the main goal is just to ensure some validity before score submit to stop bugged drivers from submitting.

}
}
else
{
Logger.Log($"Playback discrepancy detected ({playbackDiscrepancyCount} of allowed {allowed_playback_discrepancies}): {elapsedGameplayClockTime:N1} vs {elapsedValidationTime:N1}");
}

elapsedValidationTime = null;
}
}
}

#endregion

private bool speedAdjustmentsApplied;

private void addAdjustmentsToTrack()
Expand Down
8 changes: 8 additions & 0 deletions osu.Game/Screens/Play/SubmittingPlayer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,14 @@ private void submitFromFailOrQuit()

private Task submitScore(Score score)
{
var masterClock = GameplayClockContainer as MasterGameplayClockContainer;

if (masterClock?.PlaybackRateValid.Value != true)
{
Logger.Log("Score submission cancelled due to audio playback rate discrepancy.");
return Task.CompletedTask;
}

// token may be null if the request failed but gameplay was still allowed (see HandleTokenRetrievalFailure).
if (token == null)
{
Expand Down