457864 - Make wheel scrolling asynchronous

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Markus Stange [:mstange]]

Attachment: v1

This makes diagonal pixel scrolling bearable by avoiding the "stairstep" effect.

It doesn't fix bug 418351, but it might make fixing it easier.

Comment 1

[Markus Stange [:mstange]]

Fixing bug 418351 requires that we wait for all pending native scroll events to be dispatched before firing the nsRunnable. However, at least on Mac OS X, that doesn't happen:

NS_DispatchToCurrentThread(ev) appends the nsRunnable to Gecko's internal event queue. The pending native scroll events, however, are waiting in the OS's native event queue.
When the nsRunnable is appended to Gecko's internal event queue, a new native event callback is scheduled: nsThread::Dispatch -> nsThread::PutEvent -> nsBaseAppShell::OnDispatchedEvent -> nsAppShell::ScheduleNativeEventCallback.
This "native event callback" fires the events in Gecko's internal event queue.

On Mac OS X, this callback happens immediately; it doesn't wait for the pending native events. (This might be different on other platforms.) So the nsRunnable ends up being run before the pending native scroll events.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Did you try to add a flag to the first mousescroll event that there is still another coming (horizontal and vertical events), and make ESM to scroll only
after latter one?

Comment 3

[Markus Stange [:mstange]]

