Enable video stats reporting when using WebRTC

Enable video stats reporting when using WebRTC

1. Host now initiating video_stats channel and uses it to send stats
   for each frame to the client
2. The client can accept video_stats channel and reports perf numbers
   to the FrameStatsConsumer.

BUG=621691
Committed: https://crrev.com/09364636097f9c7b34349df97532291a2cfd015d
Cr-Commit-Position: refs/heads/master@{#411196}

[Sergey Ulanov, 2016-08-08 23:50:56]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2016-08-08 23:56:55]

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

[Irfan, 2016-08-09 17:00:37]

I do not fully understand the rendering side stats handling yet, will talk to
you about it

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
File remoting/protocol/webrtc_connection_to_host.h (right):

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_connection_to_host.h:77: base::TimeTicks
event_timetstamp,
nit: s/event_timetstamp/event_timestamp

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_renderer_adapter.cc (right):

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_renderer_adapter.cc:74: media_stream_ =
std::move(media_stream);
std::move on scoped_refptr is unclear to me. What does it accomplish ?

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_renderer_adapter.cc:112: void
WebrtcVideoRendererAdapter::OnVideoFrameStats(
It is not entirely clear how the logic here in OnVideoFrameStats() works
alongside similar stats logic in FrameRendered(). Perhaps, I should walk over
and understand.

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_stream.cc (right):

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:59: base::TimeTicks can_send_time;
what is this for ?

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:64: std::unique_ptr<FrameTimestamps>
timestamps;
It would be nice to have a consistency on the naming of Packet and Frame here.
They both refer to the same thing. Is it time to call VideoPacket VideoFrame ?

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:254: DCHECK(&video_stats_dispatcher_ ==
channel_dispatcher);
Are these callbacks helpful ? It sounds like we always check if the the channel
is connected

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:325: stats.capture_pending_delay =
packet.timestamps->capture_started_time -
update capture_pending_delay when there is no input event ?

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:327: stats.latest_event_timestamp =
base::TimeTicks::FromInternalValue(
This is duplicated above

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:332: stats.capture_overhead_delay =
(packet.timestamps->capture_ended_time -
Will be helpful to comment what capture_overhead_delay captures.

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:344: stats.send_pending_delay =
base::TimeDelta();
what is send_pending_delay ? How long webrtc waits before sending ?

[Sergey Ulanov, 2016-08-09 22:59:33]

Patchset #3 (id:60001) has been deleted

[Sergey Ulanov, 2016-08-10 18:07:26]

address feedback

[Sergey Ulanov, 2016-08-10 18:07:45]

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
File remoting/protocol/webrtc_connection_to_host.h (right):

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_connection_to_host.h:77: base::TimeTicks
event_timetstamp,
On 2016/08/09 17:00:37, Irfan wrote:
> nit: s/event_timetstamp/event_timestamp

Done.

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_renderer_adapter.cc (right):

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_renderer_adapter.cc:74: media_stream_ =
std::move(media_stream);
On 2016/08/09 17:00:37, Irfan wrote:
> std::move on scoped_refptr is unclear to me. What does it accomplish ?

It generates slightly more optimal code compared to simple assignment as the
ref-counter doesn't need to be incremented and then decremented.

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_renderer_adapter.cc:112: void
WebrtcVideoRendererAdapter::OnVideoFrameStats(
On 2016/08/09 17:00:37, Irfan wrote:
> It is not entirely clear how the logic here in OnVideoFrameStats() works
> alongside similar stats logic in FrameRendered(). Perhaps, I should walk over
> and understand.

Added some comments to make the logic here easier to understand.

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_stream.cc (right):

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:59: base::TimeTicks can_send_time;
On 2016/08/09 17:00:37, Irfan wrote:
> what is this for ?

It wasn't used, removed now.

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:64: std::unique_ptr<FrameTimestamps>
timestamps;
On 2016/08/09 17:00:37, Irfan wrote:
> It would be nice to have a consistency on the naming of Packet and Frame here.
> They both refer to the same thing. Is it time to call VideoPacket VideoFrame ?

packet = encoded frame
Calling it VideoFrame would make it confusing as we also have
webrtc::DesktopFrame, which is used for decoded frames.
But I agree that this distinction between video packet and frame is not clear. I
could rename VideoPacket to EncodedVideoFrame, WDYT?

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:254: DCHECK(&video_stats_dispatcher_ ==
channel_dispatcher);
On 2016/08/09 17:00:37, Irfan wrote:
> Are these callbacks helpful ? It sounds like we always check if the the
channel
> is connected

OnChannelClosed() is useful if only to log the warning. ChannelDispatcherBase
expects some callback and OnChannelInitialized() is useful for other channels.

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:325: stats.capture_pending_delay =
packet.timestamps->capture_started_time -
On 2016/08/09 17:00:37, Irfan wrote:
> update capture_pending_delay when there is no input event ?

capture_pending_delay is defined as the delay between the moment an input event
is injected and the next frame is captured. If the frame wasn't preceded with an
input event then we don't want to set this value.

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:327: stats.latest_event_timestamp =
base::TimeTicks::FromInternalValue(
On 2016/08/09 17:00:37, Irfan wrote:
> This is duplicated above

Removed it from above.

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:332: stats.capture_overhead_delay =
(packet.timestamps->capture_ended_time -
On 2016/08/09 17:00:37, Irfan wrote:
> Will be helpful to comment what capture_overhead_delay captures.

Done.

https://codereview.chromium.org/2200273003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:344: stats.send_pending_delay =
base::TimeDelta();
On 2016/08/09 17:00:37, Irfan wrote:
> what is send_pending_delay ? How long webrtc waits before sending ?

It may wait if the are other frames in the queue.

[Irfan, 2016-08-10 20:58:49]

https://codereview.chromium.org/2200273003/diff/80001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_renderer_adapter.cc (right):

https://codereview.chromium.org/2200273003/diff/80001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_renderer_adapter.cc:221: if
(host_stats_queue_.size() > kMaxQueuedStats) {
This should be client_stats_queue ?

https://codereview.chromium.org/2200273003/diff/80001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_stream.cc (right):

https://codereview.chromium.org/2200273003/diff/80001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:62: std::unique_ptr<VideoPacket>
packet;
+1 for EncodedVideoFrame

[Sergey Ulanov, 2016-08-10 22:29:57]

https://codereview.chromium.org/2200273003/diff/80001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_renderer_adapter.cc (right):

https://codereview.chromium.org/2200273003/diff/80001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_renderer_adapter.cc:221: if
(host_stats_queue_.size() > kMaxQueuedStats) {
On 2016/08/10 20:58:49, Irfan wrote:
> This should be client_stats_queue ?

Done.

https://codereview.chromium.org/2200273003/diff/80001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_stream.cc (right):

https://codereview.chromium.org/2200273003/diff/80001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:62: std::unique_ptr<VideoPacket>
packet;
On 2016/08/10 20:58:49, Irfan wrote:
> +1 for EncodedVideoFrame

Renamed PacketWithTimestamps->EncodedFrameWithTimestamps
Will rename VideoPacket in a separate CL.

[Irfan, 2016-08-10 22:35:33]

lgtm

[Sergey Ulanov, 2016-08-10 22:58:06]

Description was changed from

==========
Enable video stats reporting when using WebRTC

1. Host now initiating video_stats channel and uses it to send stats
   for each frame to the client
2. The client can accept video_stats channel and reports perf numbers
   to the FrameStatsConsumer.
==========

to

==========
Enable video stats reporting when using WebRTC

1. Host now initiating video_stats channel and uses it to send stats
   for each frame to the client
2. The client can accept video_stats channel and reports perf numbers
   to the FrameStatsConsumer.

BUG=621691
==========

[Sergey Ulanov, 2016-08-10 23:06:02]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2016-08-10 23:07:13]

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-08-11 00:08:54]

Description was changed from

==========
Enable video stats reporting when using WebRTC

1. Host now initiating video_stats channel and uses it to send stats
   for each frame to the client
2. The client can accept video_stats channel and reports perf numbers
   to the FrameStatsConsumer.

BUG=621691
==========

to

==========
Enable video stats reporting when using WebRTC

1. Host now initiating video_stats channel and uses it to send stats
   for each frame to the client
2. The client can accept video_stats channel and reports perf numbers
   to the FrameStatsConsumer.

BUG=621691
==========

[commit-bot: I haz the power, 2016-08-11 00:08:56]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-08-11 00:10:21]

Description was changed from

==========
Enable video stats reporting when using WebRTC

1. Host now initiating video_stats channel and uses it to send stats
   for each frame to the client
2. The client can accept video_stats channel and reports perf numbers
   to the FrameStatsConsumer.

BUG=621691
==========

to

==========
Enable video stats reporting when using WebRTC

1. Host now initiating video_stats channel and uses it to send stats
   for each frame to the client
2. The client can accept video_stats channel and reports perf numbers
   to the FrameStatsConsumer.

BUG=621691

Committed: https://crrev.com/09364636097f9c7b34349df97532291a2cfd015d
Cr-Commit-Position: refs/heads/master@{#411196}
==========

[commit-bot: I haz the power, 2016-08-11 00:10:22]

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