Chromoting to report roundtrip latency

Chromoting to report roundtrip latency

Doing so by sending a sequence number, essentially the timestamp in every
envet message. Capturer at the host will pick up the latest sequence number
and pass it through the pipeline. Client will then receive it and determine
the latency.

This roundtrip latency number however doesn't include time in decoding and
rendering.

BUG=None
TEST=None

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

[Jamie, 2011-04-05 09:37:30]

There seems to be some inconsistency in naming here. In some places, the new
field is referred to (accurately) as a timestamp, but in others it's referred to
as a sequence number. I think it should be referred to as a timestamp
throughout, since that's what it is.

[simonmorris, 2011-04-05 10:47:06]

I agree with Jamie that "sequence number" should be
"timestamp" throughout.

http://codereview.chromium.org/6792038/diff/4022/remoting/host/screen_recorde...
File remoting/host/screen_recorder.cc (right):

http://codereview.chromium.org/6792038/diff/4022/remoting/host/screen_recorde...
remoting/host/screen_recorder.cc:107: if (MessageLoop::current() !=
capture_loop_) {
Maybe a comment to explain why this should be run on the
capture loop? (If this is because of the "capture is
synchronous" comment below).

http://codereview.chromium.org/6792038/diff/4022/remoting/host/screen_recorde...
remoting/host/screen_recorder.cc:248: // accurate as long as capture is
synchronous as the following statement
Maybe "as long as sequence_number_ is set in the capture loop
thread", to tie into UpdateSequenceNumber() above?

http://codereview.chromium.org/6792038/diff/4022/remoting/protocol/connection...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/6792038/diff/4022/remoting/protocol/connection...
remoting/protocol/connection_to_client.h:121: int sequence_number_;
Not needed.

http://codereview.chromium.org/6792038/diff/4022/remoting/protocol/host_messa...
File remoting/protocol/host_message_dispatcher.h (right):

http://codereview.chromium.org/6792038/diff/4022/remoting/protocol/host_messa...
remoting/protocol/host_message_dispatcher.h:41: void
Initialize(ConnectionToClient* connection,
Does HostMessageDispatcher need to use ConnectionToClient,
when ConnectionToClient just passes an UpdateSequenceNumber()
request to another object? Couldn't HostMessageDispatcher
dispatch UpdateSequenceNumber() through a smaller interface?

[Wez, 2011-04-05 13:46:20]

Looks good, subject to a couple of tweaks. :)

http://codereview.chromium.org/6792038/diff/4022/remoting/client/chromoting_c...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/6792038/diff/4022/remoting/client/chromoting_c...
remoting/client/chromoting_client.cc:141: packet->client_sequence_number()) {
Checking for presence of client_sequence_number is for backwards compatibility
with older Hosts.
Checking for non-zero value is because zero means that the Host has already sent
us a VideoPacket corresponding to the most recent input event.
This isn't clear without looking at the ScreenRecorder implementation, though.

It would be better to have the client keep a record of the most-recent sequence
number it received a VideoPacket for, and only log the round-trip if it receives
one with a higher sequence number, I think.

If you'd prefer to keep the arrangement where the Host zeroes its internal idea
of the client_sequence_number then better to have that propagate to the client
as an absence of a client_sequence_number field at all, and to clarify in this
the proto that zero is not a valid sequence number for a client to send.

http://codereview.chromium.org/6792038/diff/4022/remoting/host/screen_recorde...
File remoting/host/screen_recorder.cc (right):

http://codereview.chromium.org/6792038/diff/4022/remoting/host/screen_recorde...
remoting/host/screen_recorder.cc:249: // will obtain the last sequence number
received.
"last" -> "most recent"

http://codereview.chromium.org/6792038/diff/4022/remoting/proto/internal.proto
File remoting/proto/internal.proto (right):

http://codereview.chromium.org/6792038/diff/4022/remoting/proto/internal.prot...
remoting/proto/internal.proto:28: required int64 timestamp = 1;  // Client
timestamp for event
"timestamp" -> "sequence_number"

http://codereview.chromium.org/6792038/diff/4022/remoting/proto/video.proto
File remoting/proto/video.proto (right):

http://codereview.chromium.org/6792038/diff/4022/remoting/proto/video.proto#n...
remoting/proto/video.proto:97: optional int64 client_sequence_number = 9;
This comment suggests that the Host could use the client_sequence_number for
timings itself - as we've discussed previously, it's better not to do so. 
Really the client_sequence_number is a Host-opaque sequence number supplied in
the most recent input event the Host received from the Client, right?

http://codereview.chromium.org/6792038/diff/4022/remoting/protocol/input_send...
File remoting/protocol/input_sender.cc (right):

http://codereview.chromium.org/6792038/diff/4022/remoting/protocol/input_send...
remoting/protocol/input_sender.cc:31:
message.set_timestamp(base::Time::Now().ToInternalValue());
"set_timestamp" -> "set_sequence_number"

http://codereview.chromium.org/6792038/diff/4022/remoting/protocol/input_send...
remoting/protocol/input_sender.cc:38:
message.set_timestamp(base::Time::Now().ToInternalValue());
Ditto.

[Wez, 2011-04-05 13:48:15]

On 2011/04/05 09:37:30, Jamie wrote:
> There seems to be some inconsistency in naming here. In some places, the new
> field is referred to (accurately) as a timestamp, but in others it's referred
to
> as a sequence number. I think it should be referred to as a timestamp
> throughout, since that's what it is.

From the Host's point of view, it's not a timestamp, it's an opaque Id unique to
each input event, which the Client is free to use for whatever purpose it
chooses - currently it's used to measure latency, but it might in future be used
to provide feedback in the UI as to whether the Host has processed a particular
input event, for example, for which purpose it needn't necessarily be a
timestamp.

[Alpha Left Google, 2011-04-08 00:07:20]

http://codereview.chromium.org/6792038/diff/4022/remoting/client/chromoting_c...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/6792038/diff/4022/remoting/client/chromoting_c...
remoting/client/chromoting_client.cc:141: packet->client_sequence_number()) {
On 2011/04/05 13:46:20, Wez wrote:
> Checking for presence of client_sequence_number is for backwards compatibility
> with older Hosts.
> Checking for non-zero value is because zero means that the Host has already
sent
> us a VideoPacket corresponding to the most recent input event.
> This isn't clear without looking at the ScreenRecorder implementation, though.
> 
> It would be better to have the client keep a record of the most-recent
sequence
> number it received a VideoPacket for, and only log the round-trip if it
receives
> one with a higher sequence number, I think.
> 
> If you'd prefer to keep the arrangement where the Host zeroes its internal
idea
> of the client_sequence_number then better to have that propagate to the client
> as an absence of a client_sequence_number field at all, and to clarify in this
> the proto that zero is not a valid sequence number for a client to send.

Sounds good to me. The reason I didn't want to not put client_sequence_number is
because I needed to add two places in two different encoder. Your scheme seems
better.

http://codereview.chromium.org/6792038/diff/4022/remoting/host/screen_recorde...
File remoting/host/screen_recorder.cc (right):

http://codereview.chromium.org/6792038/diff/4022/remoting/host/screen_recorde...
remoting/host/screen_recorder.cc:107: if (MessageLoop::current() !=
capture_loop_) {
On 2011/04/05 10:47:06, simonmorris wrote:
> Maybe a comment to explain why this should be run on the
> capture loop? (If this is because of the "capture is
> synchronous" comment below).

Done.

http://codereview.chromium.org/6792038/diff/4022/remoting/host/screen_recorde...
remoting/host/screen_recorder.cc:248: // accurate as long as capture is
synchronous as the following statement
On 2011/04/05 10:47:06, simonmorris wrote:
> Maybe "as long as sequence_number_ is set in the capture loop
> thread", to tie into UpdateSequenceNumber() above?

Well that's not true.. capture being synchronous is important for this to be
accurate.

http://codereview.chromium.org/6792038/diff/4022/remoting/host/screen_recorde...
remoting/host/screen_recorder.cc:249: // will obtain the last sequence number
received.
On 2011/04/05 13:46:20, Wez wrote:
> "last" -> "most recent"

Done.

http://codereview.chromium.org/6792038/diff/4022/remoting/proto/internal.proto
File remoting/proto/internal.proto (right):

http://codereview.chromium.org/6792038/diff/4022/remoting/proto/internal.prot...
remoting/proto/internal.proto:28: required int64 timestamp = 1;  // Client
timestamp for event
On 2011/04/05 13:46:20, Wez wrote:
> "timestamp" -> "sequence_number"

Done.

http://codereview.chromium.org/6792038/diff/4022/remoting/proto/video.proto
File remoting/proto/video.proto (right):

http://codereview.chromium.org/6792038/diff/4022/remoting/proto/video.proto#n...
remoting/proto/video.proto:97: optional int64 client_sequence_number = 9;
On 2011/04/05 13:46:20, Wez wrote:
> This comment suggests that the Host could use the client_sequence_number for
> timings itself - as we've discussed previously, it's better not to do so. 
> Really the client_sequence_number is a Host-opaque sequence number supplied in
> the most recent input event the Host received from the Client, right?

Done.

http://codereview.chromium.org/6792038/diff/4022/remoting/protocol/connection...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/6792038/diff/4022/remoting/protocol/connection...
remoting/protocol/connection_to_client.h:121: int sequence_number_;
On 2011/04/05 10:47:06, simonmorris wrote:
> Not needed.

Done.

http://codereview.chromium.org/6792038/diff/4022/remoting/protocol/host_messa...
File remoting/protocol/host_message_dispatcher.h (right):

http://codereview.chromium.org/6792038/diff/4022/remoting/protocol/host_messa...
remoting/protocol/host_message_dispatcher.h:41: void
Initialize(ConnectionToClient* connection,
On 2011/04/05 10:47:06, simonmorris wrote:
> Does HostMessageDispatcher need to use ConnectionToClient,
> when ConnectionToClient just passes an UpdateSequenceNumber()
> request to another object? Couldn't HostMessageDispatcher
> dispatch UpdateSequenceNumber() through a smaller interface?

We don't want to expose sequence number to the stubs because they are
conceptually different.

[simonmorris, 2011-04-08 08:18:25]

http://codereview.chromium.org/6792038/diff/4022/remoting/host/screen_recorde...
File remoting/host/screen_recorder.cc (right):

http://codereview.chromium.org/6792038/diff/4022/remoting/host/screen_recorde...
remoting/host/screen_recorder.cc:248: // accurate as long as capture is
synchronous as the following statement
On 2011/04/08 00:07:20, Alpha wrote:
> On 2011/04/05 10:47:06, simonmorris wrote:
> > Maybe "as long as sequence_number_ is set in the capture loop
> > thread", to tie into UpdateSequenceNumber() above?
> 
> Well that's not true.. capture being synchronous is important for this to be
> accurate.

OK.

http://codereview.chromium.org/6792038/diff/4022/remoting/protocol/host_messa...
File remoting/protocol/host_message_dispatcher.h (right):

http://codereview.chromium.org/6792038/diff/4022/remoting/protocol/host_messa...
remoting/protocol/host_message_dispatcher.h:41: void
Initialize(ConnectionToClient* connection,
On 2011/04/08 00:07:20, Alpha wrote:
> On 2011/04/05 10:47:06, simonmorris wrote:
> > Does HostMessageDispatcher need to use ConnectionToClient,
> > when ConnectionToClient just passes an UpdateSequenceNumber()
> > request to another object? Couldn't HostMessageDispatcher
> > dispatch UpdateSequenceNumber() through a smaller interface?
> 
> We don't want to expose sequence number to the stubs because they are
> conceptually different.

UpdateSequenceNumber() ends up at the ScreenRecorder, via
ConnectionToClient and ChromotingHost. I think
the ConnectionToClient isn't used, so I'm suggesting
removing it from that path, so that
HostMessageDispatcher isn't coupled to it. You could have
a new, small, interface for updating sequence numbers.

[Wez, 2011-05-06 18:23:21]

This seems to have stalled - does it still make sense to go in, Alpha?

[Alpha Left Google, 2011-05-06 18:42:25]

This should be still good to help measurement. I would like to check this in if
anyone can give it a LGTM. I guess simon's concern is the coding structure which
I don't think it's very critical.

[simonmorris, 2011-05-06 18:45:14]

On 2011/05/06 18:42:25, Alpha wrote:
> This should be still good to help measurement. I would like to check this in
if
> anyone can give it a LGTM. I guess simon's concern is the coding structure
which
> I don't think it's very critical.

I agree. LGTM.