Add a new overuse estimator for the delay based BWE behind experiment.

Add a new overuse estimator for the delay based BWE behind experiment.
Parse the estimation parameters from the field trial string.

BUG=webrtc:6690
Committed: https://crrev.com/afaef8bbebb8aecd32b3b7c5e47d60fd5526a126
Cr-Commit-Position: refs/heads/master@{#15126}

[terelius, 2016-11-10 13:48:09]

The CQ bit was checked by terelius@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 13:48:13]

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

[terelius, 2016-11-10 13:52:09]

terelius@webrtc.org changed reviewers:
+ stefan@webrtc.org

[terelius, 2016-11-10 13:52:09]

Please take a look.

[commit-bot: I haz the power, 2016-11-10 14:13:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-10 14:13:39]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2016-11-13 13:09:53]

Mostly nits.

I think it would be good to have a unittest for the new class though. Something
simple would be enough.

Maybe also a test to verify the experiment? It would be good to at least run
some of the existing tests for capacity drops etc:
https://chromium.googlesource.com/external/webrtc/+/59695e596bdb36acdaccbf99a...

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/delay_based_bwe.cc (right):

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.cc:70: if (2 <=
*window_points && 0 <= *smoothing_coef && *smoothing_coef <= 1 &&
I personally prefer having variables first and constants second, e.g.,
if (*window_points > 2 ...

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.cc:74: LOG(LS_ERROR) <<
"Needs at least 2 points to fit a trend line.";
Should we perhaps DCHECK on all broken configurations instead?

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/trendline_estimator.h (right):

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/trendline_estimator.h:25: //
|smoothing_coef| controls how the much we smooth out the delay
how much

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/trendline_estimator.h:26: // before fitting
the trendline. |threashold_gain| is used to scale the
threshold_gain

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/trendline_estimator.h:27: // trendline
slope for comparison to the old threshold.
Maybe add a comment that the threshold gain perhaps should be removed once the
old estimator is fully replaced?

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/trendline_estimator.h:52: unsigned int
num_of_deltas_;
Can we use an int instead?

[terelius, 2016-11-14 12:15:25]

The CQ bit was checked by terelius@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 12:15:30]

Dry run: 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, 2016-11-14 12:40:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-14 12:40:49]

Dry run: This issue passed the CQ dry run.

[terelius, 2016-11-14 13:21:39]

