858544 - Implement a better FPS counter

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

The current implementation of the FPS counter can significantly hur performance, which is very bad for a performance tools. Here are some observations from bjacob:

::: gfx/layers/opengl/CompositorOGL.cpp

@@ +89,5 @@
> +  struct Vertex2D {
> +    float x,y;
> +  };
> +  const Vertex2D vertices[] = {
> +    { -1.0f, 1.0f - 42.f / viewport[3] },

There's no reason for these viewpoint transformations here. That's a job for a
vertex shader. That's a lot of divisions, could have a significant performance
impact.

@@ +132,5 @@
> +  int txn100 = (txnFps % 1000) / 100;
> +
> +  // Feel free to comment these texture coordinates out and use one
> +  // of the ones below instead, or play around with your own values.
> +  const GLfloat texCoords[] = {

Same. All these should go into the vertex shader.

@@ +176,5 @@
> +
> +  copyprog->Activate();
> +  copyprog->SetTextureUnit(0);
> +
> +  // we're going to use client-side vertex arrays for this.

That's bad... performance implications are unclear and driver-dependant.

@@ +181,5 @@
> +  context->fBindBuffer(LOCAL_GL_ARRAY_BUFFER, 0);
> +
> +  // "COPY"
> +  context->fBlendFuncSeparate(LOCAL_GL_ONE, LOCAL_GL_ZERO,
> +                              LOCAL_GL_ONE, LOCAL_GL_ZERO);

This is bad on multiple levels.

This code is turning blend on, and not turning it back off. That's bad as it
means that it can severely degrade performance of subsequent draw calls... bad
for a perf tool.

This code is also fiddling with the blendfunc without restoring it, which could
cause hard to diagnose issues.

This is also using glBlendFuncSeparate where glBlendFunc would be enough, as
the separate alpha factors passed here are equal to the RGB ones.

Finally, this "COPY" blendfunc, with sfactor=ONE and dfactor=ZERO, amounts to
saying "no blending!" so why bother at all with blending? You might as well
glDisable(GL_BLEND) and get the exact same result.

Comment 1

[Nick Cameron [:nrc]]

And one more:

@@ +730,5 @@
+  for (size_t i = 0; i < 16; i++) {
+    context->fScissor(3*i, 0, 3, 3);
+
+    // We should do this using a single draw call
+    // instead of 16 glClear()
Rather, since there is already number-drawing code in DrawFPS, should use that.

Comment 2

[tingnan.zhang]

What is the current status of the bug? Need a hand on it?

Comment 3

[Andreas Gal :gal]

I have a very hard time believing that this matters in the real world. I would like to see some profiling data to support the problem statement above before we work on this. That having said, the bug is unowned, so if Tingnan wants to work on this more as a way to get to know the code, I don't think this work would hurt (I just don't think it would help measurably). Tingnan, happy to assign the bug to you if you want to work on this even though its unclear whether its high priority. Let me know.

Comment 4

[Nicolas Silva [:nical]]

Alternatively, the FPS counter could be made cross-platform (by using DataTextureSource and the Compositor API instead of issuing gl calls manually). That would be nice for the D3D9/D3D11 and software backends.
As Andreas said, not high priority but a good bug to play with for anyone who would like to get started hacking on Gecko.

Comment 5

[tingnan.zhang]

Thanks for the advice, Nicolas and Andreas. I would really love to play bit with this bug and see what I can do!

Comment 6

[Nicolas Silva [:nical]]

(In reply to tingnan.zhang from comment #5)
> Thanks for the advice, Nicolas and Andreas. I would really love to play bit
> with this bug and see what I can do!

Sweet! So my advice is that you do the following:

* Look at how the Compositor works (mainly it exposes a DrawQuad method that lets you compsite something on the screen and that's what we should be using here. You can find examples of how that works in ImageHost.cpp for instance.
* Look at the DataTextureSource abstraction that lets you upload a texture and use it with the compositor (DataTextureSource is defined in TextureHost.h, and has implementations for each backend. Compositor::CreateDataTextureSource lets creates one for the appropriate backend.
* and render the FPS counter with these abstractions, instead of doing it directly in GL as we do know. The resulting code will most likely be simpler and backend-independent since it is use high level abstractions, and that will be nice because it will make it possible for us to use the FPS counter on windows with the D3D backends or even the basic backend which we can't do now.

Don't hesitate to ask questions on the bug or email me. The quick pointers I just gave you rely on that you understand the context of compositing, etc. Which is not necessarily the case, so it's better to ask whenever you are not sure, rather than getting stuck.

Comment 7

[Andreas Gal :gal]

Attachment: font.txt

Comment 8

[Andreas Gal :gal]

Attachment: mkfont.py

Comment 9

[Andreas Gal :gal]

Attachment: builtin-font.h

Comment 10

[Andreas Gal :gal]

I added a couple pieces I used for an earlier attempt to make our FPS rendering code saner. Its a full small pixel size font. If you want to make the code more generic with this we could use it to render more layer diagnostics than just a number.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Take a look at bug 874781 -- I refactored the layer diagnostics there, including moving the FPS counter into generic code, as well as adding some additional features.  I was going to revive that patch at some point now that compositor/layers is a little more stable, but it might provide a good starting point.

Comment 12

[tingnan.zhang]

cool. I will start working on this according to the suggestions. Will post any questions if I don't understand. 

Thanks!

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Please also post a design & what you intend to do before you write code, for feedback!  Could save you a bunch of time :)

Comment 14

[tingnan.zhang]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #13)
> Please also post a design & what you intend to do before you write code, for
> feedback!  Could save you a bunch of time :)

Thanks! I have begun looking at the files you mentioned and the things you uploaded (It is near final of the semester so got a bit distracted from classes).

Comment 15

[James King]

tingnan, nicolas, vladv;

I'd be willing to take this one on. I know it won't be a huge improvement but I like to start small. I did the initial VAO implementation on WebGL; would like to expand my experience with the codebase. If tingnan is no longer available to finish the patch I'd be happy to pick it up.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Is anybody actively working on this?

Comment 17

[Nicolas Silva [:nical]]

The FPS counter has changed a lot since I filed this bug so I am wondering if we do need to change it further. At the moment it's not clear what this bug is supposed to fix since most of the items the bug description aren't in the code anymore.
Perhaps we should close the bug and open a new one if there are things we are still not happy about with the current FPS counter.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Ok. I just ran across while looking for open mentored bugs and noticed that this probably isn't appropriate as a mentored bug any more. Removing mentoring tags; feel free to close it you think that's appropriate.