Enable trendline experiment and bayesian bitrate estimator experiment by default.

Enable trendline experiment and bayesian bitrate estimator experiment by default.

BUG=webrtc:6566, webrtc:7415
Review-Url: https://codereview.webrtc.org/2777333003
Cr-Commit-Position: refs/heads/master@{#17491}
Committed: https://chromium.googlesource.com/external/webrtc/+/27925de9512c67c9934ba535dc83ffe04ed999fe

[stefan-webrtc, 2017-03-28 09:36:08]

Description was changed from

==========
Enable trendline experiment by default.

BUG=
==========

to

==========
Enable trendline experiment by default.

BUG=webrtc:7415
==========

[stefan-webrtc, 2017-03-28 09:36:18]

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

[stefan-webrtc, 2017-03-28 09:36:52]

Description was changed from

==========
Enable trendline experiment by default.

BUG=webrtc:7415
==========

to

==========
Enable trendline experiment by default.

BUG=webrtc:6566, webrtc:7415
==========

[terelius, 2017-03-28 12:18:21]

looks good

https://codereview.webrtc.org/2777333003/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/delay_based_bwe.h (left):

https://codereview.webrtc.org/2777333003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.h:93: const bool
in_median_slope_experiment_;
I think we can remove the median slope experiment too. Do you want to do it in
the same CL or should I make a follow up CL?

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

https://codereview.webrtc.org/2777333003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:144:
CapacityDropTestHelper(1, false, 767, 30000);
Are you happy with the new values?

https://codereview.webrtc.org/2777333003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:173: class
DelayBasedBweExperimentTest : public DelayBasedBweTest {
Shouldn't this be removed and ImprovedBitrateEstimate be enabled by default?

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

https://codereview.webrtc.org/2777333003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:414:
EXPECT_NEAR(kInitialCapacityBps, bitrate_bps, 200000u);
Do we know why this changes?

[stefan-webrtc, 2017-03-30 07:40:49]

Rebase

[stefan-webrtc, 2017-03-30 08:04:17]

Comments addressed.

[stefan-webrtc, 2017-03-30 08:27:38]

.

[stefan-webrtc, 2017-03-30 08:30:47]

https://codereview.webrtc.org/2777333003/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/delay_based_bwe.h (left):

https://codereview.webrtc.org/2777333003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.h:93: const bool
in_median_slope_experiment_;
On 2017/03/28 12:18:21, terelius wrote:
> I think we can remove the median slope experiment too. Do you want to do it in
> the same CL or should I make a follow up CL? 

Done.

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

https://codereview.webrtc.org/2777333003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:144:
CapacityDropTestHelper(1, false, 767, 30000);
On 2017/03/28 12:18:21, terelius wrote:
> Are you happy with the new values?

I think they are fine. They will change again once both this and the bayesian
estimator lands.

https://codereview.webrtc.org/2777333003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:173: class
DelayBasedBweExperimentTest : public DelayBasedBweTest {
On 2017/03/28 12:18:21, terelius wrote:
> Shouldn't this be removed and ImprovedBitrateEstimate be enabled by default?

That ended up being a separate CL, but yes, both will be default once this is
done.

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

https://codereview.webrtc.org/2777333003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:414:
EXPECT_NEAR(kInitialCapacityBps, bitrate_bps, 200000u);
On 2017/03/28 12:18:21, terelius wrote:
> Do we know why this changes?

It's needed when combining both trendline and the bayesian estimator. I guess
the bayesian estimator doesn't average out small changes as much as the old
estimator did. Since the uncertainty is based on the difference between the
measurement and the current BWE, it will be fairly certain about smaller
variations. Maybe we should introduce some minimum uncertainty?

[stefan-webrtc, 2017-03-31 10:44:13]

Description was changed from

==========
Enable trendline experiment by default.

BUG=webrtc:6566, webrtc:7415
==========

to

==========
Enable trendline experiment and bayesian bitrate estimator experiment by
default.

BUG=webrtc:6566, webrtc:7415
==========

[stefan-webrtc, 2017-03-31 10:44:51]

Merged the CL to enable the bayesian bitrate estimator with this one.

[stefan-webrtc, 2017-03-31 10:44:57]

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

[commit-bot: I haz the power, 2017-03-31 10:45:07]

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

[terelius, 2017-03-31 11:07:21]

https://codereview.webrtc.org/2777333003/diff/80001/webrtc/call/rampup_tests.cc
File webrtc/call/rampup_tests.cc (right):

https://codereview.webrtc.org/2777333003/diff/80001/webrtc/call/rampup_tests....
webrtc/call/rampup_tests.cc:422: kLowBandwidthLimitBps / 1000,
kUnlimitedCapacityBps / 1000,
Do we really want to use kUnlimitedCapacityBps in the second rampup stage?

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

https://codereview.webrtc.org/2777333003/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:136:
RateIncreaseRtpTimestampsTestHelper(1288);
I think the numbers look a lot better now compared to the first version which
did not use the new bitrate calculation. Yay!

https://codereview.webrtc.org/2777333003/diff/80001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCFieldTrials.mm (left):

https://codereview.webrtc.org/2777333003/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCFieldTrials.mm:24: NSString * const
kRTCFieldTrialMedianSlopeFilterKey = @"WebRTC-BweMedianSlopeFilter";
Can we remove this?

https://codereview.webrtc.org/2777333003/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCFieldTrials.mm:25: NSString * const
kRTCFieldTrialTrendlineFilterKey = @"WebRTC-BweTrendlineFilter";
Can we remove this?

https://codereview.webrtc.org/2777333003/diff/80001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCFieldTrials.h (left):

https://codereview.webrtc.org/2777333003/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCFieldTrials.h:26: /** Use a string
returned by RTCFieldTrialMedianSlopeFilterValue as the value. */
Can we remove this?

https://codereview.webrtc.org/2777333003/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCFieldTrials.h:31: /** Use a string
returned by RTCFieldTrialTrendlineFilterValue as the value. */
Can we remove this?

[commit-bot: I haz the power, 2017-03-31 11:20:18]

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

[commit-bot: I haz the power, 2017-03-31 11:20:20]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2017-03-31 13:28:44]

https://codereview.webrtc.org/2777333003/diff/80001/webrtc/call/rampup_tests.cc
File webrtc/call/rampup_tests.cc (right):

https://codereview.webrtc.org/2777333003/diff/80001/webrtc/call/rampup_tests....
webrtc/call/rampup_tests.cc:422: kLowBandwidthLimitBps / 1000,
kUnlimitedCapacityBps / 1000,
On 2017/03/31 11:07:21, terelius wrote:
> Do we really want to use kUnlimitedCapacityBps in the second rampup stage?

Done.

https://codereview.webrtc.org/2777333003/diff/80001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCFieldTrials.mm (left):

https://codereview.webrtc.org/2777333003/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCFieldTrials.mm:24: NSString * const
kRTCFieldTrialMedianSlopeFilterKey = @"WebRTC-BweMedianSlopeFilter";
On 2017/03/31 11:07:21, terelius wrote:
> Can we remove this?

Done.

https://codereview.webrtc.org/2777333003/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCFieldTrials.mm:25: NSString * const
kRTCFieldTrialTrendlineFilterKey = @"WebRTC-BweTrendlineFilter";
On 2017/03/31 11:07:21, terelius wrote:
> Can we remove this?

Done.

https://codereview.webrtc.org/2777333003/diff/80001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCFieldTrials.h (left):

https://codereview.webrtc.org/2777333003/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCFieldTrials.h:26: /** Use a string
returned by RTCFieldTrialMedianSlopeFilterValue as the value. */
On 2017/03/31 11:07:21, terelius wrote:
> Can we remove this?

Done.

https://codereview.webrtc.org/2777333003/diff/80001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCFieldTrials.h:31: /** Use a string
returned by RTCFieldTrialTrendlineFilterValue as the value. */
On 2017/03/31 11:07:21, terelius wrote:
> Can we remove this?

Done.

[terelius, 2017-03-31 13:55:07]

lgtm

[stefan-webrtc, 2017-03-31 13:58:48]

The CQ bit was checked by stefan@webrtc.org

[commit-bot: I haz the power, 2017-03-31 13:58:53]

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-03-31 14:38:07]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490968728660570,
"parent_rev": "9c79ed9224b37a09b9c941147f1ab10b40c39d58", "commit_rev":
"27925de9512c67c9934ba535dc83ffe04ed999fe"}

[commit-bot: I haz the power, 2017-03-31 14:38:09]

Description was changed from

==========
Enable trendline experiment and bayesian bitrate estimator experiment by
default.

BUG=webrtc:6566, webrtc:7415
==========

to

==========
Enable trendline experiment and bayesian bitrate estimator experiment by
default.

BUG=webrtc:6566, webrtc:7415

Review-Url: https://codereview.webrtc.org/2777333003
Cr-Commit-Position: refs/heads/master@{#17491}
Committed:
https://chromium.googlesource.com/external/webrtc/+/27925de9512c67c9934ba535d...
==========

[commit-bot: I haz the power, 2017-03-31 14:38:10]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/27925de9512c67c9934ba535d...

[lliuu, 2017-03-31 21:17:34]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.webrtc.org/2791743002/ by lliuu@webrtc.org.

The reason for reverting is: API changes in
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCFieldTrials.h broke internal
project.