780074 - Use a sliding time window for layers fps calculations

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The current counter uses an algorithm that gives pretty odd results in some cases.  Here's an alternate proposal designed to give a more realistic instantaneous framerate.  Deride away!

 const TimeDuration kWindow = 250ms;
 queue<TimeStamp> frames;
 float getFpsAtCurrentFrame(TimeStamp frameTime):
   // XXX this could be done more efficiently
   while ((frameTime - frames.front()) > kWindow)
     frames.pop()
   frames.push(frameTime)
   return frames.length() / kWindow

We should enqueue no more than 15 (well, 25 with current bugs) frame stamps, so the memory use should stabilize and the queue manipulations become "free" eventually.

We can increase/decrease the window however we need to get better results.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Change the fps counter to provide better estimates of instantaneous fps

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Change the fps counter to provide better estimates of instantaneous fps, rebased

Numbers look believable.  Found bug 780816.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 649538 [details] [diff] [review]
Change the fps counter to provide better estimates of instantaneous fps, rebased

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

I think the following might be a better strategy.

Instead of keeping a queue of the timestamps of the frames in the last 250ms, keep a circular buffer of previous TimeStamps.

Something like:
// average the time of the most recent frames
GetFPS() {
   int firstIndex = (index+1)%FRAME_COUNT;
   // we don't want to include frames that happened more than 250ms ago in our average
   do {
       TimeDuration total = current - frameTimes[firstIndex];
       firstIndex = (firstIndex+1)%FRAME_COUNT;
   } while (total < 250ms && firstIndex != index%FRAME_COUNT)
   return FRAME_COUNT/total.ToSeconds();
}

This avoids having to use an unbounded queue and doesn't have the 4fps resolution problem. i.e. it doesn't trade resolution for smoothness.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I can't say that I understand how this is different or better.  Your pseudocode is not entirely clear.  Maybe an example would help.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> I can't say that I understand how this is different or better.  Your
> pseudocode is not entirely clear.  Maybe an example would help.

The main difference is that your code counts the number of frames that happened in the last 250ms, my code averages the time between n frames.

e.g.

If we have the following frames times:
1,  2,  3,  4,  5,  6,  7,   8,   9,  10,  11,  12,  13,  14,  15,  16,  17
0, 15, 30, 45, 60, 75, 90, 105, 120, 135, 150, 165, 180, 198, 216, 234, 252

your code will get 16/250ms = 64fps
and will bounce between 56,60,64,68fps

My code could keep track of only 4 frame times and will correctly give
4/(252-180)*1000 = 55.5 fps (which is the actual rate in the example above) or if we want a smoother average
16/(252-15)*1000 = 67.5 fps.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)

Further if we change the first frame time from 0 to 2ms

1,  2,  3,  4,  5,  6,  7,   8,   9,  10,  11,  12,  13,  14,  15,  16,  17
2, 15, 30, 45, 60, 75, 90, 105, 120, 135, 150, 165, 180, 198, 216, 234, 252

your code would jump to 17/250ms = 68fps
My code with 4 and 16 frame windows would be unaffected. With a 17 frame window it
would change from 67.4fps to 68fps.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

OK, I understand now.  I like your proposal.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Change the fps counter to provide better estimates of instantaneous fps, v2

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Andreas, if this is r+'d while I'm in the air, it would be also be great to land in preparation for the work week.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 655393 [details] [diff] [review]
Change the fps counter to provide better estimates of instantaneous fps, v2

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

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +48,5 @@
>  int ShaderProgramOGL::sCurrentProgramKey = 0;
>  #endif
>  
> +static const double kFpsWindowMs = 250.0;
> +static const size_t kNumFrameTimeStamps = 10;

Might as well use 16 to avoid the division.

@@ +54,5 @@
> +  FPSCounter() : mCurrentFrameIndex(0) {}
> +
> +  // We keep a circular buffer of the time points at which the last K
> +  // frames were drawn.  To estimate FPS, we count the number of
> +  // frames we've drawn within the last W milliseconds.

s/W/kFPSWindowMs/

add "and divide by the amount of time since the first of those frames"

@@ +80,5 @@
> +    TimeStamp earliestFrameInWindow = aNow;
> +    size_t numFramesDrawnInWindow = 0;
> +    for (size_t i = 0; i < kNumFrameTimeStamps; ++i) {
> +      const TimeStamp& frame = mFrames[i];
> +      if (!frame.IsNull() && beginningOfWindow < frame) {

I find that (frame > beginningOfWindow) reads better than the above. You don't need to mentally switch the object you're operating on from frame.IsNull()

@@ +942,5 @@
> +  if (sDrawFPS && !mFPS) {
> +    mFPS = new FPSState();
> +  } else if (!sDrawFPS && mFPS) {
> +    mFPS = nullptr;
> +  }

Why make mFPS a pointer? To avoid the circular buffer space wastage? I'm not sure it matter too much.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #10)
> Comment on attachment 655393 [details] [diff] [review]
> Change the fps counter to provide better estimates of instantaneous fps, v2
> 
> Review of attachment 655393 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/opengl/LayerManagerOGL.cpp
> @@ +48,5 @@
> >  int ShaderProgramOGL::sCurrentProgramKey = 0;
> >  #endif
> >  
> > +static const double kFpsWindowMs = 250.0;
> > +static const size_t kNumFrameTimeStamps = 10;
> 
> Might as well use 16 to avoid the division.
> 

Changed.

> @@ +54,5 @@
> > +  FPSCounter() : mCurrentFrameIndex(0) {}
> > +
> > +  // We keep a circular buffer of the time points at which the last K
> > +  // frames were drawn.  To estimate FPS, we count the number of
> > +  // frames we've drawn within the last W milliseconds.
> 
> s/W/kFPSWindowMs/
> 
> add "and divide by the amount of time since the first of those frames"
> 

Done.

> @@ +80,5 @@
> > +    TimeStamp earliestFrameInWindow = aNow;
> > +    size_t numFramesDrawnInWindow = 0;
> > +    for (size_t i = 0; i < kNumFrameTimeStamps; ++i) {
> > +      const TimeStamp& frame = mFrames[i];
> > +      if (!frame.IsNull() && beginningOfWindow < frame) {
> 
> I find that (frame > beginningOfWindow) reads better than the above. You
> don't need to mentally switch the object you're operating on from
> frame.IsNull()
> 

Suits me.

> @@ +942,5 @@
> > +  if (sDrawFPS && !mFPS) {
> > +    mFPS = new FPSState();
> > +  } else if (!sDrawFPS && mFPS) {
> > +    mFPS = nullptr;
> > +  }
> 
> Why make mFPS a pointer? To avoid the circular buffer space wastage? I'm not
> sure it matter too much.

To avoid having to declare FPSCounter/FPSState in LayerManagerOGL.h.
That has the incidental nice property of making the
fps-counter-enabled check be |if (mFPS)|.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4915f7de0b18

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4915f7de0b18

Comment 14

[Rob Hawkes [:rob]]

I've been using the FPS counter for benchmarking some games and the latest update has introduced a couple of issues for me. I'm curious to know if a) it's caused by this bug, and b) how I can work around it.

Basically, the older FPS counter was nice and slow and allowed me to visually see the lowest and highest frame-rate that was measured. The new counter is changing value insanely fast and means that I don't have enough time to catch the values nor work out what was the lowest and highest frame-rate.

Can we slow this down at all, or at least have the option to?

Comment 15

[Rob Hawkes [:rob]]

Another issue that I'm seeing, that may or may not be caused by this bug, is that the counter shows weirdly-high values when the frame-rate starts to stutter – values all the way up to 999, it seems.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Seeing instantaneous results was the point of this bug --- the previous algorithm was only suited for uses like stable tests running for seconds.  If you have such a stable test, the old and new counters will show the same results.  If on the other hand you have a short-duration or low frame-rate test, the old counter would often not show results at all, whereas the new one will.

The weird numbers were bug 789368.