923409 - draw FPS counters with a single GL call

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Andreas Gal :gal]

This refactors drawing of the FPS counters to use a single GL draw call. Its the first in a series of patches to do that for layer composition as well.

Comment 1

[Andreas Gal :gal]

Attachment: draw-rects

Comment 2

[Andreas Gal :gal]

Comment on attachment 813473 [details] [diff] [review]
draw-rects

As a bonus, this patch removes the last use of the Copy2D shader which is an odd 1-off shader that doesn't fit neatly into our shader pipeline. We used to use Copy2D/Copy2DRect to copy intermediate surfaces but we now use the RGB layer shader(s) for that (the old layers code still uses Copy2D). With this patch and once we delete the old layers code we can drop Copy2D/Copy2DRect/

Comment 3

[Benoit Girard (:BenWa)]

This patch is going a bit into the opposite direction we were planning. I think we should move the FPS counter to LayerManagerComposite and have it use the compositor API. This of course will still draw the FPS using multiple draw call. But we need to extend the compositor API to support drawing a layer in a single call which we don't when the layer's region has one then one rect.

IMO this patch looks fine but it takes us to an intermediate we don't really want (more efficient OGL specific fps counter) vs. where we want to be (efficient cross platform fps counter). If we do the harder thing here well be further along to drawing complex layers for all platform more efficiently.

Comment 4

[Andreas Gal :gal]

I have a number of additional patches that do actually what you are asking for and also move the FPS counter as suggested (for that we need to kill LayerManagerOGL first). If you can stamp this, I will continue breaking out my patches to get to the 1-GL-call-per-layer world, which is where I am headed to as well :)

Comment 5

[Benoit Girard (:BenWa)]

Alright, I was objecting because it felt like a side-step to me but if you have a patch queue to do this I don't object with the patch itself. Let me finish the review.

Comment 6

[Andreas Gal :gal]

Ok, here is roughly what I have lined up:
- move RectTriangles into its own file, make it use vector, and make it the central and single way to specify geometries
- introduce DrawQuads as the one and central way to draw a geometry, taking RectTriangles
- introduce DrawQuad as a wrapper around DrawQuad
- teach DrawQuads about the mQuadVBO trick, and move mQuadVBO handling into VBOArena as the one and only place we implement that
- partially done: converge the 7 (including cocoa widget) different GL draw paths onto this one
- not done yet: invert the loop in layer drawing to use DrawQuads instead of several DrawQuad calls (close)

Comment 7

[Benoit Girard (:BenWa)]

(In reply to Andreas Gal :gal from comment #2)
> With
> this patch and once we delete the old layers code we can drop
> Copy2D/Copy2DRect/

Filed bug 924403 to track this work and also remember to remove Copy2D.

Comment 8

[Nicolas Silva [:nical]]

About the FPS counter, vlad wrote a patch (back in the Taipei workweek) in bug 874781 that makes the FPS counter use the compositor API with DataTextureSource. The path never landed because it needed a few modifications and rebasing. I don't know if it is relevant to the discussion here, just making sure you guys know about this.

Comment 9

[Andreas Gal :gal]

Yup, I talked to vlad about that patch. I have an unrelated and similar patch that does that and adds a full font for better diagnostics :) We will fold those two things together somehow. That having said, that approach is not ideal for HWC which is limited in the number of layers/composite operations. It might be better to write this data into the pixels of existing layers, but that might have unhappy performance side effects anywhere but on mobile.

Comment 10

[Benoit Girard (:BenWa)]

Comment on attachment 813473 [details] [diff] [review]
draw-rects

Review of attachment 813473 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +127,5 @@
> +static const float FontStride = 4.f;
> +
> +// Scale the font when drawing it to the viewport for better readability.
> +static const float FontScaleX = 2.f;
> +static const float FontScaleY = 3.f;

I think on lower DPI like some desktop screens this might be too big and start convering important screen contents but I agree it's also too small on high DPI. Maybe we should scale it based on the viewport size instead?

@@ +136,5 @@
> +
> +static void
> +AddDigits(GLContext::RectTriangles &aRects,
> +          const gfx::IntSize aViewportSize,
> +          unsigned aOffset,

We typically prefer 'unsigned int' to 'unsigned'. I'd prefer if we used the former everywhere in the patch.

Comment 11

[Andreas Gal :gal]

Will do a follow-up to scale this better when I land the full font patch.

Comment 12

[Andreas Gal :gal]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33563ee5a859

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/33563ee5a859