805028 - Progressive tile painting causes talos to return inaccurate results for checkerboarding

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Chris Lord [:cwiiis]]

LayerRender.java assumes that the display-port is never partially filled, which is incorrect when progressive tile painting is enabled. We ought to rectify this before enabling progressive tiles so that we don't get too many false-positive performance improvements.

Currently discussing solutions.

Comment 1

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

I was talking with BenWa about this over lunch, the best we came up with was to measure unpainted areas in the paint code and then pass it along via the compositor to Java. We would effectively be moving that "painted area intersect viewport" code from LayerRenderer into the bowels of gecko and passing the percentage of area painted up into Java.

Comment 2

[Benoit Girard (:BenWa)]

It didn't occur to me that this solution wouldn't work as Cwiiis pointed out on IRC. What Gecko is painting may not be what we're compositing and we're likely to produce several frames and pan significantly between any individual progressive paint so we wouldn't be able to update this score on time.

Comment 3

[Chris Lord [:cwiiis]]

Attachment: Add LayerManagerOGL::ComputeRenderIntegrity

Add a function to calculate the ratio of 'complete' rendering intersecting with the screen. Tested, seems to work, not confident with the IntermediateSurface code, but not sure it really matters.

Comment 4

[Chris Lord [:cwiiis]]

Attachment: Use ComputeRenderIntegrity when measuring checkerboarding

Comment 5

[Chris Lord [:cwiiis]]

Attachment: Add LayerManagerOGL::ComputeRenderIntegrity

Remove the pointless use of pointers instead of pass-by-reference (was legacy from a revision that didn't copy the matrix if it wasn't necessary, but I think it just complicates the code for very little gain).

Comment 6

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

Comment on attachment 675084 [details] [diff] [review]
Use ComputeRenderIntegrity when measuring checkerboarding

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

::: mobile/android/base/gfx/LayerRenderer.java
@@ +618,5 @@
>                  float checkerboard = 0.0f;
>  
>                  int screenArea = viewport.width() * viewport.height();
> +                if (screenArea > 0) {
> +                    checkerboard = 1.0f;

Is there a reason you did it this way? I think it would just as well if you left the original code as-is but initialized checkerboard to 1.0f and then multiplied checkerboard by computeRenderIntegrity() before passing it to recordCheckerboard. (i.e. no need to split the if condition and special-case the screenArea > 0 behaviour).

Comment 7

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (:kats) from comment #6)
> Comment on attachment 675084 [details] [diff] [review]
> Use ComputeRenderIntegrity when measuring checkerboarding
> 
> Review of attachment 675084 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/gfx/LayerRenderer.java
> @@ +618,5 @@
> >                  float checkerboard = 0.0f;
> >  
> >                  int screenArea = viewport.width() * viewport.height();
> > +                if (screenArea > 0) {
> > +                    checkerboard = 1.0f;
> 
> Is there a reason you did it this way? I think it would just as well if you
> left the original code as-is but initialized checkerboard to 1.0f and then
> multiplied checkerboard by computeRenderIntegrity() before passing it to
> recordCheckerboard. (i.e. no need to split the if condition and special-case
> the screenArea > 0 behaviour).

erk, I realise I have this the wrong way round :) Checkerboard = 1.0f is full checkerboard, not no checkerboard... New patch incoming...

Comment 8

[Chris Lord [:cwiiis]]

Attachment: Use ComputeRenderIntegrity when measuring checkerboarding

We discussed the short-circuiting on IRC and I guess we came to the conclusion that it doesn't really matter.

Some bad news: I discovered the checkerboard reporting was broken and would've been significantly under-reporting our checkerboarding in some cases :( This patch fixes that too, and the numbers reported look sensible to me.

Comment 9

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

Comment on attachment 675120 [details] [diff] [review]
Use ComputeRenderIntegrity when measuring checkerboarding

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

r=me with the last comment addressed.

::: mobile/android/base/gfx/LayerRenderer.java
@@ +409,5 @@
>          private RenderContext mPageContext, mScreenContext;
>          // Whether a layer was updated.
>          private boolean mUpdated;
>          private final Rect mPageRect;
> +        private final Rect mAbsolutePageRect;

I'm not sure I like this variable name but I can't come up with anything better. At some point I need to really go through all this code and rename all the variables to use hungarian notation. There's far too many Rects and RectFs floating around and I can never figure out what coordinate spaces they're in.

@@ +541,5 @@
>              mBackgroundLayer.setMask(mPageRect);
>              mBackgroundLayer.draw(mScreenContext);
>  
>              /* Draw the drop shadow, if we need to. */
> +            if (!mAbsolutePageRect.contains(RectUtils.round(mFrameMetrics.getViewport())))

So this should be equivalent to the original code, except using rounded values instead of unrounded? Any reason for doing that?

@@ +603,5 @@
>                  Region validRegion = rootLayer.getValidRegion(mPageContext);
>  
>                  /* restrict the viewport to page bounds so we don't
>                   * count overscroll as checkerboard */
> +                if (!viewport.intersect(mAbsolutePageRect)) {

I *think* this is right, but it blows my mind if I think about it too much.

@@ +614,5 @@
>  
>                  float checkerboard = 0.0f;
>  
>                  int screenArea = viewport.width() * viewport.height();
> +                if (screenArea > 0 && !validRegion.quickReject(viewport)) {

You changed this condition from doing "if viewport == validregion, assume zero checkerboard" to doing "if viewport intersects validregion, compute render integrity of 'valid' area", which makes sense to me. But the case that is wrong now (I think) is that if (screenArea > 0) && (validRegion.quickReject(viewport)) then the new code will leave checkerboard as 0.0 when it really should be 1.0f.

This should be fixable if you initialize checkerboard to (screenArea > 0 ? 1.0f : 0.0f).

Comment 10

[Chris Lord [:cwiiis]]

Pushed with fixes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/221f94d01017
https://hg.mozilla.org/integration/mozilla-inbound/rev/0592d78779d9

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/221f94d01017
https://hg.mozilla.org/mozilla-central/rev/0592d78779d9