Closed
Bug 1069037
Opened 10 years ago
Closed 10 years ago
Prevent Touch Events From Piling Up If main Thread is Busy
Categories
(Core Graveyard :: Widget: Gonk, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla36
People
(Reporter: mchang, Assigned: mchang)
References
Details
Attachments
(1 file, 5 obsolete files)
4.89 KB,
patch
|
mchang
:
review+
|
Details | Diff | Splinter Review |
:mwu pointed out a bug where if the main thread is very busy, we don't always throw out touch events. We can pile up touch events and dispatch them in rapid succession, which just adds to more load. We have a couple of ideas.
If resampling is enabled, check to make sure that the current runnable is within some configurable number of the last vsync, otherwise return. If resampling is disabled, we'll send only the last touch event.
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
Did a couple of things to prevent touch events from piling up. If touch resampling is disabled, we overwrite the last touch move event, which was what was happening before we introduced GeckoTouchDispatcher. If touch resampling is enabled, we check the vsync time versus the last touch time. If the time between the vsync event occured and when the main thread finally processes the touch is greater than 20ms, we assume the main thread is too busy and just dispatch the last touch event.
Attachment #8502785 -
Flags: review?(mwu)
Comment 2•10 years ago
|
||
Comment on attachment 8502785 [details] [diff] [review]
Prevent Touch Events From Piling Up On Main Thread
Review of attachment 8502785 [details] [diff] [review]:
-----------------------------------------------------------------
::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +151,5 @@
> + mTouchMoveEvents.push_back(aData);
> + mTouchTimeDiff = aEventTime - mLastTouchTime;
> + mLastTouchTime = aEventTime;
> + return;
> + } else if (mTouchMoveEvents.empty()) {
Using else if here is a bit confusing. I suggest just breaking this off into its own if statement.
@@ +178,5 @@
> int touchCount = mTouchMoveEvents.size();
> // Both aVsynctime and mLastTouchTime are uint64_t
> // Need to store as a signed int.
> int64_t vsyncTouchDiff = aVsyncTime - mLastTouchTime;
> + bool isDelayedTouch = vsyncTouchDiff < mDelayedTouchThreshold;
This is always true when resampling is disabled, which makes the code that checks isDelayedTouch pointless. Please check the logic here again.
Attachment #8502785 -
Flags: review?(mwu)
Assignee | ||
Comment 3•10 years ago
|
||
Have a couple of changes as talked about on irc.
1) Made code more explicit that it needs to rely on touch resampling being enabled.
2) Modified GeckoTouchDispatcher to always use mozilla::TimeStamp instead of the android event time. Code that uses this is in the VsyncFramework in bug 1048667.
3) Also changed the resampling a bit to be more accurate. Before, we could get touch events that occurred after a vsync. We would then always use the last two touch events to resample, which means we would include a touch event that occurred after the vsync event. Changed it so that we always process touch events that occur before the current vsync event.
4) Also filter heavily based on timings. I've seen 10-20 ms roundtrip timings between posting an event on the main thread and the event actually executing. I've also seen the inverse where a vsync event occurs and no touch events have been added due to scheduling issues. We no longer resample in those cases, send the last touch event, and clear touch events until we catch up again.
Attachment #8504408 -
Flags: review?(mwu)
Assignee | ||
Updated•10 years ago
|
Attachment #8502785 -
Attachment is obsolete: true
Assignee | ||
Comment 4•10 years ago
|
||
Fixed a build issue.
Attachment #8504408 -
Attachment is obsolete: true
Attachment #8504408 -
Flags: review?(mwu)
Attachment #8504427 -
Flags: review?(mwu)
Comment 5•10 years ago
|
||
Comment on attachment 8504427 [details] [diff] [review]
Refactor GeckoTouchDispatcher
Review of attachment 8504427 [details] [diff] [review]:
-----------------------------------------------------------------
Way too many changes in here. Please split into a patch that fixes *just* this bug, and we can look at refactoring as a separate thing.
::: widget/InputData.h
@@ +7,5 @@
> #define InputData_h__
>
> +#include "Units.h"
> +#include "mozilla/EventForwards.h"
> +#include "mozilla/TimeStamp.h"
Why is this being moved?
Attachment #8504427 -
Flags: review?(mwu)
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Michael Wu [:mwu] from comment #5)
> Comment on attachment 8504427 [details] [diff] [review]
> Refactor GeckoTouchDispatcher
>
> Review of attachment 8504427 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Way too many changes in here. Please split into a patch that fixes *just*
> this bug, and we can look at refactoring as a separate thing.
>
> ::: widget/InputData.h
> @@ +7,5 @@
> > #define InputData_h__
> >
> > +#include "Units.h"
> > +#include "mozilla/EventForwards.h"
> > +#include "mozilla/TimeStamp.h"
>
> Why is this being moved?
I just noticed that it wasn't alphabetically sorted, so I thought I'd clean that bit up since I was looking at that code.
Assignee | ||
Comment 7•10 years ago
|
||
Simplified a bit. Does two main things:
1) If touch resampling is disabled, we coalesce touch moves when we get a touch event.
2) If touch resampling is enabled, we check for delayed vsync events (e.g. the main thread can't process touch events fast enough). In these cases, we just send the last touch event.
Attachment #8504427 -
Attachment is obsolete: true
Attachment #8505798 -
Flags: review?(mwu)
Comment 8•10 years ago
|
||
Comment on attachment 8505798 [details] [diff] [review]
Prevent Touch Events From Piling Up On Main Thread
Review of attachment 8505798 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/thebes/gfxPrefs.h
@@ +208,5 @@
> // These times should be in nanoseconds
> DECL_GFX_PREF(Once, "gfx.touch.resample.max-predict", TouchResampleMaxPredict, int32_t, 8000000);
> DECL_GFX_PREF(Once, "gfx.touch.resample.vsync-adjust", TouchVsyncSampleAdjust, int32_t, 5000000);
> DECL_GFX_PREF(Once, "gfx.touch.resample.min-resample", TouchResampleMinTime, int32_t, 2000000);
> + DECL_GFX_PREF(Once, "gfx.touch.resample.delay-threshold", TouchResampleVsyncDelayThreshold, int32_t, -20000000);
This needs to be negative due to how the math in checking the threshold is done, which seems wrong. Make this take positive numbers and adjust the math accordingly.
::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +146,2 @@
> {
> + if (aTouch.mType == MultiTouchInput::MULTITOUCH_MOVE ) {
nit: remove the space before )
@@ +146,4 @@
> {
> + if (aTouch.mType == MultiTouchInput::MULTITOUCH_MOVE ) {
> + MutexAutoLock lock(mTouchQueueLock);
> + if (mResamplingEnabled || mTouchMoveEvents.empty()) {
I think this would be little bit more clear if there were separate blocks for each condition -
if (mResamplingEnabled) {
...
}
if (mTouchMoveEvents.empty()) {
...
} else {
...
}
Attachment #8505798 -
Flags: review?(mwu)
Assignee | ||
Comment 9•10 years ago
|
||
Updated with feedback from comment 8.
Attachment #8505798 -
Attachment is obsolete: true
Attachment #8506394 -
Flags: review?(mwu)
Comment 10•10 years ago
|
||
Comment on attachment 8506394 [details] [diff] [review]
Prevent Touch Events From Piling Up On Main Thread
Review of attachment 8506394 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, thanks.
Attachment #8506394 -
Flags: review?(mwu) → review+
Assignee | ||
Comment 11•10 years ago
|
||
Properly formatted commit message. Carrying r+. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5990cd5f7a7b
Attachment #8506394 -
Attachment is obsolete: true
Attachment #8506404 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 12•10 years ago
|
||
Keywords: checkin-needed
Comment 13•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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
•