Working on unit test and integration test. Response to other comments below.

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/delay_based_bwe.cc (right):

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.cc:70: if (2 <=
*window_points && 0 <= *smoothing_coef && *smoothing_coef <= 1 &&
On 2016/11/13 13:09:53, stefan-webrtc (holmer) wrote:
> I personally prefer having variables first and constants second, e.g.,
> if (*window_points > 2 ...

In general I agree, but for the smoothing_coef I wanted to highlight that it is
in the interval [0,1], so I wanted to write something similar to the
mathematical relation 0 <= *smoothing_coef <= 1.

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.cc:74: LOG(LS_ERROR) <<
"Needs at least 2 points to fit a trend line.";
On 2016/11/13 13:09:53, stefan-webrtc (holmer) wrote:
> Should we perhaps DCHECK on all broken configurations instead?

It would not give us any alert if we misconfigure the parameters in release
builds. Would CHECK be appropriate?

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/trendline_estimator.h (right):

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/trendline_estimator.h:25: //
|smoothing_coef| controls how the much we smooth out the delay
On 2016/11/13 13:09:53, stefan-webrtc (holmer) wrote:
> how much

Done.

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/trendline_estimator.h:26: // before fitting
the trendline. |threashold_gain| is used to scale the
On 2016/11/13 13:09:53, stefan-webrtc (holmer) wrote:
> threshold_gain

Done.

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/trendline_estimator.h:27: // trendline
slope for comparison to the old threshold.
On 2016/11/13 13:09:53, stefan-webrtc (holmer) wrote:
> Maybe add a comment that the threshold gain perhaps should be removed once the
> old estimator is fully replaced?

Done.

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/trendline_estimator.h:52: unsigned int
num_of_deltas_;
On 2016/11/13 13:09:53, stefan-webrtc (holmer) wrote:
> Can we use an int instead?

Yes except that the Kalman filter uses an unsigned int, and I want to have the
same types in both estimators. Should I change both to int?

[stefan-webrtc, 2016-11-14 13:30:58]

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/delay_based_bwe.cc (right):

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.cc:70: if (2 <=
*window_points && 0 <= *smoothing_coef && *smoothing_coef <= 1 &&
On 2016/11/14 13:21:39, terelius wrote:
> On 2016/11/13 13:09:53, stefan-webrtc (holmer) wrote:
> > I personally prefer having variables first and constants second, e.g.,
> > if (*window_points > 2 ...
> 
> In general I agree, but for the smoothing_coef I wanted to highlight that it
is
> in the interval [0,1], so I wanted to write something similar to the
> mathematical relation 0 <= *smoothing_coef <= 1.

Sounds good for smoothing_coef, but the other two doesn't really follow the same
logic?

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.cc:74: LOG(LS_ERROR) <<
"Needs at least 2 points to fit a trend line.";
On 2016/11/14 13:21:39, terelius wrote:
> On 2016/11/13 13:09:53, stefan-webrtc (holmer) wrote:
> > Should we perhaps DCHECK on all broken configurations instead?
> 
> It would not give us any alert if we misconfigure the parameters in release
> builds. Would CHECK be appropriate?

Yes, then I think we should CHECK as it would be bad if we didn't catch those
issues.

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/trendline_estimator.h (right):

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/trendline_estimator.h:52: unsigned int
num_of_deltas_;
On 2016/11/14 13:21:39, terelius wrote:
> On 2016/11/13 13:09:53, stefan-webrtc (holmer) wrote:
> > Can we use an int instead?
> 
> Yes except that the Kalman filter uses an unsigned int, and I want to have the
> same types in both estimators. Should I change both to int?

Ah, feel free to keep for now.

[terelius, 2016-11-15 17:34:37]

The CQ bit was checked by terelius@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 17:34:40]

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

[terelius, 2016-11-15 17:35:46]

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/delay_based_bwe.cc (right):

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.cc:70: if (2 <=
*window_points && 0 <= *smoothing_coef && *smoothing_coef <= 1 &&
On 2016/11/14 13:30:58, stefan-webrtc (holmer) wrote:
> On 2016/11/14 13:21:39, terelius wrote:
> > On 2016/11/13 13:09:53, stefan-webrtc (holmer) wrote:
> > > I personally prefer having variables first and constants second, e.g.,
> > > if (*window_points > 2 ...
> > 
> > In general I agree, but for the smoothing_coef I wanted to highlight that it
> is
> > in the interval [0,1], so I wanted to write something similar to the
> > mathematical relation 0 <= *smoothing_coef <= 1.
> 
> Sounds good for smoothing_coef, but the other two doesn't really follow the
same
> logic?

The if statement is redundant if we CHECK anyway. Removed.

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.cc:74: LOG(LS_ERROR) <<
"Needs at least 2 points to fit a trend line.";
On 2016/11/14 13:30:58, stefan-webrtc (holmer) wrote:
> On 2016/11/14 13:21:39, terelius wrote:
> > On 2016/11/13 13:09:53, stefan-webrtc (holmer) wrote:
> > > Should we perhaps DCHECK on all broken configurations instead?
> > 
> > It would not give us any alert if we misconfigure the parameters in release
> > builds. Would CHECK be appropriate?
> 
> Yes, then I think we should CHECK as it would be bad if we didn't catch those
> issues.

Done.

[commit-bot: I haz the power, 2016-11-15 17:37:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 17:37:52]

Dry run: Try jobs failed on following builders:
  linux_tsan2 on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/16998)

[terelius, 2016-11-15 17:45:58]

Please take another look, Stefan. Noteworthy changes are as follows:

Add unit and integration tests.

Recompute the trend line from scratch for every update instead of caching
partial sums. This is of course less efficient, but it avoids potential pitfalls
where errors accumulate over the course of many updates. We'll reconsider
caching the results and dealing with the numerical errors once we have tested
the approach.

Changed the default parameters to:
window size 15
smoothing coefficient 0.9
threshold gain 4

[stefan-webrtc, 2016-11-16 15:11:20]

lgtm

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/delay_based_bwe.cc (right):

https://codereview.webrtc.org/2489323002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.cc:70: if (2 <=
*window_points && 0 <= *smoothing_coef && *smoothing_coef <= 1 &&
On 2016/11/15 17:35:46, terelius wrote:
> On 2016/11/14 13:30:58, stefan-webrtc (holmer) wrote:
> > On 2016/11/14 13:21:39, terelius wrote:
> > > On 2016/11/13 13:09:53, stefan-webrtc (holmer) wrote:
> > > > I personally prefer having variables first and constants second, e.g.,
> > > > if (*window_points > 2 ...
> > > 
> > > In general I agree, but for the smoothing_coef I wanted to highlight that
it
> > is
> > > in the interval [0,1], so I wanted to write something similar to the
> > > mathematical relation 0 <= *smoothing_coef <= 1.
> > 
> > Sounds good for smoothing_coef, but the other two doesn't really follow the
> same
> > logic?
> 
> The if statement is redundant if we CHECK anyway. Removed.

Acknowledged.

https://codereview.webrtc.org/2489323002/diff/80001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/trendline_estimator.cc (right):

https://codereview.webrtc.org/2489323002/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/trendline_estimator.cc:22:
RTC_DCHECK(points.size() >= 2);
Perhaps store points.size() in a local variable instead of calling multiple
times. Probably a no-op, but I think it's nice when using lists. :)

[terelius, 2016-11-16 15:39:12]

https://codereview.webrtc.org/2489323002/diff/80001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/trendline_estimator.cc (right):

https://codereview.webrtc.org/2489323002/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/trendline_estimator.cc:22:
RTC_DCHECK(points.size() >= 2);
On 2016/11/16 15:11:20, stefan-webrtc (holmer) wrote:
> Perhaps store points.size() in a local variable instead of calling multiple
> times. Probably a no-op, but I think it's nice when using lists. :)

Since C++11, size() is required to be constant time.

[terelius, 2016-11-16 15:39:35]

The CQ bit was checked by terelius@webrtc.org

[commit-bot: I haz the power, 2016-11-16 15:39:43]

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, 2016-11-16 15:44:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-16 15:44:33]

Try jobs failed on following builders:
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/16281)

[terelius, 2016-11-16 16:19:00]

The CQ bit was checked by terelius@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 16:19:10]

Dry run: 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, 2016-11-16 16:23:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-16 16:23:18]

Dry run: Try jobs failed on following builders:
  win_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/16173)

