search

luangruo / classilla

Automatically exported from code.google.com/p/classilla
0 stars 0 forks source link

Optimizing Slow Scroll mode; scrolling repaints too much #28

Open GoogleCodeExporter opened 8 years ago

GoogleCodeExporter commented 8 years ago 
At least two scrolling problems (they may be the same problem) on blogspot
sites, including the nav bar. Look at the scroll patch applied in 9.0 to
see if there are more screen lines that need to be marked as dirty during
the scroll.

Original issue reported on code.google.com by classi...@floodgap.com on 6 Jul 2009 at 12:50

GoogleCodeExporter commented 8 years ago 
This is NOT 268111. That was a later regression. 1.3.1 is not vulnerable to it.

Original comment by classi...@floodgap.com on 8 Aug 2009 at 4:53

GoogleCodeExporter commented 8 years ago 
Not fixed by the new scrolling code (see issue 16).

Original comment by classi...@floodgap.com on 18 Aug 2009 at 5:05

GoogleCodeExporter commented 8 years ago 
Also appears on http://blogs.abc.net.au/queensland/612_otf/ in more obvious 
fashion.
It appears screen regions are not getting properly invalidated on scroll; when 
the
address bar dropdown covers it up and then disappears, THEN the window appears 
correctly.

Original comment by classi...@floodgap.com on 23 Aug 2009 at 5:44

GoogleCodeExporter commented 8 years ago 
The scroll glitch is system dependent. Ironically, it is least noticible on slow
systems (for example, Classic drags Classilla down on my iBook G4, and the bug 
seems
to be nearly non-existent, but on the fast 1.25GHz OS 9.2.2 development 
machine, it
is ever-present on affected pages).

Candidates for this and related issues: 86249+199847

Original comment by classi...@floodgap.com on 2 Sep 2009 at 8:32

GoogleCodeExporter commented 8 years ago 
It is reliably triggered with the cursor keys on any CPU.

Original comment by classi...@floodgap.com on 2 Sep 2009 at 8:40

GoogleCodeExporter commented 8 years ago 
Let's re-examine https://bug289353.bugzilla.mozilla.org/attachment.cgi?id=189847

Original comment by classi...@floodgap.com on 10 Sep 2009 at 5:32

GoogleCodeExporter commented 8 years ago 
Analysis of M289353 showed a few things:

- We needed to pull up a few things to 1.8, modulo the OS 9-specific code that 
1.8
removed.
- The problem is largely limited to scrolling UP. While scrolling DOWN is 
inaccurate
on the affected sites, it is usually not noticibly, and even when it is, not
obviously ugly.
- Invalidating the scroll rect fixed the problem (duh), but made scrolling 
terribly slow.

This kludge is checked in for 9.0.4:
- Scrolling DOWN is unchanged, except for the case where we scroll more than 
the size
of the viewport, which was munging iframes. In that specific case only, we 
force a
full repaint. Otherwise, we use the fast and dirty scroll.
- Scrolling UP any amount forces an invalidation of the scroll rect. This makes
scrolling up about half as fast as scrolling DOWN, but the screen renders 
correctly
and there is no stuttered "hall of mirrors" effect.
- A Y scroll of 0 forces a full repaint, to handle any side effects that 
horizontal
scrolling may endure. This hits horizontal scrolling a bit, but should not be 
near as
frequent, and covers the situation where we get scrolling events in a weird 
order.

Since this fixes the issue, but is unsatisfactory, marking as a KLUDGE and 
changing
type to Enhancement with new summary.

Original comment by classi...@floodgap.com on 12 Oct 2009 at 8:21

GoogleCodeExporter commented 8 years ago 
After fixing the overflow hidden height bug, a lot more sites are still 
frac'ing even
with this patch, so I am reopening this bug. The cause seems to be subtly 
different
than the previous cause, so the kludge may still apply.

Original comment by classi...@floodgap.com on 15 Oct 2009 at 4:41

GoogleCodeExporter commented 8 years ago 
The overflow branch failure is a completely different failure (see issue 65 for 
the
explanation).

After eating some dogfood on this bug, I have decided to back the widget 
changes out
(keeping the parts of M289353 that apply) because the scroll up is agonizingly 
slow
on some pages, and the screwed up repaint doesn't occur often enough to be worth
that, *and* you can work around it by forcing a screen repaint (covering up the 
area,
or windowshading). That, btw, does not fix issue 65, and that's why that's a
different, more insidious, and much more critical problem than this one.

Original comment by classi...@floodgap.com on 20 Oct 2009 at 4:31

GoogleCodeExporter commented 8 years ago 
The horizontal scrolling problems for undersized windows reported in issue 65 
are
actually this bug, as they respond to repaints. I'll determine if I do want to
reinstate the kludge I wrote above after I do some work on the overflow branch.

Original comment by classi...@floodgap.com on 21 Jan 2010 at 2:37

GoogleCodeExporter commented 8 years ago 
The test case for that, btw, is pretty much any current Ars Technica article.

Original comment by classi...@floodgap.com on 21 Jan 2010 at 2:45

GoogleCodeExporter commented 8 years ago 
We need to solve this for 9.1-current. There is too much fragmentation on 
various
sites, including the ones mentioned above. There are probably multiple causes 
for
those specific issues, but we can solve them all here with a single blunt 
stroke.

- The horizontal case needs to be solved by forcing a full repaint for any X 
scroll
  *or* any Y scroll of zero.

- Down scrolling will not be messed with.

- Up scrolling should force an update on some multiple. We seem to scroll 
around 16
  pixels or so, so changing every 32 (a simple bit check of the coordinates) should
  be sufficient and reduce the impact of the repaints.

