777194 - over invalidation when scrolling

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Andreas Gal :gal]

When scrolling content we occasionally repaint parts of the page we should be able to simple buffer rotate.

STR:
1. Enable paint flashing.
2. Go to http://www.calculatedriskblog.com/ or any other mostly static site. A bug in bugzilla works too.
3. Scroll down by small increments.
4. Buffer rotation can be seen most the time, except occasionally we do a full repaint.
5. We do a full repaint when we hit the bottom (can't scroll down further).

I think this is related to bad performance on mobile since we paint a lot more than we should on this kind of content.

Comment 1

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

On Windows, just scrolling http://www.calculatedriskblog.com doesn't invalidate the whole page until we reach the end, but <iframe src="http://www.calculatedriskblog.com/" style="width:500px; height:500px; -moz-transform:scale(0.8);"></iframe> shows us invalidating the whole page frequently as we scroll down with arrow keys.

Comment 2

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

Attachment: testcase

Adapted from http://people.mozilla.com/~jmuizelaar/tests/scale.html.

Open this page, click in the IFRAME and scroll down with arrow keys. We shouldn't need to invalidate other than where content has scrolled into view, but we do. I have some patches to fix this.

Comment 3

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

Attachment: better testcase

This testcase is self-contained and a bit more clear.

Comment 4

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

Attachment: Part 0: Add gfxPoint::WithinEpsilonOf

Comment 5

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

Attachment: Part 1: Use an epsilon tolerance value when comparing residual translations

Comment 6

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

Attachment: Part 2: Remove RETENTION_THRESHOLD. FrameLayerBuilder limits the complexity of visible regions anyway

This makes debugging invalidation a lot easier on Windows, and reduces the amount of repainting we have to do.

Comment 7

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

Attachment: Part 3: Add ToString() method to regions for easier logging/debugging

Not actually needed for this bug, but helps debugging.

Comment 8

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

Attachment: Part 4: When a scrolled layer's visible region includes pixels that are outside the snapped clip rect, those pixels won't be valid after scrolling

The next patch in this series exposes what I think is an existing bug, which is fixed by this patch.

Comment 9

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

Attachment: Part 5: When choosing a subpixel position to scroll to, align the new position with the position that was most recently used to rerender the scrolled layer(s)

Comment 10

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

Attachment: Part 6: Testcase

Comment 11

[Bas Schouten (:bas.schouten)]

Comment on attachment 652612 [details] [diff] [review]
Part 2: Remove RETENTION_THRESHOLD. FrameLayerBuilder limits the complexity of visible regions anyway

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

Sounds good, if this ever becomes a performance issue we can see then.

Comment 12

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

https://tbpl.mozilla.org/?tree=Try&rev=b4c6cec083d6

One big thing this patch series doesn't fix: if you scroll down with the scrollbar arrows, you will get invalidations of the entire frame, because the scrollbar chooses particular CSS-pixel scroll offsets, some of which aren't close enough to a layer pixel boundary. (Scrolling down by lines with the arrow keys avoids that because we allow a larger zone for the target scroll position.) Scrolling on mobile will still have the same problem.

An easy way to fix that for mobile is to extend the DOM scrolling APIs to allow passing in a larger zone for the allowed target scroll position.

A less easy way to fix that would be to rework scrolling more fundamentally, possibly by introducing a floating-point scroll offset that allows us to render at a slightly different offset to layout's idea of the scroll position.

The latter seems riskier, so I'm leaning towards the former for now.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 652609 [details] [diff] [review]
Part 0: Add gfxPoint::WithinEpsilonOf

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

::: gfx/thebes/gfxPoint.h
@@ +42,5 @@
>          return *this;
>      }
> +
> +    bool WithinEpsilonOf(const gfxPoint& aPoint, gfxFloat aEpsilon) {
> +        return fabs(aPoint.x - x) < aEpsilon && fabs(aPoint.y - y) < aEpsilon;

Given the name, I think <= makes more sense.

e.g. 5 is within 4 of 1.

Not sure it matters though.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 652614 [details] [diff] [review]
Part 3: Add ToString() method to regions for easier logging/debugging

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

::: gfx/src/nsRegion.cpp
@@ +1868,5 @@
> +  {
> +    if (r != mRectListHead.next) {
> +      result.AppendLiteral("; ");
> +    }
> +    result.Append(nsPrintfCString("%d,%d,%d,%d", r->x, r->y, r->XMost(), r->YMost()));

result.AppendPrintf

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 652618 [details] [diff] [review]
Part 6: Testcase

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

::: layout/base/tests/chrome/transformed_scrolling_repaints_3_window.html
@@ +76,5 @@
> +        waitForAllPaintsFlushed(nextIteration);
> +      });
> +    });
> +  });
> +}