[terelius, 2016-11-17 10:05:12]

The CQ bit was checked by terelius@webrtc.org

[terelius, 2016-11-17 10:05:12]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2489323002/#ps100001 (title:
"Include common_types.h")

[commit-bot: I haz the power, 2016-11-17 10:05:20]

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, 2016-11-17 10:06:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 10:06:50]

Try jobs failed on following builders:
  linux_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/16328)
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10346)

[terelius, 2016-11-17 10:51:28]

The CQ bit was checked by terelius@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 10:51:37]

Dry run: 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, 2016-11-17 11:07:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 11:07:38]

Dry run: Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/18697)

[terelius, 2016-11-17 11:42:35]

The CQ bit was checked by terelius@webrtc.org

[terelius, 2016-11-17 11:42:36]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2489323002/#ps120001 (title:
"Rebase")

[commit-bot: I haz the power, 2016-11-17 11:42:43]

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, 2016-11-17 11:48:21]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-17 11:48:28]

Description was changed from

==========
Add a new overuse estimator for the delay based BWE behind experiment.
Parse the estimation parameters from the field trial string.


BUG=webrtc:6690
==========

to

==========
Add a new overuse estimator for the delay based BWE behind experiment.
Parse the estimation parameters from the field trial string.

BUG=webrtc:6690

Committed: https://crrev.com/afaef8bbebb8aecd32b3b7c5e47d60fd5526a126
Cr-Commit-Position: refs/heads/master@{#15126}
==========

[commit-bot: I haz the power, 2016-11-17 11:48:29]

Patchset 7 (id:??) landed as
https://crrev.com/afaef8bbebb8aecd32b3b7c5e47d60fd5526a126
Cr-Commit-Position: refs/heads/master@{#15126}