ash: Add basic prediction code to the laser pointer.

ash: Add basic prediction code to the laser pointer.

This introduces some basic prediction code to the laser pointer. The
code predicts the state at the next presentation time and adds points
and adjusts current points to reflect this.

Presentation time is next VSYNC time + some device specific constant
that represents the time it takes from VSYNC until a change in light
can be observed by the user.

BUG=696385
TEST=ash_unittests --gtest_filter=LaserPointerControllerTest.LaserPointerPrediction

Review-Url: https://codereview.chromium.org/2745953002
Cr-Commit-Position: refs/heads/master@{#457572}
Committed: https://chromium.googlesource.com/chromium/src/+/6e29796bd01d666af693908264a2d52909da420f

[reveman, 2017-03-12 23:15:13]

Description was changed from

==========
ash: Add basic prediction code to the laser pointer.

This introduces some basic prediction code to the laser pointer. The
code predicts the state at the next presentation time and adds points
and adjusts current points to reflect this.

Presentation time is next VSYNC time + some device specific constant
that represents the time it takes from VSYNC until a change in light
can be observed by the user.

BUG=
==========

to

==========
ash: Add basic prediction code to the laser pointer.

This introduces some basic prediction code to the laser pointer. The
code predicts the state at the next presentation time and adds points
and adjusts current points to reflect this.

Presentation time is next VSYNC time + some device specific constant
that represents the time it takes from VSYNC until a change in light
can be observed by the user.

BUG=696385
==========

[reveman, 2017-03-12 23:17:31]

reveman@chromium.org changed reviewers:
+ dcastagna@chromium.org, hoegsberg@chromium.org

[Daniele Castagna, 2017-03-13 00:26:08]

Cool!
Would it be possible to track the standard deviation of the error between the
prediction and the actual samples?

Any other more appropriate statistics would be good as long as we can make sure
to quantify the quality of the prediction and avoid regressing that in the
future.

[reveman, 2017-03-14 13:09:05]

On 2017/03/13 at 00:26:08, dcastagna wrote:
> Cool!
> Would it be possible to track the standard deviation of the error between the
prediction and the actual samples?
> 
> Any other more appropriate statistics would be good as long as we can make
sure to quantify the quality of the prediction and avoid regressing that in the
future.

I added a basic trace counter for the error when replacing a point with a real
point in latest patch. It doesn't tell the whole truth but it's something that
was simple to add that is useful and we can follow up with something more
advanced if needed.

Also made the prediction code more robust. Latest patch seems to be working
quite well. PTAL.

[Daniele Castagna, 2017-03-15 19:07:28]

https://codereview.chromium.org/2745953002/diff/60001/ash/common/ash_switches.cc
File ash/common/ash_switches.cc (right):

https://codereview.chromium.org/2745953002/diff/60001/ash/common/ash_switches...
ash/common/ash_switches.cc:68: // Specifies the estimated time (in milliseconds)
from VSYNC event until when
Wouldn't this be the estimated time from when you write to the buffer until when
the pixel can be seen by the user?

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
File ash/laser/laser_pointer_controller.cc (right):

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
ash/laser/laser_pointer_controller.cc:53: if
(command_line->HasSwitch(switches::kAshEstimatedPresentationDelay)) {
nit: you can get rid of this line and the code should behave the same way.
If the flag is not passed the string will be "" and StringToInt64 will return
false.

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
File ash/laser/laser_pointer_points_unittest.cc (right):

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
ash/laser/laser_pointer_points_unittest.cc:40: points_.AddPoint(left,
base::TimeTicks::Now());
Should it be a pre-determined TimeTicks instead of calling TimeTicks::Now? Just
to make sure the tests are as deterministic as possible.

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
File ash/laser/laser_pointer_view.cc (right):

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
ash/laser/laser_pointer_view.cc:53: const SkColor kPredictionPointColor =
SkColorSetRGB(255, 0, 0);
nit: you could initialize this to kPointColor to make it clear it has to be the
same if not for debug purposes.

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
ash/laser/laser_pointer_view.cc:282: predicted_laser_points_.Clear();
This probably creates a lot of discontinuity, since we forget completely about
the previous predictions.

It might be interesting in the future to keep these around and use them to guide
the new prediction, or maybe we could just keep around speed,acc and jerk.

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
ash/laser/laser_pointer_view.cc:300: PositionArray position;
nit: positions?

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
ash/laser/laser_pointer_view.cc:321: velocity[i] = position[i] - position[i +
1];
It's really weird to see a velocity defined as delta pos. Should we divide this
deltaT, even if we set it to 1 for now?

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
ash/laser/laser_pointer_view.cc:344: velocity[0] += acceleration[0];
Again, kinda weird to see a velocity incremented by an acceleration. Should we
multiply acceleration by deltaT?

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
ash/laser/laser_pointer_view.cc:345: acceleration[0] += jerk;
Have you tried dampening jerk and acceleration at each iteration?
It could help for cases with small velocity and big acceleration/jerk.

[reveman, 2017-03-16 12:57:29]

PTAL

https://codereview.chromium.org/2745953002/diff/60001/ash/common/ash_switches.cc
File ash/common/ash_switches.cc (right):

https://codereview.chromium.org/2745953002/diff/60001/ash/common/ash_switches...
ash/common/ash_switches.cc:68: // Specifies the estimated time (in milliseconds)
from VSYNC event until when
On 2017/03/15 at 19:07:28, Daniele Castagna wrote:
> Wouldn't this be the estimated time from when you write to the buffer until
when the pixel can be seen by the user?

No, that's not the idea. The usage of this in chrome might add additional
latency based on next vsync time if it wants to be really accurate but this
should not be set to a higher value to compensate for delta between buffer
update and vsync as that's something only chrome can do accurately at run time.

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
File ash/laser/laser_pointer_controller.cc (right):

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
ash/laser/laser_pointer_controller.cc:53: if
(command_line->HasSwitch(switches::kAshEstimatedPresentationDelay)) {
On 2017/03/15 at 19:07:28, Daniele Castagna wrote:
> nit: you can get rid of this line and the code should behave the same way.
> If the flag is not passed the string will be "" and StringToInt64 will return
false.

Done.

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
File ash/laser/laser_pointer_points_unittest.cc (right):

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
ash/laser/laser_pointer_points_unittest.cc:40: points_.AddPoint(left,
base::TimeTicks::Now());
On 2017/03/15 at 19:07:28, Daniele Castagna wrote:
> Should it be a pre-determined TimeTicks instead of calling TimeTicks::Now?
Just to make sure the tests are as deterministic as possible.

Using base::TimeTicks() in latest patch as time doesn't matter for this test.

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
File ash/laser/laser_pointer_view.cc (right):

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
ash/laser/laser_pointer_view.cc:53: const SkColor kPredictionPointColor =
SkColorSetRGB(255, 0, 0);
On 2017/03/15 at 19:07:28, Daniele Castagna wrote:
> nit: you could initialize this to kPointColor to make it clear it has to be
the same if not for debug purposes.

Done.

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
ash/laser/laser_pointer_view.cc:282: predicted_laser_points_.Clear();
On 2017/03/15 at 19:07:28, Daniele Castagna wrote:
> This probably creates a lot of discontinuity, since we forget completely about
the previous predictions.
> 
> It might be interesting in the future to keep these around and use them to
guide the new prediction, or maybe we could just keep around speed,acc and jerk.

Yes, there's definitely room for improvement here. I just want to land this
initial code and make sure it's easy to understand how it works so someone
interesting in improving it can do that relatively easily.

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
ash/laser/laser_pointer_view.cc:300: PositionArray position;
On 2017/03/15 at 19:07:28, Daniele Castagna wrote:
> nit: positions?

then it should be velocities and accelerations too but fine with me if you
prefer that?

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
ash/laser/laser_pointer_view.cc:321: velocity[i] = position[i] - position[i +
1];
On 2017/03/15 at 19:07:28, Daniele Castagna wrote:
> It's really weird to see a velocity defined as delta pos. Should we divide
this deltaT, even if we set it to 1 for now?

I agree. Added a comment and about this for now. We'll fix this and use proper
calculations when b/36137953 has been fixed.

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
ash/laser/laser_pointer_view.cc:344: velocity[0] += acceleration[0];
On 2017/03/15 at 19:07:28, Daniele Castagna wrote:
> Again, kinda weird to see a velocity incremented by an acceleration. Should we
multiply acceleration by deltaT?

Added a comment for now.

https://codereview.chromium.org/2745953002/diff/60001/ash/laser/laser_pointer...
ash/laser/laser_pointer_view.cc:345: acceleration[0] += jerk;
On 2017/03/15 at 19:07:28, Daniele Castagna wrote:
> Have you tried dampening jerk and acceleration at each iteration?
> It could help for cases with small velocity and big acceleration/jerk.

I considered that and suspect that it can help make the prediction even better
but I figured it would be better to leave that as a follow up.

[Daniele Castagna, 2017-03-16 16:12:09]

lgtm

[reveman, 2017-03-16 16:26:00]

reveman@chromium.org changed reviewers:
+ derat@chromium.org

[reveman, 2017-03-16 16:26:01]

+derat for owner review
+sammiequon fyi

[reveman, 2017-03-16 16:26:06]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 16:26:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Daniel Erat, 2017-03-16 16:50:55]

can you add some simple tests for the prediction code?

https://codereview.chromium.org/2745953002/diff/120001/ash/common/ash_switche...
File ash/common/ash_switches.cc (right):

https://codereview.chromium.org/2745953002/diff/120001/ash/common/ash_switche...
ash/common/ash_switches.cc:70: const char kAshEstimatedPresentationDelay[] =
"estimated-presentation-delay";
should this start with "ash-" like (most of) the existing flags?

https://codereview.chromium.org/2745953002/diff/120001/ash/laser/laser_pointe...
File ash/laser/laser_pointer_view.cc (right):

https://codereview.chromium.org/2745953002/diff/120001/ash/laser/laser_pointe...
ash/laser/laser_pointer_view.cc:53: // Change this to a debug prediction code.
i don't quite understand this comment. would something like "Change this when
debugging prediction code." be clearer?

https://codereview.chromium.org/2745953002/diff/120001/ash/laser/laser_pointe...
ash/laser/laser_pointer_view.cc:303: for (auto& point :
base::Reversed(laser_points_.laser_points())) {
nit: const auto& ?

https://codereview.chromium.org/2745953002/diff/120001/ash/laser/laser_pointe...
ash/laser/laser_pointer_view.cc:304: // Stop adding positons if interval between
points is too large to provide
nit: positions

https://codereview.chromium.org/2745953002/diff/120001/ash/laser/laser_pointe...
ash/laser/laser_pointer_view.cc:319: // Calculate velocity.
nit: i'd drop these "Calculate ..." comments; they don't add anything

[commit-bot: I haz the power, 2017-03-16 16:57:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 16:57:03]

Dry run: This issue passed the CQ dry run.

[reveman, 2017-03-16 17:59:12]

Description was changed from

==========
ash: Add basic prediction code to the laser pointer.

This introduces some basic prediction code to the laser pointer. The
code predicts the state at the next presentation time and adds points
and adjusts current points to reflect this.

Presentation time is next VSYNC time + some device specific constant
that represents the time it takes from VSYNC until a change in light
can be observed by the user.

BUG=696385
==========

to

==========
ash: Add basic prediction code to the laser pointer.

This introduces some basic prediction code to the laser pointer. The
code predicts the state at the next presentation time and adds points
and adjusts current points to reflect this.

Presentation time is next VSYNC time + some device specific constant
that represents the time it takes from VSYNC until a change in light
can be observed by the user.

BUG=696385
TEST=ash_unittests
--gtest_filter=LaserPointerControllerTest.LaserPointerPrediction
==========

[reveman, 2017-03-16 18:00:25]

On 2017/03/16 at 16:50:55, derat wrote:
> can you add some simple tests for the prediction code?

Done. Added LaserPointerControllerTest.LaserPointerPrediction.

PTAL

https://codereview.chromium.org/2745953002/diff/120001/ash/common/ash_switche...
File ash/common/ash_switches.cc (right):

https://codereview.chromium.org/2745953002/diff/120001/ash/common/ash_switche...
ash/common/ash_switches.cc:70: const char kAshEstimatedPresentationDelay[] =
"estimated-presentation-delay";
On 2017/03/16 at 16:50:55, Daniel Erat wrote:
> should this start with "ash-" like (most of) the existing flags?

Done.

https://codereview.chromium.org/2745953002/diff/120001/ash/laser/laser_pointe...
File ash/laser/laser_pointer_view.cc (right):

https://codereview.chromium.org/2745953002/diff/120001/ash/laser/laser_pointe...
ash/laser/laser_pointer_view.cc:53: // Change this to a debug prediction code.
On 2017/03/16 at 16:50:55, Daniel Erat wrote:
> i don't quite understand this comment. would something like "Change this when
debugging prediction code." be clearer?

oh, the "a" shouldn't have been in that sentence. Changed the comment to your
recommendation.

https://codereview.chromium.org/2745953002/diff/120001/ash/laser/laser_pointe...
ash/laser/laser_pointer_view.cc:303: for (auto& point :
base::Reversed(laser_points_.laser_points())) {
On 2017/03/16 at 16:50:55, Daniel Erat wrote:
> nit: const auto& ?

Done.

https://codereview.chromium.org/2745953002/diff/120001/ash/laser/laser_pointe...
ash/laser/laser_pointer_view.cc:304: // Stop adding positons if interval between
points is too large to provide
On 2017/03/16 at 16:50:55, Daniel Erat wrote:
> nit: positions

Done.

https://codereview.chromium.org/2745953002/diff/120001/ash/laser/laser_pointe...
ash/laser/laser_pointer_view.cc:319: // Calculate velocity.
On 2017/03/16 at 16:50:55, Daniel Erat wrote:
> nit: i'd drop these "Calculate ..." comments; they don't add anything

Agree. Done.

[reveman, 2017-03-16 18:17:44]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 18:18:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 18:51:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 18:51:57]

Dry run: This issue passed the CQ dry run.

[Daniel Erat, 2017-03-16 20:18:30]

lgtm

https://codereview.chromium.org/2745953002/diff/140001/ash/laser/laser_pointe...
File ash/laser/laser_pointer_controller_test_api.h (right):

https://codereview.chromium.org/2745953002/diff/140001/ash/laser/laser_pointe...
ash/laser/laser_pointer_controller_test_api.h:27: const LaserPointerPoints&
predicted_laser_points();
unrelated to your change, but most of these could probably be const

https://codereview.chromium.org/2745953002/diff/140001/ash/laser/laser_pointe...
File ash/laser/laser_pointer_controller_unittest.cc (right):

https://codereview.chromium.org/2745953002/diff/140001/ash/laser/laser_pointe...
ash/laser/laser_pointer_controller_unittest.cc:137:
controller_test_api_.predicted_laser_points().GetNumberOfPoints());
i hope you add some tests that verify the actual points in a followup change.
:-)

https://codereview.chromium.org/2745953002/diff/140001/ash/laser/laser_pointe...
File ash/laser/laser_pointer_view.cc (right):

https://codereview.chromium.org/2745953002/diff/140001/ash/laser/laser_pointe...
ash/laser/laser_pointer_view.cc:53: // Change this when debuging prediction
code.
nit: debugging

[reveman, 2017-03-16 20:42:48]

The CQ bit was checked by reveman@chromium.org

[reveman, 2017-03-16 20:42:49]

https://codereview.chromium.org/2745953002/diff/140001/ash/laser/laser_pointe...
File ash/laser/laser_pointer_controller_test_api.h (right):

https://codereview.chromium.org/2745953002/diff/140001/ash/laser/laser_pointe...
ash/laser/laser_pointer_controller_test_api.h:27: const LaserPointerPoints&
predicted_laser_points();
On 2017/03/16 at 20:18:30, Daniel Erat wrote:
> unrelated to your change, but most of these could probably be const

Fixed in latest patch.

https://codereview.chromium.org/2745953002/diff/140001/ash/laser/laser_pointe...
File ash/laser/laser_pointer_controller_unittest.cc (right):

https://codereview.chromium.org/2745953002/diff/140001/ash/laser/laser_pointe...
ash/laser/laser_pointer_controller_unittest.cc:137:
controller_test_api_.predicted_laser_points().GetNumberOfPoints());
On 2017/03/16 at 20:18:30, Daniel Erat wrote:
> i hope you add some tests that verify the actual points in a followup change.
:-)

