1164227 - Unnecessary invalidations during fast scrolling

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

Attachment: 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.

Comment 1

[Markus Stange [:mstange]]

Attachment: 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 100f15f63762809277ef1594fb81d46a7f865eec https://reviewboard-hg.mozilla.org/gecko/

Comment 2

[Markus Stange [:mstange]]

Hmm, this has a bug. Apparently InvalidatePostTransformRegion can be called for a layer after FinishLayerInvalidation.

Comment 3

[Markus Stange [:mstange]]

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/

Comment 4

[Markus Stange [:mstange]]

With that change my test wasn't even passing. I need to figure out what's actually going on.

Comment 5

[Markus Stange [:mstange]]

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.

Comment 6

[Markus Stange [:mstange]]

Attachment: 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/

Comment 7

[Markus Stange [:mstange]]

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 8

[Markus Stange [:mstange]]

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/

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/8733/#review7631

Ship It!

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't like how complex this code all is, but I can't think of a better way to fix this.

Comment 11

[Markus Stange [:mstange]]

Yeah, same here.

Comment 12

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce5f10e3c979

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4185085cd26a

Comment 15

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a47f88a29fb

Comment 16

[Markus Stange [:mstange]]

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.

Comment 17

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7473e741405c

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7473e741405c

Comment 20

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1164227 - Don't allow invalid region simplification to invalidate unchanged scrolled contents. r=roc

Comment 21

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1164227 - Don't allow invalid region simplification to invalidate unchanged scrolled contents. r=roc

Comment 22

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5de44ecf07f

Comment 23

[Markus Stange [:mstange]]

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.