Progressive tile painting causes talos to return inaccurate results for checkerboarding

RESOLVED FIXED in mozilla19

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cwiiis, Assigned: cwiiis)

Tracking

Trunk
mozilla19
ARM
Android
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
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.
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.
(Assignee)

Comment 3

6 years ago
Created attachment 675082 [details] [diff] [review]
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.
Attachment #675082 - Flags: review?(bgirard)
(Assignee)

Comment 4

6 years ago
Created attachment 675084 [details] [diff] [review]
Use ComputeRenderIntegrity when measuring checkerboarding
Attachment #675084 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 5

6 years ago
Created attachment 675088 [details] [diff] [review]
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).
Attachment #675082 - Attachment is obsolete: true
Attachment #675082 - Flags: review?(bgirard)
Attachment #675088 - Flags: review?(bgirard)
Attachment #675088 - Flags: review?(bgirard) → review+
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).
(Assignee)

Comment 7

6 years ago
(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...
(Assignee)

Comment 8

6 years ago
Created attachment 675120 [details] [diff] [review]
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.
Attachment #675084 - Attachment is obsolete: true
Attachment #675084 - Flags: review?(bugmail.mozilla)
Attachment #675120 - Flags: review?(bugmail.mozilla)
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).
Attachment #675120 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/221f94d01017
https://hg.mozilla.org/mozilla-central/rev/0592d78779d9
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.