Set the start bitrate to the delay-based BWE.

Set the start bitrate to the delay-based BWE.

This avoids issues where the bitrate produced by the codec is far lower than the target bitrate in the beginning, which causes the delay-based BWE to be initialized accordingly.

BUG=webrtc:5079
Review-Url: https://codereview.webrtc.org/2653883002
Cr-Commit-Position: refs/heads/master@{#16327}
Committed: https://chromium.googlesource.com/external/webrtc/+/5a2c506e8e8fb4941ab428e355bf8f68954af348

[stefan-webrtc, 2017-01-24 11:50:46]

Add test.

[stefan-webrtc, 2017-01-24 11:51:01]

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

[terelius, 2017-01-24 16:05:52]

https://codereview.webrtc.org/2653883002/diff/20001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2653883002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:310: RTC_DCHECK_GT(config.bitrate_config.start_bitrate_bps,
Why not DCHECK_GE? That seems more natural to me.

https://codereview.webrtc.org/2653883002/diff/20001/webrtc/call/rampup_tests.cc
File webrtc/call/rampup_tests.cc (right):

https://codereview.webrtc.org/2653883002/diff/20001/webrtc/call/rampup_tests....
webrtc/call/rampup_tests.cc:262: (min_run_time_ms_ == -1 ||
Is min_run_time__= -1 used to signal that there is no minimum run time, and the
test should report the time as soon as it reaches the bitrate? If so, what's the
difference compared to using min_run_time_ = 0 for this.

[stefan-webrtc, 2017-01-24 16:29:43]

https://codereview.webrtc.org/2653883002/diff/20001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2653883002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:310: RTC_DCHECK_GT(config.bitrate_config.start_bitrate_bps,
On 2017/01/24 16:05:52, terelius wrote:
> Why not DCHECK_GE? That seems more natural to me.

It's just a bit unclear to me why you'd want to start with a bitrate of zero. I
could allow it, but seems like a risk for bugs where we'd potentially get stuck
if some encoder can't just produces nothing.

WDYT, should I change?

https://codereview.webrtc.org/2653883002/diff/20001/webrtc/call/rampup_tests.cc
File webrtc/call/rampup_tests.cc (right):

https://codereview.webrtc.org/2653883002/diff/20001/webrtc/call/rampup_tests....
webrtc/call/rampup_tests.cc:262: (min_run_time_ms_ == -1 ||
On 2017/01/24 16:05:52, terelius wrote:
> Is min_run_time__= -1 used to signal that there is no minimum run time, and
the
> test should report the time as soon as it reaches the bitrate? If so, what's
the
> difference compared to using min_run_time_ = 0 for this.

Right, I can change it to zero.

[stefan-webrtc, 2017-01-24 16:30:59]

Change no min to zero.

[terelius, 2017-01-25 16:41:27]

lgtm with one follow-up question:

https://codereview.webrtc.org/2653883002/diff/20001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2653883002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:310: RTC_DCHECK_GT(config.bitrate_config.start_bitrate_bps,
On 2017/01/24 16:29:43, stefan-webrtc wrote:
> On 2017/01/24 16:05:52, terelius wrote:
> > Why not DCHECK_GE? That seems more natural to me.
> 
> It's just a bit unclear to me why you'd want to start with a bitrate of zero.
I
> could allow it, but seems like a risk for bugs where we'd potentially get
stuck
> if some encoder can't just produces nothing.
> 
> WDYT, should I change?

Ok, I see. Is there a reason for allowing min_bitrate_bps_ == 0 in that case?

[stefan-webrtc, 2017-01-27 09:27:05]

https://codereview.webrtc.org/2653883002/diff/20001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2653883002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:310: RTC_DCHECK_GT(config.bitrate_config.start_bitrate_bps,
On 2017/01/25 16:41:27, terelius wrote:
> On 2017/01/24 16:29:43, stefan-webrtc wrote:
> > On 2017/01/24 16:05:52, terelius wrote:
> > > Why not DCHECK_GE? That seems more natural to me.
> > 
> > It's just a bit unclear to me why you'd want to start with a bitrate of
zero.
> I
> > could allow it, but seems like a risk for bugs where we'd potentially get
> stuck
> > if some encoder can't just produces nothing.
> > 
> > WDYT, should I change?
> 
> Ok, I see. Is there a reason for allowing min_bitrate_bps_ == 0 in that case?

Good question. It will immediately get clamped here:
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/congestion_co...

I don't want to change this rule in this CL as 0 is default in the call config.
Let's pick it up separately if we feel it's worth changing.

[stefan-webrtc, 2017-01-27 09:27:15]

The CQ bit was checked by stefan@webrtc.org

[commit-bot: I haz the power, 2017-01-27 09:27:26]

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-01-27 10:13:33]

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

[commit-bot: I haz the power, 2017-01-27 10:13:33]

Failed to apply patch for
webrtc/modules/congestion_controller/congestion_controller.cc:
While running git apply --index -p1;
  error: patch failed:
webrtc/modules/congestion_controller/congestion_controller.cc:229
  error: webrtc/modules/congestion_controller/congestion_controller.cc: patch
does not apply

Patch:       webrtc/modules/congestion_controller/congestion_controller.cc
Index: webrtc/modules/congestion_controller/congestion_controller.cc
diff --git a/webrtc/modules/congestion_controller/congestion_controller.cc
b/webrtc/modules/congestion_controller/congestion_controller.cc
index
94b3d8c6da98bbaee3a070e8e9f2421044a758b9..1fd63a43b0a63d631e83af120a87bab4d6cbb6ce
100644
--- a/webrtc/modules/congestion_controller/congestion_controller.cc
+++ b/webrtc/modules/congestion_controller/congestion_controller.cc
@@ -229,6 +229,7 @@ void CongestionController::SetBweBitrates(int
min_bitrate_bps,
   if (remote_bitrate_estimator_)
     remote_bitrate_estimator_->SetMinBitrate(min_bitrate_bps);
   min_bitrate_bps_ = min_bitrate_bps;
+  transport_feedback_adapter_.SetStartBitrate(start_bitrate_bps);
   transport_feedback_adapter_.SetMinBitrate(min_bitrate_bps_);
   MaybeTriggerOnNetworkChanged();
 }
@@ -249,6 +250,7 @@ void CongestionController::ResetBweAndBitrates(int
bitrate_bps,
     remote_bitrate_estimator_->SetMinBitrate(min_bitrate_bps);
 
   transport_feedback_adapter_.InitBwe();
+  transport_feedback_adapter_.SetStartBitrate(bitrate_bps);
   transport_feedback_adapter_.SetMinBitrate(min_bitrate_bps);
   // TODO(holmer): Trigger a new probe once mid-call probing is implemented.
   MaybeTriggerOnNetworkChanged();

[stefan-webrtc, 2017-01-27 13:08:02]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2017-01-27 13:08:02]

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/2653883002/#ps60001 (title:
".")

[commit-bot: I haz the power, 2017-01-27 13:08:05]

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-01-27 14:43:19]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485522482008680,
"parent_rev": "b0ae920faddfed0613700e9b16d32a0f0e7e18e1", "commit_rev":
"5a2c506e8e8fb4941ab428e355bf8f68954af348"}

[commit-bot: I haz the power, 2017-01-27 14:43:22]

Description was changed from

==========
Set the start bitrate to the delay-based BWE.

This avoids issues where the bitrate produced by the codec is far lower than the
target bitrate in the beginning, which causes the delay-based BWE to be
initialized accordingly.

BUG=webrtc:5079
==========

to

==========
Set the start bitrate to the delay-based BWE.

This avoids issues where the bitrate produced by the codec is far lower than the
target bitrate in the beginning, which causes the delay-based BWE to be
initialized accordingly.

BUG=webrtc:5079

Review-Url: https://codereview.webrtc.org/2653883002
Cr-Commit-Position: refs/heads/master@{#16327}
Committed:
https://chromium.googlesource.com/external/webrtc/+/5a2c506e8e8fb4941ab428e35...
==========

[commit-bot: I haz the power, 2017-01-27 14:43:23]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/5a2c506e8e8fb4941ab428e35...