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

[Nils Chr. Brause]

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>

:)

> -- 
> 2.0.4
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel