Add InputEventTimestampSource interface.

Add InputEventTimestampSource interface.

Previously event timestamps were passed to VideoStream implementations
using OnInputEventReceived(). Now VideoStream gets a pointer to
InputEventTimestampSource, and then uses it to get event timestamp
when capturing each frame. This will allow to use fake input event
timestamps in performance tests.

Committed: https://crrev.com/e84cc62bc689161a28ec35b551f6b364e72989fe
Cr-Commit-Position: refs/heads/master@{#425554}

[Sergey Ulanov, 2016-10-12 21:13:00]

sergeyu@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[Jamie, 2016-10-12 21:41:23]

https://codereview.chromium.org/2413553003/diff/1/remoting/protocol/input_eve...
File remoting/protocol/input_event_timestamps.cc (right):

https://codereview.chromium.org/2413553003/diff/1/remoting/protocol/input_eve...
remoting/protocol/input_event_timestamps.cc:20: last_timestamps_ =
InputEventTimestamps();
Why reset this here? It makes GetLastEventTimestamps non-idempotent, and it's
not obvious from the interface description why you'd want that.

https://codereview.chromium.org/2413553003/diff/1/remoting/protocol/input_eve...
File remoting/protocol/input_event_timestamps.h (right):

https://codereview.chromium.org/2413553003/diff/1/remoting/protocol/input_eve...
remoting/protocol/input_event_timestamps.h:16: // Client-side timestamps. This
value comesfrom the client clock, so it should
Nit: missing space "comesfrom"

https://codereview.chromium.org/2413553003/diff/1/remoting/protocol/video_fra...
File remoting/protocol/video_frame_pump.h (right):

https://codereview.chromium.org/2413553003/diff/1/remoting/protocol/video_fra...
remoting/protocol/video_frame_pump.h:99: // The following fields is not null
only for one frame after each incoming
s/fields/field/
s/not null/non-null/
s/only for one/for a single/

Why is this important? Is it the reason behind resetting the timestamp (see my
previous comment)? If so, then I think it should either be explicit in the
interface (call the API GetAndResetLastEventTimestamps, for example), or this
class should take care of the single-frame non-nullness requirement explicitly.

[Sergey Ulanov, 2016-10-14 19:34:26]

https://codereview.chromium.org/2413553003/diff/1/remoting/protocol/input_eve...
File remoting/protocol/input_event_timestamps.cc (right):

https://codereview.chromium.org/2413553003/diff/1/remoting/protocol/input_eve...
remoting/protocol/input_event_timestamps.cc:20: last_timestamps_ =
InputEventTimestamps();
On 2016/10/12 21:41:23, Jamie wrote:
> Why reset this here? It makes GetLastEventTimestamps non-idempotent, and it's
> not obvious from the interface description why you'd want that.

Renamed the function to TakeLastEventTimestamps() and added a comment in the
header to explain how it supposed to work.

https://codereview.chromium.org/2413553003/diff/1/remoting/protocol/input_eve...
File remoting/protocol/input_event_timestamps.h (right):

https://codereview.chromium.org/2413553003/diff/1/remoting/protocol/input_eve...
remoting/protocol/input_event_timestamps.h:16: // Client-side timestamps. This
value comesfrom the client clock, so it should
On 2016/10/12 21:41:23, Jamie wrote:
> Nit: missing space "comesfrom"

Done.

https://codereview.chromium.org/2413553003/diff/1/remoting/protocol/video_fra...
File remoting/protocol/video_frame_pump.h (right):

https://codereview.chromium.org/2413553003/diff/1/remoting/protocol/video_fra...
remoting/protocol/video_frame_pump.h:99: // The following fields is not null
only for one frame after each incoming
On 2016/10/12 21:41:23, Jamie wrote:
> s/fields/field/
> s/not null/non-null/
> s/only for one/for a single/
> 
> Why is this important? Is it the reason behind resetting the timestamp (see my
> previous comment)? If so, then I think it should either be explicit in the
> interface (call the API GetAndResetLastEventTimestamps, for example), or this
> class should take care of the single-frame non-nullness requirement
explicitly.

Done.

[Jamie, 2016-10-14 20:30:30]

lgtm

https://codereview.chromium.org/2413553003/diff/20001/remoting/protocol/fake_...
File remoting/protocol/fake_connection_to_client.h (right):

https://codereview.chromium.org/2413553003/diff/20001/remoting/protocol/fake_...
remoting/protocol/fake_connection_to_client.h:30: event_timestamps_source)
override;
Nit: Indentation.

