Closed
Bug 777194
Opened 12 years ago
Closed 12 years ago
over invalidation when scrolling
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: gal, Assigned: roc)
References
(Depends on 1 open bug)
Details
Attachments
(12 files)
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 |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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.
Assignee | ||
Comment 2•12 years ago
|
||
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
Assignee | ||
Comment 3•12 years ago
|
||
This testcase is self-contained and a bit more clear.
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #652609 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #652610 -
Flags: review?(tnikkel)
Assignee | ||
Comment 6•12 years ago
|
||
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)
Assignee | ||
Comment 7•12 years ago
|
||
Not actually needed for this bug, but helps debugging.
Attachment #652614 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 8•12 years ago
|
||
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)
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #652617 -
Flags: review?(tnikkel)
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #652618 -
Flags: review?(jmuizelaar)
Comment 11•12 years ago
|
||
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+
Assignee | ||
Comment 12•12 years ago
|
||
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•12 years ago
|
||
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 14•12 years ago
|
||
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 15•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #652610 -
Flags: review?(tnikkel) → review+
Comment 16•12 years ago
|
||
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 17•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
We don't use the java compositor in B2G.
Comment 20•12 years ago
|
||
(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 :)
Updated•12 years ago
|
Attachment #652615 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 21•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=4d6f5ea6e442
Assignee | ||
Comment 22•12 years ago
|
||
We'll use this in the next patch.
Attachment #654925 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 23•12 years ago
|
||
Attachment #654926 -
Flags: review?(tnikkel)
Assignee | ||
Comment 24•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #654926 -
Flags: review?(tnikkel) → review+
Comment 25•12 years ago
|
||
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+
Assignee | ||
Comment 26•12 years ago
|
||
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
Assignee | ||
Comment 27•12 years ago
|
||
Part 8 causes a reftest failure on Android --- layout/reftests/svg/image/image-rotate-01.svg --- which is super-disturbing.
Assignee | ||
Comment 28•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #657819 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 29•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=789170cd4510
Assignee | ||
Comment 30•12 years ago
|
||
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.
Assignee | ||
Comment 31•12 years ago
|
||
Adding some diagnostics: https://tbpl.mozilla.org/?tree=Try&rev=30c5519c28a0
Assignee | ||
Comment 32•12 years ago
|
||
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
Assignee | ||
Comment 33•12 years ago
|
||
Er, https://tbpl.mozilla.org/?tree=Try&rev=8c3ff84fdc1b
Assignee | ||
Comment 34•12 years ago
|
||
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.
Assignee | ||
Comment 35•12 years ago
|
||
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
Attachment #657819 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Whiteboard: [leave open]
Assignee | ||
Comment 36•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=c08d60f32b11
Assignee | ||
Comment 38•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=3d6e30df5959
Assignee | ||
Comment 39•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Whiteboard: [leave open]
Comment 40•12 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment 41•12 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•