Enable the bayesian bitrate estimator by default.

Enable the bayesian bitrate estimator by default.

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

[stefan-webrtc, 2017-03-14 16:03:43]

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

[stefan-webrtc, 2017-03-14 16:03:49]

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

[commit-bot: I haz the power, 2017-03-14 16:03:59]

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

[stefan-webrtc, 2017-03-14 16:05:23]

https://codereview.webrtc.org/2749803002/diff/1/webrtc/call/rampup_tests.cc
File webrtc/call/rampup_tests.cc (right):

https://codereview.webrtc.org/2749803002/diff/1/webrtc/call/rampup_tests.cc#n...
webrtc/call/rampup_tests.cc:401: kLowBandwidthLimitBps / 1000,
kUnlimitedCapacityBps / 1000,
This change is to reduce flakiness in these tests. It can happen that they
toggle a few times close to the high link capacity before we get over the high
bitrate. I think the test is basically as valuable with an unlimited capacity in
the second ramp-up phase.

[terelius, 2017-03-17 13:34:08]

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

https://codereview.webrtc.org/2749803002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:122:
CapacityDropTestHelper(1, true, 333, 0);
It would be nice to know why the send bitrate drops faster with the new
estimator. (Assuming it doesn't take too long to figure out.)

https://codereview.webrtc.org/2749803002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:175: class
DelayBasedBweExperimentTest : public DelayBasedBweTest {
Could we remove this class?

There is also a reference to "WebRTC-ImprovedBitrateEstimate" in
webrtc/sdk/objc/Framework/Classes/RTCFieldTrials.mm
Clean up now or later?

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

Description was changed from

==========
Enable the bayesian bitrate estimator by default.

BUG=webrtc:6566
==========

to

==========
Enable the bayesian bitrate estimator by default.

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

[stefan-webrtc, 2017-03-29 14:50:43]

rebase

[stefan-webrtc, 2017-03-30 07:26:48]

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

https://codereview.webrtc.org/2749803002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:122:
CapacityDropTestHelper(1, true, 333, 0);
On 2017/03/17 13:34:07, terelius wrote:
> It would be nice to know why the send bitrate drops faster with the new
> estimator. (Assuming it doesn't take too long to figure out.)

I added some logging to see what happens:

109819 Sample: 920.106689 Uncertainty: 0.735696
109819 Bitrate: 914.338196, variance: 0.491732
109962 Sample: 856.960022 Uncertainty: 0.627538
109962 Bitrate: 860.799255, variance: 0.367454
110000 Dropping capacity to 500 kbps
110118 Sample: 593.706665 Uncertainty: 3.102844
110118 Bitrate: 765.194214, variance: 3.446188
110277 Sample: 529.066650 Uncertainty: 3.085851
110277 Bitrate: 654.202271, variance: 4.476050
110427 Sample: 493.600006 Uncertainty: 2.454933
110427 Bitrate: 556.034180, variance: 3.683817
110333 BWE below 500 kbps: 472629
110561 Sample: 466.399994 Uncertainty: 1.612027
110561 Bitrate: 487.044983, variance: 2.000100
110718 Sample: 525.333313 Uncertainty: 0.786135

to me this looks like what we expect. It takes us 3 updates before we fully
trust this new metric, and the reason for that is the increased uncertainty
right after the drop, but the uncertainty is reduced after a few updates. One
could argue that we should tune it to not adapt this quickly, but I think this
is the case mostly because this being just a factor of 2 decrease. Had it been a
factor of 5 or 10 it would have been a different story.

The old estimator used a fixed window of 1 second, so that didn't have any
possibility of getting below 500 kbps this quickly.

I do think there's room for some more tuning here though, and I think one
interesting factor we haven't accounted for is the difference between send
bitrate and the BWE. Our measurement could be considered more reliable if the
send bitrate is close to the BWE, while if it's a lot lower then the acked
bitrate will not correspond to the actual bandwidth of the pipe.

https://codereview.webrtc.org/2749803002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:175: class
DelayBasedBweExperimentTest : public DelayBasedBweTest {
On 2017/03/17 13:34:08, terelius wrote:
> Could we remove this class?
> 
> There is also a reference to "WebRTC-ImprovedBitrateEstimate" in
> webrtc/sdk/objc/Framework/Classes/RTCFieldTrials.mm
> Clean up now or later?

Done.

[terelius, 2017-03-30 08:52:47]

lgtm

[stefan-webrtc, 2017-03-30 09:06:24]

The CQ bit was checked by stefan@webrtc.org

[commit-bot: I haz the power, 2017-03-30 09:06:36]

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-30 09:16:05]

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

[commit-bot: I haz the power, 2017-03-30 09:16:06]

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

[stefan-webrtc, 2017-03-30 09:25:28]

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

[stefan-webrtc, 2017-03-30 09:25:28]

Magnus, can you approve the objc changes?

[magjed_webrtc, 2017-03-30 11:20:25]

webrtc/sdk/objc/ lgtm

[stefan-webrtc, 2017-03-30 15:32:24]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2017-03-30 15:32:24]

The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2749803002/#ps80001 (title:
"AppRTC fixes.")

[commit-bot: I haz the power, 2017-03-30 15:32:30]

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-30 15:56:23]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490887944429590,
"parent_rev": "3ea3c77e93121b1ab9d5e46641e6764f2cca0d51", "commit_rev":
"c53a17f28e55c99dca7a2a47f09790af466e9334"}

[commit-bot: I haz the power, 2017-03-30 15:56:25]

Description was changed from

==========
Enable the bayesian bitrate estimator by default.

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

to

==========
Enable the bayesian bitrate estimator by default.

BUG=webrtc:6566, webrtc:7415

Review-Url: https://codereview.webrtc.org/2749803002
Cr-Commit-Position: refs/heads/master@{#17475}
Committed:
https://chromium.googlesource.com/external/webrtc/+/c53a17f28e55c99dca7a2a47f...
==========

[commit-bot: I haz the power, 2017-03-30 15:56:26]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/c53a17f28e55c99dca7a2a47f...

[lliuu, 2017-03-30 17:36:39]

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

The reason for reverting is: Looks like this has caused multiple Android webrtc
perf build bot failures in RampUpTest.UpDownUpTransportSequenceNumberRtx.