801918 - Frequent unnecessary invalidation of the entire content area while scrolling

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jonathan Watt [:jwatt]]

Attachment: testcase

When scrolling the attached testcase, we frequently invalidate the entire content area. Try scrolling up and down while keeping the black bordered div entirely in the content area. If scrolled very, very slowly, then we only invalidate the area that is revealed. If however scrolling is sped up (again, keeping the div in the content area), we start to invalidate the entire content area every so often. It seems like the faster you scroll, the more often the entire viewport is invalidated.

Comment 1

[Matt Woodrow (:mattwoodrow)]

This happens when we attempt to draw into multiple quadrants of our rotated GL texture here:

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/ThebesLayerOGL.cpp#537

We then throw out 3/4 quadrants of the old texture here:

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/ThebesLayerOGL.cpp#597

We should be able to copy all of the old data to the new texture if wrapping works correctly. It wasn't working correctly when that code was added.

Someone with more GL knowledge should be able to fix this fairly easily, cc'ing some gfx people.

Comment 2

[Jonathan Watt [:jwatt]]

Please use versioned hg links in bug comments so that what your links point to doesn't change over time. ;)

http://hg.mozilla.org/mozilla-central/file/cd3270dc35cc/gfx/layers/opengl/ThebesLayerOGL.cpp#l537

http://hg.mozilla.org/mozilla-central/file/cd3270dc35cc/gfx/layers/opengl/ThebesLayerOGL.cpp#l597

Comment 3

[Jonathan Watt [:jwatt]]

FWIW this seems to happen a lot, and as well as being a nasty perf hit in its own right it makes it more difficult to debug other invalidation bugs.

Comment 4

[Matt Woodrow (:mattwoodrow)]

I think this is a bug in BlitTextureImage:

http://hg.mozilla.org/mozilla-central/file/be64e9425b0b/gfx/gl/GLContext.cpp#l2144

This intersection won't take wrapped texture data into account.

If we fix BlitTextureImage such that it works correctly with rects that are intended to wrap around, then we can stop adding area to mRegionToDraw in ThebesLayerOGL.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Attachment: A somewhat ugly patch that accomplishes this

This patch ends up calling BlitTextureImage three extra times instead of changing BlitTextureImage to support rotated buffers.

Supporting rotated buffers in BlitTextureImage is somewhat tricky because of TiledTextureImage, and calling it three more times shouldn't be too bad (it should be better than having to repaint)

The rectangle manipulation code is a bit hairy.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 673196 [details] [diff] [review]
A somewhat ugly patch that accomplishes this

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

How often are we using TiledTextureImage for a ThebesLayerOGL (if ever)? I thought it was only for mobile, and we've switched to tiling at the ThebesLayer level instead.

If we are actually hitting this path with a TiledTextureImage, then this approach seems pretty reasonable to me.

I am of course concerned by the loss of my wonderful ascii art.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

I believe B2G uses TiledTextureImage with ThebesLayerOGL still.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Attachment: Somewhat cleaner patch

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 677360 [details] [diff] [review]
Somewhat cleaner patch

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

Looks good.

I would still prefer to see the ascii art diagrams retained though.

Comment 10

[bhavana bajaj [:bajaj]]

Is this something tickled by DLBI landings on 18 ? Or has this been the default behavior ever since ? Looking for a compelling reason to track this on 18 and have a patch on aurora at this point.

Comment 11

[Matt Woodrow (:mattwoodrow)]

No, it's not a regression.

It is, however, an annoying perf bug with a simple fix :)

Comment 12

[Alex Keybl [:akeybl]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> No, it's not a regression.
> 
> It is, however, an annoying perf bug with a simple fix :)

Please nominate for uplift in that case. Since this isn't a new regression, no need to track for a specific release. Not clear that this has enough user impact to skip the trains though.

Comment 13

[Jet Villegas (inactive)]

+1 for the FF18 uplift here. Scrolling perf goodness is always welcome :)

Comment 14

[Jeff Muizelaar [:jrmuizel]]

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

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b5c86318a3ee

Comment 16

[Jeff Muizelaar [:jrmuizel]]

It sounds like this should get backed out.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Agreed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2804a15008b2

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backout:
https://hg.mozilla.org/mozilla-central/rev/2804a15008b2

Comment 19

[Jeff Muizelaar [:jrmuizel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9707fdeafe85

Try again, this time with correct component alpha.

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9707fdeafe85