Unnecessary invalidations during fast scrolling

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

Trunk
mozilla41
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Posted file testcase
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.
/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/
Hmm, this has a bug. Apparently InvalidatePostTransformRegion can be called for a layer after FinishLayerInvalidation.
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/
Attachment #8604899 - Attachment is obsolete: true
With that change my test wasn't even passing. I need to figure out what's actually going on.
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.
/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/
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".
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.
Yeah, same here.
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.
https://hg.mozilla.org/mozilla-central/rev/7473e741405c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Attachment #8605526 - Attachment is obsolete: true
Attachment #8605526 - Flags: review?(roc)
Attachment #8620274 - Flags: review?(roc)
Attachment #8620275 - Flags: review?(roc)
Attachment #8620274 - Flags: review?(roc)
Attachment #8620275 - Flags: review?(roc)
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.