Added to latest patch.

https://codereview.chromium.org/2745953002/diff/140001/ash/laser/laser_pointe...
File ash/laser/laser_pointer_view.cc (right):

https://codereview.chromium.org/2745953002/diff/140001/ash/laser/laser_pointe...
ash/laser/laser_pointer_view.cc:53: // Change this when debuging prediction
code.
On 2017/03/16 at 20:18:30, Daniel Erat wrote:
> nit: debugging

Done.

[reveman, 2017-03-16 20:42:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
dcastagna@chromium.org, derat@chromium.org
Link to the patchset: https://codereview.chromium.org/2745953002/#ps160001
(title: "more tests and nits")

[commit-bot: I haz the power, 2017-03-16 20:43:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 21:49:58]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1489696968472120,
"parent_rev": "c7fc0726ec8cd4985cd6e416e50712b4aff1c8c7", "commit_rev":
"6e29796bd01d666af693908264a2d52909da420f"}

[commit-bot: I haz the power, 2017-03-16 21:50:44]

Description was changed from

==========
ash: Add basic prediction code to the laser pointer.

This introduces some basic prediction code to the laser pointer. The
code predicts the state at the next presentation time and adds points
and adjusts current points to reflect this.

Presentation time is next VSYNC time + some device specific constant
that represents the time it takes from VSYNC until a change in light
can be observed by the user.

BUG=696385
TEST=ash_unittests
--gtest_filter=LaserPointerControllerTest.LaserPointerPrediction
==========

to

==========
ash: Add basic prediction code to the laser pointer.

This introduces some basic prediction code to the laser pointer. The
code predicts the state at the next presentation time and adds points
and adjusts current points to reflect this.

Presentation time is next VSYNC time + some device specific constant
that represents the time it takes from VSYNC until a change in light
can be observed by the user.

BUG=696385
TEST=ash_unittests
--gtest_filter=LaserPointerControllerTest.LaserPointerPrediction

Review-Url: https://codereview.chromium.org/2745953002
Cr-Commit-Position: refs/heads/master@{#457572}
Committed:
https://chromium.googlesource.com/chromium/src/+/6e29796bd01d666af693908264a2...
==========

[commit-bot: I haz the power, 2017-03-16 21:50:46]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/6e29796bd01d666af693908264a2...