Evaluation tests

Implemented according to:
http://datatracker.ietf.org/doc/draft-ietf-rmcat-eval-test/

Added tests 5.1 - 5.8.
Added GccComparison functions.
Modified SelfFairness test.

R=stefan@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/59adf34d84b4df1b0ae032d694e470e2bc27cca0

[magalhaesc, 2015-07-16 11:24:50]

Fixing issues

[magalhaesc, 2015-07-16 11:28:05]

magalhaesc@google.com changed reviewers:
+ stefan@webrtc.org

[magalhaesc, 2015-07-16 12:23:28]

Evaluation tests

[stefan-webrtc, 2015-07-21 12:12:32]

A few first comments, I will look closer at the bwe_test.cc later.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc (right):

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:273: Random
random(0x12345678);
I'd prefer if these (PacedSelfFairness*) by default had a random seed.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:317: Random
random(0x12345678);
Same for TcpFairness*Test

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:342: // Following
test cases begin with "Evaluation" as a referrence to the
The following...

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:346:
this->RunVariableCapacitySingleFlow(GetParam());
Remove this->.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:350:
this->RunVariableCapacityTwoFlows(GetParam());
Remove this->.

Can we generalize RunVariableCapacitySingleFlows and RunVariableCapacityTwoFlows
to RunVariableCapacity(GetParam(), N), or do they test different variations in
capacity? In that case maybe they should be named to reflect that instead in
that case?

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:354:
this->RunBidirectionalFlow(GetParam());
Remove this->.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:377:
this->RunRoundTripTimeFairness(GetParam());
Remove this->

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:381:
this->RunLongTcpFairness(GetParam());
Remove this->

