1085512 - Improve GeckoTouchDispatcher Resampling Heuristics

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect)

Description

[Mason Chang [Inactive] [:mchang]]

Attachment: Improve Resampling Heuristics and Refactor GeckoTouchDispatcher

+++ This bug was initially created as a clone of Bug #1083530 +++

This patch improves the touch resampling algorithm a bit. First, since we can use bug 1083530 which uses mozilla::TimeStamps, we can delete some touch tracking code to get accurate time differences between touches.

We also need two improvements.
1) Vsync events could run ahead of touch events. For example, a touch event might be 20ms behind the last vsync event. In these cases, just dispatch the last touch event and don't resample.
2) Previously, we could resample touch events that were ahead of vsync events. For example, a vsync could occur at time t=16 ms, a touch event at time t=18ms, and due to the main thread latency, the vsync event would process at time t=19ms. We would resample the touch event that occurred AFTER vsync, which is a bug. We fix this to only resample touch events that occur before the vsync event.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: Improve Resampling Heuristics and Refactor GeckoTouchDispatcher

Rebased on master and latest patch from bug 1083530.

Comment 2

[Michael Wu [:mwu]]

Quick note - patches for review should show the function in each chunk. See https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F for recommended .hgrc settings. In particular, see the diff section. I don't insist on 8 lines of context, though I appreciate it for more complicated patches.

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: Improve Resampling Heuristics and Refactor GeckoTouchDispatcher

Proper diff with functions and more context.

Comment 4

[Mason Chang [Inactive] [:mchang]]

Attachment: Improve Resampling Heuristics

Rebased off bug 1094525. Introduces two heuristics:

1) We only resample the two touch events at or before the vsync time. Before, we could get touch events after the vsync event. By the time the main thread processed the vsync event, a touch event AFTER the vsync event would be resampled. 
2) Check to see if a vsync event is far ahead of touch events. This can happen if the android touch thread hasn't added a touch in a while. In this case, we just dispatch the last touch event and clear out the touch queue.

Comment 5

[Michael Wu [:mwu]]

Comment on attachment 8518461 [details] [diff] [review]
Improve Resampling Heuristics

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

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +285,5 @@
>   */
> +bool
> +GeckoTouchDispatcher::GetTouchEvents(MultiTouchInput& aBaseTouch,
> +                                     MultiTouchInput& aCurrentTouch,
> +                                     TimeStamp aVsyncTime)

I don't understand why this is here. Don't we always want to grab the last two events? If the vsync time runs behind the base touch time, we can choose not to resample. It shouldn't take a function like this to do that. Maybe this made more sense when there was a difference between interpolation and extrapolation?

Comment 6

[Mason Chang [Inactive] [:mchang]]

Attachment: Spreadsheet with Layer Movements

(In reply to Michael Wu [:mwu] from comment #5)
> Comment on attachment 8518461 [details] [diff] [review]
> Improve Resampling Heuristics
> 
> Review of attachment 8518461 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/GeckoTouchDispatcher.cpp
> @@ +285,5 @@
> >   */
> > +bool
> > +GeckoTouchDispatcher::GetTouchEvents(MultiTouchInput& aBaseTouch,
> > +                                     MultiTouchInput& aCurrentTouch,
> > +                                     TimeStamp aVsyncTime)
> 
> I don't understand why this is here. Don't we always want to grab the last
> two events? If the vsync time runs behind the base touch time, we can choose
> not to resample. It shouldn't take a function like this to do that. Maybe
> this made more sense when there was a difference between interpolation and
> extrapolation?

Attached is a spreadsheet detailing why we need this. In some cases, the time between a vsync notification and the main thread processing the vsync can be a couple of milliseconds. Between the vsync notification and the main thread processing the touch event, a touch event can occur. Thus, getting the latest touch event will actually be processing a touch event AFTER the vsync event. See row 4 in the spreadsheet.

On a flame, touch events come in at 13 ms. Vsyncs every 16.66 ms. The spreadsheet assumes 1 pixel per millisecond movement. Let's say for example at vsync t=49, we have a main thread delay that executes at t=53, a touch event at t=52 can occur. When we resample, if we resample the last two touch events (touch t=39, t=52), we have a layer displacement of 29 pixels, when we really want to move only 16.6 pixels. This is too far. If we resample the last two touch events before the vsync (touch t=26, t=39), we resample to the correct 16.6 px. Does that explain it?

Comment 7

[Mason Chang [Inactive] [:mchang]]

Nevermind, I had a bug in the spreedsheet, I think you're right from comment 5. I'm going to take another look at this.

Comment 8

[Mason Chang [Inactive] [:mchang]]

Attachment: Improve Resampling Heuristics

Improves touch resampling heuristics by adding one more: Don't let the vsync events get too far ahead of the touch events. If the touch input thread is lagging and we get a vsync event, just send the last touch event. Also cleans up the code a bit.

Comment 9

[Michael Wu [:mwu]]

Comment on attachment 8521015 [details] [diff] [review]
Improve Resampling Heuristics

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

Getting rid of mTouchTimeDiff and mLastTouchTime is nice.

Comment 10

[Mason Chang [Inactive] [:mchang]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6859aa8f2054

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c52d4787634a

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c52d4787634a