1072645 - Improve touch filtering logic in GeckoTouchDispatcher

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect)

Description

[Michael Wu [:mwu]]

Attachment: Use stateless expired event filtering

Here's a different expired event filter that doesn't rely on any state. As such, it should be much more resistant to getting stuck in a filtering state. (which I've managed to reproduce..)

Comment 1

[Michael Wu [:mwu]]

Attachment: Use stateless expired event filtering, v2

Just realized this code can be simplified further.

Comment 2

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

Comment on attachment 8494843 [details] [diff] [review]
Use stateless expired event filtering, v2

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

This probably doesn't handle the case where two fingers go down simultaneously and so the first touchstart event contains two touch points. However I see obvious problems in the old code (not sure why I missed them the first time around...) so this is definitely an improvement. Even if we run into the two-finger case the only consequence will be that that gesture might get incorrectly ignored and everything will be fine from the next gesture onwards.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So I did manage to hook a debugger up to mine when I was stuck in the weird state and we weren't filtering events here ...

Comment 5

[Michael Wu [:mwu]]

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

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/879502010e5d

Comment 7

[Michael Wu [:mwu]]

Comment on attachment 8494843 [details] [diff] [review]
Use stateless expired event filtering, v2

Approval Request Comment
[Feature/regressing bug #]: Regression from bug 971633 which introduced the expired event filter.
[User impact if declined]: Under heavy load, the device may stop responding to all touch input until the device is restarted.
[Describe test coverage new/current, TBPL]: No tests - this bug is difficult to trigger and tends to happen under long periods of monkey testing.
[Risks and why]: Gestures made with more than one touch point may be filtered out unnecessarily. However, the new code will recover from this, while the old code wouldn't recover.
[String/UUID change made/needed]: None.

Comment 8

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

Attachment: input_resample.mp4

weird overscrolling behavior with this commit.

Comment 9

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

Hi Michael,
We see a weird overscrolling behavior at homescreen. If we back out this patch, it will be ok.
Please check the video attachment 8496153 [details].

Comment 10

[Michael Wu [:mwu]]

I can't reproduce this on the flame.

Comment 11

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

(In reply to Michael Wu [:mwu] from comment #10)
> I can't reproduce this on the flame.

Hi Michael,
You could turn on the resample, and test again on flame device.

in gfx/thebes/gfxPrefs.h,
+pref("gfx.frameuniformity.hw-vsync", true);
+pref("gfx.touch.resample", true);

Comment 12

[Boris Chiou [:boris]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #11)
> (In reply to Michael Wu [:mwu] from comment #10)
> > I can't reproduce this on the flame.
> 
> Hi Michael,
> You could turn on the resample, and test again on flame device.
> 
> in gfx/thebes/gfxPrefs.h,
> +pref("gfx.frameuniformity.hw-vsync", true);
> +pref("gfx.touch.resample", true);

I think this wired behavior happens only when we enable the touch re-sampling, and I even cannot select any option in the settings. In other words, it may break the touch re-sampling.

Comment 13

[Michael Wu [:mwu]]

Ok. I think this exposes a bug in the touch sampling. Can you file a new bug for this?

Comment 14

[Boris Chiou [:boris]]

(In reply to Michael Wu [:mwu] from comment #13)
> Ok. I think this exposes a bug in the touch sampling. Can you file a new bug
> for this?

Sure, please check Bug 1073704. Thanks.

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

mwu - Sounds like we need to wait for bug 1073704 to be uplifted before uplifting this bug. Correct?

Comment 16

[Michael Wu [:mwu]]

Bug 1073704 is actually a direct regression from this bug. If we uplift this, I'll combine bug 1073704 with this. I accidentally deleted too much code and bug 1073704 puts it back.

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8494843 [details] [diff] [review]
Use stateless expired event filtering, v2

Aurora+. Please proceed as you said in comment 16 by combining the fix for bug 1073704 when landing this fix. You can carry forward my Aurora approval.

Comment 18

[Michael Wu [:mwu]]

Attachment: Use stateless expired event filtering, v3 (For Aurora)

Comment 19

[Michael Wu [:mwu]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/77aa40674089

Comment 20

[ying.xu]

@mwu

can you tell me the meaning of this code？

aMultiTouch.mTouches.Length() == 1

Comment 21

[ying.xu]

Hi，mwu.

I can't figure out the reason of aMultiTouch.mTouches.Length() == 1
Can you explain a little？

And I think you miss the dispatch timeout of touchmove event. Is this deliberate？
Can you explain this too？

Comment 22

[Michael Wu [:mwu]]

Entire sets of touches (touch start, touch move, touch end) must be expired, so we can only check the timestamp at certain points in the input event stream. In this case, we decide when to drop events at the beginning of a set of touches (aMultiTouch.mTouches.Length() == 1).