Add frame statistics UI to with_winit example #288

armansito · 2023-03-04T09:53:38Z

Added a module for frame time statistics and a UI layer that displays the average, minimum, and maximum frame time alongside FPS. The UI can be toggled by pressing the S key.

DJMcNab

Nothing blocking, some thoughts on potential future work.

examples/with_winit/src/lib.rs

DJMcNab · 2023-03-06T11:46:15Z

examples/with_winit/src/stats.rs

+impl<'a> Drop for FrameScope<'a> {
+    fn drop(&mut self) {
+        self.stats
+            .add_sample(self.start.elapsed().as_micros() as u64);


To ensure that this is measuring time between the same point, rather than rendering time, I think we need to inline the call to elapsed, store the new Instant, and use that as the start time in frame_scope. At the moment, we ignore input processing time, for example, which I don't think is right.

Actually working out the best measure of FPS has always eluded me.

We also should measure GPU time (e.g. using https://github.com/Wumpf/wgpu-profiler), but wiring that up is non-trivial.

We also should measure GPU time (e.g. using https://github.com/Wumpf/wgpu-profiler), but wiring that up is non-trivial.

Oh, the GPU time measurement looks neat. I haven't looked into wgpu-profiler very closely but supporting GPU timers out of the box would be great (so far I've had to rely on external tools, such as the Metal GPU Performance monitor). I'll investigate this.

I made the elapsed time measurement tighter so it should closely match the presentation time. I haven't really had time to investigate wgpu-profiler yet but I plan to spend some time on wiring it up soon.

examples/with_winit/src/stats.rs

DJMcNab · 2023-03-07T17:25:13Z

A couple other thoughts; we should record startup time (ideally both time spent in wgpu create_shader_module and seperately, time spent in our pre-processing). And we should have a way to open this on Android. What that looks like, I'm not sure

DJMcNab

Looks good, just a few thoughts on cross-platform support.

I haven't properly reviewed the code for the frame time graph, but it seems to work OK.
We can get overflow in cases of C being pressed, but that's unavoidable.

I'd also like to get some way to enable this on Android in (without plugging in a USB keyboard, which might just work), but I'm not sure what that would be.

DJMcNab · 2023-03-16T10:48:14Z

examples/with_winit/src/lib.rs

+                                    if vsync_on {
+                                        wgpu::PresentMode::Fifo
+                                    } else {
+                                        wgpu::PresentMode::Immediate


Suggested change

wgpu::PresentMode::Immediate

wgpu::PresentMode::AutoNoVsync

Should be supported on all platforms. There's also a question of doing PresentMode::AutoVsync for vsync. I don't have a machine which it would make a difference on; apparently it's only Amd Vulkan.

The alternative might also resolve #266

Done, I changed the code to always use the Auto* modes. On web this likely means that "vsync off" will be incorrect so maybe the stats UI shouldn't just blindly display "VSync: off", but it's probably not a huge deal.

DJMcNab · 2023-03-16T10:52:49Z

examples/with_winit/src/lib.rs

+            stats.add_sample(stats::Sample {
+                frame_time_us: frame_start_time.elapsed().as_micros() as u64,
+            });
+            frame_start_time = Instant::now();


Suggested change

stats.add_sample(stats::Sample {

frame_time_us: frame_start_time.elapsed().as_micros() as u64,

});

frame_start_time = Instant::now();

let new_time = Instant::now();

stats.add_sample(stats::Sample {

frame_time_us: (new_time - frame_start_time).as_micros() as u64,

});

frame_start_time = new_time;

Should be slightly more correct. In practise, it probably doesn't matter

armansito · 2023-03-16T14:42:55Z

Looks good, just a few thoughts on cross-platform support.

I haven't properly reviewed the code for the frame time graph, but it seems to work OK.

We can get overflow in cases of C being pressed, but that's unavoidable.

I'd also like to get some way to enable this on Android in (without plugging in a USB keyboard, which might just work), but I'm not sure what that would be.

I made the stats display on by default and I can see it on my Android device. Does it show up on your device?

For controls to hide it I thought of adding a "double tap" gesture or recognizing taps over some visible rect which could serve as a button.

DJMcNab · 2023-03-16T15:23:53Z

Ah yes, I hadn't realised the consequences of that; I hadn't tried it on my device. I'm not certain it being displayed by default is what we want, but I don't have a strong objection to it either.

armansito · 2023-03-16T18:14:19Z

Ah yes, I hadn't realised the consequences of that; I hadn't tried it on my device. I'm not certain it being displayed by default is what we want, but I don't have a strong objection to it either.

Also there is currently no way to clear min/max on mobile, so we'll want some UI for it. I think I can make some simple "double-tap" or some other gesture could work, maybe in another PR.

DJMcNab

Code mostly looks good to me, although I'm not convinced by the non-linearity.

So to confirm, the current status is:

Add frame statistics UI to with_winit example #288 (comment) (mobile buttons) can be done in a different PR
Add frame statistics UI to with_winit example #288 (comment) (shader startup time) should be reasonably straightforward, but isn't critical
Add frame statistics UI to with_winit example #288 (comment) wgpu_profiler - doesn't need to be this PR
Add frame statistics UI to with_winit example #288 (comment) (Colour coding) - would be nice to get in this PR - we should also have dashed lines for (at least) 16.6ms and 33.3 ms. It would also be good to use accessible colours here, rather than red and green, although the colour coding is not functional.

DJMcNab · 2023-03-17T13:57:24Z

examples/with_winit/src/stats.rs

+            // shrink the draw size of the overall average sample, so scale the size non-linearly to
+            // emphasize smaller samples.
+            let h = (*sample as f64) * 0.001 / self.frame_time_max_ms;
+            let s = Affine::scale_non_uniform(1., -h.sqrt());


This logic is non-obvious to me; I suspect a maximum height based on the maximum seen last render might be better. I'd much rather keep things linear.

I switched this to linear now. I thought using sqrt here added value initially but now that we can reset the max/min frame times with a keypress (which resets the scale) this type of visualization is not necessary.

I'll follow up with the dashed lines for 16.6ms/33.3ms as you suggested, which should provide a good relative visual anchor.

Added a module for frame time statistics and UI layer that displays the average, minimum, and maximum frame time alongside FPS. The UI can be toggled by pressing the `S` key.

Also set the stats layer toggle to be on by default until we add some UI to toggle it on mobile.

Also abandoned the FrameScope idea and revised the `Stats::add_sample` to accept a struct to accept a variety of future measurements.

… comments

… stats UI

We now track the frame time from snapshot to snapshot corresponding to the exact presentation time.

…e start time This does not account for the time spent in processing `Stats::add_sample` but it should be very close.

…mode The Auto* modes should have wider compatibility as they implement fallback behavior based on what the platform supports. This also means that on web "vsync off" will likely be incorrect.

The sqrt scale doesn't add much value any more since the max frame time can be reset with a keypress.

Also added color coding for the bar graph based on these thresholds.

armansito · 2023-03-21T18:41:02Z

Code mostly looks good to me, although I'm not convinced by the non-linearity.

So to confirm, the current status is:

#288 (comment) (mobile buttons) can be done in a different PR

#288 (comment) (shader startup time) should be reasonably straightforward, but isn't critical

#288 (comment) wgpu_profiler - doesn't need to be this PR

Working on these in follow up PRs sounds good to me.

#288 (comment) (Colour coding) - would be nice to get in this PR - we should also have dashed lines for (at least) 16.6ms and 33.3 ms. It would also be good to use accessible colours here, rather than red and green, although the colour coding is not functional.

I added color coding and lines and picked accessible colors for the coding. I added markers for 8.33, 16.66, and 30.33. I used a solid stroke instead of a dashed stroke as the dashed stroke style didn't work.

DJMcNab

I'm happy for this to land as is. I've added some more thoughts on Zulip about the scale, so they might be worth considering. We might discuss tomorrow

DJMcNab · 2023-03-21T19:27:45Z

examples/with_winit/src/stats.rs

+                    left_margin_padding,
+                    (1 + labels.len()) as f64 * text_height,
+                )) * s,
+                if *sample < 16667 {


I think a match expression should also work, but this is fine.

match *sample { ..16_667 =>..., ..33_334 => ..., .. => ... }

I think that should probably be in its own variable declaration if we used it

DJMcNab · 2023-03-21T19:28:44Z

examples/with_winit/src/stats.rs

+                        left_margin,
+                        (2. - y) * graph_max_height + thres_text_height_2,
+                    )),
+                &format!("{}", t),


Can we right-justify this? I presume that would be a job for the SimpleText layer to handle?

I think that could be neat, though it's a minor visual improvement given how small the label is rendered. @dfrg Is this straightforward to do this today with the existing code?

We don’t currently do any sort of alignment but we could probably hack something in.

This allows the graph to display at a reasonable scale in the face of fluctuations and a max recorded sample that is much larger than the current average.

armansito force-pushed the frame-stats branch from 6c83659 to c5c034b Compare March 5, 2023 18:40

armansito requested review from DJMcNab and dfrg March 5, 2023 19:36

DJMcNab reviewed Mar 6, 2023

View reviewed changes

DJMcNab mentioned this pull request Mar 7, 2023

Winit example taking very long to startup on android #289

Closed

armansito force-pushed the frame-stats branch from 9607aa3 to b36e459 Compare March 16, 2023 07:21

armansito requested a review from DJMcNab March 16, 2023 08:15

DJMcNab reviewed Mar 16, 2023

View reviewed changes

DJMcNab reviewed Mar 17, 2023

View reviewed changes

armansito and others added 10 commits March 21, 2023 11:33

[frame_stats] Add frame statistics UI to with_winit example

89fb1b8

Added a module for frame time statistics and UI layer that displays the average, minimum, and maximum frame time alongside FPS. The UI can be toggled by pressing the `S` key.

[frame_stats] Build stats layer layout based on viewport dimensions

d7bacbc

[frame_stats] Show viewport resolution on the stats layer

bacaeeb

Also set the stats layer toggle to be on by default until we add some UI to toggle it on mobile.

[frame_stats] Draw a live plot of frame time samples

1ac4a4f

Also abandoned the FrameScope idea and revised the `Stats::add_sample` to accept a struct to accept a variety of future measurements.

[frame_stats] Key binding to clear min/max frame time; address review…

306aeab

… comments

[frame_stats] Add a VSync toggle key; show the current VSync state in…

1250cdc

… stats UI

[frame_stats] Track frame time more tightly

a1c0df1

We now track the frame time from snapshot to snapshot corresponding to the exact presentation time.

[frame_stats] Reduce stats time delta between elapsed() and next fram…

02a222f

…e start time This does not account for the time spent in processing `Stats::add_sample` but it should be very close.

[frame_stats] Use wgpu::PresentMode::AutoVsync/AutoNoVsync for VSync …

8bd1bdf

…mode The Auto* modes should have wider compatibility as they implement fallback behavior based on what the platform supports. This also means that on web "vsync off" will likely be incorrect.

[frame_stats] Scale the frame time graph linearly

5ba2826

The sqrt scale doesn't add much value any more since the max frame time can be reset with a keypress.

armansito force-pushed the frame-stats branch from 148c41b to 33e6287 Compare March 21, 2023 18:36

[frame_stats] Draw thresholds for 16.66ms, 33.33ms, and 8.33 timings

e6f4f6d

Also added color coding for the bar graph based on these thresholds.

armansito force-pushed the frame-stats branch from 33e6287 to e6f4f6d Compare March 21, 2023 18:38

DJMcNab approved these changes Mar 21, 2023

View reviewed changes

armansito added 2 commits March 21, 2023 15:16

[frame_stats] Adapt the graph scale based on the current mean frame time

6f30518

This allows the graph to display at a reasonable scale in the face of fluctuations and a max recorded sample that is much larger than the current average.

[frame_stats] Use match statement for color coding graph

703d22f

armansito merged commit a1f319d into linebender:main Mar 21, 2023

armansito deleted the frame-stats branch March 21, 2023 22:43

DJMcNab mentioned this pull request Apr 8, 2023

Add GPU profiling #304

Merged

DJMcNab mentioned this pull request Apr 22, 2023

[svg] Log parse and render times separately #310

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add frame statistics UI to with_winit example #288

Add frame statistics UI to with_winit example #288

armansito commented Mar 4, 2023

DJMcNab left a comment

DJMcNab Mar 6, 2023

armansito Mar 6, 2023

armansito Mar 16, 2023

DJMcNab commented Mar 7, 2023

DJMcNab left a comment

DJMcNab Mar 16, 2023

armansito Mar 16, 2023 •

edited

Loading

DJMcNab Mar 16, 2023

armansito Mar 16, 2023

armansito commented Mar 16, 2023

DJMcNab commented Mar 16, 2023 •

edited by armansito

Loading

armansito commented Mar 16, 2023 •

edited

Loading

DJMcNab left a comment

DJMcNab Mar 17, 2023

armansito Mar 21, 2023

armansito commented Mar 21, 2023

DJMcNab left a comment

DJMcNab Mar 21, 2023

armansito Mar 21, 2023

DJMcNab Mar 21, 2023

armansito Mar 21, 2023

dfrg Mar 21, 2023

Add frame statistics UI to with_winit example #288

Add frame statistics UI to with_winit example #288

Conversation

armansito commented Mar 4, 2023

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJMcNab commented Mar 7, 2023

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armansito Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armansito commented Mar 16, 2023

DJMcNab commented Mar 16, 2023 • edited by armansito Loading

armansito commented Mar 16, 2023 • edited Loading

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armansito commented Mar 21, 2023

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armansito Mar 16, 2023 •

edited

Loading

DJMcNab commented Mar 16, 2023 •

edited by armansito

Loading

armansito commented Mar 16, 2023 •

edited

Loading