I found that the control flow in this testcase was difficult to follow. I don't have any real suggestions though.

Comment 16

[Timothy Nikkel (:tnikkel)]

Comment on attachment 652617 [details] [diff] [review]
Part 5: When choosing a subpixel position to scroll to, align the new position with the position that was most recently used to rerender the scrolled layer(s)

A stray printf leaked in here.

Comment 17

[Chris Lord [:cwiiis]]

Comment on attachment 652612 [details] [diff] [review]
Part 2: Remove RETENTION_THRESHOLD. FrameLayerBuilder limits the complexity of visible regions anyway

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

::: gfx/layers/d3d10/ThebesLayerD3D10.cpp
@@ +204,4 @@
>        nsIntRect largeRect = retainRegion.GetLargestRectangle();
>  
>        // If we had no hardware texture before, or have no retained area larger than
>        // the retention threshold, we're not retaining and are done here.

Fly-by review - this comment needs to be updated.

Comment 18

[Chris Lord [:cwiiis]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> https://tbpl.mozilla.org/?tree=Try&rev=b4c6cec083d6
> 
> One big thing this patch series doesn't fix: if you scroll down with the
> scrollbar arrows, you will get invalidations of the entire frame, because
> the scrollbar chooses particular CSS-pixel scroll offsets, some of which
> aren't close enough to a layer pixel boundary. (Scrolling down by lines with
> the arrow keys avoids that because we allow a larger zone for the target
> scroll position.) Scrolling on mobile will still have the same problem.
> 
> An easy way to fix that for mobile is to extend the DOM scrolling APIs to
> allow passing in a larger zone for the allowed target scroll position.
> 
> A less easy way to fix that would be to rework scrolling more fundamentally,
> possibly by introducing a floating-point scroll offset that allows us to
> render at a slightly different offset to layout's idea of the scroll
> position.
> 
> The latter seems riskier, so I'm leaning towards the former for now.

I'd have thought that mobile is unaffected by this, given that the check for sub-pixel alignment is #ifdef'd out with MOZ_JAVA_COMPOSITOR?

Comment 19

[Andreas Gal :gal]

We don't use the java compositor in B2G.

Comment 20

[Chris Lord [:cwiiis]]

