612407 - Add FPS counter like fraps

Status: RESOLVED FIXED

Description

[Jeff Muizelaar [:jrmuizel]]

If you run firefox with fraps running it adds a fps counter to all of the windows it would be handy if we had this functionality built in.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Created attachment 492758 [details] [diff] [review]
An OpenGL version of this

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Created attachment 495092 [details] [diff] [review]
v2

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 495092 [details] [diff] [review]
v2

I'll put:

+
+  mFPS.DrawFPS(mGLContext, GetCopy2DProgram());
+

under an #if 0

Comment 4

[Joe Drew (not getting mail)]

Comment on attachment 495092 [details] [diff] [review]
v2

>diff --git a/gfx/layers/opengl/LayerManagerOGL.cpp b/gfx/layers/opengl/LayerManagerOGL.cpp

>+void LayerManagerOGL::FPSState::DrawFPS(
>+    GLContext* context,
>+    CopyProgram *copyprog
>+    ){

Can you put the arguments list on a single line, and the { on a newline, please?

>+  static TimeStamp last;
>+  static int fcount = 0;
>+  static int fps;
>+  static bool initialized;

Shouldn't you be using the FPSState member variables?

>diff --git a/gfx/layers/opengl/LayerManagerOGL.h b/gfx/layers/opengl/LayerManagerOGL.h

>+  struct FPSState
>+  {
>+      static GLuint texture;
>+      int fps;
>+      bool initialized;

Either both initialized and texture should be static, or neither should be.

Comment 5

[Oleg Romashin (:romaxa)]

Created attachment 530303 [details] [diff] [review]
v3. restyle, + added preference

Fixed last comments, common restyle, also added preference, so we can test/check any time by just setting pref and restarting fennec

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 530303 [details] [diff] [review]
v3. restyle, + added preference

Review of attachment 530303 [details] [diff] [review]:

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +80,5 @@
   , mBackBufferFBO(0)
   , mBackBufferTexture(0)
   , mBackBufferSize(-1, -1)
   , mHasBGRA(0)
+  , mFPS()

You don't need this, since the default constructor's called anyways - right?

@@ +593,5 @@
+  const Vertex2D vertices[] = {
+    { -1.0, 1.0 - 42. / viewport[3]},
+    { -1.0, 1.0},
+    { -1.0 + 22. / viewport[2], 1.0 - 42. / viewport[3]},
+    { -1.0 + 22. / viewport[2], 1.0},

Can you add spaces before the ending "}," on all these vertices?

@@ +653,5 @@
+
+  context->fEnableVertexAttribArray(vcattr);
+  context->fEnableVertexAttribArray(tcattr);
+
+

extra blank line here.

@@ +667,5 @@
+
+  context->fDrawArrays(LOCAL_GL_TRIANGLE_STRIP, 0, 12);
+}
+
+

extra blank line here too.

Comment 7

[Oleg Romashin (:romaxa)]

Created attachment 530444 [details] [diff] [review]
Fixed nits, version to push

Comment 8

[Mounir Lamouri (:mounir)]

Backed out from cedar because of build bustage.
See:
http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1304673177.1304674061.19195.gz

Seems to fail only on Linux.

Comment 9

[Oleg Romashin (:romaxa)]

Created attachment 530819 [details] [diff] [review]
Fixed compilation on linux, typing problem

Comment 10

[Oleg Romashin (:romaxa)]

Created attachment 530820 [details] [diff] [review]
interdiff from previous patch

Comment 11

[Oleg Romashin (:romaxa)]

I was testing this patch originally only on scratchbox, now it compiles on linux too,

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 530820 [details] [diff] [review]
interdiff from previous patch

Can you explain the reason for increasing the size of all of theses types?

Comment 13

[Oleg Romashin (:romaxa)]

> Can you explain the reason for increasing the size of all of theses types?
Because it fails to compile, see:
http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1304673177.1304674061.19195.gz

576:5: error: narrowing conversion of '255' from 'int' to 'char' inside { }
606:3: error: narrowing conversion of '(1.0e+0 - (4.2e+1 / (double)viewport[3]))' from 'double' to 'float' inside { }
629:3: error: narrowing conversion of '(((double)v100 * 4.0e+0) / 6.4e+1)' from 'double' to 'const GLfloat' inside { }

Comment 14

[Oleg Romashin (:romaxa)]

I don't see this error on scratchbox mobile builds, but see it on Desktop Linux

Comment 15

[Oleg Romashin (:romaxa)]

Found similar issue discussed here:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44500#c18

Comment 16

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #13)
> > Can you explain the reason for increasing the size of all of theses types?
> Because it fails to compile, see:
> http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1304673177.1304674061.
> 19195.gz
> 
> 576:5: error: narrowing conversion of '255' from 'int' to 'char' inside { }
> 606:3: error: narrowing conversion of '(1.0e+0 - (4.2e+1 /
> (double)viewport[3]))' from 'double' to 'float' inside { }
> 629:3: error: narrowing conversion of '(((double)v100 * 4.0e+0) / 6.4e+1)'
> from 'double' to 'const GLfloat' inside { }

You should be able to fix this by using 'unsigned char' for text and switching from double literals to float literals.

Comment 17

[Oleg Romashin (:romaxa)]

Created attachment 531600 [details] [diff] [review]
Fixed compilation on linux, typing problem

Comment 18

[Oleg Romashin (:romaxa)]

checked on try and now it compiles fine.

Comment 19

[Oleg Romashin (:romaxa)]

Created attachment 531602 [details] [diff] [review]
interdiff from last r+ patch

Comment 20

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/7febcf91de21

Comment 21

[Girish Sharma [:Optimizer]]

How to see this FPS counter on firefox windows ?
I am usinf the latest hourly , which has this bug landed.

Comment 22

[Oleg Romashin (:romaxa)]

you should set layers.acceleration.draw-fps=true pref, but implementation is done only for OGL backend, so if you need D3D or something like that,  we should add related implementation for D3D backend

Comment 23

[Helder "Lthere" Magalhães]

(In reply to Oleg Romashin (:romaxa) from comment #22)
> you should set layers.acceleration.draw-fps=true pref, but implementation is
> done only for OGL backend, so if you need D3D or something like that,  we
> should add related implementation for D3D backend

I'm also on Windows. I've tried flipping the preference but nothing happened, then I stumbled on this bug/comment. I've opened bug 780467 for tracking progress on DirectX/Windows.