Rework PerformanceTracker to make it usable with WebRTC

Rework PerformanceTracker to make it usable with WebRTC

Previously PerformanceTracker was expected to be notified at the
moment when each frame is decoded and rendered. It also had
assumption that latest_event_timestamp is known for each frame
before that frame is rendered. That doesn't work with WebRTC
where frame stats will have to be sent separately from each
frame. With this change PerformanceTracker gets FrameStats
struct for each frame. That struct contains all timestamps for
the frame, which allows to log all relevant stats.

BUG=621691
Committed: https://crrev.com/2ec2751edb7e5cec1d5c3ffa8899b3a281aa486a
Cr-Commit-Position: refs/heads/master@{#402054}

[Sergey Ulanov, 2016-06-24 18:56:44]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2016-06-24 18:56:49]

Patchset #1 (id:20001) has been deleted

[Sergey Ulanov, 2016-06-24 19:04:05]

Description was changed from

==========
Refactor timestamp reporting

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

to

==========
Rework PerformanceTracker to make it usable with WebRTC

Previously PerformanceTracker was expected to be notified at the
moment when each frame is decoded and rendered. It also had
assumption that latest_event_timestamp is known for each frame
before that frame is rendered. That doesn't work with WebRTC 
where frame stats will have to be sent separately from each
frame. With this change PerformanceTracker gets FrameStats
struct for each frame. That struct contains all timestamps for
the frame, which allows to log all relevant stats.

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

[Sergey Ulanov, 2016-06-24 19:04:05]

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

[Jamie, 2016-06-24 23:53:22]

lgtm

https://codereview.chromium.org/2096643003/diff/60001/remoting/client/plugin/...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/2096643003/diff/60001/remoting/client/plugin/...
remoting/client/plugin/pepper_video_renderer_3d.cc:43: stats_.time_rendered =
base::TimeTicks::Now();
Unless you feel strongly otherwise, I think this would be better in an explicit
OnRendered method. This sort of thing is okay in a dtor if the class is defined
for use within a scope, but it seems a bit hacky here.

Failing that, at least add a comment when the list is emptied so it's clear that
something non-trivial is happening as a result.

https://codereview.chromium.org/2096643003/diff/60001/remoting/client/plugin/...
File remoting/client/plugin/pepper_video_renderer_3d.h (right):

https://codereview.chromium.org/2096643003/diff/60001/remoting/client/plugin/...
remoting/client/plugin/pepper_video_renderer_3d.h:110: // Queue of packets that
that have been received, but haven't been passed to
s/Queue/List/ (or just remove "List of" from this and the next comment).

https://codereview.chromium.org/2096643003/diff/60001/remoting/client/softwar...
File remoting/client/software_video_renderer.cc (right):

https://codereview.chromium.org/2096643003/diff/60001/remoting/client/softwar...
remoting/client/software_video_renderer.cc:132: // maintain activity on the
network.
We are no longer just dropping the packet. Add a comment explaining why we
should record the stats for empty packets?

[Sergey Ulanov, 2016-06-25 05:33:03]

Patchset #3 (id:80001) has been deleted

[Sergey Ulanov, 2016-06-25 05:34:04]

https://codereview.chromium.org/2096643003/diff/60001/remoting/client/plugin/...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/2096643003/diff/60001/remoting/client/plugin/...
remoting/client/plugin/pepper_video_renderer_3d.cc:43: stats_.time_rendered =
base::TimeTicks::Now();
On 2016/06/24 23:53:21, Jamie wrote:
> Unless you feel strongly otherwise, I think this would be better in an
explicit
> OnRendered method. This sort of thing is okay in a dtor if the class is
defined
> for use within a scope, but it seems a bit hacky here.
> 
> Failing that, at least add a comment when the list is emptied so it's clear
that
> something non-trivial is happening as a result.

Done.

https://codereview.chromium.org/2096643003/diff/60001/remoting/client/plugin/...
File remoting/client/plugin/pepper_video_renderer_3d.h (right):

https://codereview.chromium.org/2096643003/diff/60001/remoting/client/plugin/...
remoting/client/plugin/pepper_video_renderer_3d.h:110: // Queue of packets that
that have been received, but haven't been passed to
On 2016/06/24 23:53:21, Jamie wrote:
> s/Queue/List/ (or just remove "List of" from this and the next comment).

Done.

https://codereview.chromium.org/2096643003/diff/60001/remoting/client/softwar...
File remoting/client/software_video_renderer.cc (right):

https://codereview.chromium.org/2096643003/diff/60001/remoting/client/softwar...
remoting/client/software_video_renderer.cc:132: // maintain activity on the
network.
On 2016/06/24 23:53:21, Jamie wrote:
> We are no longer just dropping the packet. Add a comment explaining why we
> should record the stats for empty packets?

Done.

[Sergey Ulanov, 2016-06-25 05:34:09]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-06-25 05:34:10]

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/2096643003/#ps100001
(title: "address feedback")

[commit-bot: I haz the power, 2016-06-25 05:34:18]

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-06-25 06:10:59]

Description was changed from

==========
Rework PerformanceTracker to make it usable with WebRTC

Previously PerformanceTracker was expected to be notified at the
moment when each frame is decoded and rendered. It also had
assumption that latest_event_timestamp is known for each frame
before that frame is rendered. That doesn't work with WebRTC 
where frame stats will have to be sent separately from each
frame. With this change PerformanceTracker gets FrameStats
struct for each frame. That struct contains all timestamps for
the frame, which allows to log all relevant stats.

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

to

==========
Rework PerformanceTracker to make it usable with WebRTC

Previously PerformanceTracker was expected to be notified at the
moment when each frame is decoded and rendered. It also had
assumption that latest_event_timestamp is known for each frame
before that frame is rendered. That doesn't work with WebRTC 
where frame stats will have to be sent separately from each
frame. With this change PerformanceTracker gets FrameStats
struct for each frame. That struct contains all timestamps for
the frame, which allows to log all relevant stats.

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

[commit-bot: I haz the power, 2016-06-25 06:11:00]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-06-25 06:12:30]

Description was changed from

==========
Rework PerformanceTracker to make it usable with WebRTC

Previously PerformanceTracker was expected to be notified at the
moment when each frame is decoded and rendered. It also had
assumption that latest_event_timestamp is known for each frame
before that frame is rendered. That doesn't work with WebRTC 
where frame stats will have to be sent separately from each
frame. With this change PerformanceTracker gets FrameStats
struct for each frame. That struct contains all timestamps for
the frame, which allows to log all relevant stats.

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

to

==========
Rework PerformanceTracker to make it usable with WebRTC

Previously PerformanceTracker was expected to be notified at the
moment when each frame is decoded and rendered. It also had
assumption that latest_event_timestamp is known for each frame
before that frame is rendered. That doesn't work with WebRTC
where frame stats will have to be sent separately from each
frame. With this change PerformanceTracker gets FrameStats
struct for each frame. That struct contains all timestamps for
the frame, which allows to log all relevant stats.

BUG=621691

Committed: https://crrev.com/2ec2751edb7e5cec1d5c3ffa8899b3a281aa486a
Cr-Commit-Position: refs/heads/master@{#402054}
==========

[commit-bot: I haz the power, 2016-06-25 06:12:31]

Patchset 3 (id:??) landed as
https://crrev.com/2ec2751edb7e5cec1d5c3ffa8899b3a281aa486a
Cr-Commit-Position: refs/heads/master@{#402054}