916270 - Fill ratio improvements

Status: NEW

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: patch

Here's some improvements. I'd like to report cases we don't calculate better then returning '999' but for now it's an improvement.

Comment 1

[Andreas Gal :gal]

Can you apply the scissor rect as well? And I think 999 is fine.

Comment 2

[Benoit Girard (:BenWa)]

Scissor react should be clip react unless I'm mistaken. Best get another set of eyes before we start using for perf decisions.

Comment 3

[Andreas Gal :gal]

Yeah sorry misread. Patch has not much context. Let me read the surrounding code and then I will pretend I am qualified to review this :)

Comment 4

[Benoit Girard (:BenWa)]

Attachment: patch

The counter will show 0 if there's something it can't compute. Right now we're interested in getting the fill ratio for simple content so I haven't ran into the need to support these cases.

Comment 5

[Benoit Girard (:BenWa)]

(In reply to Andreas Gal :gal from comment #3)
> Yeah sorry misread. Patch has not much context. Let me read the surrounding
> code and then I will pretend I am qualified to review this :)

I'd like your review as well if you have feedback.

Comment 6

[Andreas Gal :gal]

Comment on attachment 805380 [details] [diff] [review]
patch

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

r=me with the nits fixed, there is no risk that we break anything here so probably not worth occupying matt's time :)

::: gfx/layers/Compositor.h
@@ +427,4 @@
>     * especially in the presence of transforms.
>     */
>    size_t mPixelsPerFrame;
> +  int32_t mPixelsFilled;

these types should not differ, and a signed counter seems odd to me, but at least make them match

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +1024,5 @@
>      maskType = MaskNone;
>    }
>  
> +  if (mPixelsFilled >= 0 && aTransform.Is2D()) {
> +    Rect boundingRect = aTransform.As2D().TransformBounds(aRect);

It would be pretty easy to do a projection here for the 3D case if you decide thats worth it (probably not).

@@ +1030,5 @@
> +    boundingRect.MoveBy(aOffset);
> +    boundingRect = boundingRect.Intersect(Rect(0, 0, mWidgetSize.width, mWidgetSize.height));
> +    mPixelsFilled += boundingRect.width * boundingRect.height;
> +  } else {
> +    mPixelsFilled = -1;

please do 0 instead, a 0 will never naturally occur here, but 999 could

Comment 7

[Benoit Girard (:BenWa)]

(In reply to Andreas Gal :gal from comment #6)
> Comment on attachment 805380 [details] [diff] [review]
> patch
> 
> Review of attachment 805380 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the nits fixed, there is no risk that we break anything here so
> probably not worth occupying matt's time :)
> 
> ::: gfx/layers/Compositor.h
> @@ +427,4 @@
> >     * especially in the presence of transforms.
> >     */
> >    size_t mPixelsPerFrame;
> > +  int32_t mPixelsFilled;
> 
> these types should not differ, and a signed counter seems odd to me, but at
> least make them match
> 
> ::: gfx/layers/opengl/CompositorOGL.cpp
> @@ +1024,5 @@
> >      maskType = MaskNone;
> >    }
> >  
> > +  if (mPixelsFilled >= 0 && aTransform.Is2D()) {
> > +    Rect boundingRect = aTransform.As2D().TransformBounds(aRect);
> 
> It would be pretty easy to do a projection here for the 3D case if you
> decide thats worth it (probably not).

We don't have all the right primitive to get this working. The intersection can be a polygon so it's a bit tricky. We get most of the win supporting this simple case so for now I rather wait until we need to support it.

> 
> @@ +1030,5 @@
> > +    boundingRect.MoveBy(aOffset);
> > +    boundingRect = boundingRect.Intersect(Rect(0, 0, mWidgetSize.width, mWidgetSize.height));
> > +    mPixelsFilled += boundingRect.width * boundingRect.height;
> > +  } else {
> > +    mPixelsFilled = -1;
> 
> please do 0 instead, a 0 will never naturally occur here, but 999 could

mPixelsFilled -1 sets the error state. When rendering if mPixelsFilled is -1 then it will draw as 0.

If this is ok I'll fix the first comment and land.

Comment 8

[Andreas Gal :gal]

Feel free to land, these nits are not worth holding this up, but why do you complicate this here? Just leave it unsigned, set it to 0, and that renders as 0. There is no reason to round trip via -1. Its strictly confusing (but again, feel free to land like this).

Comment 9

[Benoit Girard (:BenWa)]

(In reply to Andreas Gal :gal from comment #8)
> Feel free to land, these nits are not worth holding this up, but why do you
> complicate this here? Just leave it unsigned, set it to 0, and that renders
> as 0. There is no reason to round trip via -1. Its strictly confusing (but
> again, feel free to land like this).

Well I use -1 to lock it into the 'error' state. '0' means we haven't filled anything so far in the frame. So I need to differentiate the two states. Otherwise I can just introduce a bool to indicate the error state.

Comment 10

[Andreas Gal :gal]

We will always at least clear, so it won't be 0. 100 should be the minimum. In practice we clear with a color layer for nsCanvasBackground instead of glClear, so that forces the 100.

Comment 11

[Benoit Girard (:BenWa)]

(In reply to Andreas Gal :gal from comment #10)
> We will always at least clear, so it won't be 0. 100 should be the minimum.
> In practice we clear with a color layer for nsCanvasBackground instead of
> glClear, so that forces the 100.

Maybe you have the hunk in DrawQuad confused with the hunk in EndFrame. If the pixel filled count is say 4k when we encounter something we don't support it and I make it as 0, then the pixel filled count will get incremented back to say w*h on the next DrawQuad call. I'm using -1 to have it remain in the error state and not increment further when we draw the next quad. i.e. once we hit something we don't support the counter locks for the frame.

|if (mPixelsFilled > 0 && mPixelsPerFrame > 0) {| will make sure |fillRatio = 0| and that if we hit something we don't support we will draw 0 even if mPixelsFilled is -1. So yes 0 will be shown in the counter for -1 pixels filled.

Comment 12

[Benoit Girard (:BenWa)]

Documentation for this feature:
https://developer.mozilla.org/en-US/docs/Performance/OverfillCounter

Comment 13

[Andreas Gal :gal]

Alright, I think you are right. Lets land it :)