Btw, can this be implemented with RunFairnessTest(GetParam(), 1, 1...) instead?

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:389:
this->RunMultipleShortTcpFairness(GetParam(),
Remove this->

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:395:
this->RunPauseResumeFlows(GetParam());
Remove this->

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:402:
this->RunVariableCapacitySingleFlow(GetParam());
Remove this-> here and all below.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:404:
gcc_test.set_plot_available_capacity(false);
Maybe make this part of a second constructor of BweTest?

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimators_test.cc
(right):

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimators_test.cc:353:
const int64_t kOffsetMs[] = {0, 5000, 10000, 15000};
Previously I think we had random offsets here. I still want it to be a bit
random so that we can run several runs and get different results, since this is
used for measuring performance and optimizing parameters.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test.h (right):

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test.h:165:
SetDefaultParameters(this);
This is weird. Why not just SetDefaultParameters();? Or should
SetDefaultParameters() be static? I think you could basically use constructor
delegation instead of this.

[magalhaesc, 2015-07-21 14:46:03]

Comments addressed

[magalhaesc, 2015-07-21 14:46:53]

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc (right):

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:273: Random
random(0x12345678);
On 2015/07/21 12:12:32, stefan-webrtc (holmer) wrote:
> I'd prefer if these (PacedSelfFairness*) by default had a random seed.

Done.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:317: Random
random(0x12345678);
On 2015/07/21 12:12:31, stefan-webrtc (holmer) wrote:
> Same for TcpFairness*Test

Done.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:342: // Following
test cases begin with "Evaluation" as a referrence to the
On 2015/07/21 12:12:31, stefan-webrtc (holmer) wrote:
> The following...

Done.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:346:
this->RunVariableCapacitySingleFlow(GetParam());
On 2015/07/21 12:12:31, stefan-webrtc (holmer) wrote:
> Remove this->.

Done.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:350:
this->RunVariableCapacityTwoFlows(GetParam());
On 2015/07/21 12:12:31, stefan-webrtc (holmer) wrote:
> Remove this->.
> 
> Can we generalize RunVariableCapacitySingleFlows and
RunVariableCapacityTwoFlows
> to RunVariableCapacity(GetParam(), N), or do they test different variations in
> capacity? In that case maybe they should be named to reflect that instead in
> that case?

They test different variations in capacity. Names updated.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:354:
this->RunBidirectionalFlow(GetParam());
On 2015/07/21 12:12:31, stefan-webrtc (holmer) wrote:
> Remove this->.

Done.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:377:
this->RunRoundTripTimeFairness(GetParam());
On 2015/07/21 12:12:31, stefan-webrtc (holmer) wrote:
> Remove this->

Done.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:381:
this->RunLongTcpFairness(GetParam());
On 2015/07/21 12:12:31, stefan-webrtc (holmer) wrote:
> Remove this->
> 
> Btw, can this be implemented with RunFairnessTest(GetParam(), 1, 1...)
instead?

Yes, with a few modifications

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:381:
this->RunLongTcpFairness(GetParam());
On 2015/07/21 12:12:31, stefan-webrtc (holmer) wrote:
> Remove this->
> 
> Btw, can this be implemented with RunFairnessTest(GetParam(), 1, 1...)
instead?

Done.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:389:
this->RunMultipleShortTcpFairness(GetParam(),
On 2015/07/21 12:12:31, stefan-webrtc (holmer) wrote:
> Remove this->

Done.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:395:
this->RunPauseResumeFlows(GetParam());
On 2015/07/21 12:12:32, stefan-webrtc (holmer) wrote:
> Remove this->

Done.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:402:
this->RunVariableCapacitySingleFlow(GetParam());
On 2015/07/21 12:12:31, stefan-webrtc (holmer) wrote:
> Remove this-> here and all below.

Done.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:404:
gcc_test.set_plot_available_capacity(false);
On 2015/07/21 12:12:31, stefan-webrtc (holmer) wrote:
> Maybe make this part of a second constructor of BweTest?

Done.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimators_test.cc
(right):

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimators_test.cc:353:
const int64_t kOffsetMs[] = {0, 5000, 10000, 15000};
On 2015/07/21 12:12:32, stefan-webrtc (holmer) wrote:
> Previously I think we had random offsets here. I still want it to be a bit
> random so that we can run several runs and get different results, since this
is
> used for measuring performance and optimizing parameters.

Right, done.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test.h (right):

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test.h:165:
SetDefaultParameters(this);
On 2015/07/21 12:12:32, stefan-webrtc (holmer) wrote:
> This is weird. Why not just SetDefaultParameters();? Or should
> SetDefaultParameters() be static? I think you could basically use constructor
> delegation instead of this.

Right. 
It was supposed to be static, but actually it wasn't being called elsewhere.

[magalhaesc, 2015-07-21 14:55:55]

Comments addressed

[magalhaesc, 2015-07-21 15:01:28]

Comments addressed

[magalhaesc, 2015-07-21 16:01:25]

Patchset #3 (id:40001) has been deleted

[magalhaesc, 2015-07-21 16:01:34]

Patchset #3 (id:60001) has been deleted

[magalhaesc, 2015-07-21 17:05:46]

Thanks for the review.
Comments addressed

[stefan-webrtc, 2015-07-22 09:30:29]

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc (right):

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:273: Random
random(0x12345678);
On 2015/07/21 14:46:53, magalhaesc wrote:
> On 2015/07/21 12:12:32, stefan-webrtc (holmer) wrote:
> > I'd prefer if these (PacedSelfFairness*) by default had a random seed.
> 
> Done.

I think you misunderstood. I wanted you to not have a fixed constant for the
seed passed to Random's constructor. Instead do:
Random random(Clock::GetRealTimeClock()->TimeInMicroseconds());

Or possibly use rand() instead?

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:350:
this->RunVariableCapacityTwoFlows(GetParam());
On 2015/07/21 14:46:53, magalhaesc wrote:
> On 2015/07/21 12:12:31, stefan-webrtc (holmer) wrote:
> > Remove this->.
> > 
> > Can we generalize RunVariableCapacitySingleFlows and
> RunVariableCapacityTwoFlows
> > to RunVariableCapacity(GetParam(), N), or do they test different variations
in
> > capacity? In that case maybe they should be named to reflect that instead in
> > that case?
> 
> They test different variations in capacity. Names updated.

Thanks! Maybe also make it possible to pass in the number of flows here, since
it now says MultipleFlows?

https://codereview.webrtc.org/1237393002/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc (right):

https://codereview.webrtc.org/1237393002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:317:
static_cast<int64_t>(random_.Rand(0, 2 * kAverageOffsetMs), 0)};
This line looks strange. static_cast<>() doesn't take two arguments I believe?

[magalhaesc, 2015-07-22 11:22:54]

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc (right):

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:273: Random
random(0x12345678);
On 2015/07/22 09:30:29, stefan-webrtc (holmer) wrote:
> On 2015/07/21 14:46:53, magalhaesc wrote:
> > On 2015/07/21 12:12:32, stefan-webrtc (holmer) wrote:
> > > I'd prefer if these (PacedSelfFairness*) by default had a random seed.
> > 
> > Done.
> 
> I think you misunderstood. I wanted you to not have a fixed constant for the
> seed passed to Random's constructor. Instead do:
> Random random(Clock::GetRealTimeClock()->TimeInMicroseconds());
> 
> Or possibly use rand() instead?

Yes, I had misunderstood. Since we're using the method Random::Rand(low_bound,
up_bound), I'll initialize it with a random seed.

