Event smoothing in CrOS gesture recognizer.

Event smoothing in aura gesture recognizer.

Each GesturePoint owns a VelocityCalculator, which maintains a history
of touch positions and times, and gives the GesturePoint its velocity.

An ordinary least squares regression is used to calculate the velocities.

BUG=110229
TEST=aura_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120835

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=120996

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=121201

[rjkroege, 2012-02-02 15:58:36]

http://codereview.chromium.org/9310031/diff/2001/ui/aura/gestures/gesture_poi...
File ui/aura/gestures/gesture_point.cc (right):

http://codereview.chromium.org/9310031/diff/2001/ui/aura/gestures/gesture_poi...
ui/aura/gestures/gesture_point.cc:40: const double event_timestamp_seconds =
event.time_stamp().InSecondsF();
if our compiler is doing the right thing with SSE, float would give us a
non-trivial performance boost.

http://codereview.chromium.org/9310031/diff/2001/ui/aura/gestures/gesture_poi...
ui/aura/gestures/gesture_point.cc:42: velocity_calculator_.PointSeen(event.x(),
you need to move these over one char I think.

http://codereview.chromium.org/9310031/diff/2001/ui/aura/gestures/velocity_ca...
File ui/aura/gestures/velocity_calculator.cc (right):

http://codereview.chromium.org/9310031/diff/2001/ui/aura/gestures/velocity_ca...
ui/aura/gestures/velocity_calculator.cc:57: float tt = 0;  // sum_i(t_i * t_i)
you are using doubles somewhere else. and here, you use a float. either you need
all the bits... or you don't. :-)

http://codereview.chromium.org/9310031/diff/2001/ui/aura/gestures/velocity_ca...
ui/aura/gestures/velocity_calculator.cc:62: tt += (buffer_[i].time - mean_time)
* (buffer_[i].time - mean_time);
this algorithm can be numerically unstable. See:
http://www.jstor.org/pss/2156903  -- is your matrix always well conditioned?

http://codereview.chromium.org/9310031/diff/2001/ui/aura/gestures/velocity_ca...
File ui/aura/gestures/velocity_calculator.h (right):

http://codereview.chromium.org/9310031/diff/2001/ui/aura/gestures/velocity_ca...
ui/aura/gestures/velocity_calculator.h:28: typedef std::vector<Point>
HistoryBuffer;
since the array is fixed size, why not an array?

[Ian Vollick, 2012-02-02 16:40:23]

http://codereview.chromium.org/9310031/diff/2001/ui/aura/gestures/velocity_ca...
File ui/aura/gestures/velocity_calculator.h (right):

http://codereview.chromium.org/9310031/diff/2001/ui/aura/gestures/velocity_ca...
ui/aura/gestures/velocity_calculator.h:19: void Velocity(float* x, float*y);
Since we want to get the velocity after every point is added, it would make
sense to update the calculated velocities in PointSeen, and then you could have
simple accessors like velocity_x() and velocity_y() for getting the current
estimates.

http://codereview.chromium.org/9310031/diff/2001/ui/aura/gestures/velocity_ca...
ui/aura/gestures/velocity_calculator.h:20: void ClearHistory();
newline between lines 20 and 21

[rjkroege, 2012-02-02 20:24:16]

LGTM

http://codereview.chromium.org/9310031/diff/10/ui/aura/gestures/velocity_calc...
File ui/aura/gestures/velocity_calculator.cc (right):

http://codereview.chromium.org/9310031/diff/10/ui/aura/gestures/velocity_calc...
ui/aura/gestures/velocity_calculator.cc:57: mean_x /= buffer_size_;
compute the reciprocal once and then multiply?

http://codereview.chromium.org/9310031/diff/10/ui/aura/gestures/velocity_calc...
File ui/aura/gestures/velocity_calculator.h (right):

http://codereview.chromium.org/9310031/diff/10/ui/aura/gestures/velocity_calc...
ui/aura/gestures/velocity_calculator.h:15: class AURA_EXPORT VelocityCalculator
{
Am not convinced that you need this. This class is not seen outside of the GR
rubric

[sadrul, 2012-02-02 20:34:01]

http://codereview.chromium.org/9310031/diff/10/ui/aura/gestures/velocity_calc...
File ui/aura/gestures/velocity_calculator.cc (right):

http://codereview.chromium.org/9310031/diff/10/ui/aura/gestures/velocity_calc...
ui/aura/gestures/velocity_calculator.cc:31: UpdateVelocity();
I kind of liked the way you did it in the first patchset. PointSeen is called a
lot often and a lot more (potentially for every touch) than x_velocity() or
y_velocity() (only at the end of a scroll gesture). It fells like computing the
actual velocities on demand would be better, rather than spending cycles
computing velocities for every touch?

[sadrul, 2012-02-02 22:04:06]

Looks good. Left some style nits.

http://codereview.chromium.org/9310031/diff/12009/ui/aura/gestures/velocity_c...
File ui/aura/gestures/velocity_calculator.cc (right):

http://codereview.chromium.org/9310031/diff/12009/ui/aura/gestures/velocity_c...
ui/aura/gestures/velocity_calculator.cc:11: , num_valid_entries_(0)
Wha. This isn't webkit land! :p

http://codereview.chromium.org/9310031/diff/12009/ui/aura/gestures/velocity_c...
File ui/aura/gestures/velocity_calculator.h (right):

http://codereview.chromium.org/9310031/diff/12009/ui/aura/gestures/velocity_c...
ui/aura/gestures/velocity_calculator.h:15: class AURA_EXPORT VelocityCalculator
{
Avoid AURA_EXPORT here. GesturePoint isn't exported, so this doesn't need to be
exported either.

http://codereview.chromium.org/9310031/diff/12009/ui/aura/gestures/velocity_c...
ui/aura/gestures/velocity_calculator.h:35: HistoryBuffer buffer_;
Use a scoped_ptr here so you don't have to explicitly delete in the destructor.

http://codereview.chromium.org/9310031/diff/12009/ui/aura/gestures/velocity_c...
ui/aura/gestures/velocity_calculator.h:43: bool velocities_stale_;
DISALLOW_COPY_AND_ASSIGN(VelocityCalculator);

http://codereview.chromium.org/9310031/diff/12009/ui/aura/gestures/velocity_c...
ui/aura/gestures/velocity_calculator.h:45: }  // namespace aura
newline after line-44

http://codereview.chromium.org/9310031/diff/12009/ui/aura/gestures/velocity_c...
File ui/aura/gestures/velocity_calculator_unittest.cc (right):

http://codereview.chromium.org/9310031/diff/12009/ui/aura/gestures/velocity_c...
ui/aura/gestures/velocity_calculator_unittest.cc:13: static void
AddPoints(VelocityCalculator* velocityCalculator,
newlines after lines 12 and 28

[tdresser, 2012-02-02 22:25:42]

http://codereview.chromium.org/9310031/diff/12009/ui/aura/gestures/velocity_c...
File ui/aura/gestures/velocity_calculator.h (right):

http://codereview.chromium.org/9310031/diff/12009/ui/aura/gestures/velocity_c...
ui/aura/gestures/velocity_calculator.h:15: class AURA_EXPORT VelocityCalculator
{
On 2012/02/02 22:04:06, sadrul wrote:
> Avoid AURA_EXPORT here. GesturePoint isn't exported, so this doesn't need to
be
> exported either.
AURA_EXPORT is necessary for the unit tests to run.

[sadrul, 2012-02-02 22:31:10]

LGTM

http://codereview.chromium.org/9310031/diff/12009/ui/aura/gestures/velocity_c...
File ui/aura/gestures/velocity_calculator.h (right):

http://codereview.chromium.org/9310031/diff/12009/ui/aura/gestures/velocity_c...
ui/aura/gestures/velocity_calculator.h:15: class AURA_EXPORT VelocityCalculator
{
On 2012/02/02 22:25:42, tdresser wrote:
> On 2012/02/02 22:04:06, sadrul wrote:
> > Avoid AURA_EXPORT here. GesturePoint isn't exported, so this doesn't need to
> be
> > exported either.
> AURA_EXPORT is necessary for the unit tests to run.
> 

Aah, OK. Makes sense.

http://codereview.chromium.org/9310031/diff/2004/ui/aura/gestures/gesture_poi...
File ui/aura/gestures/gesture_point.cc (right):

http://codereview.chromium.org/9310031/diff/2004/ui/aura/gestures/gesture_poi...
ui/aura/gestures/gesture_point.cc:9: #include "base/basictypes.h"
sort alphabetically!

[Ian Vollick, 2012-02-02 23:31:10]

http://codereview.chromium.org/9310031/diff/2004/ui/aura/gestures/velocity_ca...
File ui/aura/gestures/velocity_calculator.cc (right):

http://codereview.chromium.org/9310031/diff/2004/ui/aura/gestures/velocity_ca...
ui/aura/gestures/velocity_calculator.cc:76: mean_time *= buffer_size_i;
Will this not create a floating point value that gets converted back to an
int64, losing your precision?

[Ian Vollick, 2012-02-03 15:03:31]

On 2012/02/03 14:08:07, tdresser wrote:

Looks great! Might be nice if there was a unit test that checked that things
work even if we have extremely big times that differ by small numbers of
milliseconds.

[tdresser, 2012-02-03 18:30:58]

On 2012/02/03 15:03:31, vollick wrote:
> On 2012/02/03 14:08:07, tdresser wrote:
> 
> Looks great! Might be nice if there was a unit test that checked that things
> work even if we have extremely big times that differ by small numbers of
> milliseconds.
I've added a unit test which makes it clear that we need to use division of
int64's in order to retain accuracy.

[Ian Vollick, 2012-02-03 18:40:27]

On 2012/02/03 18:39:47, tdresser wrote:

Nice. LGTM

[sky, 2012-02-03 21:40:32]

Since others reviewed the gesture changes I only looked at the gyp change (yes).
LGTM

[commit-bot: I haz the power, 2012-02-06 15:50:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/9310031/3021

[commit-bot: I haz the power, 2012-02-06 18:22:58]

Try job failure for 9310031-3021 (retry) on win_rel for step "browser_tests".
It's a second try, previously, steps "content_unittests, browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-02-06 20:13:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/9310031/3021

[commit-bot: I haz the power, 2012-02-06 23:15:46]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac_rel, revision is 120638, job name
was 9310031-3021 (previous was lost) (previous was lost) (retry) (retry).

[commit-bot: I haz the power, 2012-02-07 16:14:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/9310031/3021

[commit-bot: I haz the power, 2012-02-07 22:07:09]

Change committed as 120835

[michaeln, 2012-02-07 22:41:06]

hi, this failed to compile on clang so was reverted.
Revert revision 120845


Here's the build output from
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...


In file included from ui/aura/gestures/gesture_point.cc:5:
In file included from ./ui/aura/gestures/gesture_point.h:10:
./ui/aura/gestures/velocity_calculator.h:20:3:error: [chromium-style] Complex
destructor has an inline body.
  ~VelocityCalculator() {}
  ^
  CXX(target) out/Debug/obj.target/aura/ui/aura/gestures/velocity_calculator.o
In file included from ui/aura/gestures/gesture_point.cc:5:
./ui/aura/gestures/gesture_point.h:28:3:error: [chromium-style] Complex
destructor has an inline body.
  virtual ~GesturePoint() {}
  ^
In file included from ui/aura/gestures/velocity_calculator.cc:5:
./ui/aura/gestures/velocity_calculator.h:20:3:error: [chromium-style] Complex
destructor has an inline body.
  ~VelocityCalculator() {}
  ^
1 error generated.
make: *** [out/Debug/obj.target/aura/ui/aura/gestures/velocity_calculator.o]
Error 1
  CXX(target) out/Debug/obj.target/aura/ui/aura/gestures/gesture_sequence.o
In file included from ui/aura/gestures/gesture_sequence.cc:5:
In file included from ./ui/aura/gestures/gesture_sequence.h:9:
In file included from ./ui/aura/gestures/gesture_point.h:10:
./ui/aura/gestures/velocity_calculator.h:20:3:error: [chromium-style] Complex
destructor has an inline body.
  ~VelocityCalculator() {}
  ^
2 errors generated.
In file included from ui/aura/gestures/gesture_sequence.cc:5:
In file included from ./ui/aura/gestures/gesture_sequence.h:9:
./ui/aura/gestures/gesture_point.h:28:3:error: [chromium-style] Complex
destructor has an inline body.
  virtual ~GesturePoint() {}
  ^
make: *** [out/Debug/obj.target/aura/ui/aura/gestures/gesture_point.o] Error 1
  CXX(target) out/Debug/obj.target/glue/webkit/glue/webkitplatformsupport_impl.o
In file included from ui/aura/gestures/gesture_recognizer_aura.cc:11:
In file included from ./ui/aura/gestures/gesture_sequence.h:9:
In file included from ./ui/aura/gestures/gesture_point.h:10:
./ui/aura/gestures/velocity_calculator.h:20:3:error: [chromium-style] Complex
destructor has an inline body.
  ~VelocityCalculator() {}
  ^
In file included from ui/aura/gestures/gesture_recognizer_aura.cc:11:
In file included from ./ui/aura/gestures/gesture_sequence.h:9:
./ui/aura/gestures/gesture_point.h:28:3:error: [chromium-style] Complex
destructor has an inline body.
  virtual ~GesturePoint() {}
  ^
2 errors generated.
make: *** [out/Debug/obj.target/aura/ui/aura/gestures/gesture_recognizer_aura.o]
Error 1
  CXX(target) out/Debug/obj.target/ui/ui/base/accelerators/accelerator.o
2 errors generated.
make: *** [out/Debug/obj.target/aura/ui/aura/gestures/gesture_sequence.o] Error
1
  CXX(target) out/Debug/obj.target/ui/ui/base/resource/resource_bundle_posix.o
  CXX(target) out/Debug/obj.target/ui/ui/base/text/bytes_formatting.o

[commit-bot: I haz the power, 2012-02-08 15:34:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/9310031/36002

[commit-bot: I haz the power, 2012-02-08 16:00:03]

Try job failure for 9310031-36002 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...