Skip to content
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

Jump to bottom

Fix performance when viewing the tail end of a long / zoomed-in waveform #5843

Merged
merged 5 commits into from
Jun 17, 2023
Merged

Fix performance when viewing the tail end of a long / zoomed-in waveform #5843

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9bf1305
Add test coverage of slow performing scenario
peppy Jun 16, 2023
64c3ff1
Apply NRT to `WaveformGraph`
peppy Jun 16, 2023
ed1f41f
Avoid expensive loop in waveform draw node to improve performance
peppy Jun 16, 2023
d12f05e
Reword comment
bdach Jun 17, 2023
1fe574a
Merge branch 'master' into fix-long-waveform-performance
bdach Jun 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 24 additions & 2 deletions osu.Framework.Tests/Visual/Drawables/TestSceneWaveform.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ public partial class TestSceneWaveform : FrameworkTestScene
private Track track;
private Waveform waveform;
private Container<Drawable> waveformContainer;
private readonly Bindable<float> zoom = new BindableFloat(1) { MinValue = 0.1f, MaxValue = 20 };
private readonly BindableFloat zoom = new BindableFloat(1) { MinValue = 0.1f, MaxValue = 2000 };

private ScrollContainer<Drawable> scroll;

private ITrackStore store;

Expand Down Expand Up @@ -75,7 +77,7 @@ private void load(Game game, AudioManager audio)
},
},
},
new BasicScrollContainer(Direction.Horizontal)
scroll = new BasicScrollContainer(Direction.Horizontal)
{
RelativeSizeAxes = Axes.Both,
Child = waveformContainer = new FillFlowContainer
Expand Down Expand Up @@ -108,6 +110,26 @@ public void SetUpSteps()
AddStep("Load stereo track", () => loadTrack(true));
}

/// <summary>
/// When zooming in very close – or even zooming in a normal amount on a very long track – the number of points in the waveform
/// can become very high (in the millions).
///
/// In this case, we need to be careful no iteration is performed over the point data. This tests the case of being scrolled to the
/// far end of the waveform, which is the worse-case-scenario and requires special consideration.
/// </summary>
[Test]
public void TestHighZoomEndOfTrackPerformance()
{
TestWaveform graph = null;

AddStep("create waveform", () => waveformContainer.Child = graph = new TestWaveform(track, 1) { Waveform = waveform });
AddUntilStep("wait for load", () => graph.Regenerated);

AddStep("set zoom to highest", () => zoom.Value = zoom.MaxValue);

AddStep("seek to end", () => scroll.ScrollToEnd());
}

[Test]
public void TestMonoTrack()
{
Expand Down
37 changes: 17 additions & 20 deletions osu.Framework/Graphics/Audio/WaveformGraph.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// 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.

#nullable disable

using System;
using System.Collections.Generic;
using System.Linq;
Expand All @@ -29,8 +27,8 @@ namespace osu.Framework.Graphics.Audio
/// </summary>
public partial class WaveformGraph : Drawable
{
private IShader shader;
private Texture texture;
private IShader shader = null!;
private Texture texture = null!;

[BackgroundDependencyLoader]
private void load(ShaderManager shaders, IRenderer renderer)
Expand Down Expand Up @@ -61,12 +59,12 @@ public float Resolution
}
}

private Waveform waveform;
private Waveform? waveform;

/// <summary>
/// The <see cref="Framework.Audio.Track.Waveform"/> to display.
/// </summary>
public Waveform Waveform
public Waveform? Waveform
{
get => waveform;
set
Expand Down Expand Up @@ -174,10 +172,10 @@ protected override bool OnInvalidate(Invalidation invalidation, InvalidationSour
return result;
}

private CancellationTokenSource cancelSource = new CancellationTokenSource();
private CancellationTokenSource? cancelSource = new CancellationTokenSource();

private long resampledVersion;
private Waveform.Point[] resampledPoints;
private Waveform.Point[]? resampledPoints;
private int? resampledPointCount;
private double resampledMaxHighIntensity;
private double resampledMaxMidIntensity;
Expand All @@ -186,7 +184,7 @@ protected override bool OnInvalidate(Invalidation invalidation, InvalidationSour
private void queueRegeneration() => Scheduler.AddOnce(() =>
{
int requiredPointCount = (int)Math.Max(0, Math.Ceiling(DrawWidth * Scale.X) * Resolution);
if (requiredPointCount == resampledPointCount && !cancelSource.IsCancellationRequested)
if (requiredPointCount == resampledPointCount && cancelSource?.IsCancellationRequested != false)
return;

cancelGeneration();
Expand Down Expand Up @@ -259,10 +257,10 @@ protected override void Dispose(bool isDisposing)

private class WaveformDrawNode : DrawNode
{
private IShader shader;
private Texture texture;
private IShader shader = null!;
private Texture? texture;

private List<Waveform.Point> points;
private List<Waveform.Point>? points;

private Vector2 drawSize;

Expand Down Expand Up @@ -319,7 +317,7 @@ public override void ApplyState()
}
}

private IVertexBatch<TexturedVertex2D> vertexBatch;
private IVertexBatch<TexturedVertex2D>? vertexBatch;

public override void Draw(IRenderer renderer)
{
Expand All @@ -342,17 +340,16 @@ public override void Draw(IRenderer renderer)

float separation = drawSize.X / (points.Count - 1);

for (int i = 0; i < points.Count - 1; i++)
// Equates to making sure that rightX >= localMaskingRectangle.Left (see assertions in loop, which enforce the same thing).
bdach marked this conversation as resolved.
Show resolved Hide resolved
// Without this pre-check, very long waveform displays can get slow just from running the loop below (point counts in excess of 1mil).
int startIndex = (int)Math.Clamp(localMaskingRectangle.Left / separation, 0, points.Count - 1);
int endIndex = (int)Math.Clamp(localMaskingRectangle.Right / separation + 1, 0, points.Count - 1);
Comment on lines +345 to +346
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

The index choice here seems correct (visual inspection). Translating these from the original conditions gets a bit dicey with the < >, but it's also fine if we err on the side of caution and make them include one more on each side.

So if this comes up in review, it's fine to decrement / increment the start / end if required. But visually this does seem to be what we want. Maybe adjusting MaskingSmoothness in a masking scenario might require more?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at the maths this does appear to be correct to me. Doesn't seem like any adjustments are needed.


for (int i = startIndex; i < endIndex; i++)
{
float leftX = i * separation;
float rightX = (i + 1) * separation;

if (rightX < localMaskingRectangle.Left)
continue;

if (leftX > localMaskingRectangle.Right)
break; // X is always increasing

Color4 frequencyColour = baseColour;

// colouring is applied in the order of interest to a viewer.
Expand Down