over invalidation when scrolling

RESOLVED FIXED in mozilla18

Status

()

defect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: gal, Assigned: roc)

Tracking

(Depends on 1 bug)

Trunk
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(12 attachments)

728 bytes, text/html
Details
14.89 KB, text/html
Details
785 bytes, patch
jrmuizel
: review+
Details | Diff | Splinter Review
1.26 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
4.67 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
3.40 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
3.08 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
13.79 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.75 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
1.56 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
1.27 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.15 KB, patch
tnikkel
: review+
roc
: checkin+
Details | Diff | Splinter Review
Reporter

Description

7 years ago
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.
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.
Posted file 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.
Assignee: nobody → roc
Posted file better testcase
This testcase is self-contained and a bit more clear.
This makes debugging invalidation a lot easier on Windows, and reduces the amount of repainting we have to do.
Attachment #652612 - Flags: review?(bas.schouten)
Not actually needed for this bug, but helps debugging.
Attachment #652614 - Flags: review?(jmuizelaar)
The next patch in this series exposes what I think is an existing bug, which is fixed by this patch.
Attachment #652615 - Flags: review?(matt.woodrow)
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.
Attachment #652612 - Flags: review?(bas.schouten) → review+
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 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.
Attachment #652609 - Flags: review?(jmuizelaar) → review+
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
Attachment #652614 - Flags: review?(jmuizelaar) → review+
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.
Attachment #652618 - Flags: review?(jmuizelaar) → review+
Attachment #652610 - Flags: review?(tnikkel) → review+
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.
Attachment #652617 - Flags: review?(tnikkel) → review+
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.
(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?
Reporter

Comment 19

7 years ago
We don't use the java compositor in B2G.
(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 :)
Attachment #652615 - Flags: review?(matt.woodrow) → review+
We'll use this in the next patch.
Attachment #654925 - Flags: review?(bas.schouten)
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.
Attachment #654926 - Flags: review?(tnikkel) → review+
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.
Attachment #654925 - Flags: review?(bas.schouten) → review+
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
Part 8 causes a reftest failure on Android --- layout/reftests/svg/image/image-rotate-01.svg --- which is super-disturbing.
The Android failure reveals an existing bug in our image-snapping code when the CTM is a rotation by a multiple of 90 degrees.
Attachment #657819 - Flags: review?(tnikkel)
Attachment #657819 - Flags: review?(tnikkel) → review+
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.
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
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.
Whiteboard: [leave open]
Whiteboard: [leave open]
Blocks: 780762
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.
Blocks: 793494
Depends on: 795085

Updated

7 years ago
Depends on: 808491
Depends on: 820808

Updated

6 years ago
Depends on: 934705

Updated

6 years ago
No longer depends on: 934705

Updated

5 years ago
Depends on: 1098275
You need to log in before you can comment on or make changes to this bug.