Adding cpu measurments to video_quality_tests

Adding cpu measurments to video_quality_tests

BUG=webrtc:7095
Review-Url: https://codereview.webrtc.org/2711493002
Cr-Commit-Position: refs/heads/master@{#16791}
Committed: https://chromium.googlesource.com/external/webrtc/+/df92c5cb8c34c7c43e9f0ec66914a86deeeda626

[ilnik, 2017-02-21 12:58:11]

ilnik@webrtc.org changed reviewers:
+ sprang@webrtc.org

[sprang_webrtc, 2017-02-21 15:43:20]

looks good, just some minor suggestions

https://codereview.webrtc.org/2711493002/diff/40001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2711493002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:200: // Start time measurements
nit: End comments with period.

https://codereview.webrtc.org/2711493002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:201: cpu_time_ -=
rtc::GetProcessCpuTimeNanos();
I'm not sure we should start measuring time from the construction time of the
analyzer. Then we'll include some setup time, may include open files for the
capturer etc. 

Maybe you can set the start time in Wait()? Possibly add an explicit Start()
method, might be other things that should go there as well.

https://codereview.webrtc.org/2711493002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:201: cpu_time_ -=
rtc::GetProcessCpuTimeNanos();
I was a bit confused of the usage here at first. Can you maybe rename them
cpu_time_start_offset_ or something and don't start the negative offset. Then
just take the delta in a local variable at the end?

https://codereview.webrtc.org/2711493002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:341: cpu_time_ +=
rtc::GetThreadCpuTimeNanos();
Is this scope significant?
I think it would be more readable if you create a temp variable here. Maybe
int64_t comparison_cpu_time_start_offset_ = rtc::GetThreadCpuTimeNanos();

https://codereview.webrtc.org/2711493002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:374: cpu_time_ -=
rtc::GetThreadCpuTimeNanos();
...and here did something like
cpu_time_start_offset_ += rtc::GetThreadCpuTimeNanos() -
comparison_cpu_time_start_offset_;

https://codereview.webrtc.org/2711493002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:626: cpu_time_ +=
rtc::GetThreadCpuTimeNanos();
Could do same thing with a local temp timestamp here as above.

[ilnik, 2017-02-22 08:43:28]

https://codereview.webrtc.org/2711493002/diff/40001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2711493002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:200: // Start time measurements
On 2017/02/21 15:43:20, språng wrote:
> nit: End comments with period.

Done.

https://codereview.webrtc.org/2711493002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:201: cpu_time_ -=
rtc::GetProcessCpuTimeNanos();
On 2017/02/21 15:43:20, språng wrote:
> I was a bit confused of the usage here at first. Can you maybe rename them
> cpu_time_start_offset_ or something and don't start the negative offset. Then
> just take the delta in a local variable at the end?

I created special functions for manipulating cpu_time_. I hope now it's clear
that's happening there.

https://codereview.webrtc.org/2711493002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:201: cpu_time_ -=
rtc::GetProcessCpuTimeNanos();
On 2017/02/21 15:43:20, språng wrote:
> I'm not sure we should start measuring time from the construction time of the
> analyzer. Then we'll include some setup time, may include open files for the
> capturer etc. 
> 
> Maybe you can set the start time in Wait()? Possibly add an explicit Start()
> method, might be other things that should go there as well.

Done.

https://codereview.webrtc.org/2711493002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:341: cpu_time_ +=
rtc::GetThreadCpuTimeNanos();
On 2017/02/21 15:43:20, språng wrote:
> Is this scope significant?
> I think it would be more readable if you create a temp variable here. Maybe
> int64_t comparison_cpu_time_start_offset_ = rtc::GetThreadCpuTimeNanos();

Done it the other way.

https://codereview.webrtc.org/2711493002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:374: cpu_time_ -=
rtc::GetThreadCpuTimeNanos();
On 2017/02/21 15:43:20, språng wrote:
> ...and here did something like
> cpu_time_start_offset_ += rtc::GetThreadCpuTimeNanos() -
> comparison_cpu_time_start_offset_;

Acknowledged.

https://codereview.webrtc.org/2711493002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:626: cpu_time_ +=
rtc::GetThreadCpuTimeNanos();
On 2017/02/21 15:43:20, språng wrote:
> Could do same thing with a local temp timestamp here as above.

Acknowledged.

[ilnik, 2017-02-22 14:18:30]

Description was changed from

==========
Adding cpu measurments to video_quality_tests

BUG=webrtc:7095
==========

to

==========
Adding cpu measurments to video_quality_tests

BUG=webrtc:7095
==========

[ilnik, 2017-02-22 14:18:30]

ilnik@webrtc.org changed reviewers:
+ brandtr@webrtc.org

[sprang_webrtc, 2017-02-22 16:44:03]

lgtm with nits

https://codereview.webrtc.org/2711493002/diff/60001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2711493002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:442: double GetCpuUsage() {
nit: GetCpuUsagePercent()

https://codereview.webrtc.org/2711493002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:1434:
analyzer.StartMeasuringCpuProcessTime();
I think you can move this down even further. Definitely past
StartEncodedFrameLogs, but maybe it makes sense to even have it just before
video_capturer_->Start(), as no payload data will actually go through the
pipeline until we start producing frames..

[ilnik, 2017-02-23 09:00:59]

https://codereview.webrtc.org/2711493002/diff/60001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2711493002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:442: double GetCpuUsage() {
On 2017/02/22 16:44:03, språng wrote:
> nit: GetCpuUsagePercent()

Done.

https://codereview.webrtc.org/2711493002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:1434:
analyzer.StartMeasuringCpuProcessTime();
On 2017/02/22 16:44:03, språng wrote:
> I think you can move this down even further. Definitely past
> StartEncodedFrameLogs, but maybe it makes sense to even have it just before
> video_capturer_->Start(), as no payload data will actually go through the
> pipeline until we start producing frames..

Done.

[ilnik, 2017-02-23 09:02:13]

The CQ bit was checked by ilnik@webrtc.org

[ilnik, 2017-02-23 09:02:14]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2711493002/#ps80001 (title:
"Implemented Sprang@ comments")

[commit-bot: I haz the power, 2017-02-23 09:02:25]

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

[commit-bot: I haz the power, 2017-02-23 10:08:44]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487840533762660,
"parent_rev": "abc0080df81066ce5f2de62f37c06c343b05ed16", "commit_rev":
"df92c5cb8c34c7c43e9f0ec66914a86deeeda626"}

[commit-bot: I haz the power, 2017-02-23 10:08:46]

Description was changed from

==========
Adding cpu measurments to video_quality_tests

BUG=webrtc:7095
==========

to

==========
Adding cpu measurments to video_quality_tests

BUG=webrtc:7095

Review-Url: https://codereview.webrtc.org/2711493002
Cr-Commit-Position: refs/heads/master@{#16791}
Committed:
https://chromium.googlesource.com/external/webrtc/+/df92c5cb8c34c7c43e9f0ec66...
==========

[commit-bot: I haz the power, 2017-02-23 10:08:47]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/df92c5cb8c34c7c43e9f0ec66...