mouse: only clear scroll remainders after 1 second
When scrolling slowly on the Microsoft Surface Precision mouse, with
high-resolution scrolling enabled and with the wheel in smooth mode, the
page wasn't moving. This was due to `IntegralGestureFilterInterpreter`
resetting its remainders every time `SyncInterpret` was called (i.e.
every evdev `SYN_REPORT`). (Apparently, the reason the remainders need
resetting at all is to allow recordings to be replayed consistently.)
Changing it to only reset after a second of inactivity fixes this.
The 1-second deadline meant using a timer, so also moved some common
logic from `FlingStopFilterInterpreter` into `FilterInterpreter`.
BUG=chromium:888172
TEST=scroll slowly on MS Surface Precision, check that the page scrolls;
run touchtests and unit tests
Change-Id: Ic5a4ef887100badf2fa29162232a6ff3cc0148bd
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/gestures/+/2135286
Tested-by: Harry Cutts <hcutts@chromium.org>
Reviewed-by: Andrew de los Reyes <adlr@chromium.org>
Reviewed-by: Sean O'Brien <seobrien@chromium.org>
Commit-Queue: Harry Cutts <hcutts@chromium.org>
diff --git a/Makefile b/Makefile
index 68ac097..0c058d7 100644
--- a/Makefile
+++ b/Makefile
@@ -50,6 +50,7 @@
$(OBJDIR)/box_filter_interpreter_unittest.o \
$(OBJDIR)/click_wiggle_filter_interpreter_unittest.o \
$(OBJDIR)/command_line.o \
+ $(OBJDIR)/filter_interpreter_unittest.o \
$(OBJDIR)/fling_stop_filter_interpreter_unittest.o \
$(OBJDIR)/gestures_unittest.o \
$(OBJDIR)/iir_filter_interpreter_unittest.o \
diff --git a/include/filter_interpreter.h b/include/filter_interpreter.h
index e1ff60b..4b33f26 100644
--- a/include/filter_interpreter.h
+++ b/include/filter_interpreter.h
@@ -4,6 +4,7 @@
#include <memory>
+#include <gtest/gtest.h>
#include <json/value.h>
#include "gestures/include/interpreter.h"
@@ -19,6 +20,11 @@
// Interface for all filter interpreters.
class FilterInterpreter : public Interpreter, public GestureConsumer {
+ FRIEND_TEST(FilterInterpreterTest, DeadlineSettingNoDeadlines);
+ FRIEND_TEST(FilterInterpreterTest, DeadlineSettingLocalOnly);
+ FRIEND_TEST(FilterInterpreterTest, DeadlineSettingNextOnly);
+ FRIEND_TEST(FilterInterpreterTest, DeadlineSettingLocalBeforeNext);
+ FRIEND_TEST(FilterInterpreterTest, DeadlineSettingNextBeforeLocal);
public:
FilterInterpreter(PropRegistry* prop_reg,
Interpreter* next,
@@ -40,6 +46,17 @@
virtual void SyncInterpretImpl(HardwareState* hwstate, stime_t* timeout);
virtual void HandleTimerImpl(stime_t now, stime_t* timeout);
+ // When we need to call HandlerTimer on next_, or 0.0 if no outstanding timer.
+ stime_t next_timer_deadline_ = 0.0;
+ // Sets the next timer deadline, taking into account the deadline needed for
+ // this interpreter and the one from the next in the chain.
+ stime_t SetNextDeadlineAndReturnTimeoutVal(stime_t now,
+ stime_t local_deadline,
+ stime_t next_timeout);
+ // Utility method for determining whether the timer callback is for this
+ // interpreter or one further down the chain.
+ bool ShouldCallNextTimer(stime_t local_deadline);
+
std::unique_ptr<Interpreter> next_;
private:
diff --git a/include/fling_stop_filter_interpreter.h b/include/fling_stop_filter_interpreter.h
index 3c82bc3..25b0912 100644
--- a/include/fling_stop_filter_interpreter.h
+++ b/include/fling_stop_filter_interpreter.h
@@ -42,7 +42,6 @@
bool NeedsExtraTime(const HardwareState& hwstate) const;
bool FlingStopNeeded(const Gesture& gesture) const;
void UpdateFlingStopDeadline(const HardwareState& hwstate);
- stime_t SetNextDeadlineAndReturnTimeoutVal(stime_t now, stime_t next_timeout);
// Has the deadline has already been extended once
bool already_extended_;
@@ -65,8 +64,6 @@
// When we should send fling-stop, or 0.0 if not set.
stime_t fling_stop_deadline_;
- // When we need to call HandlerTimer on next_, or 0.0 if no outstanding timer.
- stime_t next_timer_deadline_;
// Device class (e.g. touchpad, mouse).
GestureInterpreterDeviceClass devclass_;
diff --git a/include/integral_gesture_filter_interpreter.h b/include/integral_gesture_filter_interpreter.h
index 4db5386..5bbb789 100644
--- a/include/integral_gesture_filter_interpreter.h
+++ b/include/integral_gesture_filter_interpreter.h
@@ -32,6 +32,10 @@
float hscroll_remainder_, vscroll_remainder_;
float hscroll_ordinal_remainder_, vscroll_ordinal_remainder_;
+ bool can_clear_remainders_;
+
+ stime_t remainder_reset_deadline_;
+ virtual void HandleTimerImpl(stime_t now, stime_t *timeout);
};
} // namespace gestures
diff --git a/src/filter_interpreter.cc b/src/filter_interpreter.cc
index 0d5bc92..220cefb 100644
--- a/src/filter_interpreter.cc
+++ b/src/filter_interpreter.cc
@@ -43,4 +43,27 @@
log_->Clear();
next_->Clear();
}
+
+stime_t FilterInterpreter::SetNextDeadlineAndReturnTimeoutVal(
+ stime_t now, stime_t local_deadline, stime_t next_timeout) {
+ next_timer_deadline_ = next_timeout > 0.0 ? now + next_timeout : 0.0;
+ stime_t local_timeout =
+ local_deadline <= 0.0 ? -1.0 : std::max(local_deadline - now, 0.0);
+
+ if (next_timeout <= 0.0 && local_timeout <= 0.0)
+ return -1.0;
+ if (next_timeout <= 0.0)
+ return local_timeout;
+ if (local_timeout <= 0.0)
+ return next_timeout;
+ return std::min(next_timeout, local_timeout);
+}
+
+bool FilterInterpreter::ShouldCallNextTimer(stime_t local_deadline) {
+ if (local_deadline > 0.0 && next_timer_deadline_ > 0.0)
+ return local_deadline > next_timer_deadline_;
+ else
+ return next_timer_deadline_ > 0.0;
+}
+
} // namespace gestures
diff --git a/src/filter_interpreter_unittest.cc b/src/filter_interpreter_unittest.cc
new file mode 100644
index 0000000..6d629fc
--- /dev/null
+++ b/src/filter_interpreter_unittest.cc
@@ -0,0 +1,69 @@
+// Copyright 2020 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <string>
+
+#include <gtest/gtest.h>
+
+#include "gestures/include/gestures.h"
+#include "gestures/include/filter_interpreter.h"
+#include "gestures/include/unittest_util.h"
+#include "gestures/include/util.h"
+
+using std::string;
+
+namespace gestures {
+
+class FilterInterpreterTest : public ::testing::Test {
+ public:
+ FilterInterpreterTest() : interpreter(NULL, NULL, NULL, false) {}
+
+ protected:
+ FilterInterpreter interpreter;
+};
+
+TEST_F(FilterInterpreterTest, DeadlineSettingNoDeadlines) {
+ stime_t timeout_val =
+ interpreter.SetNextDeadlineAndReturnTimeoutVal(10000.0, 0.0, 0.0);
+ EXPECT_FLOAT_EQ(-1.0, timeout_val);
+ EXPECT_FLOAT_EQ(0.0, interpreter.next_timer_deadline_);
+}
+
+TEST_F(FilterInterpreterTest, DeadlineSettingLocalOnly) {
+ stime_t timeout_val =
+ interpreter.SetNextDeadlineAndReturnTimeoutVal(10000.0, 10001.0, 0.0);
+ EXPECT_FLOAT_EQ(1.0, timeout_val);
+ EXPECT_FLOAT_EQ(0.0, interpreter.next_timer_deadline_);
+
+ EXPECT_FALSE(interpreter.ShouldCallNextTimer(10001.00));
+}
+
+TEST_F(FilterInterpreterTest, DeadlineSettingNextOnly) {
+ stime_t timeout_val =
+ interpreter.SetNextDeadlineAndReturnTimeoutVal(10000.0, 0.0, 1.0);
+ EXPECT_FLOAT_EQ(1.0, timeout_val);
+ EXPECT_FLOAT_EQ(10001.0, interpreter.next_timer_deadline_);
+
+ EXPECT_TRUE(interpreter.ShouldCallNextTimer(0.0));
+}
+
+TEST_F(FilterInterpreterTest, DeadlineSettingLocalBeforeNext) {
+ stime_t timeout_val =
+ interpreter.SetNextDeadlineAndReturnTimeoutVal(10000.0, 10001.0, 2.0);
+ EXPECT_FLOAT_EQ(1.0, timeout_val);
+ EXPECT_FLOAT_EQ(10002.0, interpreter.next_timer_deadline_);
+
+ EXPECT_FALSE(interpreter.ShouldCallNextTimer(10001.0));
+}
+
+TEST_F(FilterInterpreterTest, DeadlineSettingNextBeforeLocal) {
+ stime_t timeout_val =
+ interpreter.SetNextDeadlineAndReturnTimeoutVal(10000.0, 10002.0, 1.0);
+ EXPECT_FLOAT_EQ(1.0, timeout_val);
+ EXPECT_FLOAT_EQ(10001.0, interpreter.next_timer_deadline_);
+
+ EXPECT_TRUE(interpreter.ShouldCallNextTimer(10002.0));
+}
+
+} // namespace gestures
diff --git a/src/fling_stop_filter_interpreter.cc b/src/fling_stop_filter_interpreter.cc
index d266011..4c557ea 100644
--- a/src/fling_stop_filter_interpreter.cc
+++ b/src/fling_stop_filter_interpreter.cc
@@ -17,7 +17,6 @@
prev_gesture_type_(kGestureTypeNull),
fling_stop_already_sent_(false),
fling_stop_deadline_(0.0),
- next_timer_deadline_(0.0),
devclass_(devclass),
fling_stop_timeout_(prop_reg, "Fling Stop Timeout", 0.03),
fling_stop_extra_delay_(prop_reg, "Fling Stop Extra Delay", 0.055) {
@@ -49,6 +48,7 @@
}
next_->SyncInterpret(hwstate, &next_timeout);
*timeout = SetNextDeadlineAndReturnTimeoutVal(hwstate->timestamp,
+ fling_stop_deadline_,
next_timeout);
}
@@ -114,31 +114,9 @@
prev_touch_cnt_ = hwstate.touch_cnt;
}
-stime_t FlingStopFilterInterpreter::SetNextDeadlineAndReturnTimeoutVal(
- stime_t now,
- stime_t next_timeout) {
- next_timer_deadline_ = next_timeout >= 0.0 ? now + next_timeout : 0.0;
- stime_t local_timeout = fling_stop_deadline_ == 0.0 ? -1.0 :
- std::max(fling_stop_deadline_ - now, 0.0);
-
- if (next_timeout < 0.0 && local_timeout < 0.0)
- return -1.0;
- if (next_timeout < 0.0)
- return local_timeout;
- if (local_timeout < 0.0)
- return next_timeout;
- return std::min(next_timeout, local_timeout);
-}
-
void FlingStopFilterInterpreter::HandleTimerImpl(stime_t now,
stime_t* timeout) {
- bool call_next = false;
- if (fling_stop_deadline_ > 0.0 && next_timer_deadline_ > 0.0)
- call_next = fling_stop_deadline_ > next_timer_deadline_;
- else
- call_next = next_timer_deadline_ > 0.0;
-
- if (!call_next) {
+ if (!ShouldCallNextTimer(fling_stop_deadline_)) {
if (fling_stop_deadline_ > now) {
Err("Spurious callback. now: %f, fs deadline: %f, next deadline: %f",
now, fling_stop_deadline_, next_timer_deadline_);
@@ -151,7 +129,8 @@
fling_stop_already_sent_ = true;
stime_t next_timeout = next_timer_deadline_ == 0.0 ? -1.0 :
std::max(0.0, next_timer_deadline_ - now);
- *timeout = SetNextDeadlineAndReturnTimeoutVal(now, next_timeout);
+ *timeout = SetNextDeadlineAndReturnTimeoutVal(now, fling_stop_deadline_,
+ next_timeout);
return;
}
// Call next_
@@ -162,7 +141,8 @@
}
stime_t next_timeout = -1.0;
next_->HandleTimer(now, &next_timeout);
- *timeout = SetNextDeadlineAndReturnTimeoutVal(now, next_timeout);
+ *timeout = SetNextDeadlineAndReturnTimeoutVal(now, fling_stop_deadline_,
+ next_timeout);
}
} // namespace gestures
diff --git a/src/integral_gesture_filter_interpreter.cc b/src/integral_gesture_filter_interpreter.cc
index 3360e63..d2f2ce4 100644
--- a/src/integral_gesture_filter_interpreter.cc
+++ b/src/integral_gesture_filter_interpreter.cc
@@ -8,6 +8,7 @@
#include "gestures/include/gestures.h"
#include "gestures/include/interpreter.h"
+#include "gestures/include/logging.h"
#include "gestures/include/tracer.h"
namespace gestures {
@@ -19,16 +20,51 @@
hscroll_remainder_(0.0),
vscroll_remainder_(0.0),
hscroll_ordinal_remainder_(0.0),
- vscroll_ordinal_remainder_(0.0) {
+ vscroll_ordinal_remainder_(0.0),
+ remainder_reset_deadline_(-1.0) {
InitName();
}
void IntegralGestureFilterInterpreter::SyncInterpretImpl(
HardwareState* hwstate, stime_t* timeout) {
- if (hwstate->finger_cnt == 0 && hwstate->touch_cnt == 0)
- hscroll_ordinal_remainder_ = vscroll_ordinal_remainder_ =
- hscroll_remainder_ = vscroll_remainder_ = 0.0;
- next_->SyncInterpret(hwstate, timeout);
+ can_clear_remainders_ = hwstate->finger_cnt == 0 && hwstate->touch_cnt == 0;
+ stime_t next_timeout = -1.0;
+ next_->SyncInterpret(hwstate, &next_timeout);
+ *timeout = SetNextDeadlineAndReturnTimeoutVal(
+ hwstate->timestamp, remainder_reset_deadline_, next_timeout);
+}
+
+void IntegralGestureFilterInterpreter::HandleTimerImpl(
+ stime_t now, stime_t *timeout) {
+ if (ShouldCallNextTimer(remainder_reset_deadline_)) {
+ if (next_timer_deadline_ > now) {
+ Err("Spurious callback. now: %f, next deadline: %f",
+ now, next_timer_deadline_);
+ return;
+ }
+
+ stime_t next_timeout = -1.0;
+ next_->HandleTimer(now, &next_timeout);
+ *timeout = SetNextDeadlineAndReturnTimeoutVal(now,
+ remainder_reset_deadline_,
+ next_timeout);
+ } else {
+ if (remainder_reset_deadline_ > now) {
+ Err("Spurious callback. now: %f, remainder reset deadline: %f",
+ now, remainder_reset_deadline_);
+ return;
+ }
+ if (can_clear_remainders_)
+ hscroll_ordinal_remainder_ = vscroll_ordinal_remainder_ =
+ hscroll_remainder_ = vscroll_remainder_ = 0.0;
+
+ remainder_reset_deadline_ = -1.0;
+ stime_t next_timeout = next_timer_deadline_ <= 0.0 ? -1.0 :
+ std::max(0.0, next_timer_deadline_ - now);
+ *timeout = SetNextDeadlineAndReturnTimeoutVal(now,
+ remainder_reset_deadline_,
+ next_timeout);
+ }
}
namespace {
@@ -69,6 +105,7 @@
ProduceGesture(Gesture(kGestureFling, copy.start_time, copy.end_time,
0, 0, GESTURES_FLING_TAP_DOWN));
}
+ remainder_reset_deadline_ = copy.end_time + 1.0;
break;
default:
ProduceGesture(gesture);
diff --git a/src/integral_gesture_filter_interpreter_unittest.cc b/src/integral_gesture_filter_interpreter_unittest.cc
index 77c19db..0821eca 100644
--- a/src/integral_gesture_filter_interpreter_unittest.cc
+++ b/src/integral_gesture_filter_interpreter_unittest.cc
@@ -38,7 +38,7 @@
}
virtual void HandleTimer(stime_t now, stime_t* timeout) {
- EXPECT_TRUE(false);
+ ADD_FAILURE() << "HandleTimer on the next interpreter shouldn't be called";
}
Gesture return_value_;
@@ -88,7 +88,8 @@
ASSERT_EQ(arraysize(expected_types), arraysize(expected_y));
for (size_t i = 0; i < arraysize(expected_x); i++) {
- Gesture* out = wrapper.SyncInterpret(&hs, NULL);
+ stime_t timeout;
+ Gesture* out = wrapper.SyncInterpret(&hs, &timeout);
if (out)
EXPECT_EQ(expected_types[i], out->type) << "i = " << i;
if (out == NULL) {
@@ -116,26 +117,30 @@
// causing finger, dx, dy, fingers, buttons down, buttons mask, hwstate:
base_interpreter->return_values_.push_back(
- Gesture(kGestureScroll, 0, 0, 3.9, 0.0));
+ Gesture(kGestureScroll, 10000.00, 10000.00, 3.9, 0.0));
+ Gesture empty_gesture = Gesture();
+ empty_gesture.start_time = 10000.01;
+ empty_gesture.end_time = 10000.01;
+ base_interpreter->return_values_.push_back(empty_gesture);
base_interpreter->return_values_.push_back(
- Gesture());
- base_interpreter->return_values_.push_back(
- Gesture(kGestureScroll, 0, 0, .2, 0.0));
+ Gesture(kGestureScroll, 10001.02, 10001.02, .2, 0.0));
FingerState fs = { 0, 0, 0, 0, 1, 0, 0, 0, 1, 0 };
HardwareState hs[] = {
make_hwstate(10000.00, 0, 1, 1, &fs),
make_hwstate(10000.01, 0, 0, 0, NULL),
- make_hwstate(10000.02, 0, 1, 1, &fs),
+ make_hwstate(10001.02, 0, 1, 1, &fs),
};
size_t iter = 0;
- Gesture* out = wrapper.SyncInterpret(&hs[iter++], NULL);
+ stime_t timeout;
+ Gesture* out = wrapper.SyncInterpret(&hs[iter++], &timeout);
ASSERT_NE(reinterpret_cast<Gesture*>(NULL), out);
EXPECT_EQ(kGestureTypeScroll, out->type);
- out = wrapper.SyncInterpret(&hs[iter++], NULL);
+ out = wrapper.SyncInterpret(&hs[iter++], &timeout);
EXPECT_EQ(reinterpret_cast<Gesture*>(NULL), out);
- out = wrapper.SyncInterpret(&hs[iter++], NULL);
+ wrapper.HandleTimer(10001.02, &timeout);
+ out = wrapper.SyncInterpret(&hs[iter++], &timeout);
EXPECT_EQ(reinterpret_cast<Gesture*>(NULL), out);
}
@@ -159,10 +164,60 @@
};
size_t iter = 0;
- Gesture* out = wrapper.SyncInterpret(&hs[iter++], NULL);
+ stime_t timeout;
+ Gesture* out = wrapper.SyncInterpret(&hs[iter++], &timeout);
EXPECT_EQ(reinterpret_cast<Gesture*>(NULL), out);
- out = wrapper.SyncInterpret(&hs[iter++], NULL);
+ out = wrapper.SyncInterpret(&hs[iter++], &timeout);
EXPECT_EQ(reinterpret_cast<Gesture*>(NULL), out);
}
+// A bunch of scroll gestures with dy < 1 (such as the MS Surface Precision
+// mouse produces) should be combined into a smaller number of gestures.
+TEST(IntegralGestureFilterInterpreterTest, SlowScrollTest) {
+ IntegralGestureFilterInterpreterTestInterpreter* base_interpreter =
+ new IntegralGestureFilterInterpreterTestInterpreter;
+ IntegralGestureFilterInterpreter interpreter(base_interpreter, NULL);
+ TestInterpreterWrapper wrapper(&interpreter);
+
+ base_interpreter->return_values_.push_back(
+ Gesture(kGestureScroll, 10000.00, 10000.00, 0.0, 0.4));
+ base_interpreter->return_values_.push_back(
+ Gesture(kGestureScroll, 10000.05, 10000.05, 0.0, 0.4));
+ base_interpreter->return_values_.push_back(
+ Gesture(kGestureScroll, 10000.10, 10000.10, 0.0, 0.4));
+ base_interpreter->return_values_.push_back(
+ Gesture(kGestureScroll, 10000.15, 10000.15, 0.0, 0.4));
+ base_interpreter->return_values_.push_back(
+ Gesture(kGestureScroll, 10000.20, 10000.20, 0.0, 0.4));
+
+ HardwareState hs[] = {
+ make_hwstate(10000.00, 0, 0, 0, NULL),
+ make_hwstate(10000.05, 0, 0, 0, NULL),
+ make_hwstate(10000.10, 0, 0, 0, NULL),
+ make_hwstate(10000.15, 0, 0, 0, NULL),
+ make_hwstate(10000.20, 0, 0, 0, NULL),
+ };
+
+ size_t iter = 0;
+ stime_t timeout;
+ // The first two gestures should just add to the vertical scroll remainder.
+ Gesture* out = wrapper.SyncInterpret(&hs[iter++], &timeout);
+ EXPECT_EQ(reinterpret_cast<Gesture*>(NULL), out);
+ out = wrapper.SyncInterpret(&hs[iter++], &timeout);
+ EXPECT_EQ(reinterpret_cast<Gesture*>(NULL), out);
+ // Then the remainder exceeds 1 so we should get a gesture.
+ out = wrapper.SyncInterpret(&hs[iter++], &timeout);
+ EXPECT_NE(reinterpret_cast<Gesture*>(NULL), out);
+ EXPECT_EQ(kGestureTypeScroll, out->type);
+ EXPECT_FLOAT_EQ(1.0, out->details.scroll.dy);
+ // The next event just adds to the remainder again.
+ out = wrapper.SyncInterpret(&hs[iter++], &timeout);
+ EXPECT_EQ(reinterpret_cast<Gesture*>(NULL), out);
+ // Then the remainder exceeds 1 again.
+ out = wrapper.SyncInterpret(&hs[iter++], &timeout);
+ EXPECT_NE(reinterpret_cast<Gesture*>(NULL), out);
+ EXPECT_EQ(kGestureTypeScroll, out->type);
+ EXPECT_FLOAT_EQ(1.0, out->details.scroll.dy);
+}
+
} // namespace gestures
