Add unit tests for sub-components of CaptureScheduler.

Add unit tests for sub-components of CaptureScheduler.

Unit tests are added for RateCounter, RunningAverage and CaptureScheduler.

BUG=166275
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190172

[Wez, 2013-03-18 18:10:22]

PTAL

[alexeypa (please no reviews), 2013-03-18 19:39:39]

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
File remoting/base/rate_counter.cc (right):

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.cc:7: #include "base/logging.h"
Is this header actually used?

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.cc:14: }
nit: Add CHECK(time_window_.InMilliseconds() > 0)

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.cc:29: base::Time current_time = CurrentTime();
Should CurrentTime() be called under the lock?

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.cc:33: return
static_cast<double>(base::Time::kMillisecondsPerSecond) * sum_ /
nit: Since it is converting to double anyway it will be shorter if you write it
as:

return static_cast<double>(sum_) / time_window_.InSecondsF();

Or even:

return sum_ / time_window_.InSecondsF();

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
File remoting/base/rate_counter.h (right):

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.h:26: RateCounter(base::TimeDelta time_window);
nit: explicit

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.h:37: void SetCurrentTimeForTest(base::Time
current_time);
It might be better to pass |current_time| explicitly to Record() and Rate()
methods. This way RateCounter will be able to work with virtual time.

Regardless of how |current_time| is passed, it is important to check that it is
not decreasing between the calls, otherwise the logic in this class will break.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.h:47: typedef std::pair<base::Time, int64> DataPoint;
nit: I think this can be moved down, closer to |data_points_| declaration.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.h:50: base::TimeDelta time_window_;
nit: |time_window_| can be const.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.h:52: // Protects |data_points_| and |sum_|.
nit: It also protects |time_window_|.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.h:53: base::Lock lock_;
nit: How about inheriting this class from base::NonThreadSafe instead? Is it
going to be a large change?

