Make the BWE threshold adaptive.

Make the BWE threshold adaptive.

This improves self-fairness and competing for resources with TCP flows.

BUG=4711
Committed: https://crrev.com/c62642c7a662a2a88293b82192e2240049f0cbb9
Cr-Commit-Position: refs/heads/master@{#9545}

[stefan-webrtc, 2015-06-08 11:19:21]

stefan@webrtc.org changed reviewers:
+ gaetano.carlucci@gmail.com, ldecicco@gmail.com, mflodman@webrtc.org

[stefan-webrtc, 2015-06-08 11:19:22]

Hi guys,

This patch contains the final changes. Please take a look and review the code.

Thanks!

[stefan-webrtc, 2015-06-09 09:36:26]

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

[stefan-webrtc, 2015-06-16 21:28:38]

Ping, it would be great if you guys could take a first look at this.

[mflodman, 2015-06-22 04:55:23]

Review started, but I don't think I'm the best person to verify the new
algorithm.

[Gaetano Carlucci, 2015-06-25 11:58:36]

The main issue we see is that the receiver needs to know the Maximum Bitrate
that a sender can produce. This is problematic since it would require some
signalling from sender to receiver at connection establishment.

In order to address this issue we suggest to remove the dependency on the
maximum bitrate R_max. We have run some experiments and simulations and checked
that this change does not affect system performance in terms of capacity, delay,
and fairness.

Gaetano and Luca

https://codereview.webrtc.org/1151603008/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/overuse_detector.cc (right):

https://codereview.webrtc.org/1151603008/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:47:
k_down_(0.000175) {
We should Update the values of K_up_ and K_down_ according to the IETF draft

https://codereview.webrtc.org/1151603008/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:66: int
incoming_bitrate) {
remove incoming_bitrate

https://codereview.webrtc.org/1151603008/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:111: int
incoming_bitrate) {
remove incoming_bitrate

https://codereview.webrtc.org/1151603008/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:119: 
remove  kMaxBitrateBps and kMinBitrateBps

https://codereview.webrtc.org/1151603008/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:135:
static_cast<double>(kMaxBitrateBps);
remove the fine tuning of k_down_

https://codereview.webrtc.org/1151603008/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:139:
static_cast<double>(kMaxBitrateBps);
remove the fine tuning of k_up_

https://codereview.webrtc.org/1151603008/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/overuse_detector.h (right):

https://codereview.webrtc.org/1151603008/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/overuse_detector.h:49: int
incoming_bitrate);
we can remove the input incoming_bitrate

[stefan-webrtc, 2015-06-25 12:37:13]

Comments addressed.

https://codereview.webrtc.org/1151603008/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/overuse_detector.cc (right):

https://codereview.webrtc.org/1151603008/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:47:
k_down_(0.000175) {
On 2015/06/25 11:58:36, gaetano.carlucci wrote:
> We should Update the values of K_up_ and K_down_ according to the IETF draft

Done.

https://codereview.webrtc.org/1151603008/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:66: int
incoming_bitrate) {
On 2015/06/25 11:58:36, gaetano.carlucci wrote:
> remove incoming_bitrate

Done.

https://codereview.webrtc.org/1151603008/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:111: int
incoming_bitrate) {
On 2015/06/25 11:58:36, gaetano.carlucci wrote:
> remove incoming_bitrate

Done.

https://codereview.webrtc.org/1151603008/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:119: 
On 2015/06/25 11:58:36, gaetano.carlucci wrote:
> remove  kMaxBitrateBps and kMinBitrateBps

Done.

https://codereview.webrtc.org/1151603008/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:135:
static_cast<double>(kMaxBitrateBps);
On 2015/06/25 11:58:36, gaetano.carlucci wrote:
> remove the fine tuning of k_down_

Done.

https://codereview.webrtc.org/1151603008/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:139:
static_cast<double>(kMaxBitrateBps);
On 2015/06/25 11:58:36, gaetano.carlucci wrote:
> remove the fine tuning of k_up_

Done.

https://codereview.webrtc.org/1151603008/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/overuse_detector.h (right):

https://codereview.webrtc.org/1151603008/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/overuse_detector.h:49: int
incoming_bitrate);
On 2015/06/25 11:58:36, gaetano.carlucci wrote:
> we can remove the input incoming_bitrate

Done.

[stefan-webrtc, 2015-06-29 15:19:16]

I think this is now ready for a thorough review which looks at code style and
implementation details so that we can land it this week.

[mflodman, 2015-07-03 08:29:56]

A few general questions.

https://codereview.webrtc.org/1151603008/diff/220001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right):

https://codereview.webrtc.org/1151603008/diff/220001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:158: }
Why was the kRcAboveMax removed?

https://codereview.webrtc.org/1151603008/diff/220001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/overuse_detector.cc (right):

https://codereview.webrtc.org/1151603008/diff/220001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:31: const size_t
kMinExperimentLength =
Can you comment this.

https://codereview.webrtc.org/1151603008/diff/220001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:38:
threshold_(12.5),
Why 12.5 instead of 25?

https://codereview.webrtc.org/1151603008/diff/220001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:106: void
OveruseDetector::UpdateThreshold(double modified_offset, int64_t now_ms) {
LG for the parts I know about this algorithm, but I don't have the full
knowledge to be 100% sure.

[Gaetano Carlucci, 2015-07-03 10:54:29]

Some answers to the comments.

Gaetano

https://codereview.webrtc.org/1151603008/diff/220001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right):

https://codereview.webrtc.org/1151603008/diff/220001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:158: }
On 2015/07/03 08:29:55, mflodman wrote:
> Why was the kRcAboveMax removed?

kRcNearMax, kRcAboveMax and kRcMaxUnknown were introduced as a simple strategy
to adapt the threshold based on a debatable statistic approach.
The plan is to remove all of them (kRcNearMax, kRcAboveMax and kRcMaxUnknown)
and improve the increase phase of the algorithm (possibly when we move all the
algorithm logic at the sender).
We have left for now only kRcNearMax and kRcMaxUnknown just to switch between
multiplicative/additive increase in order to tune the algorithm speed in
increase phase.

https://codereview.webrtc.org/1151603008/diff/220001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/overuse_detector.cc (right):

https://codereview.webrtc.org/1151603008/diff/220001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:38:
threshold_(12.5),
On 2015/07/03 08:29:56, mflodman wrote:
> Why 12.5 instead of 25?

This aims to speed up the convergence of the adaptive threshold. If you look the
dynamics of the adaptive threshold here
http://c3lab.poliba.it/images/4/43/Ietf89-gcc.pdf (slide 4&5) it takes some
seconds to converge at the beginning of the video call. A smaller value reduces
this time.

[stefan-webrtc, 2015-07-03 13:45:11]

https://codereview.webrtc.org/1151603008/diff/220001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right):

https://codereview.webrtc.org/1151603008/diff/220001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:158: }
On 2015/07/03 08:29:55, mflodman wrote:
> Why was the kRcAboveMax removed?

We decided to simplify back-off and have the back-off factor be constant (0.85)
to improve fairness. We may want to try to improve it later in a more controlled
way, to ensure we still have good fairness and low delay.

https://codereview.webrtc.org/1151603008/diff/220001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/overuse_detector.cc (right):

https://codereview.webrtc.org/1151603008/diff/220001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:31: const size_t
kMinExperimentLength =
On 2015/07/03 08:29:55, mflodman wrote:
> Can you comment this.

I changed it to be sizeof(kEnabledPrefix) - 1 instead.

https://codereview.webrtc.org/1151603008/diff/220001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:38:
threshold_(12.5),
On 2015/07/03 10:54:28, gaetano.carlucci wrote:
> On 2015/07/03 08:29:56, mflodman wrote:
> > Why 12.5 instead of 25?
> 
> This aims to speed up the convergence of the adaptive threshold. If you look
the
> dynamics of the adaptive threshold here
> http://c3lab.poliba.it/images/4/43/Ietf89-gcc.pdf (slide 4&5) it takes some
> seconds to converge at the beginning of the video call. A smaller value
reduces
> this time.

I think this makes a very little difference in practice, and most of the time
when we were under congestion the threshold was 12.5 before. Therefore I think
it's safe to set it to 12.5 as default. The option would be to set it to 25, but
then we still don't have the old behavior, and I would really like to avoid
keeping the logic to switch between 12.5 and 25...

[stefan-webrtc, 2015-07-03 13:45:32]

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

[commit-bot: I haz the power, 2015-07-03 13:45:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1151603008/280001

[mflodman, 2015-07-03 14:30:16]

Very quick review and LGTM since I'll go on vacation, but might be good with
another pair of eyes on the last upload.

[commit-bot: I haz the power, 2015-07-03 16:00:09]

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

[commit-bot: I haz the power, 2015-07-03 16:00:10]

Dry run: This issue passed the CQ dry run.

[pbos-webrtc, 2015-07-06 10:01:39]

pbos@webrtc.org changed reviewers:
+ pbos@webrtc.org

[pbos-webrtc, 2015-07-06 10:01:41]

Some initial thoughts on the experiment wireup. I might've missed some parts, so
let me know if-so. :)

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/common_types.h#ne...
webrtc/common_types.h:754: struct OverUseDetectorOptions {
(Not related to review: This looks like a very specific type, why not put it in
remote_bitrate_estimator/something.h?)

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/paced_sender.cc:178: bytes_remaining_ = std::max(-500 *
target_rate_kbps_ / 8, bytes_remaining_);
Named constants please.

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/overuse_detector.cc (right):

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:48:
in_experiment_(false),
Can you make a struct of experiment settings (if it's not just in_experiment_)?
Also, is the base class what needs to know about the experiment? (Does it apply
to all subclasses?)

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:110: if
(!AdaptiveThresholdExperimentIsEnabled())
Call in constructor instead, make in_experiment_ const.

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:142: if
(!experiment_set_) {
remove experiment_set_, this should not be lazy.

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/overuse_detector.h (right):

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.h:44: virtual void
InitializeExperiment();
Don't like this being virtual, pref. checking in_experiment_ in subclasses and
applying experiment magic, then everything should be ready-to-rock after
construction.

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/random.cc (right):

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/random.cc:21: const float kScale =
1.0f / 0xffffffff;
Is this a remotely good rand() function? If you know where this came from, can
you document where the implementation came from? Googling 0x531FDB97 I only get
hits in webrtc/.

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/random.h (right):

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/random.h:23: // Return pseudo
random number in the interval [0.0, 1.0].
pseudo-random number

[stefan-webrtc, 2015-07-06 10:54:26]

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/common_types.h#ne...
webrtc/common_types.h:754: struct OverUseDetectorOptions {
On 2015/07/06 10:01:40, pbos-webrtc wrote:
> (Not related to review: This looks like a very specific type, why not put it
in
> remote_bitrate_estimator/something.h?)

It will be deleted later at some point. It is old and was part of the public ViE
interface, couldn't be in modules/.

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/paced_sender.cc:178: bytes_remaining_ = std::max(-500 *
target_rate_kbps_ / 8, bytes_remaining_);
On 2015/07/06 10:01:40, pbos-webrtc wrote:
> Named constants please.

Done.

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/overuse_detector.cc (right):

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:48:
in_experiment_(false),
On 2015/07/06 10:01:40, pbos-webrtc wrote:
> Can you make a struct of experiment settings (if it's not just
in_experiment_)?
> Also, is the base class what needs to know about the experiment? (Does it
apply
> to all subclasses?)

There are only sublcasses in tests. Don't think it's worth making a struct.

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:110: if
(!AdaptiveThresholdExperimentIsEnabled())
On 2015/07/06 10:01:40, pbos-webrtc wrote:
> Call in constructor instead, make in_experiment_ const.

Fine with me. The reason for doing it this way was to make sure that the logic
with calling the AdaptiveThresholdExperimentIsEnabled() actually worked, but now
that most of it is overridden anyway it doesn't add much value.

in_experiment_ can't easily be made const for testing purposes though. The
subclass has to be able to change it.

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:142: if
(!experiment_set_) {
On 2015/07/06 10:01:40, pbos-webrtc wrote:
> remove experiment_set_, this should not be lazy.

Done.

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/overuse_detector.h (right):

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.h:44: virtual void
InitializeExperiment();
On 2015/07/06 10:01:40, pbos-webrtc wrote:
> Don't like this being virtual, pref. checking in_experiment_ in subclasses and
> applying experiment magic, then everything should be ready-to-rock after
> construction.

Done.

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/random.cc (right):

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/random.cc:21: const float kScale =
1.0f / 0xffffffff;
On 2015/07/06 10:01:40, pbos-webrtc wrote:
> Is this a remotely good rand() function? If you know where this came from, can
> you document where the implementation came from? Googling 0x531FDB97 I only
get
> hits in webrtc/.

I don't know where it came from, sorry.

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/test/random.h (right):

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/test/random.h:23: // Return pseudo
random number in the interval [0.0, 1.0].
On 2015/07/06 10:01:41, pbos-webrtc wrote:
> pseudo-random number

Done.

[pbos-webrtc, 2015-07-06 11:13:26]

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/overuse_detector.h (right):

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.h:44: virtual void
InitializeExperiment();
On 2015/07/06 10:54:26, stefan-webrtc (holmer) wrote:
> On 2015/07/06 10:01:40, pbos-webrtc wrote:
> > Don't like this being virtual, pref. checking in_experiment_ in subclasses
and
> > applying experiment magic, then everything should be ready-to-rock after
> > construction.
> 
> Done.

This is still virtual, does it even have to be externally visible or can it go
under private:? or protected:?

[pbos-webrtc, 2015-07-06 11:14:24]

https://codereview.webrtc.org/1151603008/diff/320001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/overuse_detector.cc (right):

https://codereview.webrtc.org/1151603008/diff/320001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:55:
InitializeExperiment();
Only call under in_experiment_?

https://codereview.webrtc.org/1151603008/diff/320001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:137: void
OveruseDetector::InitializeExperiment() {
DCHECK(in_experiment_);

[stefan-webrtc, 2015-07-06 12:16:13]

PTAL at the change to field_trial.cc as well.

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/overuse_detector.h (right):

https://codereview.webrtc.org/1151603008/diff/280001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.h:44: virtual void
InitializeExperiment();
On 2015/07/06 11:13:26, pbos-webrtc wrote:
> On 2015/07/06 10:54:26, stefan-webrtc (holmer) wrote:
> > On 2015/07/06 10:01:40, pbos-webrtc wrote:
> > > Don't like this being virtual, pref. checking in_experiment_ in subclasses
> and
> > > applying experiment magic, then everything should be ready-to-rock after
> > > construction.
> > 
> > Done.
> 
> This is still virtual, does it even have to be externally visible or can it go
> under private:? or protected:?

Done.

https://codereview.webrtc.org/1151603008/diff/320001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/overuse_detector.cc (right):

https://codereview.webrtc.org/1151603008/diff/320001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:55:
InitializeExperiment();
On 2015/07/06 11:14:23, pbos-webrtc wrote:
> Only call under in_experiment_?

Done.

https://codereview.webrtc.org/1151603008/diff/320001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:137: void
OveruseDetector::InitializeExperiment() {
On 2015/07/06 11:14:23, pbos-webrtc wrote:
> DCHECK(in_experiment_);

Done.

[pbos-webrtc, 2015-07-06 13:30:17]

https://codereview.webrtc.org/1151603008/diff/380001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/overuse_detector.cc (right):

https://codereview.webrtc.org/1151603008/diff/380001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:140:
DCHECK(in_experiment_);
If this is now the only place where in_experiment_ is actually used, remove it
and do if (AdaptiveThresh...Enabled()) InitializeExperiment(); in ctor.

https://codereview.webrtc.org/1151603008/diff/380001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:154:
std::stringstream ss(experiment_string.substr(kEnabledPrefixLength + 1));
Easier to do this?
  return sscanf(experiment_string.c_str(), "%d,%d", k_up, k_down) == 2;

It won't complain on invalid strings ("Experiment-0.2,0.5-yolo$wag49"), but it
should still parse the correct string safely. I don't think we have to protect
for incorrect strings accidentally being read, we should just not crash on
anything and I think this does it. :)

(Please also test locally that it works.)

https://codereview.webrtc.org/1151603008/diff/380001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/overuse_detector_unittest.cc
(right):

https://codereview.webrtc.org/1151603008/diff/380001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector_unittest.cc:610:
overuse_detector_.reset(new OveruseDetector(options_));
Don't you want to do InitFieldTrialsFromString(""); in TearDown() ?

https://codereview.webrtc.org/1151603008/diff/380001/webrtc/test/field_trial.cc
File webrtc/test/field_trial.cc (right):

https://codereview.webrtc.org/1151603008/diff/380001/webrtc/test/field_trial....
webrtc/test/field_trial.cc:52: if (trials_string.empty()) {
trials_string == "" reads better

[stefan-webrtc, 2015-07-06 14:09:15]

Cleanup unused stringstream.

[pbos-webrtc, 2015-07-07 08:21:31]

lgtm

https://codereview.webrtc.org/1151603008/diff/500001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/overuse_detector.cc (right):

https://codereview.webrtc.org/1151603008/diff/500001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:56: if
(in_experiment_)
I meant, do you need it at all or can this be if
(AdaptiveThresholdExperimentIsEnabled()) InitializeExperiment(); ?

https://codereview.webrtc.org/1151603008/diff/500001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:139: void
OveruseDetector::InitializeExperiment() {
InitializeAdaptiveThresholdExperiment

[stefan-webrtc, 2015-07-07 08:31:33]

https://codereview.webrtc.org/1151603008/diff/380001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/overuse_detector_unittest.cc
(right):

https://codereview.webrtc.org/1151603008/diff/380001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector_unittest.cc:610:
overuse_detector_.reset(new OveruseDetector(options_));
On 2015/07/06 13:30:17, pbos-webrtc wrote:
> Don't you want to do InitFieldTrialsFromString(""); in TearDown() ?

Not necessary since all unittests call InitFieldTrialsFromString("") by default,
see webrtc/test/test_suite.cc.

https://codereview.webrtc.org/1151603008/diff/420001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/overuse_detector.cc (right):

https://codereview.webrtc.org/1151603008/diff/420001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:113: if
(!in_experiment_)
I don't want to remove the in_experiment_ flag because that would require me to
parse the string every time we update the threshold.

https://codereview.webrtc.org/1151603008/diff/500001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/overuse_detector.cc (right):

https://codereview.webrtc.org/1151603008/diff/500001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:56: if
(in_experiment_)
On 2015/07/07 08:21:30, pbos-webrtc wrote:
> I meant, do you need it at all or can this be if
> (AdaptiveThresholdExperimentIsEnabled()) InitializeExperiment(); ?

See my comment that I forgot to publish...

I don't want to parse the experiment string every time thresholds are updated.

https://codereview.webrtc.org/1151603008/diff/500001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/overuse_detector.cc:139: void
OveruseDetector::InitializeExperiment() {
On 2015/07/07 08:21:30, pbos-webrtc wrote:
> InitializeAdaptiveThresholdExperiment

Prefer not keeping the name short here since I only have a single experiment in
this file.

[stefan-webrtc, 2015-07-07 08:32:27]

Last comments addressed.

[stefan-webrtc, 2015-07-07 08:32:55]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2015-07-07 08:32:56]

The patchset sent to the CQ was uploaded after l-g-t-m from mflodman@webrtc.org,
pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1151603008/#ps520001 (title:
"Last comments addressed.")

[commit-bot: I haz the power, 2015-07-07 08:33:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1151603008/520001

[commit-bot: I haz the power, 2015-07-07 08:51:42]

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

[commit-bot: I haz the power, 2015-07-07 08:51:43]

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

(exceeded global retry quota)

[stefan-webrtc, 2015-07-07 09:01:24]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2015-07-07 09:01:25]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1151603008/#ps540001 (title:
"Fix finch test issue.")

[commit-bot: I haz the power, 2015-07-07 09:01:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1151603008/540001

[commit-bot: I haz the power, 2015-07-07 09:21:01]

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

[commit-bot: I haz the power, 2015-07-07 09:21:03]

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

(exceeded global retry quota)

[stefan-webrtc, 2015-07-07 10:38:02]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2015-07-07 10:38:02]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1151603008/#ps560001 (title:
"Fix string length issue.")

[commit-bot: I haz the power, 2015-07-07 10:38:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1151603008/560001

[commit-bot: I haz the power, 2015-07-07 11:20:40]

Committed patchset #30 (id:560001)

[commit-bot: I haz the power, 2015-07-07 11:20:47]

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