(In reply to Andreas Gal :gal from comment #19)
> We don't use the java compositor in B2G.

ah, I assumed mobile = Firefox Mobile. We should probably be specific (and if b2g solves this properly, it'd be great to share this with Fx Mobile).

Also, for the record, we don't use a Java compositor in Mobile either, I think there's a bug open to rename that define :)

Comment 21

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

https://tbpl.mozilla.org/?tree=Try&rev=4d6f5ea6e442

Comment 22

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

Attachment: Part 7: Add gfx3DMatrix::NudgeToIntegers

We'll use this in the next patch.

Comment 23

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

Attachment: Part 8: Nudge transform computed in ChooseScaleAndSetTransform

Comment 24

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

Part 8 fixes a test failure in WinXP layout/reftests/transform-3d/preserve3d-1a.html. I'm hopeful it might fix some other problems as well.

Comment 25

[Bas Schouten (:bas.schouten)]

Comment on attachment 654925 [details] [diff] [review]
Part 7: Add gfx3DMatrix::NudgeToIntegers

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

I hope this isn't going to be in a performance critical path :) This seems like a decent amount of cycles.

Comment 26

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

I had to add background:white to transformed_scrolling_repaints_3_window.html because otherwise it was transparent, which triggered some component-alpha stuff that caused Mac scrolling to require constant repainting.

https://tbpl.mozilla.org/?tree=Try&rev=956f8d608633

Comment 27

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

Part 8 causes a reftest failure on Android --- layout/reftests/svg/image/image-rotate-01.svg --- which is super-disturbing.

Comment 28

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

Attachment: Part 9: fix image snapping code

The Android failure reveals an existing bug in our image-snapping code when the CTM is a rotation by a multiple of 90 degrees.

Comment 29

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

https://tbpl.mozilla.org/?tree=Try&rev=789170cd4510

Comment 30

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

Everything's green on try except that on Windows XP, test_transformed_scrolling_repaints.html and test_transformed_scrolling_repaints_2.html fail consistently.

I reproduced that failure once in my Windows XP VM, but it normally doesn't happen; I haven't been able to reproduce it enough to debug it.

I think I will disable those tests on XP and land.

Comment 31

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

Adding some diagnostics: https://tbpl.mozilla.org/?tree=Try&rev=30c5519c28a0

Comment 32

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

Those didn't help at all, except by showing that "ContainerState::CreateOrRecycleThebesLayer InvalidateEntireThebesLayer due to subpixel offset change" happens no more on Windows XP than on Windows 7, so is not the cause of the test failure. I.e. the patches seem to work on Windows XP.

Another push with slightly better diagnostics:
https://tbpl.mozilla.org/?tree=Try&rev=7c6f0da12ec5

Comment 33

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

Er, https://tbpl.mozilla.org/?tree=Try&rev=8c3ff84fdc1b

Comment 34

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

And https://tbpl.mozilla.org/?tree=Try&rev=1444e9040e41 for lots of patches with the failing tests disabled on Windows XP. This one's for landing.

Comment 35

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

Comment on attachment 657819 [details] [diff] [review]
Part 9: fix image snapping code

Part 9 checked into inbound.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b2fe6b88ca4

Comment 36

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

https://tbpl.mozilla.org/?tree=Try&rev=c08d60f32b11

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5b2fe6b88ca4

Comment 38

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

https://tbpl.mozilla.org/?tree=Try&rev=3d6e30df5959

Comment 39

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f8b8d69d23a
https://hg.mozilla.org/integration/mozilla-inbound/rev/2365bb8a416a
https://hg.mozilla.org/integration/mozilla-inbound/rev/d756d091d9c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/22a46f5dca2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/89003d5bfa07
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b2e43e83d0f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ef195a022a
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e2c7ed818d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/54d5957d6dbb
https://hg.mozilla.org/integration/mozilla-inbound/rev/64d4cb9b7be4
https://hg.mozilla.org/integration/mozilla-inbound/rev/be9f5e549658

Matt's work in bug 788044 shook things up a bit, in a good way. The patches in this bug no longer cause failures on Windows XP, in fact they make test_transformed_scrolling_repaints_2 pass (again) on all platforms, and they make test_transformed_scrolling_repaints pass on 64-bit Linux. Changeset be9f5e549658 reenables those tests.

Comment 40

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/1f8b8d69d23a
https://hg.mozilla.org/mozilla-central/rev/2365bb8a416a
https://hg.mozilla.org/mozilla-central/rev/d756d091d9c8
https://hg.mozilla.org/mozilla-central/rev/22a46f5dca2b
https://hg.mozilla.org/mozilla-central/rev/89003d5bfa07
https://hg.mozilla.org/mozilla-central/rev/2b2e43e83d0f
https://hg.mozilla.org/mozilla-central/rev/f2ef195a022a
https://hg.mozilla.org/mozilla-central/rev/2e2c7ed818d6
https://hg.mozilla.org/mozilla-central/rev/54d5957d6dbb
https://hg.mozilla.org/mozilla-central/rev/64d4cb9b7be4
https://hg.mozilla.org/mozilla-central/rev/be9f5e549658

Comment 41

[[e-mail]]

Comment on attachment 652614 [details] [diff] [review]
Part 3: Add ToString() method to regions for easier logging/debugging

>+#include "nsString.h"
This change makes it impossible to use nsIWidget in non-libxul code. This might be nsIWidget's fault for erroneously including nsRegion.h so I will investigate and file an appropriate followup bug.