Last Comment Bug 612407 - Add FPS counter like fraps
: Add FPS counter like fraps
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Oleg Romashin (:romaxa)
:
Mentors:
Depends on:
Blocks: 683709 683745 780467
  Show dependency treegraph
 
Reported: 2010-11-15 14:37 PST by Jeff Muizelaar [:jrmuizel]
Modified: 2012-08-05 05:53 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
An OpenGL version of this (6.42 KB, patch)
2010-11-23 12:55 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Review
v2 (7.98 KB, patch)
2010-12-03 13:17 PST, Jeff Muizelaar [:jrmuizel]
joe: review-
Details | Diff | Review
v3. restyle, + added preference (9.81 KB, patch)
2011-05-05 07:05 PDT, Oleg Romashin (:romaxa)
joe: review+
Details | Diff | Review
Fixed nits, version to push (9.81 KB, patch)
2011-05-05 15:15 PDT, Oleg Romashin (:romaxa)
joe: review+
Details | Diff | Review
Fixed compilation on linux, typing problem (9.82 KB, patch)
2011-05-07 02:30 PDT, Oleg Romashin (:romaxa)
jmuizelaar: review-
Details | Diff | Review
interdiff from previous patch (1.51 KB, patch)
2011-05-07 02:31 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Review
Fixed compilation on linux, typing problem (11.77 KB, patch)
2011-05-11 06:00 PDT, Oleg Romashin (:romaxa)
jmuizelaar: review+
Details | Diff | Review
interdiff from last r+ patch (3.27 KB, patch)
2011-05-11 06:05 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Review

Description Jeff Muizelaar [:jrmuizel] 2010-11-15 14:37:15 PST
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] 2010-11-23 12:55:41 PST
Created attachment 492758 [details] [diff] [review]
An OpenGL version of this
Comment 2 Jeff Muizelaar [:jrmuizel] 2010-12-03 13:17:59 PST
Created attachment 495092 [details] [diff] [review]
v2
Comment 3 Jeff Muizelaar [:jrmuizel] 2010-12-07 09:13:39 PST
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) 2011-03-17 10:09:20 PDT
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) 2011-05-05 07:05:46 PDT
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) 2011-05-05 13:14:39 PDT
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) 2011-05-05 15:15:54 PDT
Created attachment 530444 [details] [diff] [review]
Fixed nits, version to push
Comment 8 Mounir Lamouri (:mounir) 2011-05-06 03:16:48 PDT
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) 2011-05-07 02:30:22 PDT
Created attachment 530819 [details] [diff] [review]
Fixed compilation on linux, typing problem
Comment 10 Oleg Romashin (:romaxa) 2011-05-07 02:31:48 PDT
Created attachment 530820 [details] [diff] [review]
interdiff from previous patch
Comment 11 Oleg Romashin (:romaxa) 2011-05-07 02:32:30 PDT
I was testing this patch originally only on scratchbox, now it compiles on linux too,
Comment 12 Jeff Muizelaar [:jrmuizel] 2011-05-07 09:15:30 PDT
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) 2011-05-07 23:51:53 PDT
> 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) 2011-05-07 23:52:41 PDT
I don't see this error on scratchbox mobile builds, but see it on Desktop Linux
Comment 15 Oleg Romashin (:romaxa) 2011-05-07 23:57:01 PDT
Found similar issue discussed here:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44500#c18
Comment 16 Jeff Muizelaar [:jrmuizel] 2011-05-09 06:39:55 PDT
(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) 2011-05-11 06:00:09 PDT
Created attachment 531600 [details] [diff] [review]
Fixed compilation on linux, typing problem
Comment 18 Oleg Romashin (:romaxa) 2011-05-11 06:00:37 PDT
checked on try and now it compiles fine.
Comment 19 Oleg Romashin (:romaxa) 2011-05-11 06:05:20 PDT
Created attachment 531602 [details] [diff] [review]
interdiff from last r+ patch
Comment 20 Oleg Romashin (:romaxa) 2011-05-11 07:50:37 PDT
http://hg.mozilla.org/mozilla-central/rev/7febcf91de21
Comment 21 Girish Sharma [:Optimizer] 2011-05-11 20:09:01 PDT
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) 2011-05-11 20:38:36 PDT
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 2012-08-05 05:53:51 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.