(In reply to comment #2)
> Did you try to add a flag to the first mousescroll event that there is still
> another coming (horizontal and vertical events), and make ESM to scroll only
> after latter one?

I didn't, and with this patch it's not necessary - the nsRunnable is only dispatched after both Gecko scroll events of a diagonal scroll gesture have been processed. (The native Cocoa mouse scroll event contains information about both axis in the same event. We split it in two Gecko events.)

When I said "this callback happens immediately", I didn't mean that it fires synchronously - it fires immediately after the current native event has been processed. In the diagonal scrolling case, that's after both Gecko scroll events have been processed.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #3)
> I didn't, and with this patch it's not necessary 
Right, but if the other approach works well, then this patch isn't needed.

Comment 5

[Markus Stange [:mstange]]

Why would you prefer the other approach? It sounds rather ugly to me, to be honest.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

That would change only mousescroll handling, and only in case there is both
horizontal and vertical scrolling. The patch changes scroll handling in many
other cases too (if I read it correctly), so it is more regression-risky.

Comment 7

[Markus Stange [:mstange]]

True. And I just realized that this patch probably breaks my tests for bug 350471; they assume that scrollTop/Left changes synchronously after a scroll event.

Comment 8

[Mats Palmgren (inactive)]

+  mView->ExecuteAsyncScroll();

I think mView could be destroyed at that point.

Comment 9

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

Unfortunately I think scrollTop/scrollLeft really do have to change synchronously after a scroll request. Certainly it would be confusing if setting scrollTop did not immediately return the same value from scrollTop.

The plan for compositor is to make the actual rendering of scrolling asynchronous. I guess that would fix this bug.

Comment 10

[Markus Stange [:mstange]]

(In reply to comment #9)
> Unfortunately I think scrollTop/scrollLeft really do have to change
> synchronously after a scroll request. Certainly it would be confusing if
> setting scrollTop did not immediately return the same value from scrollTop.

Good point.

> The plan for compositor is to make the actual rendering of scrolling
> asynchronous. I guess that would fix this bug.

OK, that sounds like the right thing to do.

Comment 11

[Markus Stange [:mstange]]

I just realized that this approach might work after all: Smooth scrolling is asynchronous, too, so we could just tweak the patch to only use the nsRunnable in those cases where smooth scrolling would be appropriate, i.e. only when ::ScrollTo is caused by a mouse wheel event.

Comment 12

[Markus Stange [:mstange]]

Attachment: v2

This patch addresses both comment 8 and comment 9 by unifying smooth scrolling and asynchronous scrolling:
 - I now use a timer instead of an nsRunnable; the timer is stopped when the
   view is destroyed, so the problem in comment 8 can't happen.
 - I use the NS_VMREFRESH_DEFERRED flag in the update flags to indicate that
   scrolling should be handled asynchronously. This flag is only set in the
   ::ScrollByXXX methods. This should avoid the scrollTop problem of comment 9.

I haven't run mochitests yet. I also need to update the test for bug 350471 since it assumes that the scroll position changes synchronously after a scroll event.

And now the good news: Combined with the patch in bug 459319, this patch fixes bug 418351! I've created a build that contains both patches: http://tests.themasta.com/fastscrolling_firefox-3.1b2pre.en-US.mac.dmg

Comment 13

[Markus Stange [:mstange]]

Instead of making nsChildView::Update a no-op (which is what bug 459319 does), we could also remove the call to ForceViewUpdate in nsEventStateManager::DoScrollText and the call to nsViewManager::UpdateViewAfterScroll in nsScrollPortView::Scroll - these are the only callers of nsChildView::Update while scrolling. But that makes me wonder why these calls were introduced in the first place.

Comment 14

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

The problem is that if the repaint of the window is delayed too much after the bitblit phase of the scroll, you get a nasty effect where one part of the window has been copied but the old copy is still visible.

Comment 15

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

Your patch looks good, and it's a great idea, but I don't like the fact that it makes scriptable methods like window.scrollByLines asynchronous. I think you should add flags parameters to the nsIScrollableView methods, and then just pass DEFERRED for the scrolling triggered by mousewheel in nsEventStateManager.

Comment 16

[Markus Stange [:mstange]]

Attachment: v3 [checked in]

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Tested v3, "stairstep" is gone and at least here bug 418351 is pretty much gone.
Maybe not quite as fast as Safari, but pretty close.
bug 459319 would make things even a bit faster.

/me trying to get used to OSX.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

I wonder if removing nsEventStateManager::ForceViewUpdate could still make any difference.
Would have to debug if it is ever called when nsChildView::mView::needsDisplay returns true, or something.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Seems like it doesn't save anything, at least not when scrolling tbox page. 
It is just useless call to update, when there isn't anything to update.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 343656 [details] [diff] [review]
v3 [checked in]

>     else
>-      scrollView->ScrollByLines(scrollX, scrollY);
>+      scrollView->ScrollByLines(scrollX, scrollY,
>+                                NS_VMREFRESH_SMOOTHSCROLL | NS_VMREFRESH_DEFERRED);
Does this mean that smoothscroll is always used for scrollbylines?

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 343656 [details] [diff] [review]
v3 [checked in]


>+  PRBool isSmoothScroll = (aUpdateFlags & NS_VMREFRESH_SMOOTHSCROLL) &&
>+                          IsSmoothScrollingEnabled();
OK, it doesn't mean that.

Comment 22

[Markus Stange [:mstange]]

Attachment: fix tests [checked in]

Comment 23

[Markus Stange [:mstange]]

Attachment: remove NS_SCROLL_PROPERTY_ALWAYS_BLIT from ScrollTo callers [checked in]

There are many places where ScrollTo gets called with NS_SCROLL_PROPERTY_ALWAYS_BLIT, although it's ignored completely. The only place where NS_SCROLL_PROPERTY_ALWAYS_BLIT makes a difference is when calling SetScrollProperties (and there's only one line that does that).

In fact, with my patch NS_SCROLL_PROPERTY_ALWAYS_BLIT is misinterpreted as NS_VMREFRESH_DEFERRED (they're both 1), which is bad.

In this patch I make sure that NS_SCROLL_PROPERTY_ALWAYS_BLIT is never passed to ScrollTo.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 344464 [details] [diff] [review]
fix tests [checked in]

>+    setTimeout(function() {
Would it be possible to use "scroll" event? It is dispatched after scrolling.
Timeouts may in some cases cause random tbox orange.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #24)
> (From update of attachment 344464 [details] [diff] [review])
> >+    setTimeout(function() {
> Would it be possible to use "scroll" event? It is dispatched after scrolling.
I guess that would work when there isn't any scrolling happening.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

er, wouldn't work

Comment 27

[Markus Stange [:mstange]]

pushed:
http://hg.mozilla.org/mozilla-central/rev/cd77e64a8aea
http://hg.mozilla.org/mozilla-central/rev/296089ec0903
http://hg.mozilla.org/mozilla-central/rev/1622ca12ba25

Comment 28

[Markus Stange [:mstange]]

I backed out because there are still two tests that need fixing:
- /tests/docshell/test/navigation/test_bug430723.html
- /tests/editor/libeditor/base/tests/test_selection_move_commands.xul

http://hg.mozilla.org/mozilla-central/rev/1e477902f74d

Comment 29

[Markus Stange [:mstange]]

I was wrong. The test failures were caused by the fact that I landed the wrong version of the patch (v2 instead of v3). This is quite embarassing.

I relanded, this time hopefully the right ones.
http://hg.mozilla.org/mozilla-central/rev/89b91ae29a3d
http://hg.mozilla.org/mozilla-central/rev/6ea413efbe9a

Comment 30

[Markus Stange [:mstange]]

Attachment: leak fix [checked in]

> -  if (mSmoothScroll) {
> +  if (mAsyncScroll && mAsyncScroll->mIsSmoothScroll) {
>      ...
>    } else {
>      ...
> +    mAsyncScroll = new AsyncScroll;
This caused a leak.

With smaug's help I landed this fix:
http://hg.mozilla.org/mozilla-central/rev/9f4dddb8d5f3

Comment 31

[Boris Zbarsky [:bzbarsky]]

Could this have caused bug 462793?

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #14)
> The problem is that if the repaint of the window is delayed too much after the
> bitblit phase of the scroll, you get a nasty effect where one part of the
> window has been copied but the old copy is still visible.
Shouldn't it be enough if we mark all the relevant (OSX) widgets with needsDisplay YES and let OS to handle painting? Then whenever OS decides to
paint something, all the dirty widgets get redrawn. Or am I missing something here?

Comment 33

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

Maybe.

Scrolling has two parts: bitblitting part of the window from one location to another, and repainting the rest. Those two parts need to happen close together in time or you get obvious transient glitches. That's why we force an update right after blitting instead of waiting for the OS to get around to doing it.

Does that make sense?

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Makes sense, but what doesn't make sense is that with the patch for bug 459319, 
there are no glitches. I wonder if nsChildView does the right thing with bitblitting, maybe it repaints too much.
There is probably too many updates or too large repainting happening,
because bug 459319 doesn't cause problems.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Disabling bitblitting on OSX certainly doesn't make scrolling slower, but
I think it actually makes it faster, at least on flash-heavy pages.

Comment 36

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

It certainly can be faster on some pages, but what about just a big window with a plain text document in it?

Comment 37

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

or a big window with lots of big images in it. And scroll by lines with the down-arrow.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #36)
> but what about just a big window with a plain text document in it?
Couldn't see any difference.

(In reply to comment #37)
> or a big window with lots of big images in it. And scroll by lines with the
> down-arrow.
nor here.

Comment 39

[Markus Stange [:mstange]]

Even with
PR_Sleep(PR_MillisecondsToInterval(200));
at the beginning of the drawRect method in nsChildView.mm I can't see any glitches (with the patch for bug 459319).

Comment 40

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

I'm confused. I've fixed bugs in the past on Mac where we were scrolling slowly because we were repainting the whole window by mistake. So how can it not matter now?