Closed Bug 805028 Opened 12 years ago Closed 12 years ago

Progressive tile painting causes talos to return inaccurate results for checkerboarding

Categories

(Core :: Graphics: Layers, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: cwiiis, Assigned: cwiiis)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
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)
Attachment #675084 - Flags: review?(bugmail.mozilla)
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).
(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...
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+
Status: ASSIGNED → RESOLVED
Closed: 12 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.

Attachment

General

Created:
Updated:
Size: