1124597 - Improve the FPS counter

Status: RESOLVED WONTFIX

(Core :: Graphics, defect, P3)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fps.patch

The current FPS counter seems to include idle time which is confusing. This patch fixes that by skipping idle time until it finds a good set of timestamps.

I'd like it if once we were idle, the FPS counter showed something other than "the last known framerate". But drawing either the histogram or a line of the FPS in addition to the raw numbers would make that a lot clearer.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8553008 [details] [diff] [review]
fps.patch

Review of attachment 8553008 [details] [diff] [review]:
-----------------------------------------------------------------

So if I understand correctly, we'll display the FPS of the last 1 second of data if we were continuously compositing or an estimated FPS if there was an idle time in between. 

e.g, the FPS data over the last second since TimeStamp::Now() is:

A) 200 ms composites
B) 400 ms idle
C) 400 ms composites

We'd extrapolate the FPS from point (C) only. Just want to make sure I understand the patch and intention correctly.

::: gfx/layers/composite/FPSCounter.cpp
@@ -152,5 @@
> - *
> - * GetFPS iterates from aTimestamp, which is the current frame.
> - * Then starting at frame 12, going back to frame 11, 10, etc, we calculate
> - * the duration of the recorded frame timestamp from aTimestamp.
> - * Once duration is greater than 1 second, we return how many frames

This comment still seems valid, at least in principle. The only difference is that we can ignore idle times, unless I'm missing something.

@@ +164,5 @@
> +IsIdleInterval(TimeDuration aDuration)
> +{
> +  return aDuration.ToSeconds() > 0.3;
> +}
> +

Any particular reason we use 300 ms? It's probably a large enough number, but why 300?

@@ +173,2 @@
>  {
> +  // Skip the most recent frame so we have at least one frame to read. 

nit: delete trailing whitespace.

@@ +201,5 @@
> +    frameCount++;
> +    totalTime += interval.ToSeconds();
> +  }
> +
> +  return double(frameCount) / totalTime;

The only thing I don't like about this is that we will extrapolate the FPS during the first second after an idle period. For example, if we've counted 30 frames over 0.5 seconds, then hit an idle period, we'd return 60 FPS here which isn't accurate as we could have a long composite after.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Mason Chang [:mchang] from comment #1)
> So if I understand correctly, we'll display the FPS of the last 1 second of
> data if we were continuously compositing or an estimated FPS if there was an
> idle time in between. 
> 
> e.g, the FPS data over the last second since TimeStamp::Now() is:
> 
> A) 200 ms composites
> B) 400 ms idle
> C) 400 ms composites
> 
> We'd extrapolate the FPS from point (C) only. Just want to make sure I
> understand the patch and intention correctly.

Yes. Though this patch doesn't really include the most recent frame which I don't like, I'll fix that.

> Any particular reason we use 300 ms? It's probably a large enough number,
> but why 300?

Nah, it could probably be any number.

> The only thing I don't like about this is that we will extrapolate the FPS
> during the first second after an idle period. For example, if we've counted
> 30 frames over 0.5 seconds, then hit an idle period, we'd return 60 FPS here
> which isn't accurate as we could have a long composite after.

Do you mean a single frame of a long composite? If so - yeah, that's true, but it doesn't strike me that that is bad unless we are rapidly going in between idle and not idle for single-frames. In that case either number will be pretty weird, I'd rather just have a graph to visualize it.

If there's more than one frame after the idle period it'll be more accurate (for some definition of accurate), since including the idle frames doesn't really mean anything.

Comment 3

[Mason Chang [Inactive] [:mchang]]

(In reply to David Anderson [:dvander] from comment #2)
> Do you mean a single frame of a long composite? If so - yeah, that's true,
> but it doesn't strike me that that is bad unless we are rapidly going in
> between idle and not idle for single-frames. In that case either number will
> be pretty weird, I'd rather just have a graph to visualize it.
> 
> If there's more than one frame after the idle period it'll be more accurate
> (for some definition of accurate), since including the idle frames doesn't
> really mean anything.

True, well the only case I'm worried about is someone starting a scroll/composite and taking that number right off the bat as accurate. The current implementation is accurate in that it is really the number of composites in the last second. But counting FPS has always been an inaccurate thing :/