970751 - Deliver input events more consistently per composite to make scrolling smoother

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect, P1)

Description

[jeremy.kim.leo [:jeremykimleo]]

pre-condition : contact has dummy data(500)
----------------------------------------------------
contact's drag            FPS      Unifomity(ms/s)
----------------------------------------------------
nexus-4 on android(JB):  60.57        8.15
nexus-4 on ffos(v1.4) :  60.47       16.10(*)  
----------------------------------------------------

FPS looks good.
but, when we compared it with android, the Unifomity of FirefoxOS(*) is 2-times lower than android. so, Android looks more smoother and more responsive than FirefoxOS

Investigate root cause on FFOS, and improve them.

Comment 1

[Andreas Gal :gal]

Can you define uniformity? On a display with 60 hz refresh rate, how can we achieve 60.47 fps?

Comment 2

[jeremy.kim.leo [:jeremykimleo]]

(In reply to Andreas Gal :gal from comment #1)
> Can you define uniformity? 
:: in generally, I think that the ideal should be the same level as the android because of same hardware.

> On a display with 60 hz refresh rate, how can we achieve 60.47 fps?
:: yes right. the maximum of fps is 60fps on 60hz refresh rate.
+0.47 means a tolerance of measure equipment. the value above 60fps is meaningless.

Comment 3

[Andreas Gal :gal]

How do you measure uniformity?

Comment 4

[jeremy.kim.leo [:jeremykimleo]]

the uniformity can measure and analyse by high speed camera. 
the uniformity is the standard deviation of average which is moving of all frames.
if the moving of all frames is totally equal, the uniformity will be 0mm/sec because there has no deviation. 

for examples, the height of each bars 4~5 characters. the average is 4.5 and very small the standard deviation.

        |   | | |   |
      | | | | | | | | | |
      | | | | | | | | | |
      | | | | | | | | | |
      | | | | | | | | | |
--------------------------------
      0 1 2 3 4 5 6 7 8 9   
      
on the contrary, in case ffos, the height of each bar has 2~5 characters, 
the height of bar which is the moving of each frame is irregular.

        |   |   |   |   |
        |   |   |   |   |
        | | |   | | | | |
      | | | | | | | | | |
      | | | | | | | | | |
--------------------------------
      0 1 2 3 4 5 6 7 8 9

Comment 5

[Andreas Gal :gal]

I am surprised by this result. The buffer swap is blocking on the refresh rate, so the buffer swap should happen exactly every 16.6ms. We hit 60fps, so we are clearly managing to deliver a new frame for each buffer swap. Delivering it early shouldn't matter--the driver should wait until the refresh is ready. We know we aren't delivering late, because otherwise we would drop under 60fps since the driver will wait for the next sync interval. So I guess I don't understand your results here.

Comment 6

[Jinho Hwang [:jeffhwang]]

Let me summarize it. 
First of all, it's all about frame movement happened by touch move events.
And I explain a little bit more details about the test conditions which makes the result.

Test conditions. 
  1. A machine finger moves on a touch screen with a constant speed
  2. Track the movement of an object displayed on the screen
  3. Calculate the standard deviation of average speed for the object.


The reason why the result shows non-uniform graph is that..
Now a days, most of the touch devices has 100Hz scan rate or over.
since compositor do buffer swap every 16.6ms according to screen refresh rate as 60Hz,
probably, we are getting one or two touch move events each 16.6ms.
This makes the non-uniform graph on the result.

Assume that, the device has a touchscreen with 100Hz scan rate and we move a finger 10mm every 10ms. 
and we have an object displayed on screen and move it as long as touch move event comes.
              
In this case, the object on screen can move 10mm at once screen refresh but some times 20mm 
because of the difference between the touch scan rate and the screen refresh rate.
So, the standard deviation of average speed for the object is not uniform.

Comment 7

[jeremy.kim.leo [:jeremykimleo]]

yes right, 
the buffer swap will be happened on every 16.6ms. so, it shows 60fps.

but, as my understandings, the change between each frame determined by touch events.
the almost of touch driver will do as 100hz(this means b2g hits touch event every 10ms when dragging)

when the buffer swap occurs every 16.6ms, 
at this time, the touch events can be 1 or 2 events because of doing 100hz.
(also, if the painting to fill to frame buffer is delayed, the EventQueue of b2g can be stacked more events)

if the painting triggered by 1 events, it will be displayed as much as 1 events.

but, If b2g has more than one event, those events will be merged with last one by this function.
(also, if the painting to fill to frame buffer is delayed, the EventQueue of b2g can be stacked more events)
in nsAppShell.cpp
void GeckoInputDispatcher::notifyMotion(const NotifyMotionArgs* args)
{
  {
        MutexAutoLock lock(mQueueLock);
        if (!mEventQueue.empty() &&
             mEventQueue.back().type == UserInputData::MOTION_DATA &&
           ((mEventQueue.back().action & AMOTION_EVENT_ACTION_MASK) ==
             AMOTION_EVENT_ACTION_MOVE ||
            (mEventQueue.back().action & AMOTION_EVENT_ACTION_MASK) ==
             AMOTION_EVENT_ACTION_HOVER_MOVE))
            mEventQueue.back() = data; <-------------- not push, mEventQueue only has last one.
        else
            mEventQueue.push(data);
    }
}

and, at this time, the painting will be presented as much as all merged events
so, finally, the change by merged events is bigger than one event. and, 
i think this is making a irregular uniformity every frames.

Comment 8

[Andreas Gal :gal]

I see. Are the touch pad events that we pipe through libui relative offset? In that case instead of simply overwriting the last queued value we should accumulate the x/y offsets in that value. Correct?

Comment 9

[Michael Wu [:mwu]]

The events should be in absolute coordinates.

Bug 930939 tracks some work being done to move input event handling to a separate thread to minimize lag. This would also minimize event dropping due to contention on the main thread.

Comment 10

[Andreas Gal :gal]

Michael, if the coordinates are absolute, help me understand how dropping an event can create an non-uniformity in scroll behavior. I guess I still don't understand what exactly the bug here is measuring then.

Comment 11

[Peter Chang[:pchang]]

(In reply to Andreas Gal :gal from comment #10)
> Michael, if the coordinates are absolute, help me understand how dropping an
> event can create an non-uniformity in scroll behavior. I guess I still don't
> understand what exactly the bug here is measuring then.

Is it related to vsync?
I think Android also tried to merge the touch events but the rendering of touch event is aligned with vsync. Therefore, I assume they could get the small uniformity.

Comment 12

[Vincent Lin[:vilin]]

I think refresh driver is more critical in this Bug, but not vsync.

Simplify the Comment 7..
Tick(16ms) is gonna to refresh x movement while Tick(32ms) is gonna to refresh 3x movement.
Which makes the bad uniformity of movement periodically.
This scenario is because of the difference of refresh rate and T-Evt rate.

Tick (ms)  0         16          32        48           64
T-Evt(ms)  0     10     20     30     40     50     60
movement   0      x     2x     3x     4x     5x     6x

Maybe it has to interpolate the coordinates of T-Evts while merging them.
Then T-Evt transmission has to be aware of refresh rate(refresh driver in B2G).
I heard Android's Choreographer module does similar mechanism(merge and interpolation), but it actually leveraged vsync.

Bug 930939 is another core problem as Michael mentioned.
I will randomly block the event transmission and make events arrive content process irregularly.

Comment 13

[jeremy.kim.leo [:jeremykimleo]]

(In reply to Vincent Lin[:vilin] from comment #12)

Thank you for the great simplified summery. :)

> I think refresh driver is more critical in this Bug, but not vsync.
> Maybe it has to interpolate the coordinates of T-Evts while merging them.

  Actually, the bottleneck is not the merging events mentioned on comment #7.
  GeckoInputDispatcher dispatches every events comes from input reader in 10ms as the scan rate.
  
> Then T-Evt transmission has to be aware of refresh rate(refresh driver in
> B2G).
> I heard Android's Choreographer module does similar mechanism(merge and
> interpolation), but it actually leveraged vsync.

As I understand Choreographer do dispatch pending T-events just before the UI thread do drawing a new frame.
The InputConsumer hold pending T-events and do the interpolation for the coordinate of T-events 
when it needs to dispatch.
It is leveraged vsync yes..

> I will randomly block the event transmission and make events arrive content
> process irregularly.

We did a experimental test with dispatching events every 16ms(in a separate thread) and interpolating(actually extrapolating) coordinate of pending events.
We got better(small) uniformity.

We guess the best way to get more small uniformity on B2G is applying V-sync and done every painting and compositing in a proper time(the screen refresh rate.. before swap buffer happened on mdp).

Comment 14

[Vincent Lin[:vilin]]

(In reply to jeremy.kim.leo from comment #13)
> (In reply to Vincent Lin[:vilin] from comment #12)
> 
> Thank you for the great simplified summery. :)
> 
> > I think refresh driver is more critical in this Bug, but not vsync.
> > Maybe it has to interpolate the coordinates of T-Evts while merging them.
> 
>   Actually, the bottleneck is not the merging events mentioned on comment #7.
>   GeckoInputDispatcher dispatches every events comes from input reader in
> 10ms as the scan rate.

Anyway, at the moment refresh driver tick comes, only the effect of the latest event will be sent to compositor.


>   
> > Then T-Evt transmission has to be aware of refresh rate(refresh driver in
> > B2G).
> > I heard Android's Choreographer module does similar mechanism(merge and
> > interpolation), but it actually leveraged vsync.
> 
> As I understand Choreographer do dispatch pending T-events just before the
> UI thread do drawing a new frame.
> The InputConsumer hold pending T-events and do the interpolation for the
> coordinate of T-events 
> when it needs to dispatch.
> It is leveraged vsync yes..
> 
> > I will randomly block the event transmission and make events arrive content
> > process irregularly.
> 
> We did a experimental test with dispatching events every 16ms(in a separate
> thread) and interpolating(actually extrapolating) coordinate of pending
> events.
> We got better(small) uniformity.

A separate thread means it's Not B2G main thread ?
Can you attach the patch ?

> 
> We guess the best way to get more small uniformity on B2G is applying V-sync
> and done every painting and compositing in a proper time(the screen refresh
> rate.. before swap buffer happened on mdp).

Comment 15

[Andre Graziani (:graziani)]

Attachment: Test code to improve uniformity

(In reply to Vincent Lin[:vilin] from comment #14)
> 
> Can you attach the patch ?

Follow the patch attached.
This is just a test code, sorry for the mess... I didn't expect to share it.

Comment 16

[Vincent Lin[:vilin]]

(In reply to andre.graziani from comment #15)
> Created attachment 8375253 [details] [diff] [review]
> Test code to improve uniformity
> 
> (In reply to Vincent Lin[:vilin] from comment #14)
> > 
> > Can you attach the patch ?
> 
> Follow the patch attached.
> This is just a test code, sorry for the mess... I didn't expect to share it.

Even you create a dedicated thread for input dispatcher.
Finally, this patch still dispatch a runnable to main thread. It's not what Bug 930939 wanna fix and I think this part of the patch won't gain anything. The better uniformity may be contributed by the interpolation.

Comment 17

[Andre Graziani (:graziani)]

Attachment: Test code to improve uniformity v2

In the test code attached (v2), we implemented an interpolation algorithm using a timer.
The timer is responsible for dispatching touch events at a rate that matches the layer frame rate. 

Touch move events that are accumulated between two dispatches are interpolated.
Multi touch cases were also considered.

Vincent, does it make sense for you to interpolate touch move events before we dispatch them to window?

With bug 930939, the interpolation could be done on the new thread. But for now, we are using the timer for that.

Comment 18

[Andre Graziani (:graziani)]

Attachment: ScreenShot006.png

This is the result of the test code v2 compared with the original source.
It was obtained using scroll-graph with an orangutan script performing a drag on Contacts list with constant speed.

It represents how much the layer changed at each refresh.

Because the drag is constant, we would expect also a constant delta at each refresh.

Original code shows what we’ve discussed before (sometimes displaces 2x than others).

The red one (interpolation) provides a better uniformity. The last last sample's high value is probably caused by accumulated events in the main thread.

We are currently talking to Mozilla QA to discuss how to express the uniformity in a number. But for now this visualization with scroll graph can give us an idea.

Comment 19

[Andreas Gal :gal]

We should consider taking this for 1.4. Please triage.

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8383545 [details] [diff] [review]
Test code to improve uniformity v2

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

::: widget/gonk/nsAppShell.cpp
@@ +208,5 @@
> +    case AMOTION_EVENT_ACTION_UP:
> +        msg = NS_MOUSE_BUTTON_UP;
> +        break;
> +    }
> +

We're guaranteed not to hit the default: in the switch?

Comment 21

[Jinho Hwang [:jeffhwang]]

(In reply to Milan Sreckovic [:milan] from comment #20)
> We're guaranteed not to hit the default: in the switch?

Thank you for review.
This switch was moved from dispatchOnce(), the caller, to the inside of sendMouseEvent().
( http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nsAppShell.cpp#674.)

So, we think it's okay.
But we can also put default: case with return there to make it clear.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

I have some local patches to make the compositor thread run at a real-time priority.  It seems to improve the uniformity of compositions in my profiles.  I'm working on cleaning up the patches now.  I'm doing this as part of bug 977975 work.

Adding a ni? to myself to link the final patches here to see if they help with this test.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Renaming this bug to more accurately describe the goal here. Please feel free to fix it if I misunderstood.

Comment 24

[Jason Smith [:jsmith]]

Going to wait for Ben's input before triaging this.

Comment 25

[Preeti Raghunath(:Preeti)]

Awaiting land of bug 977975 and if the fix doesn't fix this issue, we'll block on bug 970751 as well.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

If I understand correctly, bug 980241 is also directly relevant to this problem.

Comment 27

[Vincent Lin[:vilin]]

(In reply to Andre Graziani (:graziani) from comment #17)
> Created attachment 8383545 [details] [diff] [review]
> Test code to improve uniformity v2
> 
> In the test code attached (v2), we implemented an interpolation algorithm
> using a timer.
> The timer is responsible for dispatching touch events at a rate that matches
> the layer frame rate. 
> 
> Touch move events that are accumulated between two dispatches are
> interpolated.
> Multi touch cases were also considered.
> 
> Vincent, does it make sense for you to interpolate touch move events before
> we dispatch them to window?
> 
> With bug 930939, the interpolation could be done on the new thread. But for
> now, we are using the timer for that.

I'm not sure if the interpolation algorithm is the best, but instead of firing a Timer I would prefer to let it align with Vsync. (bug 980241)

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Jeremy, can you try with the patches from bug 980027 to see if running the compositor with RT priority helps your test case?  You may also want to modify b2g/app/b2g.js to adjust the priority to different values to see their impact.  The prefs, added by those patches, are:

  pref("hal.gonk.compositor.nice", -8);
  pref("hal.gonk.compositor.rt_priority", 0);

Comment 29

[jeremy.kim.leo [:jeremykimleo]]

(In reply to Ben Kelly [:bkelly] from comment #28)
> Jeremy, can you try with the patches from bug 980027 to see if running the
> compositor with RT priority helps your test case?  You may also want to
> modify b2g/app/b2g.js to adjust the priority to different values to see
> their impact.  The prefs, added by those patches, are:
> 
>   pref("hal.gonk.compositor.nice", -8);
>   pref("hal.gonk.compositor.rt_priority", 0);

thanks bkelly. 
i already is doing and will share result ASAP

Comment 30

[jeremy.kim.leo [:jeremykimleo]]

Attachment: result-with-bug980027.png

this is result with Bug 980027 on Nexus4

                   Original     With Bug 980027
Num of Refreshes      27               29

what I see is that the patch effects to the number of refresh (increasing 2 refreshes). 
It’s good and we can expect more FPS from reducing compositing time delay.

But, the result of uniformity is no change. it still bad.
in addtionally, i didn't changes of preference value on b2g.js

Comment 31

[Jinho Hwang [:jeffhwang]]

(In reply to Vincent Lin[:vilin] from comment #27)
> I'm not sure if the interpolation algorithm is the best, but instead of
> firing a Timer I would prefer to let it align with Vsync. (bug 980241)

Good to know that you are working on Vsync. :)
I agree with u, when we have Vsync for B2G, we don't need the Timer anymore.
Interpolating events with Vsync is what we want from the beginning here 
to improve the uniformity.

Comment 32

[Vincent Lin[:vilin]]

Attachment: resampling.png

The chart is logged to show the improvement of resample.

x: timestamp in ms
y: horizontal position

Blue dot: Event notification in inputReader thread(priority:112)
Red dot: Event fetch in main thread(priority:120)
Yellow dot: w/o resample
Green dot: w/t resample

After resampling, the movement of position looks smoother.

The Blue one and Red one are supposed to be very close i.e. notification and fetch should be paired ideally. But around timestamp 1957176, we can see main thread can't fetch event in time. There's chance that 2 events or more were notified before being fetched by main thread. Instead of overwriting the existing queued data in our current code, my WIP is gonna to store it for later  resampling with vsync timestamp.

http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nsAppShell.cpp#828

Comment 33

[Vincent Lin[:vilin]]

Attachment: bug-970751-fix.patch

WIP~
Just rebased to newer version.
Gonna do what was mentioned in Comment 32.

Comment 34

[Vincent Lin[:vilin]]

Attachment: resample.png

Comment 35

[Vincent Lin[:vilin]]

Let this bug for touch event resampling only. Decouple this bug from vsync. We have bug 991483 to associate with vsync instead.

Comment 36

[Vincent Lin[:vilin]]

Attachment: bug-970751-fix.patch

WIP~
Register with info. of observer type.

Comment 37

[Vincent Lin[:vilin]]

Attachment: bug-970751-fix.patch

make notify() off-main-threading. The WIP just dispatches it to main thread and return to the caller(VsyncObserverManager) so the caller can notify the next observer(Compositor). To ensure the most up-to-date APZC info affects the following composition. This bug must make sure touch event is sent before return  to the caller in Notify() function.

http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nsAppShell.cpp#273

Due to we have Apps running in B2G process, it's better to return to caller as capturer->TryCapture() is called here(where APZC is done).
http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nsWindow.cpp#259
Or DispatchEvent() will cost several ms here which will make notification to the following observers delayed.
http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nsWindow.cpp#269

Comment 38

[u459114]

Vincent,
You did two things in this patch
1. Touch Normalization 
2. Align input dispatch with vsync

I suggest, at last, divide this two things into different patches. Each patch works on one thing, it's easier for reviewer to understand your design concept.

Meanwhile, I have a question here
In this prototype, you create a new event process flow. After receive vsync notification, sent out touch event in VsyncEventRunnable::Run, which broadcast touch event to TabParent/ AsyncPanZoomController
Where you block original Event Process flow?

Comment 39

[Vincent Lin[:vilin]]

Attachment: bug-970751-fix.patch

Include "gfx.silk-enable" preference.

Comment 40

[Mason Chang [Inactive] [:mchang]]

From conference call with LG / FxOS Perf team - Jeff can no longer work on this. Vincent, since you've had the last bug, can you take this bug please? Thanks

Comment 41

[Vincent Lin[:vilin]]

@Jerry,

Please update this bug with your VsyncDispatcher protocol.

Comment 42

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Please see bug 987529, we have a WIP including this input part patch.
Maybe Jeremy counld test the unifomity again as comment 0.

I will split the patch in bug 987529 and update to this bug.

Comment 43

[Jinho Hwang [:jeffhwang]]

Hi Jerry,
Sorry for the late reply.
I'll test the uniformity again with patches in bug 987529.
It will take some time. I'll let you know when I have the result.
Thanks.

Comment 44

[Jinho Hwang [:jeffhwang]]

Hi Jerry,

The test results are attached at 
https://bugzilla.mozilla.org/show_bug.cgi?id=987529#c88

Thanks.

Comment 45

[Mason Chang [Inactive] [:mchang]]

Hi Jerry, I wanted to land this bug outside of project silk and outside of 987529. Are you ok with that? Thanks!

Comment 46

[Mason Chang [Inactive] [:mchang]]

Hi Jerry, sorry, from comment 45, are you ok with me landing this outside of silk? Thanks!

Comment 47

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Yes, we can do the input resampling without silk.
We should use the uniformity checking patch to prove that we get the better result. :)
Thanks, Mason.

Comment 48

[Mason Chang [Inactive] [:mchang]]

Attachment: interpolator.patch

Adds a GeckoTouchDispatcher where we resample. However, we can't resample anything because we always process touch events as soon as they come in, thus we depend on silk's vsync functionality. I talked with Jerry. He will pull out the vsync dispatch functionality from silk and land that before silk, then we can land this before silk as well.

Comment 49

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Extract Touch Event Process into GeckoTouchDispatcher

This portion just extracts the functionality out of nsAppShell.cpp into a GeckoTouchDispatcher.cpp file. From comment https://bugzilla.mozilla.org/show_bug.cgi?id=1027263#c4, this is just laying the groundwork to do the touch interpolation. There is no additional functionality in this patch, just refactoring.

Comment 50

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Enable GeckoTouchDispatcher to dispatch events on vsync

This portion relies on the vsync patches from bug 987529. This reworks nsAppShell.cpp and GeckoTouchDispatcher to process touch events when we receive the vsync signal.

Comment 51

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 3: Interpolate Touch Events

Part 3 does the actual touch interpolation and resampling. It takes the last two touch events in the queue and generates a resampled touch event that is actually sent through Gecko to process.

Comment 52

[Michael Wu [:mwu]]

Comment on attachment 8465805 [details] [diff] [review]
Part 1: Extract Touch Event Process into GeckoTouchDispatcher

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

::: widget/gonk/GeckoTouchDispatcher.h
@@ +17,5 @@
> +
> +#ifndef GECKO_TOUCH_INPUT_DISPATCHER_h
> +#define GECKO_TOUCH_INPUT_DISPATCHER_h
> +
> +using namespace android;

"using namespace" is not permitted in headers.

@@ +34,5 @@
> +} // end mozilla namespace
> +
> +using namespace mozilla;
> +
> +class GeckoTouchDispatcher : public virtual RefBase

I think you should use the mfbt refcounting rather than Android's.

@@ +38,5 @@
> +class GeckoTouchDispatcher : public virtual RefBase
> +{
> +public:
> +    GeckoTouchDispatcher();
> +    void SendTouchEvent(UserInputData& aData);

This code should define its own class/struct for the data it actually cares about, and then UserInputData can be restructured to use that instead. Then UserInputData doesn't need to be pulled out to nsAppShell.h.

Comment 53

[Mason Chang [Inactive] [:mchang]]

Attachment: Frame Uniformity on Master

Attached are frame uniformity measurements on the vertical homescreen on a nexus 4. This is from master. These results can be pasted here - http://people.mozilla.org/~mchang/FrameUniformity.html

Comment 54

[Mason Chang [Inactive] [:mchang]]

Attachment: Frame Uniformity with the interpolator

Results for frame uniformity with just touch interpolation.

Comment 55

[Mason Chang [Inactive] [:mchang]]

Attachment: Frame Uniformity Results with Silk

Frame Uniformity results with an alpha silk implementation.

Comment 56

[Mason Chang [Inactive] [:mchang]]

After talking with Michael, we would like to get rid of UserInputData for touch events alltogether. Instead, we should directly convert gonk touch data into MultiTouchInput and pass that through to GeckoTouchDispatcher. In GeckoTouchDispatcher, we can convert MultiTouchInput -> WidgetTouchInput for the parent process to handle.

Comment 57

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Enable MultiTouchInput to translate to WidgetMouseEvent and WidgetTouchEvent

From comment 56, these sets of patches directly translate gonk NotifyMotionArgs* to MultiTouchInput data instead of UserInputdata. This first set of patches enables support to translate MultiTouchInput into WidgetTouchInput and WidgetMouseInput, which is what we currently translate UserInputData into when we dispatch touch events through gecko.

Comment 58

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Translate NotifyMotionArgs to MultiTouchInput

Here we change nsAppShell::notifyMotion to convert NotifyMotionArgs into MultiTouchInput. I also clean up some of the sendTouchEvent and sendMouseEvent methods and remove them from nsAppShell and into GeckoTouchDispatcher in another part of this patch. With this patch, we no longer dispatchOnce when we get an event. Instead, when notifyVsync is called (which is added in another patch), we interpolate and dispatch the resampled event.

Comment 59

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 3: Create GeckoTouchDispatcher

Part 3 includes the removed functionality from part2 in nsAppShell to dispatch touch events. Since GeckoTouchDispatcher::SendTouchEvents is called on the vsync thread, we repost the event to the main thread. Then we convert the MultiTouchInput into WidgetTouchInput, and if required, dispatch a WidgetMouseEvent. We also add some locks to ensure thread safety. These locks will be deleted once bug 930939 lands, as we can then dispatch the touch event directly to APZ on the vsync thread. However, WidgetInputEvents must be dispatched on the main thread because they touch the DOM.

Comment 60

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 4: Resample Touch Events and hook into Vsync

Here, when we get a touch event from gonk, we add the event to a queue. When vsync occurs in nsAppShell, we add the queued touch events to the interpolator. Finally, in GeckoTouchDispatcher, we resample the last two touch events and dispatch the touch event through gecko. This part includes the modifications to nsAppShell to listen to vsync.

Comment 61

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 5: Modifications to Vsync Dispatcher

Just some modifications to the vsync dispatcher in bug 987529. Note, all 5 patches were built on the patches in bug 987529, so just applying these patches to master will not build. Just trying to get the reviews in so when bug 987529 does land, this can land as well.

Comment 62

[Michael Wu [:mwu]]

Comment on attachment 8467280 [details] [diff] [review]
Part 2: Translate NotifyMotionArgs to MultiTouchInput

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

There's a lot of code shuffling in here that doesn't appear to be relevant. I don't mind cleanups, but please keep cleanups strictly to the code you actually need to touch. If you want to do other cleanups, put them in another patch. Unrelated cleanups make it more difficult to actually understand what you're doing.

::: widget/gonk/nsAppShell.cpp
@@ +71,4 @@
>  
>  #include "mozilla/Preferences.h"
>  #include "GeckoProfiler.h"
> +#include "InputData.h"              // For Multi Touch

I don't think the comment for this header provides much value.

::: widget/gonk/nsAppShell.h
@@ +32,4 @@
>  void DispatchPendingEvent();
>  bool ProcessNextEvent();
>  void NotifyEvent();
> +} // end namespace mozilla

If you want to do this, do:

// namespace mozilla

Comment 63

[Michael Wu [:mwu]]

Comment on attachment 8467279 [details] [diff] [review]
Part 1: Enable MultiTouchInput to translate to WidgetMouseEvent and WidgetTouchEvent

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

::: widget/InputData.h
@@ +198,5 @@
> +  void ConvertToWidgetTouchEvent(WidgetTouchEvent& aWidgetTouchEvent) const;
> +
> +
> +  #ifdef MOZ_WIDGET_GONK
> +  // Need this because we sometimes send mouse key events to certain gaia apps.

We might have b2g generate mouse events at a higher layer. People were looking at removing mouse event generation code entirely.

What is a mouse key event?

::: widget/xpwidgets/InputData.cpp
@@ +11,4 @@
>  #include "mozilla/MouseEvents.h"
>  #include "mozilla/TouchEvents.h"
>  
> +#ifdef MOZ_WIDGET_GONK

These changes seem like they do not belong in xpwidgets if they need ifdef MOZ_WIDGET_GONK. xpwidgets is for cross platform widget stuff that is useful for all widget backends.

Comment 64

[Michael Wu [:mwu]]

Comment on attachment 8467283 [details] [diff] [review]
Part 3: Create GeckoTouchDispatcher

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

This should've been combined with the previous patch. It's difficult to understand the previous code without seeing where the code is moving to. There's also a mix of two space and four space indent, which should never happen within the same file.

Comment 65

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Enable MultiTouchInput to translate to WidgetMouseEvent and WidgetTouchEvent

(In reply to Michael Wu [:mwu] from comment #63)
> Comment on attachment 8467279 [details] [diff] [review]
> Part 1: Enable MultiTouchInput to translate to WidgetMouseEvent and
> WidgetTouchEvent
> 
> Review of attachment 8467279 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/InputData.h
> @@ +198,5 @@
> > +  void ConvertToWidgetTouchEvent(WidgetTouchEvent& aWidgetTouchEvent) const;
> > +
> > +
> > +  #ifdef MOZ_WIDGET_GONK
> > +  // Need this because we sometimes send mouse key events to certain gaia apps.
> 
> We might have b2g generate mouse events at a higher layer. People were
> looking at removing mouse event generation code entirely.
> 

Do we have a bug for that? Otherwise, lots of things break if we don't dispatch the mouse event.

> What is a mouse key event?
> 

Changed the comments to better reflect what's actually going on, just mouse events. I think I named them mouse key events because I saw notifyKey() and never changed the comments during the WIP.

> ::: widget/xpwidgets/InputData.cpp
> @@ +11,4 @@
> >  #include "mozilla/MouseEvents.h"
> >  #include "mozilla/TouchEvents.h"
> >  
> > +#ifdef MOZ_WIDGET_GONK
> 
> These changes seem like they do not belong in xpwidgets if they need ifdef
> MOZ_WIDGET_GONK. xpwidgets is for cross platform widget stuff that is useful
> for all widget backends.

Moved to widget/gonk/InputData.cpp

Comment 66

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Clean notifyMotion and dispatch touch event code from nsAppShell

From comment 62, splitting the patch into two patches. One that cleans up nsAppShell, and the other that adds vsync dispatch and the logic to translate notifyMotionArgs to a MultiTouchInput. This part cleans up nsAppShell.

Comment 67

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 3: Convert NotifyMotionArgs and dispatch MultiTouchInput

Also from comment 64, combined the patches that notifyMotion in nsAppShell with the logic in GeckoTouchDispatcher.

Comment 68

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 4: Resample Touch Events

Part 4: The logic in GeckoTouchDispatcher to resample touch events.

Comment 69

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 5: Modifications to Vsync Dispatcher

Just some changes to dispatch input events from the vsync dispatch framework. No need to review. From https://bugzilla.mozilla.org/show_bug.cgi?id=987527#c55, this bug and bug 930939 will require the vsync dispatch code.

Comment 70

[Michael Wu [:mwu]]

Comment on attachment 8467976 [details] [diff] [review]
Part 2: Clean notifyMotion and dispatch touch event code from nsAppShell

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

It looks like parts of this cleans up the mess left behind from bug 987529 part 8. That's not gonna pass review, so don't base this patch on that.

This patch doesn't make sense by itself. It removes a lot of functionality rather than being cleanup focused.

::: widget/gonk/nsAppShell.cpp
@@ -839,5 @@
>      }
>      case UserInputData::KEY_DATA: {
>          if (!mKeyDownCount) {
>              // No pending events, the filter state can be updated.
> -            mKeyEventsFiltered = isExpired(data);

We need a replacement for expired key event filtering if you're removing this line.

Comment 71

[Michael Wu [:mwu]]

Comment on attachment 8467979 [details] [diff] [review]
Part 3: Convert NotifyMotionArgs and dispatch MultiTouchInput

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

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +1,3 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> +/* vim: set ts=2 sw=2 sts=2 tw=80 et: */
> +/* Copyright 2012 Mozilla Foundation and Mozilla contributors

Wrong year.

@@ +41,5 @@
> +#include "mozilla/dom/Touch.h"
> +#include "nsGkAtoms.h"
> +#include "nsScreenManagerGonk.h"
> +#include "nsThreadUtils.h"
> +#include "nsWindow.h"

There are way too many unnecessary includes here.

@@ +50,5 @@
> +#include "InputData.h"          // For MultiTouch
> +#include "FrameMetrics.h"       // For ScrollableLayerGuid
> +
> +#define LOG(args...)                                            \
> +  __android_log_print(ANDROID_LOG_INFO, "Gonk" , ## args)

LOG defined, but never actually used.

@@ +154,5 @@
> +}
> +
> +/***
> + * Some touch events get sent as key events.
> + * We can't directly translate MultiTouchInput -> Key Events

I think this is a typo.

::: widget/gonk/GeckoTouchDispatcher.h
@@ +17,5 @@
> +
> +#ifndef GECKO_TOUCH_INPUT_DISPATCHER_h
> +#define GECKO_TOUCH_INPUT_DISPATCHER_h
> +
> +using namespace android;

"using namespace" is prohibited in headers.

::: widget/gonk/nsAppShell.cpp
@@ +1083,5 @@
>      NotifyEvent();
>  }
>  
> +/* static */
> +bool

Why was this even removed in the first place?

Comment 72

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Michael Wu [:mwu] from comment #71)
> Comment on attachment 8467979 [details] [diff] [review]
> Part 3: Convert NotifyMotionArgs and dispatch MultiTouchInput
> ::: widget/gonk/GeckoTouchDispatcher.cpp
> @@ +1,3 @@
> > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> > +/* vim: set ts=2 sw=2 sts=2 tw=80 et: */
> > +/* Copyright 2012 Mozilla Foundation and Mozilla contributors
> 
> Wrong year.

Just happened to notice the modeline is out-of-date as well.  Current preferred is here:

  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line

Comment 73

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8467979 [details] [diff] [review]
Part 3: Convert NotifyMotionArgs and dispatch MultiTouchInput

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

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +50,5 @@
> +#include "InputData.h"          // For MultiTouch
> +#include "FrameMetrics.h"       // For ScrollableLayerGuid
> +
> +#define LOG(args...)                                            \
> +  __android_log_print(ANDROID_LOG_INFO, "Gonk" , ## args)

Log is used in PrintUniformity

Comment 74

[Kartikaya Gupta (email:[e-mail])]

I stole the ConvertToWidgetTouchEvent function from the "part 1" patch here and put it on bug 1049136. I also modified it a bit so the signature looks like this:

WidgetTouchEvent ToWidgetTouchEvent(nsIWidget* aWidget) const;

There were a couple of bugs in the implementation that I fixed as well (a missing break; use of int instead of size_t). I'll ask mwu for review on it once the rest of the patches on that bug are ready but this is just a heads-up.

Also for the record I would prefer having a parallel ToWidgetMouseEvent function live on InputData and be available on all widget backends. I will need one for Fennec, unless we move that code deeper into Gecko (nsPresShell/nsEventStateManager somewhere).

Comment 75

[Mason Chang [Inactive] [:mchang]]

Attachment: Interpolate Touch Events

One patch after talking with :mwu as it may be easier to review. The overall architecture is that we remove all the dispatchOnce(), sendMouseEvent and sendTouchEVent from nsAppShell. We translate NotifyMotionArgs* in GeckoInputDispatcher::notifyMotion into MultiTouchInput. We no longer use UserInputData anywhere.

I still have to document why we chose the touch interpolation algorithm implemented here, which is quite long. I'm still writing that up and will add that to the comments in GeckoTouchDispatcher::Resample when done. 

Every vsync, we add the touch events into GeckoTouchDispatcher::AddTouchToInterpolator. Then, we resample touch events in GeckoTouchDispatcher to create smoother scrolling. Finally, we dispatch the touch event in GeckoTouchDispatcher > nsWindow. If need be, we also dispatch the mouse event.

Comment 76

[Mason Chang [Inactive] [:mchang]]

Attachment: Interpolate Touch Events

Little bit more clean up. Also, you can ignore the GonkVsyncDispatcher and HwcComposer changes. Those need to be worked out in a different bug that this work is based on.

Comment 77

[Mason Chang [Inactive] [:mchang]]

Attachment: Frame Uniformity with the interpolator

Frame Uniformity with new touch interpolation algorithm.

Comment 78

[Mason Chang [Inactive] [:mchang]]

Attachment: Interpolate Touch Events

Updated to include comments on how and why we interpolate touch events.

Comment 79

[Mason Chang [Inactive] [:mchang]]

Attachment: Interpolate Touch Events

Rebased off master. Removed MultiTouchInput::ConvertToWidgetTouchEvent since bug 1049136 landed, from comment 74. Now GeckoTouchDispatcher::DispatchTouchEvent uses MultiTouchInput::ToWidgetTouchEvent.

Comment 80

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8471636 [details] [diff] [review]
Interpolate Touch Events

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

::: widget/InputData.h
@@ +193,5 @@
>    }
>  
> +  #ifdef MOZ_WIDGET_GONK
> +  // Need this because we sometimes send mouse events to certain gaia apps.
> +  void ConvertToWidgetMouseEvent(WidgetMouseEvent& aWidgetMouseEvent) const;

I would really like to see this look like "WidgetMouseEvent ToWidgetMouseEvent(nsIWidget* aWidget) const"

Comment 81

[Mason Chang [Inactive] [:mchang]]

Attachment: Interpolate Touch Events

Implemented Comment 80. Also, I fixed a bug with resampling multiple fingers. Now we track each finger independently and resample each finger independently at each touch point.

Comment 82

[Michael Wu [:mwu]]

Comment on attachment 8473081 [details] [diff] [review]
Interpolate Touch Events

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

::: widget/InputData.h
@@ +193,5 @@
>    }
>  
> +  #ifdef MOZ_WIDGET_GONK
> +  // Used to track filtered touches
> +  int32_t mPointerIndex;

This should be unnecessary.

@@ +206,5 @@
> +   * However, once gaia stops listening to mouse events, we should be able to delete this
> +   * Blocking bug 1005815.
> +   */
> +  WidgetMouseEvent ToWidgetMouseEvent(nsIWidget* aWidget) const;
> +  int32_t mInputAction;

You shouldn't need this - touch start and end don't need to be converted to mouse moves if there's more than one touch.

::: widget/gonk/HwcComposer2D.cpp
@@ +29,4 @@
>  #include "cutils/properties.h"
>  #include "gfx2DGlue.h"
>  #include "nsAppShell.h"
> +#include "GonkVsyncDispatcher.h"

The changes in this file appear to be unrelated.

::: widget/gonk/nsAppShell.cpp
@@ -119,4 @@
>  {
>      gAppShell->NotifyNativeEvent();
>  }
> -

unnecessary change

@@ -134,4 @@
>  
>  struct Touch {
>      int32_t id;
> -    PointerCoords coords;

unnecessary change

@@ +824,5 @@
> +    int touchCount = args->pointerCount;
> +    MOZ_ASSERT(touchCount <= MAX_POINTERS);
> +    TimeStamp timestamp = TimeStamp::Now();
> +    Modifiers modifiers = getDOMModifiers(args->metaState);
> +    MultiTouchInput::MultiTouchType touchType = getMultiTouchType(action);

This is the only user of getMultiTouchType, so inline it.

@@ +848,5 @@
> +
> +    {
> +        // Don't overwrite last touch since we touch interpolate
> +        MutexAutoLock lock(mQueueLock);
> +        mTouchEventQueue.push(touchData);

Why queue events? Can we push them to GeckoTouchDispatcher directly?

Comment 83

[Mason Chang [Inactive] [:mchang]]

(In reply to Michael Wu [:mwu] from comment #82)
> Comment on attachment 8473081 [details] [diff] [review]
> Interpolate Touch Events
> 
> Review of attachment 8473081 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/InputData.h
> @@ +193,5 @@
> >    }
> >  
> > +  #ifdef MOZ_WIDGET_GONK
> > +  // Used to track filtered touches
> > +  int32_t mPointerIndex;
> 
> This should be unnecessary.
> 

We need this to filter out touch events if there is a delay in processing them. Mostly for the logic here - http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsAppShell.cpp#761

> @@ +206,5 @@
> > +   * However, once gaia stops listening to mouse events, we should be able to delete this
> > +   * Blocking bug 1005815.
> > +   */
> > +  WidgetMouseEvent ToWidgetMouseEvent(nsIWidget* aWidget) const;
> > +  int32_t mInputAction;
> 
> You shouldn't need this - touch start and end don't need to be converted to
> mouse moves if there's more than one touch.
> 

I'm not sure I understand? At the moment, we currently always send a mouse event if APZ didn't capture the touch event (http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsAppShell.cpp#797). Touch starts are converted to mouse up/down so we still have to translate MultiTouchInput -> WidgetMouseEvent. Can you please clarify?

> ::: widget/gonk/HwcComposer2D.cpp
> @@ +29,4 @@
> >  #include "cutils/properties.h"
> >  #include "gfx2DGlue.h"
> >  #include "nsAppShell.h"
> > +#include "GonkVsyncDispatcher.h"
> 
> The changes in this file appear to be unrelated.
> 
> ::: widget/gonk/nsAppShell.cpp
> @@ -119,4 @@
> >  {
> >      gAppShell->NotifyNativeEvent();
> >  }
> > -
> 
> unnecessary change

Will fix.

> 
> @@ -134,4 @@
> >  
> >  struct Touch {
> >      int32_t id;
> > -    PointerCoords coords;
> 
> unnecessary change
> 

Will fix.
> @@ +824,5 @@
> > +    int touchCount = args->pointerCount;
> > +    MOZ_ASSERT(touchCount <= MAX_POINTERS);
> > +    TimeStamp timestamp = TimeStamp::Now();
> > +    Modifiers modifiers = getDOMModifiers(args->metaState);
> > +    MultiTouchInput::MultiTouchType touchType = getMultiTouchType(action);
> 
> This is the only user of getMultiTouchType, so inline it.
> 

I don't like having the large switch statement in the middle of the method, its a coding preference. Not a fan of having more functions?

> @@ +848,5 @@
> > +
> > +    {
> > +        // Don't overwrite last touch since we touch interpolate
> > +        MutexAutoLock lock(mQueueLock);
> > +        mTouchEventQueue.push(touchData);
> 
> Why queue events? Can we push them to GeckoTouchDispatcher directly?

We currently do a bunch of work to track if the touch event should be filtered, for example if we process it too late. I wanted to keep that logic in nsAppShell, so that's why I don't push the event directly to GeckoTouchDispatcher. If we want to move that logic into GeckoTouchDispatcher, that is fine. Thanks for the feedback.

Comment 84

[Michael Wu [:mwu]]

(In reply to Mason Chang [:mchang] from comment #83)
> (In reply to Michael Wu [:mwu] from comment #82)
> > Comment on attachment 8473081 [details] [diff] [review]
> > Interpolate Touch Events
> > 
> > Review of attachment 8473081 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: widget/InputData.h
> > @@ +193,5 @@
> > >    }
> > >  
> > > +  #ifdef MOZ_WIDGET_GONK
> > > +  // Used to track filtered touches
> > > +  int32_t mPointerIndex;
> > 
> > This should be unnecessary.
> > 
> 
> We need this to filter out touch events if there is a delay in processing
> them. Mostly for the logic here -
> http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsAppShell.cpp#761
> 

That code uses IDs now, not indexes.

> > @@ +206,5 @@
> > > +   * However, once gaia stops listening to mouse events, we should be able to delete this
> > > +   * Blocking bug 1005815.
> > > +   */
> > > +  WidgetMouseEvent ToWidgetMouseEvent(nsIWidget* aWidget) const;
> > > +  int32_t mInputAction;
> > 
> > You shouldn't need this - touch start and end don't need to be converted to
> > mouse moves if there's more than one touch.
> > 
> 
> I'm not sure I understand? At the moment, we currently always send a mouse
> event if APZ didn't capture the touch event
> (http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsAppShell.
> cpp#797). Touch starts are converted to mouse up/down so we still have to
> translate MultiTouchInput -> WidgetMouseEvent. Can you please clarify?
> 

Only the first touch needs to be converted to mouse events. If there's more than one touch point in a touch start or end, it can be ignored because we're already sending mouse events for the first touch. If that's not the current behavior, you can change it.

> > @@ +824,5 @@
> > > +    int touchCount = args->pointerCount;
> > > +    MOZ_ASSERT(touchCount <= MAX_POINTERS);
> > > +    TimeStamp timestamp = TimeStamp::Now();
> > > +    Modifiers modifiers = getDOMModifiers(args->metaState);
> > > +    MultiTouchInput::MultiTouchType touchType = getMultiTouchType(action);
> > 
> > This is the only user of getMultiTouchType, so inline it.
> > 
> 
> I don't like having the large switch statement in the middle of the method,
> its a coding preference. Not a fan of having more functions?
> 

This is unnecessary. Splitting code into more functions means one has to jump to more places to read all of the code. If there's only one caller, it should be inlined. The only case where I would split out code is if the nesting would be too deep otherwise.

> > @@ +848,5 @@
> > > +
> > > +    {
> > > +        // Don't overwrite last touch since we touch interpolate
> > > +        MutexAutoLock lock(mQueueLock);
> > > +        mTouchEventQueue.push(touchData);
> > 
> > Why queue events? Can we push them to GeckoTouchDispatcher directly?
> 
> We currently do a bunch of work to track if the touch event should be
> filtered, for example if we process it too late. I wanted to keep that logic
> in nsAppShell, so that's why I don't push the event directly to
> GeckoTouchDispatcher. If we want to move that logic into
> GeckoTouchDispatcher, that is fine. Thanks for the feedback.

Yeah, move it to GeckoTouchDispatcher. It's not particularly gonk specific, so it belongs in GeckoTouchDispatcher.

Comment 85

[Mason Chang [Inactive] [:mchang]]

(In reply to Michael Wu [:mwu] from comment #84)
> 
> > > @@ +206,5 @@
> > > > +   * However, once gaia stops listening to mouse events, we should be able to delete this
> > > > +   * Blocking bug 1005815.
> > > > +   */
> > > > +  WidgetMouseEvent ToWidgetMouseEvent(nsIWidget* aWidget) const;
> > > > +  int32_t mInputAction;
> > > 
> > > You shouldn't need this - touch start and end don't need to be converted to
> > > mouse moves if there's more than one touch.
> > > 
> > 
> > I'm not sure I understand? At the moment, we currently always send a mouse
> > event if APZ didn't capture the touch event
> > (http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsAppShell.
> > cpp#797). Touch starts are converted to mouse up/down so we still have to
> > translate MultiTouchInput -> WidgetMouseEvent. Can you please clarify?
> > 
> 
> Only the first touch needs to be converted to mouse events. If there's more
> than one touch point in a touch start or end, it can be ignored because
> we're already sending mouse events for the first touch. If that's not the
> current behavior, you can change it. 

There are cases where every touch event has to be converted into mouse events. For example, at the moment closing the rocket bar. Since no events are captured, we send the mouse events to generate a click event. Also, touch ends are not always captured and have to be converted into mouse events. So we have more than just touch starts that need to be converted :(.

Comment 86

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP - Interpolate Touch Events

Rebased off master and fixed things from comment 84. WIP because after UX review, we have to figure out how to improve velocity for small swipes. Since we perform touch interpolation, touches "feel" slower. This WIP uses the midpoint touch interpolation algorithm for the first 5 touches, then switches to last touch last sample. It feels a little better, but not quite. Something is still off.

Comment 87

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mason Chang [:mchang] from comment #85)
> > Only the first touch needs to be converted to mouse events. If there's more
> > than one touch point in a touch start or end, it can be ignored because
> > we're already sending mouse events for the first touch. If that's not the
> > current behavior, you can change it. 
> 
> There are cases where every touch event has to be converted into mouse
> events. For example, at the moment closing the rocket bar. Since no events
> are captured, we send the mouse events to generate a click event. Also,
> touch ends are not always captured and have to be converted into mouse
> events. So we have more than just touch starts that need to be converted :(.

What mwu is saying is that if the touch input has more than one touch point (i.e. it's a pinch or something, where multiple fingers are down) you don't need to send mouse inputs for it. The case you are describing with the rocketbar is a single-touch-point type of input and does need the mouse events like you describe.

Comment 88

[Mason Chang [Inactive] [:mchang]]

Attachment: Frame Uniformity with Android Touch Interpolation Algorithm

Comment 89

[Mason Chang [Inactive] [:mchang]]

Attachment: Interpolate Touch Events

Changelog:

1) Updated to use Android's touch interpolation / extrapolation algorithm.
2) Fixed most nits from comment 82.
3) Directly feed touch events on nsAppShell::NotifyMotion to GeckoTouchDispatcher.
4) Send touch start events ASAP instead of waiting for vsync. 

I will write a blog post describing Android's touch interpolation / extrapolation algorithm which should help review the code.

Comment 90

[Mason Chang [Inactive] [:mchang]]

Here is android's touch resampling algorithm - http://www.masonchang.com/blog/2014/8/25/androids-touch-resampling-algorithm

Comment 91

[Mason Chang [Inactive] [:mchang]]

Attachment: Resample Touch Events

Changeset: 

Put the android touch resampling constants behind a pref. 
Put the resampling as a pref. If it is off, we maintain the current behavior.

Comment 92

[Michael Wu [:mwu]]

Comment on attachment 8479368 [details] [diff] [review]
Resample Touch Events

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

Going to review GeckoTouchDispatcher more closely, but I figured I should post the comments I have for everything else now.

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +119,5 @@
> +bool
> +GeckoTouchDispatcher::HaveTouchData()
> +{
> +  MutexAutoLock lock(mTouchQueueLock);
> +  return  !mTouchMoveEvents.empty() ||

Too many spaces after return.

@@ +179,5 @@
> +static bool
> +isExpiredTouch(const MultiTouchInput& aTouch)
> +{
> +    uint64_t timeNowMs = systemTime(SYSTEM_TIME_MONOTONIC) / 1000000;
> +    return (timeNowMs - aTouch.mTime) > kInputExpirationThresholdMs;

Inconsistent indentation. Please check for these things before requesting review.

@@ +305,5 @@
> +bool
> +GeckoTouchDispatcher::ShouldInterpolate(TimeStamp aTimeStamp)
> +{
> +  MutexAutoLock lock(mTouchQueueLock);
> +  std::vector<MultiTouchInput>::iterator it; 

whitespace at the end of the line.

@@ +482,5 @@
> +                                        bool aInterpolate)
> +{
> +#ifdef LOG_RESAMPLE_DATA
> +  const char* interpolate = "interpolate";
> +  const char* extrapolate = "extrapolate";

This is pointless. Just inline it..

@@ +531,5 @@
> +GeckoTouchDispatcher::DispatchMouseEvent(mozilla::MultiTouchInput& aMultiTouch,
> +    bool aForwardToChildren)
> +{
> +  if (aMultiTouch.mTouches.Length() == 0) {
> +    printf_stderr("Touch Event has no touches, probably bad things will happen");

Why printf_stderr instead of LOG?

@@ +534,5 @@
> +  if (aMultiTouch.mTouches.Length() == 0) {
> +    printf_stderr("Touch Event has no touches, probably bad things will happen");
> +  }
> +
> +  nsIWidget* aWidget = nullptr;

inline.

::: widget/gonk/InputData.cpp
@@ +13,5 @@
> +
> +using namespace dom;
> +
> +WidgetMouseEvent
> +MultiTouchInput::ToWidgetMouseEvent(nsIWidget* aWidget) const

Move this to GeckoTouchDispatcher.cpp.

::: widget/gonk/nsAppShell.cpp
@@ +162,4 @@
>  };
>  
> +static
> +mozilla::Modifiers

Put the return type on the same line as static.

@@ +548,4 @@
>      // popped and dispatched on the main thread.
>      mozilla::Mutex mQueueLock;
>      std::queue<UserInputData> mEventQueue;
> +

Unrelated whitespace change

@@ -691,4 @@
>      *outConfig = mConfig;
>  }
>  
> -

Unrelated whitespace change

@@ +610,2 @@
>  static bool
> +isExpiredKey(UserInputData& aData)

Unnecessary change.

@@ +631,4 @@
>  
>      switch (data.type) {
>      case UserInputData::MOTION_DATA: {
> +        NS_WARNING("Should not dispatch touch events here anymore");

MOZ_ASSERT_UNREACHABLE might be more appropriate.

@@ +690,5 @@
>      gAppShell->NotifyNativeEvent();
>  }
>  
> +MultiTouchInput::MultiTouchType
> +GeckoInputDispatcher::getMultiTouchType(int32_t action)

Inline this.

@@ +726,5 @@
> +
> +    touch.id = id;
> +    // Interesting that the radius is a square?
> +    nsIntPoint radius = nsIntPoint(touch.coords.getAxisValue(AMOTION_EVENT_AXIS_SIZE),
> +            touch.coords.getAxisValue(AMOTION_EVENT_AXIS_SIZE));

The radius and rotation angle calculation in the old code is wrong, so I think you can just omit it in the new code. Bug 1055961 will fix the radius/rotation.

@@ +729,5 @@
> +    nsIntPoint radius = nsIntPoint(touch.coords.getAxisValue(AMOTION_EVENT_AXIS_SIZE),
> +            touch.coords.getAxisValue(AMOTION_EVENT_AXIS_SIZE));
> +
> +    nsIntPoint point = nsIntPoint(floor(touch.coords.getX() + 0.5),
> +            floor(touch.coords.getY() + 0.5));

Why nsIntPoint? Why not directly convert into the int point type we'll actually want?

@@ +732,5 @@
> +    nsIntPoint point = nsIntPoint(floor(touch.coords.getX() + 0.5),
> +            floor(touch.coords.getY() + 0.5));
> +
> +    float rotationAngle = 0;
> +    float force =  touch.coords.getAxisValue(AMOTION_EVENT_AXIS_PRESSURE);

Giving these names doesn't buy much.

@@ +773,5 @@
> +    }
> +
> +    int filterIndex = action & AMOTION_EVENT_ACTION_POINTER_INDEX_MASK;
> +    filterIndex >>= AMOTION_EVENT_ACTION_POINTER_INDEX_SHIFT;
> +    touchData.mFilterIndex = filterIndex;

Don't store the index in touchData - pass an *id* directly to the touch dispatcher.

@@ +1065,5 @@
>      hal::NotifyScreenConfigurationChange(nsScreenGonk::GetConfiguration());
>  }
> +
> +bool
> +nsAppShell::NotifyVsync(TimeStamp vsyncTimeStamp)

Remove this and associated code. The touch dispatcher should be notified of the vsync directly instead of going through nsAppShell.

Comment 93

[Mason Chang [Inactive] [:mchang]]

try build with slight modification for bug 987527

https://tbpl.mozilla.org/?tree=Try&rev=185162b78307

Comment 94

[Mason Chang [Inactive] [:mchang]]

Attachment: Resample Touch Events

Updated with comments from comment 92. Thanks again!

Comment 95

[Mason Chang [Inactive] [:mchang]]

Attachment: Resample Touch Events

Re-read the last comment about passing the touch ID. Made more sense after dinner :). Updated to pass the ID.

Comment 96

[Michael Wu [:mwu]]

Comment on attachment 8479558 [details] [diff] [review]
Resample Touch Events

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

This code uses TimeStamp a bunch - I don't recommend it. TimeStamps always come from Gecko, but the vsync and touch events we get come from outside of gecko, so using the times provided for us rather than TimeStamp is going to be more accurate. One tricky thing though is that we only store 32 bit times in MultiTouchInput, but the system provided times are int64_t.

Storing events in three different queues seems somewhat questionable to me. Seems like you really only want to know how many move events there are in the queue, and that can be done more simply with a counter rather than having three dedicated queues.

Still not done wrapping my head around this code but this is all the code I can read for tonight.

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +559,5 @@
> +  }
> +}
> +
> +WidgetMouseEvent
> +MultiTouchInput::ToWidgetMouseEvent(nsIWidget* aWidget) const

I meant that you should move it here and make it local to this file instead of making it part of MultiTouchInput.

::: widget/gonk/GeckoTouchDispatcher.h
@@ +26,5 @@
> +class TimeStamp;
> +class TimeDuration;
> +} // mozilla namespace
> +
> +class GeckoTouchDispatcher

I think it's worth sticking this in the mozilla namespace. Should reduce the amount of mozilla:: spam.

::: widget/gonk/nsAppShell.cpp
@@ +689,3 @@
>  
> +    float force =  touch.coords.getAxisValue(AMOTION_EVENT_AXIS_PRESSURE);
> +    ScreenIntPoint point = ScreenIntPoint::FromUnknownPoint(gfx::IntPoint(

Going through IntPoint to convert to ScreenIntPoint seems rather roundabout still.

@@ +747,5 @@
> +        }
> +    }
> +
> +    int filterIndex = action & AMOTION_EVENT_ACTION_POINTER_INDEX_MASK;
> +    filterIndex >>= AMOTION_EVENT_ACTION_POINTER_INDEX_SHIFT;

The code here is identical to the code above that determines touchIndex.

Comment 97

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: enable hwc hw-vsync

With this patch, we will enable the hw-vsync for input resampling.
we need to handle the case that we init the vsync event failed(or with the gfxPrefs::FrameUniformityHWVsyncEnabled() off).
In this case, we need the original input processing flow.

Comment 98

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8479739 [details] [diff] [review]
enable hwc hw-vsync

Base on the bug 987527, enable the hw vsync event.

Comment 99

[Mason Chang [Inactive] [:mchang]]

Attachment: Resample Touch Events

Updated with comments from 96. Also incorporated Jerry's patch from comment 98.

In terms of 3 touch queues, I removed touch start and touch end. Before, I was resampling the touch end state as well so I needed to dispatch touch ends at vsync time before sending touch starts, otherwise there would be weird bugs when pinching with multiple fingers. I thought about it some more, and we can gain back some accuracy by sending touch ends directly and without waiting on vsync which makes more sense to me with Android's algorithm. Now we send touch starts and touch ends immediately and only queue / resample touch moves.

As for using the uint64_t timestamps provided by hardware and events, I think we can use them. The algorithms only rely on the difference between the vsync events, which should be 16.6 ms and the difference between the touch events which should be 10ms, which fit into uint32_t. This patch doesn't have that updated work yet, but has everything else. I'll update the patch to not use timestamps.

Comment 100

[Michael Wu [:mwu]]

Comment on attachment 8479987 [details] [diff] [review]
Resample Touch Events

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

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +16,5 @@
> + */
> +
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include <utils/Timers.h>       // For SYSTEM_TIME_MONOTONIC

I don't think these header comments are too useful.. Nobody is going to maintain them well enough to make them useful, and there are tools for finding unused includes (like IWYU) which are more effective than comments.

@@ +112,5 @@
> +  return sTouchDispatcher->SendTouchEvents(aVsyncTimeStamp);
> +}
> +
> +bool
> +GeckoTouchDispatcher::HaveTouchData()

inline

@@ +130,5 @@
> +GeckoTouchDispatcher::SendTouchEvents(TimeStamp aVsyncTime)
> +{
> +  MOZ_ASSERT(mResamplingEnabled, "Resampling must be enabled");
> +  bool haveData = HaveTouchData();
> +  if (!NS_IsMainThread()) {

Should assert that this is the main thread, and ensure callers use DispatchTouchEventsMainThread instead.

@@ +157,5 @@
> +      mTouchMoveEvents.clear();
> +      // fall through
> +    }
> +    default:
> +      NS_DispatchToMainThread(new DispatchSingleTouchMainThread(this, aData));

How does this even work? aData is allocated on stack, and you're passing it to another thread.

@@ +166,5 @@
> +  }
> +}
> +
> +bool
> +GeckoTouchDispatcher::IsFilteredTouch(const MultiTouchInput& aTouch)

This isn't being used to filter touch start and touch end anymore.

@@ +177,5 @@
> +  return false;
> +}
> +
> +void
> +GeckoTouchDispatcher::UpdateTouchState(const MultiTouchInput& aTouch, int32_t id)

inline

@@ +264,5 @@
> +{
> +  float alpha = aFrameDiff.ToMilliseconds() / aTouchDiff.ToMilliseconds();
> +  aOutTouch = aCurrent;
> +
> +  // Make sure we only resample the correct finger.

Touches aren't guaranteed to maintain the same index though. They're only guaranteed to have the same ID.

@@ +271,5 @@
> +    SingleTouchData other = aOther.mTouches[i];
> +    MOZ_ASSERT(current.mIdentifier == other.mIdentifier);
> +
> +    ScreenIntPoint currentTouchPoint = current.mScreenPoint;
> +    ScreenIntPoint otherTouchPoint= other.mScreenPoint;

Space before =

@@ +379,5 @@
> +  }
> +}
> +
> +void
> +GeckoTouchDispatcher::AddTouchToInterpolator(MultiTouchInput& aMultiTouch)

inline

::: widget/gonk/HwcComposer2D.cpp
@@ +153,5 @@
>          mRBSwapSupport = false;
>      }
> +    
> +    if (RegisterHwcEventCallback()) {
> +      EnableVsync(true);

Wrong indentation.

@@ +230,4 @@
>  void
>  HwcComposer2D::Vsync(int aDisplay, int64_t aTimestamp)
>  {
> +  GeckoTouchDispatcher::NotifyVsync(TimeStamp::Now());

Wrong indentation.

::: widget/gonk/nsAppShell.cpp
@@ +684,5 @@
> +{
> +    ::Touch touch;
> +    int32_t id = args->pointerProperties[aIndex].id;
> +    memcpy(&touch.coords, &args->pointerCoords[aIndex], sizeof(*args->pointerCoords));
> +    touch.id = id;

Copying the touch just to copy it to another structure is silly.

@@ +730,5 @@
> +    }
> +
> +    MultiTouchInput touchData(touchType, time, timestamp, modifiers);
> +
> +    /***

Three asterisks is kinda weird.. almost javadoc, but not quite..

Comment 101

[Michael Wu [:mwu]]

Comment on attachment 8479987 [details] [diff] [review]
Resample Touch Events

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

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +157,5 @@
> +      mTouchMoveEvents.clear();
> +      // fall through
> +    }
> +    default:
> +      NS_DispatchToMainThread(new DispatchSingleTouchMainThread(this, aData));

Nevermind, I see that you're making a copy in DispatchSingleTouchMainThread.

Comment 102

[Mason Chang [Inactive] [:mchang]]

Attachment: Resample Touch Events

Changeset:

* Things from comment 100.
* Stop using timestamp, carry the last touch time event in GeckoTouchDispatcher in nanoseconds. Replaced all extrapolation / interpolation to be based on nanosecond event vsync timing / touch timing.

Comment 103

[Mason Chang [Inactive] [:mchang]]

Attachment: Resample Touch Events (v10)

Updated to fix a bug with pinch to zoom crashing.

Comment 104

[Michael Wu [:mwu]]

Comment on attachment 8480218 [details] [diff] [review]
Resample Touch Events

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

Reading through the code a bit more closely, it looks like the filtering code was not actually ported over to GeckoTouchDispatcher. There's code keeping track of the number of touches down on the screen, but there's no flag indicating that touches should be thrown out - mTouchEventsFiltered in the old code.

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +48,5 @@
> +
> +// Amount of time in MS before an input is considered expired.
> +static const uint64_t kInputExpirationThresholdMs = 1000;
> +static int64_t millisectonanosec(float ms) { return (int64_t)(ms * 1000000.0); }
> +static int32_t nanosectomillisec(int64_t nanosec) { return nanosec / 1000000; }

Should camel case this so it's a bit easier to read.

@@ +63,5 @@
> +  mResamplingEnabled = gfxPrefs::TouchResampling() &&
> +                       gfxPrefs::FrameUniformityHWVsyncEnabled();
> +  mVsyncAdjust = millisectonanosec((float) gfxPrefs::TouchVsyncSampleAdjust());
> +  mMaxPredict =  millisectonanosec((float) gfxPrefs::TouchResampleMaxPredict());
> +  mMinResampleTime = millisectonanosec((float) gfxPrefs::TouchResampleMinTime());

Might be simpler to just have the prefs take nanoseconds to begin with.

@@ +152,5 @@
> +  }
> +
> +  if (mResamplingEnabled) {
> +    switch (aData.mType) {
> +    case MultiTouchInput::MULTITOUCH_MOVE:

case indentation here is inconsistent with the rest of the file.

@@ +153,5 @@
> +
> +  if (mResamplingEnabled) {
> +    switch (aData.mType) {
> +    case MultiTouchInput::MULTITOUCH_MOVE:
> +    {

A block isn't necessary here.

@@ +162,5 @@
> +    }
> +    case MultiTouchInput::MULTITOUCH_END:
> +    {
> +      MutexAutoLock lock(mTouchQueueLock);
> +      mTouchMoveEvents.clear();

Is this a good idea? If the user has two fingers down and lifts one finger, the other finger that is still on the screen can't be resampled immediately after.

@@ +217,5 @@
> +  DispatchTouchEvent(touchMove);
> +}
> +
> +float
> +GeckoTouchDispatcher::LERPAlpha(int start, int end, double alpha)

Can make this a static function not in GeckoTouchDispatcher.

@@ +235,5 @@
> +SingleTouchData
> +GeckoTouchDispatcher::GetTouchByID(int32_t id, MultiTouchInput& aMultiTouch)
> +{
> +  for (size_t i = 0; i < aMultiTouch.mTouches.Length(); i++) {
> +    SingleTouchData aTouch = aMultiTouch.mTouches[i];

An 'a' prefix isn't appropriate for a local variable - it's only for arguments. We don't really need a local variable here though.

@@ +241,5 @@
> +      return aTouch;
> +    }
> +  }
> +
> +  MOZ_ASSERT_UNREACHABLE("Could not find a touch with the given id\n");

Given a set of events like this:

BEGIN
MOVE
BEGIN
MOVE

Wouldn't the second move have an ID that the first move doesn't have?

(looks like you just uploaded a new patch that fixes this.)

@@ +327,5 @@
> +    mTouchMoveEvents.push_back(currentTouch);
> +    currentTouchTime = mLastTouchEvent;
> +  }
> +
> +  MOZ_ASSERT(millisectonanosec((float) currentTouch.mTime) < aSampleTime);

Casting mTime to float is a lossy operation..

::: widget/gonk/GeckoTouchDispatcher.h
@@ +17,5 @@
> +
> +#ifndef GECKO_TOUCH_INPUT_DISPATCHER_h
> +#define GECKO_TOUCH_INPUT_DISPATCHER_h
> +#include <utils/BitSet.h>       // For BitSet32
> +#include "Units.h"              // For ScreenIntPoint

More header comments.

@@ +81,5 @@
> +  uint64_t mVsyncAdjust;
> +  uint64_t mMaxPredict;
> +  uint64_t mMinResampleTime;
> +  uint64_t mTouchTimeDiff;
> +  uint64_t mLastTouchEvent;

mLastTouchTime makes more sense to me - mLastTouchEvents makes me expect an actual event object.

::: widget/gonk/HwcComposer2D.cpp
@@ +29,5 @@
>  #include "cutils/properties.h"
>  #include "gfx2DGlue.h"
> +#include "mozilla/TimeStamp.h"
> +#include "GeckoTouchDispatcher.h"
> +#include "InputData.h"                      // For MultiTouchInput

More header comments.

::: widget/gonk/nsAppShell.cpp
@@ +678,5 @@
>      gAppShell->NotifyNativeEvent();
>  }
>  
> +void
> +GeckoInputDispatcher::addMultiTouch(MultiTouchInput& aMultiTouch,

This function doesn't need to be part of GeckoInputDispatcher.

Comment 105

[Mason Chang [Inactive] [:mchang]]

Attachment: Resample Touch Events (v11)

Updated from comments in 104.

> @@ +162,5 @@
> > +    }
> > +    case MultiTouchInput::MULTITOUCH_END:
> > +    {
> > +      MutexAutoLock lock(mTouchQueueLock);
> > +      mTouchMoveEvents.clear();
> 
> Is this a good idea? If the user has two fingers down and lifts one finger,
> the other finger that is still on the screen can't be resampled immediately
> after.

Will churn on it more in the next patch.

> @@ +235,5 @@
> > +SingleTouchData
> > +GeckoTouchDispatcher::GetTouchByID(int32_t id, MultiTouchInput& aMultiTouch)
> > +{
> > +  for (size_t i = 0; i < aMultiTouch.mTouches.Length(); i++) {
> > +    SingleTouchData aTouch = aMultiTouch.mTouches[i];
> 
> An 'a' prefix isn't appropriate for a local variable - it's only for
> arguments. We don't really need a local variable here though.
> 

Fixed the 'a' prefix. Since we return the value, I didn't want to inline the array access twice.

Comment 106

[Mason Chang [Inactive] [:mchang]]

Attachment: Resample Touch Events (v12)

Changelog:

* Clear touch move events when we have 0 touch down count, as in we have have no more fingers on the screen. I wasn't sure of a clean way to do it before, but now it works.
* GeckoTouchDispatcher::DispatchTouchMove holds the mTouchQueueLock during the resampling logic and releases it before sending the touch event to nsWindow. We had lots of locking / unocking in between which made the code cumbersome but I thought it would make it faster. Actually holding the 1 lock through the whole method seems to have made things better.
* We sometimes had errors where we got another touch event after the vsync notification but before the main thread could process the touch event because the main thread was too busy. In those cases, we just send the last touch event. I'll probably have to test to see if interpolating is better in that case.

Comment 107

[Mason Chang [Inactive] [:mchang]]

Attachment: Resample Touch Events (v13)

Fixed a one line build error on debug builds. Successful try:

https://tbpl.mozilla.org/?tree=Try&rev=f2a7a210bc1c

Been manually testing for an hour now. Everything seems to work including pinching, scrolling, dialogs across multiple apps. Still doing more manual testing.

Comment 108

[Michael Wu [:mwu]]

Comment on attachment 8480837 [details] [diff] [review]
Resample Touch Events (v13)

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

Looks pretty close now. I think I'd like kats to take a look over the new algorithm in the next patch.

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +140,5 @@
> +        MutexAutoLock lock(mTouchQueueLock);
> +        mTouchMoveEvents.push_back(aData);
> +        mTouchTimeDiff = aEventTime - mLastTouchTime;
> +        mLastTouchTime = aEventTime;
> +        break;

This function can be simplified slightly by returning here so the default exit just dispatches the event to the main thread.

@@ +190,5 @@
> +  return float(start) + (float(end - start) * alpha);
> +}
> +
> +ScreenIntPoint
> +GeckoTouchDispatcher::ResamplePoint(ScreenIntPoint aCurrent, ScreenIntPoint aOther, double aAlpha)

I think you can just avoid the floating point math entirely here. Also just noticed - this doesn't use any state from GeckoTouchDispatcher so it can be a local static function too.

@@ +199,5 @@
> +  return samplePoint;
> +}
> +
> +SingleTouchData
> +GeckoTouchDispatcher::GetTouchByID(SingleTouchData aCurrentTouch, MultiTouchInput& aOtherTouch)

Can be a local static function.

@@ +219,5 @@
> +
> +void
> +GeckoTouchDispatcher::ResampleTouch(MultiTouchInput& aOutTouch, MultiTouchInput& aCurrent,
> +                                    MultiTouchInput& aOther, int64_t aFrameDiff,
> +                                    uint64_t aTouchDiff, bool aInterpolate)

Can be a local static function.

@@ +325,5 @@
> +void
> +GeckoTouchDispatcher::PrintResampleData(ScreenIntPoint aCurrent, ScreenIntPoint aOther,
> +                                        ScreenIntPoint aResample, double aAlpha,
> +                                        uint64_t aTouchDiff, int64_t aFrameDiff,
> +                                        bool aInterpolate)

Inline

@@ +343,5 @@
> +#endif
> +}
> +
> +void
> +GeckoTouchDispatcher::PrintUniformityInfo(const MultiTouchInput& aMultiTouch)

Inline

@@ +373,5 @@
> +GeckoTouchDispatcher::DispatchMouseEvent(MultiTouchInput& aMultiTouch,
> +    bool aForwardToChildren)
> +{
> +  if (aMultiTouch.mTouches.Length() == 0) {
> +    MOZ_ASSERT_UNREACHABLE("Touch Event has no touches, probably bad things will happen");

An ordinary MOZ_ASSERT is more appropriate here.

@@ +416,5 @@
> +  }
> +
> +  if (mTouchEventsFiltered) {
> +    MutexAutoLock lock(mTouchQueueLock);
> +    mTouchMoveEvents.clear();

I think you'll clear move events which don't need to be filtered out if you do this.

@@ +463,5 @@
> +                        WidgetMouseEvent::eReal, WidgetMouseEvent::eNormal);
> +
> +  SingleTouchData firstTouch = aMultiTouch.mTouches[0];
> +  event.refPoint.x = (int) firstTouch.mScreenPoint.x;
> +  event.refPoint.y = (int) firstTouch.mScreenPoint.y;

mScreenPoint is already in int, no?

::: widget/gonk/HwcComposer2D.cpp
@@ +28,4 @@
>  #include "mozilla/StaticPtr.h"
>  #include "cutils/properties.h"
>  #include "gfx2DGlue.h"
> +#include "mozilla/TimeStamp.h"

Why do we need to include this?

@@ +29,5 @@
>  #include "cutils/properties.h"
>  #include "gfx2DGlue.h"
> +#include "mozilla/TimeStamp.h"
> +#include "GeckoTouchDispatcher.h"
> +#include "InputData.h"

Why do we need to include this?

Comment 109

[Mason Chang [Inactive] [:mchang]]

Attachment: Resample Touch Events (v14)

Changelog:

* Implemented things from comment 108
* Use references for SingleTouchData instead of copying them. Also made them const.
* Made many member variables int64_t instead of uint64_t. If you divide a int64_t / uint64_t, the int64_t gets promoted up to uint64_t which breaks negative number divide. 

From comment 100, :mwu would also like :kats to take a look at this.

Comment 110

[Mason Chang [Inactive] [:mchang]]

Attachment: Resample Touch Events (v15)

Fixed an assertion which caused a build issue on debug b2g builds.

Comment 111

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8481017 [details] [diff] [review]
Resample Touch Events (v14)

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

Some overall comments:
0) I didn't look through mwu's previous comments so sorry if I contradicted or duplicated stuff he already said. In case of contradiction, go with what he said
1) When generating patches for review please generate them with 5 or 8 lines of context rather than 3 as it makes it some parts easier to review
2) The algorithm seems to be sound, although I believe the code could be simplified in some cases and made more general at the same time
3) If more than one GeckoTouchDispatcher is ever created in a process, or if it is destroyed while the gecko main event loop is still spinning then we have a problem (see comments below). I don't believe these are true at the moment though.
4) The prefs should really be integer prefs representing the times in nanoseconds. It seems kinda silly to keep them as floats in milliseconds and then convert them.

Of the issues I mention only a couple are serious but even those are unlikely to occur in practice or with the default prefs so I'm ok with landing this patch and then addressing those issues in a follow-up bug or patch, particularly since we want to get this landed in 34 ASAP.

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +50,5 @@
> +static const uint64_t kInputExpirationThresholdMs = 1000;
> +static int64_t millisecToNanosec(float ms) { return (int64_t)(ms * 1000000.0); }
> +static int32_t nanosecToMillisec(int64_t nanosec) { return nanosec / 1000000; }
> +
> +static GeckoTouchDispatcher* sTouchDispatcher;

Make this a StaticRefPtr<GeckoTouchDispatcher>. Actually I'm kind of confused because you've made this class ref-counted but the only RefPtr to it is in nsAppShell. I think either all the pointers to it should be refcounted or none of them should be. It seems error-prone to leave some raw pointers when the object could get deleted out from under them. The lifetime of this is not clear to me.

For example, what happens if the app shell gets destroyed while you have a pending DispatchTouchEventsMainThread that hasn't run yet? The GeckoTouchDispatcher* in that class will be a dangling pointer and it will crash when run. It seems to me like you want to use the NS_INLINE_DECL_THREADSAFE_REFCOUNTING macro instead of NS_INLINE_DECL_REFCOUNTING, and then use nsRefPtr everywhere you currently use GeckoTouchDispatcher* (except this static one which would be a StaticRefPtr)

@@ +65,5 @@
> +                       gfxPrefs::FrameUniformityHWVsyncEnabled();
> +  mVsyncAdjust = millisecToNanosec((float) gfxPrefs::TouchVsyncSampleAdjust());
> +  mMaxPredict =  millisecToNanosec((float) gfxPrefs::TouchResampleMaxPredict());
> +  mMinResampleTime = millisecToNanosec((float) gfxPrefs::TouchResampleMinTime());
> +  sTouchDispatcher = this;

Will there ever be more than one of these created? You might want to assert sTouchDispatcher is null here.

@@ +160,5 @@
> +    MutexAutoLock lock(mTouchQueueLock);
> +    int touchCount = 0;
> +    if (mTouchMoveEvents.empty()) {
> +      return;
> +    } else {

mozilla style guide says no else-after-return. The else is redundant.

@@ +164,5 @@
> +    } else {
> +      touchCount = mTouchMoveEvents.size();
> +    }
> +
> +    bool resample = (touchCount != 1) &&

For clarity do |touchCount > 1|

@@ +168,5 @@
> +    bool resample = (touchCount != 1) &&
> +               (aVsyncTime - mLastTouchTime > mMinResampleTime) &&
> +               // Since this is the main thread, if we took too long to dispatch
> +               // just send the last touch
> +               (aVsyncTime > mLastTouchTime);

This last clause in the condition is redundant. If aVsyncTime <= mLastTouchTime then aVsyncTime - mLastTouchTime will be <= 0. So unless mMinResampleTime is negative the previous condition will be false anyway. I suppose somebody could set a negative pref value but I don't know if you need to bother dealing with that.

@@ +183,5 @@
> +  DispatchTouchEvent(touchMove);
> +}
> +
> +static int
> +LERP(int start, int end, int64_t aFrameDiff, int64_t aTouchDiff)

This function could use a better name. It's a standard linear interpolation (in the mathematical sense, which includes extrapolation) formula.

@@ +203,5 @@
> +  // We can have situations where a previous touch event had 2 fingers
> +  // and we lift 1 finger off. In those cases, we won't find the touch event
> +  // with given id, so just return the current touch, which will be resampled
> +  // without modification and dispatched.
> +  return aCurrentTouch;

Nice, that's an elegant solution :)

@@ +249,5 @@
> +// and with the future touch event past sample time
> +int32_t
> +GeckoTouchDispatcher::InterpolateTouch(MultiTouchInput& aOutTouch, uint64_t aSampleTime)
> +{
> +  MOZ_RELEASE_ASSERT(mTouchMoveEvents.size() >= 2);

Add a MOZ_ASSERT(mTouchQueueLock.AssertCurrentThreadOwns());

@@ +260,5 @@
> +  mTouchMoveEvents.clear();
> +  mTouchMoveEvents.push_back(futureTouch);
> +  uint64_t currentTouchTime = mLastTouchTime - mTouchTimeDiff;
> +
> +  MOZ_ASSERT(currentTouchTime < aSampleTime);

I don't think you can reasonably assert this. If the mVsyncAdjust is large, then you might get two touch move events between the sample time and the vsync, in which case currentTouchTime will be >= aSampleTime here. It totally depends on the hardware event frequency and the value of mVsyncAdjust. I'd say just get rid of this assert. Also the comment above that says "currentTouch < SampleTime < futureTouch" doesn't cover this scenario. Note also that the actual ResampleTouch algorithm doesn't care what order these things are in; it seems to me like you're over-emphasizing the distinction between interpolation and extrapolation when in fact you can mostly treat them as the same from an algorithmic perspective. The fact that ResampleTouch doesn't use the aInterpolate flag in the actual algorithm is proof of that. The two Interpolate/ExtrapolateTouch functions could probably be combined into one, although I don't know if you prefer to keep them separate.

@@ +273,5 @@
> +// and extrapolates them to sample time.
> +int32_t
> +GeckoTouchDispatcher::ExtrapolateTouch(MultiTouchInput& aOutTouch, uint64_t aSampleTime)
> +{
> +  MOZ_RELEASE_ASSERT(mTouchMoveEvents.size() >= 2);

Add a MOZ_ASSERT(mTouchQueueLock.AssertCurrentThreadOwns());

@@ +299,5 @@
> +  ResampleTouch(aOutTouch, currentTouch, prevTouch, frameDiff, mTouchTimeDiff, false);
> +  return -nanosecToMillisec(frameDiff);
> +}
> +
> +/***

/** not /***

@@ +305,5 @@
> + * interpolation. The algorithm takes the vsync time and subtracts 5 ms and
> + * creates a sample time. All touch events are relative to this sample time.
> + * If the last touch event occurs AFTER this marker this, but before sample time, we
> + * interpolate the last two touch events. If the last touch event occurs BEFORE
> + * this sample time, we extrapolate the last two touch events to the sample 

nit: trailing whitespace

@@ +322,5 @@
> +    touchTimeAdjust = ExtrapolateTouch(aOutTouch, sampleTime);
> +  }
> +
> +  aOutTouch.mTimeStamp += TimeDuration::FromMilliseconds(touchTimeAdjust);
> +  aOutTouch.mTime += touchTimeAdjust;

It seems like the purpose of these two lines is to adjust the timestamps on aOutTouch to be the sample time. So why not just assign these two fields directly to the sample time in the ResampleTouch function? Then you can also remove the return values from InterpolateTouch and ExtrapolateTouch.

@@ +332,5 @@
> +GeckoTouchDispatcher::DispatchMouseEvent(MultiTouchInput& aMultiTouch,
> +                                         bool aForwardToChildren)
> +{
> +  if (aMultiTouch.mTouches.Length() == 0) {
> +    MOZ_ASSERT("Touch Event has no touches, probably bad things will happen");

The only call site to this function ensures that there is exactly one touch point in the MultiTouchInput so this check is unnecessary. Also MOZ_ASSERT() takes an expression and optionally an explanation string as a second argument so this code is bogus anyway.

@@ +336,5 @@
> +    MOZ_ASSERT("Touch Event has no touches, probably bad things will happen");
> +  }
> +
> +  WidgetMouseEvent mouseEvent = ToWidgetMouseEvent(aMultiTouch, nullptr);
> +  mouseEvent.mFlags.mNoCrossProcessBoundaryForwarding = !aForwardToChildren;

If the event type of mouseEvent is NS_EVENT_NULL then just return and don't dispatch it.

@@ +400,5 @@
> +    const SingleTouchData& firstTouch = aMultiTouch.mTouches[0];
> +    const ScreenIntPoint& touchPoint = firstTouch.mScreenPoint;
> +
> +    LOG("UniformityInfo %s %llu %d %d", touchAction, systemTime(SYSTEM_TIME_MONOTONIC),
> +        (int) touchPoint.x, (int) touchPoint.y);

As of bug 1057642 landing you shouldn't need the (int) casts.

@@ +438,5 @@
> +                         WidgetMouseEvent::eReal, WidgetMouseEvent::eNormal);
> +
> +  const SingleTouchData& firstTouch = aMultiTouch.mTouches[0];
> +  event.refPoint.x = (int) firstTouch.mScreenPoint.x;
> +  event.refPoint.y = (int) firstTouch.mScreenPoint.y;

Ditto on the casts

::: widget/gonk/GeckoTouchDispatcher.h
@@ +3,5 @@
> +/* Copyright 2014 Mozilla Foundation and Mozilla contributors
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at

Shouldn't this be entirely MPL licensed? I don't know but that would be my guess. Ditto on GeckoTouchDispatcher.cpp

@@ +16,5 @@
> + */
> +
> +#ifndef GECKO_TOUCH_INPUT_DISPATCHER_h
> +#define GECKO_TOUCH_INPUT_DISPATCHER_h
> +#include <utils/BitSet.h>

nit: add a blank line after the #define

@@ +28,5 @@
> +
> +class GeckoTouchDispatcher
> +{
> +public:
> +NS_INLINE_DECL_REFCOUNTING(GeckoTouchDispatcher)

indent this. Also it would be better to move the public: down below this macro, since the macro internally expands to protected and public blocks and it's safer not to rely on it restoring back to public visibility.

@@ +60,5 @@
> +  int64_t mVsyncAdjust;
> +  int64_t mMaxPredict;
> +  int64_t mMinResampleTime;
> +  int64_t mTouchTimeDiff;
> +  uint64_t mLastTouchTime;

A sentence of documentation on each of these would help a lot. A few sentences of class-level documentation (which provides an outline of the algorithm and introduces concepts that you can use when documenting these fields) wouldn't hurt either.

@@ +64,5 @@
> +  uint64_t mLastTouchTime;
> +};
> +
> +} // mozilla
> +#endif // GECKO_TOUCH_INPUT_DISPATCHER

_h

::: widget/gonk/nsAppShell.cpp
@@ +542,4 @@
>      mozilla::Mutex mQueueLock;
>      std::queue<UserInputData> mEventQueue;
>      sp<EventHub> mEventHub;
> +    nsRefPtr<GeckoTouchDispatcher>   mTouchDispatcher;

keep only one space between type and name

@@ +664,3 @@
>      {
>          MutexAutoLock lock(mQueueLock);
>          if (!mEventQueue.empty() &&

Shouldn't this stuff be removed too? This code is just coalescing motion events in the event queue but now you're not even putting any motion events in the event queue so it's pointless to do this.

@@ +730,5 @@
> +    // the touch end since the touch array has all fingers, not just the touch
> +    // that we want to end
> +    int touchIndex = args->action & AMOTION_EVENT_ACTION_POINTER_INDEX_MASK;
> +    touchIndex >>= AMOTION_EVENT_ACTION_POINTER_INDEX_SHIFT;
> +    int filterId = touchData.mTouches[touchIndex].mIdentifier;

filterId is never used, remove it. Also you might as well move the touchIndex variable inside the if block below since it's not used in the else half.

Comment 112

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8481345 [details] [diff] [review]
Resample Touch Events (v15)

I don't know what you changed in this one but see my comments on the other patch. Assuming the change was trivial you can carry the r+

Comment 113

[Mason Chang [Inactive] [:mchang]]

Attachment: Diff with Kats Comments

(In reply to (away|aug30-sep3) Kartikaya Gupta (email:[e-mail]) from comment #111)
> Comment on attachment 8481017 [details] [diff] [review]
> Resample Touch Events (v14)
> 
> Review of attachment 8481017 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 4) The prefs should really be integer prefs representing the times in
> nanoseconds. It seems kinda silly to keep them as floats in milliseconds and
> then convert them.

Personally, I like preferences being easier to understand since they change more often, changed it though.

> 
> Of the issues I mention only a couple are serious but even those are
> unlikely to occur in practice or with the default prefs so I'm ok with
> landing this patch and then addressing those issues in a follow-up bug or
> patch, particularly since we want to get this landed in 34 ASAP.
> 
> ::: widget/gonk/GeckoTouchDispatcher.cpp
> @@ +50,5 @@
> > +static const uint64_t kInputExpirationThresholdMs = 1000;
> > +static int64_t millisecToNanosec(float ms) { return (int64_t)(ms * 1000000.0); }
> > +static int32_t nanosecToMillisec(int64_t nanosec) { return nanosec / 1000000; }
> > +
> > +static GeckoTouchDispatcher* sTouchDispatcher;
> 
> Make this a StaticRefPtr<GeckoTouchDispatcher>. Actually I'm kind of
> confused because you've made this class ref-counted but the only RefPtr to
> it is in nsAppShell. I think either all the pointers to it should be
> refcounted or none of them should be. It seems error-prone to leave some raw
> pointers when the object could get deleted out from under them. The lifetime
> of this is not clear to me.
> 
> For example, what happens if the app shell gets destroyed while you have a
> pending DispatchTouchEventsMainThread that hasn't run yet? The
> GeckoTouchDispatcher* in that class will be a dangling pointer and it will
> crash when run. It seems to me like you want to use the
> NS_INLINE_DECL_THREADSAFE_REFCOUNTING macro instead of
> NS_INLINE_DECL_REFCOUNTING, and then use nsRefPtr everywhere you currently
> use GeckoTouchDispatcher* (except this static one which would be a
> StaticRefPtr)

Thanks! I still need a better crash course in all the RefPtr stuff. Can you please double check this patch? I think I put the nsRefPtr in the right places.

> This last clause in the condition is redundant. If aVsyncTime <=
> mLastTouchTime then aVsyncTime - mLastTouchTime will be <= 0. So unless
> mMinResampleTime is negative the previous condition will be false anyway. I
> suppose somebody could set a negative pref value but I don't know if you
> need to bother dealing with that.

Good catch! Since both are unsigned ints though, have to store it as a signed int
to make this true.

> @@ +183,5 @@
> > +  DispatchTouchEvent(touchMove);
> > +}
> > +
> > +static int
> > +LERP(int start, int end, int64_t aFrameDiff, int64_t aTouchDiff)
> 
> This function could use a better name. It's a standard linear interpolation
> (in the mathematical sense, which includes extrapolation) formula.

Isn't LERP the standard linear interpolation term? http://en.wikipedia.org/wiki/Linear_interpolation
Renamed to interpolate.

> @@ +322,5 @@
> > +    touchTimeAdjust = ExtrapolateTouch(aOutTouch, sampleTime);
> > +  }
> > +
> > +  aOutTouch.mTimeStamp += TimeDuration::FromMilliseconds(touchTimeAdjust);
> > +  aOutTouch.mTime += touchTimeAdjust;
> 
> It seems like the purpose of these two lines is to adjust the timestamps on
> aOutTouch to be the sample time. So why not just assign these two fields
> directly to the sample time in the ResampleTouch function? Then you can also
> remove the return values from InterpolateTouch and ExtrapolateTouch.

The problem is that the mTime is an int32_t whereas sampleTime is uint64_t in nanoseconds.
When we extrapolate, we have to make the return value positive since it's negative when we resample.
This seemed like the cleanest way.

> ::: widget/gonk/GeckoTouchDispatcher.h
> @@ +3,5 @@
> > +/* Copyright 2014 Mozilla Foundation and Mozilla contributors
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at
> 
> Shouldn't this be entirely MPL licensed? I don't know but that would be my
> guess. Ditto on GeckoTouchDispatcher.cpp

I went on the safe side. Since the resampling is done in Android and android is Apache License
along with many other files in widget/gonk, I kept it the same.

> @@ +664,3 @@
> >      {
> >          MutexAutoLock lock(mQueueLock);
> >          if (!mEventQueue.empty() &&
> 
> Shouldn't this stuff be removed too? This code is just coalescing motion
> events in the event queue but now you're not even putting any motion events
> in the event queue so it's pointless to do this.

I think this is just the way the patch ended up. The queue is all for notifyKey, which still goes
through the original event dispatch mechanism in nsAppShell. You can see the delete of the bracket above.
We have no queue for notifyMotion.

Comment 114

[Mason Chang [Inactive] [:mchang]]

Attachment: Resample Touch Events (v16)

Comment 115

[Mason Chang [Inactive] [:mchang]]

Version 16 has implemented kats' comments from comment 111.

Comment 116

[Mason Chang [Inactive] [:mchang]]

Attachment: Resample Touch Events (v17)

Forgot to address one more comment from kats. Included in this patch.

Comment 117

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8481480 [details] [diff] [review]
Resample Touch Events (v17)

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

Looks good, with the coalescing code also removed from notifyKey as we discussed on irc.

::: widget/gonk/GeckoTouchDispatcher.h
@@ +78,5 @@
> +
> +  // The time difference between the last two touch move events
> +  int64_t mTouchTimeDiff;
> +
> +  // The system time since the last touch event occured

s/since/at which/

Comment 118

[Mason Chang [Inactive] [:mchang]]

Attachment: Resample Touch Events (v18)

Updated with kats comments from comment 117. The biggest issue was that we were coalescing key events in nsAppShell. Deleted that code.

Comment 119

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8481519 [details] [diff] [review]
Resample Touch Events (v18)

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

Carrying r+ from kats.

Comment 120

[Michael Wu [:mwu]]

Comment on attachment 8481480 [details] [diff] [review]
Resample Touch Events (v17)

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

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +35,5 @@
> +#include <utils/BitSet.h>
> +#include "mozilla/TimeStamp.h"
> +#include "gfxPrefs.h"
> +#include "nsDebug.h"
> +#include "libui/Input.h"

sort

@@ +64,5 @@
> +  mResamplingEnabled = gfxPrefs::TouchResampling() &&
> +                       gfxPrefs::FrameUniformityHWVsyncEnabled();
> +
> +  mVsyncAdjust = gfxPrefs::TouchVsyncSampleAdjust();
> +  mMaxPredict =  gfxPrefs::TouchResampleMaxPredict();

excess spaces after =

@@ +94,5 @@
> +class DispatchSingleTouchMainThread : public nsRunnable
> +{
> +public:
> +  DispatchSingleTouchMainThread(GeckoTouchDispatcher* aTouchDispatcher,
> +                                MultiTouchInput aTouch)

Making aTouch a reference might help here.

@@ +107,5 @@
> +    return NS_OK;
> +  }
> +
> +private:
> +  nsRefPtr<GeckoTouchDispatcher> mTouchDispatcher;

RefPtr from mfbt (mfbt/RefPtr.h) is generally preferred for new code over nsRefPtr. That also has its own special way of making a class refcounted that uses templates rather than macros. This can be a followup.

@@ +354,5 @@
> +    case MultiTouchInput::MULTITOUCH_CANCEL:
> +      mTouchDownCount--;
> +      if (mTouchDownCount == 0) {
> +        MutexAutoLock lock(mTouchQueueLock);
> +        mTouchMoveEvents.clear();

It's racy to clear mTouchMoveEvents here. We can look into this in a followup though.

@@ +440,5 @@
> +
> +  return event;
> +}
> +
> +} // mozilla

// namespace mozilla

::: widget/gonk/GeckoTouchDispatcher.h
@@ +17,5 @@
> +
> +#ifndef GECKO_TOUCH_INPUT_DISPATCHER_h
> +#define GECKO_TOUCH_INPUT_DISPATCHER_h
> +
> +#include <utils/BitSet.h>

Shouldn't need this anymore.

@@ +55,5 @@
> +  void SendTouchEvent(MultiTouchInput& aData);
> +  void DispatchMouseEvent(MultiTouchInput& aMultiTouch,
> +                          bool aForwardToChildren);
> +
> +  bool mEnabledUniformityInfo;

move this bool over with the other bools

@@ +82,5 @@
> +  // The system time since the last touch event occured
> +  uint64_t mLastTouchTime;
> +};
> +
> +} // mozilla

// namespace mozilla

Comment 121

[Mason Chang [Inactive] [:mchang]]

Attachment: Resample Touch Events (v19)

Found a bug on debug builds. Since the GeckoInputDispatcher is initialized early on, GeckoTouchDispatcher was reading gfxPrefs off main thread and before anything else in Gecko was reading gfxPrefs, which was crashing debug builds. See http://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPrefs.h?from=gfxPrefs.h&case=true#13. I changed GeckoInputDispatcher to dispatch a task to the main thread to init the GeckoTouchDispatcher. I don't really like this, another option is to wait until we get a touch event to initialize GeckoTouchDispatcher, but that also seems not very good.

Comment 122

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8481566 [details] [diff] [review]
Resample Touch Events (v19)

Carrying r+ from kats. Try build:

https://tbpl.mozilla.org/?tree=Try&rev=a026dec91ab4

Comment 123

[Mason Chang [Inactive] [:mchang]]

The other option from comment 121 is in a follow up patch, if UX signs off and we can enable touch resampling by default, we can bring in the constants back into the code so we won't need gfxPrefs anymore. Then we could delete all the init on main thread code.

Comment 124

[Kartikaya Gupta (email:[e-mail])]

(In reply to Michael Wu [:mwu] from comment #120)
> 
> RefPtr from mfbt (mfbt/RefPtr.h) is generally preferred for new code over
> nsRefPtr. That also has its own special way of making a class refcounted
> that uses templates rather than macros. This can be a followup.
> 

Last I heard we were preferring nsRefPtr over RefPtr...

Comment 125

[Mason Chang [Inactive] [:mchang]]

Attachment: Resample Touch Events (v20)

Fixed items in comment 120. The debug build issue has been resolved by properly initializing the gfxPrefs service on the main thread in GeckoTouchDispatcher's constructor instead of creating a new runnable.

Comment 126

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8481614 [details] [diff] [review]
Resample Touch Events (v20)

Carrying r+=kats.

Comment 127

[Mason Chang [Inactive] [:mchang]]

Attachment: Resample Touch Events (v21)

Deleted dead forward declaration and removed InputData.h include from nsAppShell.

Comment 128

[Mason Chang [Inactive] [:mchang]]

Try build with version 21 - https://tbpl.mozilla.org/?tree=Try&rev=6a9df84f6f8e

Comment 129

[Michael Wu [:mwu]]

Comment on attachment 8481620 [details] [diff] [review]
Resample Touch Events (v21)

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

Looks good to land - thanks!

Comment 130

[Mason Chang [Inactive] [:mchang]]

Attachment: Bug 970751. Resample touch events. r=mwu,kats

Carrying r+ from kats and mwu. Updated to have proper commit message.

Comment 131

[Mason Chang [Inactive] [:mchang]]

In inbound - https://hg.mozilla.org/integration/b2g-inbound/rev/3ab215c50844

Comment 132

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for leaks in a bunch of tests: https://hg.mozilla.org/integration/b2g-inbound/rev/e0fb09d7b65f

https://tbpl.mozilla.org/php/getParsedLog.php?id=47070962&tree=B2g-Inbound

Comment 133

[Kartikaya Gupta (email:[e-mail])]

Attempting a couple of fixes:

https://tbpl.mozilla.org/?tree=Try&rev=c33f535030f3
https://tbpl.mozilla.org/?tree=Try&rev=a1e69f696611

Comment 134

[Mason Chang [Inactive] [:mchang]]

Try with ClearOnShutdown for the GeckoTouchDispatcher static pointer - https://tbpl.mozilla.org/?tree=Try&rev=49f395e5dc0c

Comment 135

[Mason Chang [Inactive] [:mchang]]

Successful try from comment 134 and https://tbpl.mozilla.org/?tree=Try&rev=21af994e64f9 has all the platforms. b2g-inbound looks orange for consistent B2g Desktop Linux x64 Debug Gip, which is a gaia ui test. Not sure if I Should push to inbound or wait until that gets resolved :/. Hopefully it gets fixed later today, but it is a holiday weekend. I'll be watching the tree.

Comment 136

[Mason Chang [Inactive] [:mchang]]

The perma fail looks like bug 996252 - https://bugzilla.mozilla.org/show_bug.cgi?id=996252

Comment 137

[Mason Chang [Inactive] [:mchang]]

Attachment: Resample Touch Events r=mwu,kats

Updated to fix leaks. Calls ClearOnShutdown on GeckoTouchDispatcher::sTouchDispatcher. Successful try builds in comment 135. Carrying r+.

Comment 138

[Mason Chang [Inactive] [:mchang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/cd57ffe57293

Comment 139

[Mason Chang [Inactive] [:mchang]]

Backout for intermittent orange - https://hg.mozilla.org/integration/b2g-inbound/rev/4959e1de32de

Comment 140

[Ryan VanderMeulen [:RyanVM]]

The debug mochitest-1 failures aren't yours, if that's why you backed out. Those are fallout from the MediaStreamGraph refactor that landed a few days ago.

Comment 141

[Mason Chang [Inactive] [:mchang]]

Thanks for the follow up Ryan. I won't be on for the next 4 hours so I'll try again tomorrow.

Comment 142

[Mason Chang [Inactive] [:mchang]]

Have a try to test if intermittent on m-c of 202721:1db35d2c9a2f - https://tbpl.mozilla.org/?tree=Try&rev=8983bddafc05

Lots of retests here and all green - https://tbpl.mozilla.org/?tree=Try&rev=49f395e5dc0c, but b2g-inbound went less perma-orange after the backout.

Comment 143

[Mason Chang [Inactive] [:mchang]]

Lots more successful tries from comment 142. Trying again in inbound - https://hg.mozilla.org/integration/b2g-inbound/rev/935836821a9d

Comment 144

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/935836821a9d

Comment 145

[Gabriele Svelto [:gsvelto]]

This is breaking the hamachi builds, tapping anything triggers the following assertion:

Assertion failure: false (MOZ_ASSERT_UNREACHABLE: Could not assign a touch type), at /home/gsvelto/projects/mozilla-central/widget/gonk/nsAppShell.cpp:709

This is affecting not only our Flame-less developers but also contributors who are using Hamachis for their work. I suggest backing this out unless it can be fixed soon; I'm willing to help if help is needed.

Comment 146

[Milan Sreckovic [:milan] (needinfo for best results)]

Mason, do you have a Hamachi to work with? Gabriele, do you know what the value of action parameter to switch is at the time we hit the assertion?

Comment 147

[Mason Chang [Inactive] [:mchang]]

Doh, I have a hamachi. Doing the set up now and will test.

Comment 148

[Gabriele Svelto [:gsvelto]]

Printing out the action here:

http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsAppShell.cpp#685

This is what I get for a touch-slide-and-lift-my-finger action:

action = 0x00000a
action = 00000000
action = 0x000002
action = 0x000002
action = 0x000002
...
action = 0x000002
action = 0x000002
action = 0x000002
action = 0x000001
action = 0x000009
action = 0x000007

Replacing the assertion here with a return seems to make touch events work fine (apparently):

http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsAppShell.cpp#709

Comment 149

[Gabriele Svelto [:gsvelto]]

I think we're asserting on the first event which is an AMOTION_EVENT_ACTION_HOVER_EXIT and isn't handled by the switch.

Comment 150

[Mason Chang [Inactive] [:mchang]]

Attachment: Follow up to fix assertion on hamachi

On a hamachi, we usually get the following order of touch events:

AMOTION_EVENT_ACTION_HOVER_EXIT
AMOTION_EVENT_ACTION_HOVER_ENTER
AMOTION_EVENT_ACTION_HOVER_MOVE

Before this patch, the previous nsAppShell::sendTouchEvents would ignore all three touch events. We would still send one mouse event for the AMOTION_EVENT_ACTION_HOVER_MOVE. 

This current patch will ignore all three events. On hamachi, this seemed to work. I could scroll, open / close rocket bar, do edge swipe gestures, type on the keyboard, and set the ADB settings to adb + devtools. The current change in behavior is that we don't send a mouse move on AMOTION_EVENT_ACTION_HOVER_MOVE. The flame device does not seem to create these hover action items.

If we want to keep the current behavior, we could create a new MultiTouchInput::MULTITOUCH_HOVER_MOVE here: http://dxr.mozilla.org/mozilla-central/source/widget/InputData.h?from=InputData.h&case=true#166 and check for that case, or keep around the raw input data action and track that in GeckoTouchDispatcher::DispatchTouchEvent.

Comment 151

[Michael Wu [:mwu]]

Comment on attachment 8483604 [details] [diff] [review]
Follow up to fix assertion on hamachi

I prefer explicitly ignoring the hover events, and continuing to assert if we get an unexpected event type.

Comment 152

[Michael Wu [:mwu]]

This bug should not be reopened unless it is backed out. Let's fix the hamachi regression in a new bug.