If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Status

()

Core
Graphics: Layers
4 years ago
2 years ago

People

(Reporter: BenWa, Unassigned)

Tracking

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 804653 [details] [diff] [review]
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.
(Reporter)

Updated

4 years ago
Attachment #804653 - Attachment is patch: true

Comment 1

4 years ago
Can you apply the scissor rect as well? And I think 999 is fine.
(Reporter)

Comment 2

4 years ago
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

4 years ago
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 :)
(Reporter)

Comment 4

4 years ago
Created attachment 805380 [details] [diff] [review]
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.
Assignee: nobody → bgirard
Attachment #804653 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #805380 - Flags: review?(matt.woodrow)
(Reporter)

Comment 5

4 years ago
(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

4 years ago
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
Attachment #805380 - Flags: review?(matt.woodrow) → review+
(Reporter)

Comment 7

4 years ago
(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

4 years ago
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).
(Reporter)

Comment 9

4 years ago
(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

4 years ago
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.
(Reporter)

Comment 11

4 years ago
(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.
(Reporter)

Comment 12

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

Comment 13

4 years ago
Alright, I think you are right. Lets land it :)
(Reporter)

Updated

3 years ago
Blocks: 1034283
(Reporter)

Updated

2 years ago
Assignee: bgirard → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.