From: Ryo Munakata <ryomnk...@gmail.com> To: "Nils Chr. Brause" <nilschrbra...@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" <nilschrbra...@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 <nilschrbra...@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 <ryomnk...@gmail.com> _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel