Improve touch filtering logic in GeckoTouchDispatcher

RESOLVED FIXED in Firefox 34, Firefox OS v2.1

Status

()

Core
Widget: Gonk
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mwu, Assigned: mwu)

Tracking

unspecified
mozilla35
All
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8494837 [details] [diff] [review]
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..)
Attachment #8494837 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 1

3 years ago
Created attachment 8494843 [details] [diff] [review]
Use stateless expired event filtering, v2

Just realized this code can be simplified further.
Attachment #8494837 - Attachment is obsolete: true
Attachment #8494837 - Flags: review?(bugmail.mozilla)
Attachment #8494843 - Flags: review?(bugmail.mozilla)
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.
Attachment #8494843 - Flags: review?(bugmail.mozilla) → review+
Duplicate of this bug: 1072086
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 ...
(Assignee)

Comment 5

3 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/879502010e5d
https://hg.mozilla.org/mozilla-central/rev/879502010e5d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
(Assignee)

Comment 7

3 years ago
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.
Attachment #8494843 - Flags: approval-mozilla-aurora?
Created attachment 8496153 [details]
input_resample.mp4

weird overscrolling behavior with this commit.
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].
Flags: needinfo?(mwu)
(Assignee)

Comment 10

3 years ago
I can't reproduce this on the flame.
Flags: needinfo?(mwu)
(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);
Flags: needinfo?(mwu)
(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.
(Assignee)

Comment 13

3 years ago
Ok. I think this exposes a bug in the touch sampling. Can you file a new bug for this?
Flags: needinfo?(mwu)
(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.
Blocks: 1073704
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
mwu - Sounds like we need to wait for bug 1073704 to be uplifted before uplifting this bug. Correct?
Flags: needinfo?(mwu)
(Assignee)

Comment 16

3 years ago
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.
Flags: needinfo?(mwu)
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.
Attachment #8494843 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 18

3 years ago
Created attachment 8498490 [details] [diff] [review]
Use stateless expired event filtering, v3 (For Aurora)
(Assignee)

Comment 19

3 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/77aa40674089
status-b2g-v2.1: affected → fixed
status-firefox33: --- → wontfix
status-firefox34: --- → fixed
status-firefox35: --- → fixed

Comment 20

3 years ago
@mwu

can you tell me the meaning of this code?

aMultiTouch.mTouches.Length() == 1

Comment 21

3 years ago
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?
Flags: needinfo?(mwu)
(Assignee)

Comment 22

3 years ago
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).
Flags: needinfo?(mwu)
You need to log in before you can comment on or make changes to this bug.