https://codereview.webrtc.org/1237393002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:350:
this->RunVariableCapacityTwoFlows(GetParam());
On 2015/07/22 09:30:29, stefan-webrtc (holmer) wrote:
> On 2015/07/21 14:46:53, magalhaesc wrote:
> > On 2015/07/21 12:12:31, stefan-webrtc (holmer) wrote:
> > > Remove this->.
> > > 
> > > Can we generalize RunVariableCapacitySingleFlows and
> > RunVariableCapacityTwoFlows
> > > to RunVariableCapacity(GetParam(), N), or do they test different
variations
> in
> > > capacity? In that case maybe they should be named to reflect that instead
in
> > > that case?
> > 
> > They test different variations in capacity. Names updated.
> 
> Thanks! Maybe also make it possible to pass in the number of flows here, since
> it now says MultipleFlows?

Yes, it will be better. Done.

[magalhaesc, 2015-07-22 11:24:52]

https://codereview.webrtc.org/1237393002/diff/80001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc (right):

https://codereview.webrtc.org/1237393002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/bwe_simulations.cc:317:
static_cast<int64_t>(random_.Rand(0, 2 * kAverageOffsetMs), 0)};
On 2015/07/22 09:30:29, stefan-webrtc (holmer) wrote:
> This line looks strange. static_cast<>() doesn't take two arguments I believe?

Actually since we are upcasting here it is ok remove the static_cast.

[magalhaesc, 2015-07-22 11:28:32]

Comments addressed

[magalhaesc, 2015-07-22 11:51:17]

Comments addressed, fixed int type issues

[magalhaesc, 2015-07-22 12:50:55]

Fixed BweTest bug

[magalhaesc, 2015-07-22 12:55:39]

Patchset #5 (id:120001) has been deleted

[magalhaesc, 2015-07-22 15:42:28]

Thanks, comments addressed.

[stefan-webrtc, 2015-07-23 11:33:33]

A few small comments, but with those addressed lgtm.

https://codereview.webrtc.org/1237393002/diff/140001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test.cc (right):

https://codereview.webrtc.org/1237393002/diff/140001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test.cc:434: int flow_id =
bwe_type;  // Arbitrary value.
Hm, looks weird. Why not just 0?

https://codereview.webrtc.org/1237393002/diff/140001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test.cc:472: // flow_id);
Delete or uncomment.

https://codereview.webrtc.org/1237393002/diff/140001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test.cc:495: DelayFilter
down_filter(&downlink_, flow_ids);
I think it would make sense to move the down_filter to after the receivers are
instantiated to make it more clear that they are on the downlink (after
receivers).

[magalhaesc, 2015-07-23 12:11:23]

Comments addressed [3]

[magalhaesc, 2015-07-23 12:11:53]

Thanks, comments addressed

https://codereview.webrtc.org/1237393002/diff/140001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test.cc (right):

https://codereview.webrtc.org/1237393002/diff/140001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test.cc:434: int flow_id =
bwe_type;  // Arbitrary value.
On 2015/07/23 11:33:33, stefan-webrtc (holmer) wrote:
> Hm, looks weird. Why not just 0?

Done.

https://codereview.webrtc.org/1237393002/diff/140001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test.cc:472: // flow_id);
On 2015/07/23 11:33:33, stefan-webrtc (holmer) wrote:
> Delete or uncomment.

Done.

https://codereview.webrtc.org/1237393002/diff/140001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/bwe_test.cc:495: DelayFilter
down_filter(&downlink_, flow_ids);
On 2015/07/23 11:33:33, stefan-webrtc (holmer) wrote:
> I think it would make sense to move the down_filter to after the receivers are
> instantiated to make it more clear that they are on the downlink (after
> receivers).

Right, done.

[magalhaesc, 2015-07-23 13:40:53]

Committed patchset #6 (id:160001) manually as
59adf34d84b4df1b0ae032d694e470e2bc27cca0 (presubmit successful).