- At some point add a pref for this so that people who really care can force 
repaints
  all the livelong day.

Marking Critical, because this must be solved before ship (fortunately, unlike 
issue
65 and 9.0.4 I have a good idea what I need to do).

Original comment by classi...@floodgap.com on 26 Jan 2010 at 5:09

GoogleCodeExporter commented 8 years ago 
After extensive testing, we're looking at the following in ScrollBits:

- We force an Invalidate(PR_FALSE) all the time.
- Forget doing CopyBits because we're repainting everything anyway, so no point 
in
doing it twice.
- Factored out some region computing code that was for CopyBits, which we're 
not doing.

Amazingly this gets us probably to 80% of the old scrolling speed, and is much 
less
nasty with artifacting. It's more flickery because of duff double buffering 
(issue
65) but it's infinitely better quality.

Now to see if I can eliminate the Invalidate out of ::Scroll(), and then issue 
99.

Original comment by classi...@floodgap.com on 29 Jan 2010 at 6:15

GoogleCodeExporter commented 8 years ago 
I have further optimized this code. After more testing, the scrolling was about 
half
as slow in practice.

Now there is code in ScrollBits to reduce the number of full Invalidate()s. A 
full
Invalidate occurs whenever scrolling changes direction, or scrolling is 
horizontal.
Otherwise, up and down scrolling alternates between repainting the scrolling 
region
and the other half, and the scrolling direction half. This is pretty much good 
on
just about everything and is around 60-70% the speed. It is not what it should 
be,
but I don't think I have a whole lot of choice.

I'm going to leave this bug open to try to wring more out of it later. I 
suspect the
real solution is more frequently invalidating views.

Original comment by classi...@floodgap.com on 29 Jan 2010 at 7:40

GoogleCodeExporter commented 8 years ago 
Also added issue 100.

Original comment by classi...@floodgap.com on 30 Jan 2010 at 4:59

GoogleCodeExporter commented 8 years ago 
Still not quite good enough. Half scrolling shears like mad. So this is the 
final
algorithm DANG IT. Slow scrolling now simply forces full screen invalidates, but
disables CopyBits, which is expensive but much less so.

- See issue 100 -- we check the status of the slow scroll pref when the window 
widget
is initialized.
- If this widget is fast-scrollable (such as XUL trees), always fast scroll,
regardless of the pref setting. To determine this, we pass a flag in
NS_WIDGET_ALWAYS_FAST_SCROLL custom created for this process.
- If the user has chosen slow scroll, always slow scroll.
- Otherwise: 
  - If we are horizontally clipped (the horizontal scrollbar is showing), always slow
scroll. This isn't as bad as it sounds because with small windows (like my PB 
1400),
we have much fewer pixels to repaint. To determine this, the scroll code now 
passes
the nsIScrollableView it is backing so that the widget can ask its dimensions.
  - If we are scrolling horizontally, a special optimized case of the above, always
slow scroll.
  - If the view manager has disabled double buffering for some reason (see issue 14
and issue 65), always slow scroll. This is actually faster and better looking. 
To
determine this, the scroll code now passes the nsIViewManager it is backing, 
and the
view manager remembers its last flags, so that the widget can ask if the flags 
had
the double buffering flag on or not.
  - If the user changes direction of the scroll, force an invalidate for this single
scroll only.
  - If the user has not scrolled within a certain interval (right now 0.25sec), force
an invalidate for this single scroll only.
  - Otherwise,
    - Fast scroll.

This isn't perfect, but it is nearly the same speed, and most glitches are now 
made
transient except for those few intransigent sites that need slow scroll on all 
the
time. When I figure out a way to reliably detect these, then Slow Scroll can go 
away.

I swear, this is the last time I'm going to be in this code for 9.1.

Original comment by classi...@floodgap.com on 31 Jan 2010 at 11:54

GoogleCodeExporter commented 8 years ago 
I lied, I touched it some more. The scroll interval idea broke down on some 
sites
with complex layouts, causing them to fail the timeout and then repaint, over 
and
over, as if the scroll had stalled. This got really irritating, so I took that 
code out.

Instead, to get around the rounding problem I simply expanded the invalidated 
window
region by inTopDelta pixels MORE in the scrolling direction. This forces a
synchronized repaint exactly where we want it and overpaints only the minimum 
number
of lines we need to whitewash any lines or glitches. This works nearly 
perfectly.

The rest of the code is the same, including the ALWAYS_FAST_SCROLL, the 
horizontal
clip, the view manager/scroll view code and the changing direction. This is a 
lot
better and may be as good as we can get.

Original comment by classi...@floodgap.com on 11 Feb 2010 at 3:55

GoogleCodeExporter commented 8 years ago

Original comment by classi...@floodgap.com on 23 May 2010 at 9:48

GoogleCodeExporter commented 8 years ago 
Continuing in the awful kludge vein, because issue 96 hits some sites it 
shouldn't
hit, there is one more change. Command is now the "you're doing it wrong" key 
for
scrolling. If you hold it down when it is using a heuristic that causes it to 
slow
scroll, it will fast scroll (this helps when issue 96 causes some pages to lay 
out
too wide that don't need to slow scroll). If you hold it down when it is using a
heuristic that causes it to fast scroll, it will slow scroll (so you don't have 
to
run to Use Slow Scroll all the time).

At all other times, when it is not using a heuristic, it will do the right thing
regardless of Command (i.e., XUL always fast scrolls; Use Slow Scroll on always 
slow
scrolls; pages impacted by issue 65 always slow scroll).

Original comment by classi...@floodgap.com on 3 Jun 2010 at 4:01