Same question about RunningAverage.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
File remoting/base/rate_counter_unittest.cc (right):

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter_unittest.cc:12: TEST(RateCounterTest,
OneSecondWindow) {
nit: Add the case when Record() is never called.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter_unittest.cc:35: EXPECT_EQ(expected,
rate_counter.Rate());
nit: Verify the result of Rate() when |i| is 0 as well.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter_unittest.cc:43: const size_t kWindowSeconds =
arraysize(kTestValues) + 1;
I guess it is better to have kWindowsSeconds = arraysize(kTestValues) and do not
add kTestValues[0] to |expected| to emphasize that the time window is (now -
time_window, now]. I.e. it includes |now| but does not include |now -
time_window|.

It is even better to explain that in the comment in front of  RateCounter to
give an idea how it works.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/running_...
File remoting/base/running_average_unittest.cc (right):

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/running_...
remoting/base/running_average_unittest.cc:12: TEST(RunningAverageTest,
OneElementWindow) {
nit: Add the case when Record() wasn't called at all.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/running_...
remoting/base/running_average_unittest.cc:43: expected /= i+1;
nit: i + 1

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/host/capture_...
File remoting/host/capture_scheduler.cc (right):

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/host/capture_...
remoting/host/capture_scheduler.cc:48: (kRecordingCpuConsumption *
num_of_processors_);
nit: Unrelated to this CL but I think this formula is wrong when there are more
that two processors because we only have two threads. It also does not account
for threads, used by VP8 codec itself.

PS. Cores with hyper-threading enabled should probably count as half of a core.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/host/capture_...
File remoting/host/capture_scheduler_unittest.cc (right):

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/host/capture_...
remoting/host/capture_scheduler_unittest.cc:59: { 360, 290, 233, 133, 80, 80,
133, 233 }, // One core.
nit: How are there values obtained? If CaptureScheduler changes will these
values need to be regenerated by running this test and printing them out?

It might be better to calculate them here, instead of having the expected result
hardcoded.

[Wez, 2013-03-19 23:23:14]

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
File remoting/base/rate_counter.cc (right):

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.cc:7: #include "base/logging.h"
On 2013/03/18 19:39:39, alexeypa wrote:
> Is this header actually used?

No; used it during development. It's needed for the suggested CHECKs, though.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.cc:14: }
On 2013/03/18 19:39:39, alexeypa wrote:
> nit: Add CHECK(time_window_.InMilliseconds() > 0)

Done.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.cc:29: base::Time current_time = CurrentTime();
On 2013/03/18 19:39:39, alexeypa wrote:
> Should CurrentTime() be called under the lock?

No, I don't think so.  This way it captures the time at which the call was made.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.cc:33: return
static_cast<double>(base::Time::kMillisecondsPerSecond) * sum_ /
On 2013/03/18 19:39:39, alexeypa wrote:
> nit: Since it is converting to double anyway it will be shorter if you write
it
> as:
> 
> return static_cast<double>(sum_) / time_window_.InSecondsF();
> 
> Or even:
> 
> return sum_ / time_window_.InSecondsF();

Done.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
File remoting/base/rate_counter.h (right):

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.h:26: RateCounter(base::TimeDelta time_window);
On 2013/03/18 19:39:39, alexeypa wrote:
> nit: explicit

Done.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.h:37: void SetCurrentTimeForTest(base::Time
current_time);
On 2013/03/18 19:39:39, alexeypa wrote:
> It might be better to pass |current_time| explicitly to Record() and Rate()
> methods. This way RateCounter will be able to work with virtual time.

That would require the caller to pass base::Time::Now() on every call, which
doesn't seem worth it right now.

> Regardless of how |current_time| is passed, it is important to check that it
is
> not decreasing between the calls, otherwise the logic in this class will
break.

True, although the only code calling this will be tests. :)

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.h:47: typedef std::pair<base::Time, int64> DataPoint;
On 2013/03/18 19:39:39, alexeypa wrote:
> nit: I think this can be moved down, closer to |data_points_| declaration.

According to the style guide, it actually needs to move up, before the methods.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.h:50: base::TimeDelta time_window_;
On 2013/03/18 19:39:39, alexeypa wrote:
> nit: |time_window_| can be const.

Done.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.h:52: // Protects |data_points_| and |sum_|.
On 2013/03/18 19:39:39, alexeypa wrote:
> nit: It also protects |time_window_|.

Nope.  |time_window| is set once, at init time (see your comment above re it
being const).

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter.h:53: base::Lock lock_;
On 2013/03/18 19:39:39, alexeypa wrote:
> nit: How about inheriting this class from base::NonThreadSafe instead? Is it
> going to be a large change?
> 
> Same question about RunningAverage.

I wanted to avoid too much refactoring in this CL, but it does look like both
Record and Rate are called on the plugin thread, so yes, that should work.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
File remoting/base/rate_counter_unittest.cc (right):

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter_unittest.cc:12: TEST(RateCounterTest,
OneSecondWindow) {
On 2013/03/18 19:39:39, alexeypa wrote:
> nit: Add the case when Record() is never called.

Done.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter_unittest.cc:35: EXPECT_EQ(expected,
rate_counter.Rate());
On 2013/03/18 19:39:39, alexeypa wrote:
> nit: Verify the result of Rate() when |i| is 0 as well.

Done.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/rate_cou...
remoting/base/rate_counter_unittest.cc:43: const size_t kWindowSeconds =
arraysize(kTestValues) + 1;
On 2013/03/18 19:39:39, alexeypa wrote:
> I guess it is better to have kWindowsSeconds = arraysize(kTestValues) and do
not
> add kTestValues[0] to |expected| to emphasize that the time window is (now -
> time_window, now]. I.e. it includes |now| but does not include |now -
> time_window|.
> 
> It is even better to explain that in the comment in front of  RateCounter to
> give an idea how it works.

I've reworded the comments and updated this test to have an N-1 length window.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/running_...
File remoting/base/running_average_unittest.cc (right):

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/running_...
remoting/base/running_average_unittest.cc:12: TEST(RunningAverageTest,
OneElementWindow) {
On 2013/03/18 19:39:39, alexeypa wrote:
> nit: Add the case when Record() wasn't called at all.

Done.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/base/running_...
remoting/base/running_average_unittest.cc:43: expected /= i+1;
On 2013/03/18 19:39:39, alexeypa wrote:
> nit: i + 1

Done.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/host/capture_...
File remoting/host/capture_scheduler.cc (right):

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/host/capture_...
remoting/host/capture_scheduler.cc:48: (kRecordingCpuConsumption *
num_of_processors_);
On 2013/03/18 19:39:39, alexeypa wrote:
> nit: Unrelated to this CL but I think this formula is wrong when there are
more
> that two processors because we only have two threads. It also does not account
> for threads, used by VP8 codec itself.

Yes, we should review our scheduling.  There are several issues:
- No matter how many cores there are, there's no point trying to capture faster
than |capture_time_| or |encode_time_|.
- Since we use two encode threads encoding may be taking up to double
|encode_time_|.
- We should avoid varying the capture delay too much and aim for e.g. 60, 30,
15fps depending on capture & encode performance.

> PS. Cores with hyper-threading enabled should probably count as half of a
core.

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/host/capture_...
File remoting/host/capture_scheduler_unittest.cc (right):

https://chromiumcodereview.appspot.com/12803008/diff/1/remoting/host/capture_...
remoting/host/capture_scheduler_unittest.cc:59: { 360, 290, 233, 133, 80, 80,
133, 233 }, // One core.
On 2013/03/18 19:39:39, alexeypa wrote:
> nit: How are there values obtained? If CaptureScheduler changes will these
> values need to be regenerated by running this test and printing them out?
> 
> It might be better to calculate them here, instead of having the expected
result
> hardcoded.

Then we'd just have two copies of the same calculation, though, and having
manually-calculated values here also makes it possible to sanity-check the
numbers by eye.

[alexeypa (please no reviews), 2013-03-19 23:36:55]

lgtm

https://chromiumcodereview.appspot.com/12803008/diff/10001/remoting/base/rate...
File remoting/base/rate_counter.h (right):

https://chromiumcodereview.appspot.com/12803008/diff/10001/remoting/base/rate...
remoting/base/rate_counter.h:12: #include "base/synchronization/lock.h"
nit: lock.h is not needed any more.

https://chromiumcodereview.appspot.com/12803008/diff/10001/remoting/base/rate...
File remoting/base/rate_counter_unittest.cc (right):

https://chromiumcodereview.appspot.com/12803008/diff/10001/remoting/base/rate...
remoting/base/rate_counter_unittest.cc:72: for (size_t i = 1; i <
arraysize(kTestValues); ++i) {
nit: I guess kTestValues[0] should also be recorded but not added to |expected|.

https://chromiumcodereview.appspot.com/12803008/diff/10001/remoting/base/runn...
File remoting/base/running_average.h (right):

https://chromiumcodereview.appspot.com/12803008/diff/10001/remoting/base/runn...
remoting/base/running_average.h:11: #include "base/synchronization/lock.h"
nit: lock.h is not needed any more.

[Wez, 2013-03-20 00:17:26]

https://chromiumcodereview.appspot.com/12803008/diff/10001/remoting/base/rate...
File remoting/base/rate_counter.h (right):

https://chromiumcodereview.appspot.com/12803008/diff/10001/remoting/base/rate...
remoting/base/rate_counter.h:12: #include "base/synchronization/lock.h"
On 2013/03/19 23:36:55, alexeypa wrote:
> nit: lock.h is not needed any more.

Done.

https://chromiumcodereview.appspot.com/12803008/diff/10001/remoting/base/rate...
File remoting/base/rate_counter_unittest.cc (right):

https://chromiumcodereview.appspot.com/12803008/diff/10001/remoting/base/rate...
remoting/base/rate_counter_unittest.cc:72: for (size_t i = 1; i <
arraysize(kTestValues); ++i) {
On 2013/03/19 23:36:55, alexeypa wrote:
> nit: I guess kTestValues[0] should also be recorded but not added to
|expected|.

Done.

https://chromiumcodereview.appspot.com/12803008/diff/10001/remoting/base/runn...
File remoting/base/running_average.h (right):

https://chromiumcodereview.appspot.com/12803008/diff/10001/remoting/base/runn...
remoting/base/running_average.h:11: #include "base/synchronization/lock.h"
On 2013/03/19 23:36:55, alexeypa wrote:
> nit: lock.h is not needed any more.

Done.

[commit-bot: I haz the power, 2013-03-20 00:19:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/12803008/21001

[commit-bot: I haz the power, 2013-03-20 03:42:36]

Retried try job too often on win_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-03-21 20:26:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/12803008/21001

[commit-bot: I haz the power, 2013-03-22 18:44:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/12803008/21001

[commit-bot: I haz the power, 2013-03-22 20:10:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/12803008/21001

[commit-bot: I haz the power, 2013-03-23 15:33:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/12803008/21001

[commit-bot: I haz the power, 2013-03-24 03:53:41]

Change committed as 190172