Closed
Bug 1164227
Opened 9 years ago
Closed 9 years ago
Unnecessary invalidations during fast scrolling
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: mstange, Assigned: mstange)
Details
Attachments
(3 files, 2 obsolete files)
STR: 1. Load the testcase. 2. Enable paint flashing. 3. Scroll slowly up and down in the scroll box. 4. Scroll quickly up and down in the scroll box. Expected results: Only the newly-visible horizontal strips of the scrolled contents should be invalidated. The red box should never be redrawn. Actual results: Many times during fast scrolling (but not all the time), the invalidation region is much bigger than it needs to be and causes the red box to be redrawn. I see this on many sites, but it's particularly easy to reproduce on http://planet.mozilla.org/ (enable paint flashing to test): If you scroll slowly, everything looks the way you want it to look, but once you start scrolling more quickly, we invalidate way too much. The reason this happens is the mInvalidRegion.SimplifyOutward(20) call in Client(Tiled)PaintedLayer::InvalidateRegion: We invalidate both stuff that's being scrolled out of view and stuff that's newly appearing in a fairly arbitrary order, so our invalid region contains both stuff above and below the contents that we'd like to stay valid. Then we simplify the region and get something that intersects stuff that we really don't need to invalidate.
Assignee | ||
Comment 1•9 years ago
|
||
/r/8677 - Bug 1164227 - Don't allow invalid region simplification to invalidate unchanged scrolled contents. r=roc Pull down this commit: hg pull -r 100f15f63762809277ef1594fb81d46a7f865eec https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 2•9 years ago
|
||
Hmm, this has a bug. Apparently InvalidatePostTransformRegion can be called for a layer after FinishLayerInvalidation.
Assignee | ||
Comment 3•9 years ago
|
||
Comment on attachment 8604899 [details] MozReview Request: bz://1164227/mstange /r/8677 - Bug 1164227 - Don't allow invalid region simplification to invalidate unchanged scrolled contents. r=roc Pull down this commit: hg pull -r 00cee7580e9f5954dff9cc265731a2143c153da2 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Updated•9 years ago
|
Attachment #8604899 -
Attachment is obsolete: true
Assignee | ||
Comment 4•9 years ago
|
||
With that change my test wasn't even passing. I need to figure out what's actually going on.
Assignee | ||
Comment 5•9 years ago
|
||
That was happening because in FinishPaintedLayerData we don't know the visible region of the layer yet. We only set it during PostprocessRetainedLayers. And most of DLBI actually happens after PostprocessRetainedLayers, during FrameLayerBuilder::WillEndTransaction. But some of it happens before, e.g. getting invalidations from inactive layer managers. New patch coming up.
Assignee | ||
Comment 6•9 years ago
|
||
/r/8733 - Bug 1164227 - Don't allow invalid region simplification to invalidate unchanged scrolled contents. r=roc Pull down this commit: hg pull -r e4eb1a64a173e607a68278d802400c1fa8b90a68 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 7•9 years ago
|
||
Once we have APZ + tiling everywhere, we should be able to use a much simpler solution: In that case, the intersection of the old and the new layer contents will be aligned to tiles, so if we changed the region simplification step in InvalidateRegion from "simplify to 20 rects" to "simplify to one rect per tile", or to "snap all rects outwards to tile boundaries", then this region simplification step wouldn't cause any invalidations that intersect the "common rect".
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8605526 [details] MozReview Request: bz://1164227/mstange /r/8733 - Bug 1164227 - Don't allow invalid region simplification to invalidate unchanged scrolled contents. r=roc Pull down this commit: hg pull -r e4eb1a64a173e607a68278d802400c1fa8b90a68 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8605526 -
Flags: review?(roc)
I don't like how complex this code all is, but I can't think of a better way to fix this.
Assignee | ||
Comment 11•9 years ago
|
||
Yeah, same here.
Assignee | ||
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce5f10e3c979
Comment 13•9 years ago
|
||
Backed out for B2G wordwrap-01.html timeouts. https://treeherder.mozilla.org/logviewer.html#?job_id=9974756&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/31459ec4ef93
Assignee | ||
Comment 14•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4185085cd26a
Assignee | ||
Comment 15•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a47f88a29fb
Assignee | ||
Comment 16•9 years ago
|
||
DUMB I had misspelled GetVisibleRegion() as GetValidRegion(). Duh. It'd still be nice to know why the layer's valid region didn't cover its whole visible region, and why that only happens on B2G, but I'm not going to pursue it further in this bug. For completeness, the valid region of the PaintedLayer in question had bounds: 0, 1, 142, 100 and its visible region was the rectangle 0, 1, 143, 100 (so an additional 1 pixel column at the right). This was happening in the test layout/reftests/text/wordbreak-1.html on B2G R19 in the ICS emulator.
Assignee | ||
Comment 17•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7473e741405c
Comment 18•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7473e741405c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 19•9 years ago
|
||
Attachment #8605526 -
Attachment is obsolete: true
Attachment #8605526 -
Flags: review?(roc)
Attachment #8620274 -
Flags: review?(roc)
Attachment #8620275 -
Flags: review?(roc)
Assignee | ||
Comment 20•9 years ago
|
||
Assignee | ||
Comment 21•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8620274 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Attachment #8620275 -
Flags: review?(roc)
Comment 22•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/f5de44ecf07f
Assignee | ||
Comment 23•8 years ago
|
||
I backed out this patch because bug 1236043 made it unnecessary. The test from this bug is still in the tree and still passing, so this bug is still fixed.
You need to log in
before you can comment on or make changes to this bug.
Description
•