https://codereview.chromium.org/2413553003/diff/20001/remoting/protocol/host_...
File remoting/protocol/host_event_dispatcher.h (right):

https://codereview.chromium.org/2413553003/diff/20001/remoting/protocol/host_...
remoting/protocol/host_event_dispatcher.h:37: // timestamps for each event.
I don't understand this comment. The host timestamp is documented as being the
time at which the event was received, not when it was injected. Are you saying
that current implementations measure the latter instead?

[Sergey Ulanov, 2016-10-14 21:04:20]

https://codereview.chromium.org/2413553003/diff/20001/remoting/protocol/fake_...
File remoting/protocol/fake_connection_to_client.h (right):

https://codereview.chromium.org/2413553003/diff/20001/remoting/protocol/fake_...
remoting/protocol/fake_connection_to_client.h:30: event_timestamps_source)
override;
On 2016/10/14 20:30:30, Jamie wrote:
> Nit: Indentation.

This was formatted by clang-format and I think it's correct indentation
(variable name is indented 4 spaces relative to the type name).

https://codereview.chromium.org/2413553003/diff/20001/remoting/protocol/host_...
File remoting/protocol/host_event_dispatcher.h (right):

https://codereview.chromium.org/2413553003/diff/20001/remoting/protocol/host_...
remoting/protocol/host_event_dispatcher.h:37: // timestamps for each event.
On 2016/10/14 20:30:30, Jamie wrote:
> I don't understand this comment. The host timestamp is documented as being the
> time at which the event was received, not when it was injected. Are you saying
> that current implementations measure the latter instead?

The current implementation matches the comments for host_timestamp.
InputEventTimestamps.host_timestamp is the time when the event was received and
that's what this timestamps source will return. The point here is that it's not
an ideal approach. It would be better if latest_event_timestamp field in
VideoStats message was set according to injected events, not as they are
received. In other words currently we don't account for event injection delay
when measuring interactive latency.

Rephrased this TODO and also updated comment for host_timestamp

[Sergey Ulanov, 2016-10-14 21:04:53]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-10-14 21:04:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/2413553003/#ps40001
(title: "reword TODO")

[commit-bot: I haz the power, 2016-10-14 21:05:25]

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, 2016-10-14 21:48:04]

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

[commit-bot: I haz the power, 2016-10-14 21:48:05]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Sergey Ulanov, 2016-10-15 05:52:14]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-10-15 05:52:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/2413553003/#ps60001
(title: "msvc compilation")

[commit-bot: I haz the power, 2016-10-15 05:52:28]

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, 2016-10-15 06:54:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-15 06:56:51]

Description was changed from

==========
Add InputEventTimestampSource interface.

Previously event timestamps were passed to VideoStream implementations
using OnInputEventReceived(). Now VideoStream gets a pointer to
InputEventTimestampSource, and then uses it to get event timestamp
when capturing each frame. This will allow to use fake input event
timestamps in performance tests.
==========

to

==========
Add InputEventTimestampSource interface.

Previously event timestamps were passed to VideoStream implementations
using OnInputEventReceived(). Now VideoStream gets a pointer to
InputEventTimestampSource, and then uses it to get event timestamp
when capturing each frame. This will allow to use fake input event
timestamps in performance tests.

Committed: https://crrev.com/e84cc62bc689161a28ec35b551f6b364e72989fe
Cr-Commit-Position: refs/heads/master@{#425554}
==========

[commit-bot: I haz the power, 2016-10-15 06:56:53]

Patchset 4 (id:??) landed as
https://crrev.com/e84cc62bc689161a28ec35b551f6b364e72989fe
Cr-Commit-Position: refs/heads/master@{#425554}