Closed
Bug 1072645
Opened 10 years ago
Closed 10 years ago
Improve touch filtering logic in GeckoTouchDispatcher
Categories
(Core Graveyard :: Widget: Gonk, defect)
Tracking
(firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)
RESOLVED
FIXED
mozilla35
People
(Reporter: mwu, Assigned: mwu)
References
Details
Attachments
(3 files, 1 obsolete file)
2.97 KB,
patch
|
kats
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.26 MB,
video/mp4
|
Details | |
2.22 KB,
patch
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
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 2•10 years ago
|
||
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+
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•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/879502010e5d
Comment 6•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/879502010e5d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Assignee | ||
Comment 7•10 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?
Comment 8•10 years ago
|
||
weird overscrolling behavior with this commit.
Comment 9•10 years ago
|
||
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)
Comment 11•10 years ago
|
||
(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)
Comment 12•10 years ago
|
||
(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•10 years ago
|
||
Ok. I think this exposes a bug in the touch sampling. Can you file a new bug for this?
Flags: needinfo?(mwu)
Comment 14•10 years ago
|
||
(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.
Updated•10 years ago
|
status-b2g-v2.1:
--- → affected
status-b2g-v2.2:
--- → fixed
Comment 15•10 years ago
|
||
mwu - Sounds like we need to wait for bug 1073704 to be uplifted before uplifting this bug. Correct?
Flags: needinfo?(mwu)
Assignee | ||
Comment 16•10 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 17•10 years ago
|
||
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•10 years ago
|
||
Assignee | ||
Comment 19•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/77aa40674089
Updated•10 years ago
|
Comment 20•10 years ago
|
||
@mwu can you tell me the meaning of this code? aMultiTouch.mTouches.Length() == 1
Comment 21•10 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•10 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)
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•