Last Comment Bug 771154 - Large amount of over painting on some pages on fennec
: Large amount of over painting on some pages on fennec
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Chris Lord [:cwiiis]
:
Mentors:
Depends on:
Blocks: check2
  Show dependency treegraph
 
Reported: 2012-07-05 07:52 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2012-08-29 16:02 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
A somewhat simplified test case (734.53 KB, text/html)
2012-07-05 12:30 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details
Another much simplied test case (6.57 KB, text/html)
2012-08-08 16:44 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details
A somewhat simplifed test case that reproduces on beta (113.67 KB, text/html)
2012-08-09 15:50 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details
Invalidate only the bounds of newly visible frames (3.61 KB, patch)
2012-08-22 16:35 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Invalidate only the bounds of newly visible frames (v2) (6.91 KB, patch)
2012-08-23 02:05 PDT, Chris Lord [:cwiiis]
roc: review+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2012-07-05 07:52:01 PDT
You can see this problem by turning on paint flashing.
It happens on cnn.com and reddit comment threads. It does not happen on http://people.mozilla.org/~jmuizelaar/

I'll try to make a reduced test case.
Comment 1 Jeff Muizelaar [:jrmuizel] 2012-07-05 12:30:06 PDT
Created attachment 639428 [details]
A somewhat simplified test case
Comment 3 Jeff Muizelaar [:jrmuizel] 2012-08-08 14:24:13 PDT
On the attached test case we end up calling SetInvalidateAllThebesContent() the same happens on cnn.com
Comment 4 Jeff Muizelaar [:jrmuizel] 2012-08-08 16:44:23 PDT
Created attachment 650362 [details]
Another much simplied test case

In this case adding position: relative causes us to repaint everything.
Comment 5 Jeff Muizelaar [:jrmuizel] 2012-08-09 07:30:16 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> Created attachment 650362 [details]
> Another much simplied test case
> 
> In this case adding position: relative causes us to repaint everything.

Turns out this case is a recent regression for which I have filed bug 781516
Comment 6 Jeff Muizelaar [:jrmuizel] 2012-08-09 15:50:53 PDT
Created attachment 650706 [details]
A somewhat simplifed test case that reproduces on beta

This test case also has problems with position:relative and happens on beta.
Comment 7 Chris Lord [:cwiiis] 2012-08-22 03:59:23 PDT
The situations where everything is invalidated in this test case seem to be due to the underlying frame changing when merging clip items. We had this same problem with scroll layers in bug 729534. I'll attempt a similar solution, but this does feel like a very delicate situation to me.

Perhaps we should always swap frames when merging? Or perhaps merged frames should share DisplayListItemDataEntry's and the invalid region should be stored there instead of on the frame?
Comment 8 Chris Lord [:cwiiis] 2012-08-22 04:23:56 PDT
(In reply to Chris Lord [:cwiiis] from comment #7)
> The situations where everything is invalidated in this test case seem to be
> due to the underlying frame changing when merging clip items. We had this
> same problem with scroll layers in bug 729534. I'll attempt a similar
> solution, but this does feel like a very delicate situation to me.

This diagnosis wasn't correct (or, not the whole story) - when I scroll down on this page, at some point some frames appear and cause a full invalidation. Sometimes they disappear shortly after, which I assume is due to the display-port changing size and causing them to be visible only momentarily.

I suppose the real issue here is that any new merged frame becoming visible triggers an entire invalidation of that layer. In this situation, instead of invalidating the entire layer, could we not just invalidate the frame bounds?
Comment 9 Chris Lord [:cwiiis] 2012-08-22 16:35:07 PDT
Created attachment 654418 [details] [diff] [review]
Invalidate only the bounds of newly visible frames

This patch changes the behaviour of a missing ThebesLayerInvalidation property from meaning "invalidate the entire layer" to "invalidate the area of this frame".

I'm not sure if I've done this correctly, or if this is even a valid thing to do, but it does fix a fair few of the bad invalidation cases and it seems to pass tests: https://tbpl.mozilla.org/?tree=Try&rev=64b4d0d9296f (this was looking like it'd end up all green when I last saw it, it's taking a ridiculously long time to complete...)
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-22 17:34:55 PDT
Comment on attachment 654418 [details] [diff] [review]
Invalidate only the bounds of newly visible frames

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2165,3 @@
>    } else {
>      // The region was deleted to indicate that everything should be
>      // invalidated.

"or the frame has just become visible".

@@ +2178,5 @@
> +  }
> +
> +  aState.AddInvalidThebesContent(invalidRegion.
> +    ScaleToOutsidePixels(scaleParameters.mXScale, scaleParameters.mYScale,
> +                         aState.GetAppUnitsPerDevPixel()));

I'm afraid this is going to break invalidation when zooming. See bug 776836.

We might need to add a flag to RefCountedRegion to indicate that the region is infinite and everything should be invalidated, and set that flag where we currently delete the region, and here in BuildContainerLayerFor take the SetInvalidateAllThebesContent path when that flag is set.
Comment 11 Chris Lord [:cwiiis] 2012-08-23 02:05:26 PDT
Created attachment 654548 [details] [diff] [review]
Invalidate only the bounds of newly visible frames (v2)

This adds an mIsInfinite to RefCountedRegion, which gets set when invalidating untrusted geometry (instead of deleting the region). This should mean that the clear-frame-rect code is only executed for newly visible frames(?)
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-23 03:39:53 PDT
Comment on attachment 654548 [details] [diff] [review]
Invalidate only the bounds of newly visible frames (v2)

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

Super.
Comment 13 Chris Lord [:cwiiis] 2012-08-23 04:15:43 PDT
Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/8bc22ea75c01
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-08-23 19:21:26 PDT
https://hg.mozilla.org/mozilla-central/rev/8bc22ea75c01

Note You need to log in before you can comment on or make changes to this bug.