[PATCH v2 weston] gears: fix invalid calculation of the first FPS

[Ryo Munakata]

From: Ryo Munakata <ryomnktml at gmail.com>
To: "Nils Chr. Brause" <nilschrbrause at gmail.com>
Subject: Re: [PATCH v2 weston] gears: fix invalid calculation of the first FPS
Date: Sun, 10 Aug 2014 23:51:40 +0900
X-Mailer: Sylpheed 3.4.2 (GTK+ 2.24.24; x86_64-unknown-linux-gnu)

On Sun, 10 Aug 2014 16:02:35 +0200
"Nils Chr. Brause" <nilschrbrause at gmail.com> wrote:

> On Sun, Aug 10, 2014 at 06:26:36PM +0900, Ryo Munakata wrote:
> > At the calculation of the first FPS, gears has initialized last
> > FPS time with gettimeofday().
> > But the callback_data passed in the callback of wl_surface_frame()
> > is the current time, in milliseconds, with an undefined base.
> > Because of this subtracting last FPS time from callback_data makes no sense.
> >  For example, below is the result of running weston-gears on weston with
> > drm backend:
> > 
> > $ weston-gears
> > Warning: FPS count is limited by the wayland compositor or monitor refresh rate
> > 1 frames in 1094460.125 seconds =  0.000 FPS
> > 301 frames in  5.016 seconds = 60.008 FPS
> > 301 frames in  5.016 seconds = 60.008 FPS
> > 301 frames in  5.016 seconds = 60.008 FPS
> > 
> > As you can see, the the first FPS value is something odd.
> 
> I'm not seeing this on my system, because weston wants to use a
> CLOCK_REALTIME clock instead of a CLOCK_MONOTONIC clock here. But I
> agree this is a small cosmetic flaw on system, that use a
> CLOCK_MONOTONIC clock instead.
> 
> > 
> > This patch fixes it by initializing last FPS time with the callback_data passed in
> > the first callback.
> > ---
> >  clients/gears.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/clients/gears.c b/clients/gears.c
> > index 95f0bb2..2413b31 100644
> > --- a/clients/gears.c
> > +++ b/clients/gears.c
> > @@ -23,6 +23,7 @@
> >  #include "config.h"
> >  
> >  #include <stdint.h>
> > +#include <stdbool.h>
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> > @@ -208,8 +209,13 @@ static void
> >  update_fps(struct gears *gears, uint32_t time)
> >  {
> >  	long diff_ms;
> > +	static bool first_call = true;
> >  
> > -	gears->frames++;
> > +	if (first_call) {
> > +		gears->last_fps = time;
> > +		first_call = false;
> > +	} else
> > +		gears->frames++;
> >  
> 
> I certainly like the usage of stdbool for boolean values here.
> 
> >  	diff_ms = time - gears->last_fps;
> >  
> > @@ -437,8 +443,6 @@ gears_create(struct display *display)
> >  	gears->view.rotx = 20.0;
> >  	gears->view.roty = 30.0;
> >  
> > -	gettimeofday(&tv, NULL);
> > -	gears->last_fps = tv.tv_sec * 1000 + tv.tv_usec / 1000;
> >  	printf("Warning: FPS count is limited by the wayland compositor or monitor refresh rate\n");
> >  
> >  	glEnable(GL_NORMALIZE);
> 
> I like this patch. It appears to be correct, it compiles, it certainly
> fixes the problem, its code style matches the rest of weston, it is
> well readable and I don't see any other way to properly fix
> this. Therefore:
> 
> Reviewed-by: Nils Chr. Brause <nilschrbrause at gmail.com>
> 
> :)

Hi Nils.

Thank you for reviewing my patch.

But I forgot to remove an unused variable, struct timeval tv.
So now I sent out the third version of my patch.

Thanks, anyway.

P.S.
Nils, sorry for sending this twice.
-- 
Ryo Munakata <ryomnktml at gmail.com>