Probing: Add support for exponential startup probing

Probing: Add support for exponential startup probing

Adds support for exponentially probing the bandwidth at start-up to allow
ramp-up to real capacity of the network.

BUG=webrtc:6332
R=philipel@webrtc.org, stefan@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/b2540bb99fa974bc8cb66eeff9c6b6d7fd0c36eb

[Irfan, 2016-08-12 06:24:52]

Description was changed from

==========
Probing: Add support for exponential startup probing

Adds support for exponentially probing the bandwidth at start-up to allow
ramp-up to real capacity of the network.

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

to

==========
Probing: Add support for exponential startup probing

Adds support for exponentially probing the bandwidth at start-up to allow
ramp-up to real capacity of the network.

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

[Irfan, 2016-08-12 06:24:53]

isheriff@chromium.org changed reviewers:
+ philipel@webrtc.org, stefan@webrtc.org, terelius@webrtc.org

[Irfan, 2016-08-12 06:25:58]

PTAL. I have rebased this on top of philip's CL.

The tests will need fixes, but this is ready for review as an independent
change.

Note that I have not added support for multiple packets per probe yet. Planning
to do that as a separate CL.

[Irfan, 2016-08-16 06:44:42]

PS#2 adds some updates based on changes in
https://codereview.chromium.org/2239143002/

I think I need to rebase on at least one CL that went in today. 

Looking forward to feedback before I go ahead and clean up tests and add some
tests.

[stefan-webrtc, 2016-08-16 11:27:05]

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti...
File webrtc/modules/congestion_controller/delay_based_bwe.cc (right):

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti...
webrtc/modules/congestion_controller/delay_based_bwe.cc:128: // Overuse
End the comments with "."

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti...
webrtc/modules/congestion_controller/delay_based_bwe.cc:140:
controller_->OnProbingBitrateMeasured(0);
This looks a bit hacky. It's not really that we measured a zero bitrate on a
probe. Should we have a separate method to stop probing, or a parameter to say
so explicitly?

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti...
webrtc/modules/congestion_controller/delay_based_bwe.cc:148: // Been a while
since we updated
Move this inside the else if below instead to make it clear that it doesn't
belong to this block.

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/b...
File webrtc/modules/pacing/bitrate_prober.cc (right):

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/b...
webrtc/modules/pacing/bitrate_prober.cc:148: RTC_DCHECK(probing_state_ ==
State::kActive);
Isn't it good to notify the prober about recent packets even though it's not
active? That way it will know what's been sent before and can better recommend a
packet size on line 142.

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/b...
File webrtc/modules/pacing/bitrate_prober_unittest.cc (right):

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/b...
webrtc/modules/pacing/bitrate_prober_unittest.cc:20: constexpr int
probe_multipliers[2] = {3, 6};
kProbeMultipliers

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
webrtc/modules/pacing/paced_sender.cc:340: min_bitrate_to_probe_further_ = 4 *
estimated_bitrate_bps_;
What is different between being here and in kStartupProbing? Here we have a
factor of 4 while in startup we have a factor of 1.25?

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
webrtc/modules/pacing/paced_sender.cc:369: }
Isn't this adding a lot of complexity to paced_sender.cc when it could as well
be in birtate_prober.cc?

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
webrtc/modules/pacing/paced_sender.cc:436: estimated_bitrate_bps_ = bitrate_bps;
I'm not sure this is the right way to set this. It will also be set via
PacedSender::SetEstimatedBitrate(int bitrate_bps), and that estimate will
possibly be different as it takes lost packets into account. Can we have a
single way of setting the estimated bitrate to PacedSender?

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
webrtc/modules/pacing/paced_sender.cc:438: LOG(LS_WARNING) <<
"OnProbingBitrateMeasured while not waiting on results";
Should we simply DCHECK on this?

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
webrtc/modules/pacing/paced_sender.cc:440: }
To keep the pacer simpler, I think it would be nice to move this into
bitrate_prober.cc. WDYT?

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
webrtc/modules/pacing/paced_sender.cc:528: if (probe_cluster_id !=
PacketInfo::kNotAProbe) {
Why only notify about sent probe packets?

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
File webrtc/modules/pacing/paced_sender.h (right):

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
webrtc/modules/pacing/paced_sender.h:79: static const size_t kMinProbePacketSize
= 1000;
I'm not sure about introducing this change. It will break the behavior at low
bitrates, e.g., if the bitrate is set to 300 kbps we may never have 1000 bytes
packets.

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
webrtc/modules/pacing/paced_sender.h:188: int bitrate_below_expected_count_
GUARDED_BY(critsect_) = 0;
Initialize in initializer list instead.

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/remote_b...
File webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc
(left):

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/remote_b...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc:139: void
TransportFeedbackAdapter::OnReceiveBitrateChanged(
Nice to move this out.

[philipel, 2016-08-16 14:44:43]

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

https://codereview.webrtc.org/2235373004/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe.cc:121:
probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(info, 2);
Seems like probe_bitrate_estimator.h/.cc haven't been uploaded, can you upload a
patch with them included?

[Irfan, 2016-08-16 16:04:17]

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti...
File webrtc/modules/congestion_controller/delay_based_bwe.cc (right):

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti...
webrtc/modules/congestion_controller/delay_based_bwe.cc:121:
probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(info, 2);
On 2016/08/16 14:44:43, philipel wrote:
> Seems like probe_bitrate_estimator.h/.cc haven't been uploaded, can you upload
a
> patch with them included?

https://codereview.chromium.org/2239143002/ is the CL which has changes in
probe_bitrate_estimator.

[Irfan, 2016-08-16 18:12:47]

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti...
File webrtc/modules/congestion_controller/delay_based_bwe.cc (right):

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti...
webrtc/modules/congestion_controller/delay_based_bwe.cc:128: // Overuse
On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote:
> End the comments with "."

Will make these full sentence comments and add full stop.

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti...
webrtc/modules/congestion_controller/delay_based_bwe.cc:140:
controller_->OnProbingBitrateMeasured(0);
On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote:
> This looks a bit hacky. It's not really that we measured a zero bitrate on a
> probe. Should we have a separate method to stop probing, or a parameter to say
> so explicitly?

Will make it a well-defined name instead. Works for you ?

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/congesti...
webrtc/modules/congestion_controller/delay_based_bwe.cc:148: // Been a while
since we updated
On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote:
> Move this inside the else if below instead to make it clear that it doesn't
> belong to this block.

Done.

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/b...
File webrtc/modules/pacing/bitrate_prober.cc (right):

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/b...
webrtc/modules/pacing/bitrate_prober.cc:148: RTC_DCHECK(probing_state_ ==
State::kActive);
On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote:
> Isn't it good to notify the prober about recent packets even though it's not
> active? That way it will know what's been sent before and can better recommend
a
> packet size on line 142.

We initiate probing only when there is an incoming packet > kMinProbePacketSize
and is added to queue.

The RecommendedPacketSize() is used for padding and should be triggered at the
earliest on the second probe packet (since we have atleast one in queue).

Once we initiate probing, we keep sending probes until done.

The last_packet_size_ is used to decide on delta between probes (except on first
probe when it is immediately sent). All this seems to indicate we really only
need to track last_packet_size_ on the probes themselves. 

By doing so, we will be able to have stricter constraints on the method (add
DCHECK on state we expect to be called) and calling only on probes keeps it
simpler.

Let me know if I have missed something here. I can revert this behavior if there
is a problem.

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/b...
File webrtc/modules/pacing/bitrate_prober_unittest.cc (right):

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/b...
webrtc/modules/pacing/bitrate_prober_unittest.cc:20: constexpr int
probe_multipliers[2] = {3, 6};
On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote:
> kProbeMultipliers

Done.

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
webrtc/modules/pacing/paced_sender.cc:340: min_bitrate_to_probe_further_ = 4 *
estimated_bitrate_bps_;
On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote:
> What is different between being here and in kStartupProbing? Here we have a
> factor of 4 while in startup we have a factor of 1.25?

The multiplers are 3 and 6 on the very first time (if you recall, we kept this
to avoid slowing down ramp up). so we place a requirement of 4.

Afterwards, the multiplier is 2 and we keep a requirement of 1.25.

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
webrtc/modules/pacing/paced_sender.cc:369: }
On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote:
> Isn't this adding a lot of complexity to paced_sender.cc when it could as well
> be in birtate_prober.cc?

I will evaluate a way to simplify paced_sender and see if it works withm moving
to bitrate_prober.

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
webrtc/modules/pacing/paced_sender.cc:436: estimated_bitrate_bps_ = bitrate_bps;
On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote:
> I'm not sure this is the right way to set this. It will also be set via
> PacedSender::SetEstimatedBitrate(int bitrate_bps), and that estimate will
> possibly be different as it takes lost packets into account. Can we have a
> single way of setting the estimated bitrate to PacedSender?

This is a good point. The reason its happening here now is to sync up on bitrate
the moment pacing state is updated.

I will see if we can get rid of duplicate setting of bitrate.

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
webrtc/modules/pacing/paced_sender.cc:438: LOG(LS_WARNING) <<
"OnProbingBitrateMeasured while not waiting on results";
On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote:
> Should we simply DCHECK on this?

Done.

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
webrtc/modules/pacing/paced_sender.cc:440: }
On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote:
> To keep the pacer simpler, I think it would be nice to move this into
> bitrate_prober.cc. WDYT?

Right, will evaluate simplifiying pacer

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
webrtc/modules/pacing/paced_sender.cc:528: if (probe_cluster_id !=
PacketInfo::kNotAProbe) {
On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote:
> Why only notify about sent probe packets?

I clarified this in an earlier reply. Let me know if I have missed something
here.

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
File webrtc/modules/pacing/paced_sender.h (right):

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
webrtc/modules/pacing/paced_sender.h:79: static const size_t kMinProbePacketSize
= 1000;
On 2016/08/16 11:27:04, stefan-webrtc (holmer) wrote:
> I'm not sure about introducing this change. It will break the behavior at low
> bitrates, e.g., if the bitrate is set to 300 kbps we may never have 1000 bytes
> packets.

I will go ahead and revert this. Since I plan to add support for multiple
packets per probe, this should not be needed.

https://codereview.chromium.org/2235373004/diff/20001/webrtc/modules/pacing/p...
webrtc/modules/pacing/paced_sender.h:188: int bitrate_below_expected_count_
GUARDED_BY(critsect_) = 0;
On 2016/08/16 11:27:05, stefan-webrtc (holmer) wrote:
> Initialize in initializer list instead.

Will do, but I seem to recall seeing in style this being encouraged (I cant find
it now though)

[Irfan, 2016-08-16 23:23:54]

PTAL. The big changes are moving most of the logic to bitrate_prober (thanks for
the suggestion, it actually looks a lot cleaner) and removing redundant probing
callback. I have also rebased on top of Philip's latest CL.

[Irfan, 2016-08-18 20:32:18]

isheriff@chromium.org changed reviewers:
+ danilchap@webrtc.org

[Irfan, 2016-08-18 20:33:07]

+ Danil in case he has time to look as well

This is the design doc:

https://docs.google.com/document/d/1CQHLFZhknzEGV9PoD0TD0tKZonnXAf8UzJyUcZ-dI...

[danilchap, 2016-08-19 17:19:34]

I'm not deep enough in BWE part, so mostly style comments.

Is it reasonable to ask to split this CL into refactoring for the change (like
uint32_t->int for bitrate and routing UpdateDelayBasedEstimate singnal via
CongestionController instead of TransportFeedbackAdapter)
and the actual functional change?

https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/delay_based_bwe.h (right):

https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe.h:25: #include
"webrtc/modules/congestion_controller/include/congestion_controller.h"
prefer forward declaration over including header with full one.

https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/include/congestion_controller.h
(left):

https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/congestion_controller.h:31: class
BitrateController;
Why this class need full instead of forward definition of BitrateController?

https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.cc (right):

https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:78: case State::kWaitForResult:
shouldn't you start probing from State::kComplete too?

https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:124: default:
remove default.
Missing state would be noticed at compile time, instead of runtime then.

https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:130: RTC_DCHECK(probing_state_ !=
State::kDisabled);
RTC_DCHECK_NE

https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:176: if (packet_size_last_sent_ != 0 &&
probing_state_ == State::kSending) {
probing_state == State::kSending seems no longer needed this line.

https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:198: RTC_DCHECK(probing_state_ ==
State::kSending);
RTC_DCHECK_EQ

https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:208: RTC_DCHECK(probing_state_ ==
State::kSending);
RTC_DCHECK_EQ

https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.h (right):

https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.h:27: enum class State {
what move this enum type into public part?
No public functions return or accept it as a parameter.

[Irfan, 2016-08-23 05:46:57]

I moved the uint32-> int cleanup into a separate CL.

Philip/Stefan, This is ready for a review before I can fix up the tests. PTAL.

https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/congesti...
File webrtc/modules/congestion_controller/delay_based_bwe.h (right):

https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/congesti...
webrtc/modules/congestion_controller/delay_based_bwe.h:25: #include
"webrtc/modules/congestion_controller/include/congestion_controller.h"
On 2016/08/19 17:19:34, danilchap wrote:
> prefer forward declaration over including header with full one.

Done

https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/congesti...
File webrtc/modules/congestion_controller/include/congestion_controller.h
(left):

https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/congesti...
webrtc/modules/congestion_controller/include/congestion_controller.h:31: class
BitrateController;
On 2016/08/19 17:19:34, danilchap wrote:
> Why this class need full instead of forward definition of BitrateController?

due to use of std::unique_ptr<> I see complaints from compiler on my system.

https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/pacing/b...
File webrtc/modules/pacing/bitrate_prober.cc (right):

https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/pacing/b...
webrtc/modules/pacing/bitrate_prober.cc:78: case State::kWaitForResult:
On 2016/08/19 17:19:34, danilchap wrote:
> shouldn't you start probing from State::kComplete too?

kComplete indicates we do not expect any results from probing and we are done
with probing.

kSending and kWaitForResults are basically states where the prober expects
feedback to decide if it should ramp up again. so it makes sense to look at
feedback only in these states.

Let me know if that is not clear.

https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/pacing/b...
webrtc/modules/pacing/bitrate_prober.cc:124: default:
On 2016/08/19 17:19:34, danilchap wrote:
> remove default.
> Missing state would be noticed at compile time, instead of runtime then.

Done.

https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/pacing/b...
webrtc/modules/pacing/bitrate_prober.cc:130: RTC_DCHECK(probing_state_ !=
State::kDisabled);
On 2016/08/19 17:19:34, danilchap wrote:
> RTC_DCHECK_NE

RTC_DCHECK_NE does not handle scoped enums properly. Something to look at
outside this CL.

https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/pacing/b...
webrtc/modules/pacing/bitrate_prober.cc:176: if (packet_size_last_sent_ != 0 &&
probing_state_ == State::kSending) {
On 2016/08/19 17:19:34, danilchap wrote:
> probing_state == State::kSending seems no longer needed this line.

Done.

https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/pacing/b...
webrtc/modules/pacing/bitrate_prober.cc:198: RTC_DCHECK(probing_state_ ==
State::kSending);
On 2016/08/19 17:19:34, danilchap wrote:
> RTC_DCHECK_EQ

see above

https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/pacing/b...
webrtc/modules/pacing/bitrate_prober.cc:208: RTC_DCHECK(probing_state_ ==
State::kSending);
On 2016/08/19 17:19:34, danilchap wrote:
> RTC_DCHECK_EQ

see above

https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/pacing/b...
File webrtc/modules/pacing/bitrate_prober.h (right):

https://codereview.chromium.org/2235373004/diff/40001/webrtc/modules/pacing/b...
webrtc/modules/pacing/bitrate_prober.h:27: enum class State {
On 2016/08/19 17:19:34, danilchap wrote:
> what move this enum type into public part?
> No public functions return or accept it as a parameter.

Done. It used to return in an earlier PS.

[danilchap, 2016-08-23 17:59:29]

https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.cc (right):

https://codereview.webrtc.org/2235373004/diff/40001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:78: case State::kWaitForResult:
On 2016/08/23 05:46:57, Irfan wrote:
> On 2016/08/19 17:19:34, danilchap wrote:
> > shouldn't you start probing from State::kComplete too?
> 
> kComplete indicates we do not expect any results from probing and we are done
> with probing.
> 
> kSending and kWaitForResults are basically states where the prober expects
> feedback to decide if it should ramp up again. so it makes sense to look at
> feedback only in these states.
> 
> Let me know if that is not clear.

So it is not a coincidence it is same states as in functions just above.
May be then replace the switch with 
if (IsExpectingProbingResult()) {
 .... 
}

https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/delay_based_bwe.cc (right):

https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe.cc:151: rtc::CritScope
lock(&crit_);
UpdateEsimate is called when this lock is hold already.
Add EXCLUSIVE_LOCKS_REQUIRED(crit_) to it's definition in .h instead of retaking
the lock here.

https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.cc (right):

https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:116: min_bitrate_to_probe_further_ = 4 *
estimated_bitrate_bps_;
it probably make more sense to derive this value from just added probe, not
estimated_bitrate_bps_, Or derive probe bitrate from estimated_bitrate_bps_ too.
e.g.
const int kStartProbeBps = 1800000;
CreateProbeCluster(kStartProbeBps / 2,...);
CreateProbeCluster(kStartProbeBps,...);
min_bitrate_to_probe_further_ = kStartProbeBps * 2 / 3;

https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:122: if ((now_ms -
time_last_probe_sent_ms_) >
this check has nothing to do with IncomingPacket (a new media packet created to
be paced).
The kInit block uses only packet_size,
The kWaitForResult block uses only now_ms.
i.e. this two blocks of code feel too unrelated to be in same function.

May be move this block into own function and call it from PacedSender::Process?

[philipel, 2016-08-25 10:39:56]

https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.cc (right):

https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:78: void
BitrateProber::SetEstimatedBitrate(int bitrate_bps) {
In order to make the bitrate prober more general I would suggest we move the
logic to perform exponential probing to the congestion controller (now that the
delay based bwe takes a congestion controller instead of a remote bitrate
observer).

https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:85: if (min_bitrate_to_probe_further_ &&
min_bitrate_to_probe_further != kDisableExponentialProbing

https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:112: CreateProbeCluster(900000,
kProbeDeltasPerCluster + 1);
Why move the creation of initial probe clusters here?

https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:122: if ((now_ms -
time_last_probe_sent_ms_) >
I don't think this will ever occur. If we don't have packets to send we send
padding instead and PacketSent is still called.

[stefan-webrtc, 2016-09-02 13:10:22]

https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/delay_based_bwe.cc (right):

https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe.cc:131:
remote_rate_.SetEstimate(probing_bps, info.arrival_time_ms);
Should we DCHECK here somewhere that probing_bps isn't smaller than the current
estimate?

https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/include/congestion_controller.h
(right):

https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/congestion_controller.h:18:
#include "webrtc/modules/bitrate_controller/include/bitrate_controller.h"
Why is this change needed?

https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/congestion_controller.h:104: bool
IsExpectingProbingResults();
I'm not super happy about making these public. These should be internal to the
congestion controller and not be exposed.

https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.cc (right):

https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:78: void
BitrateProber::SetEstimatedBitrate(int bitrate_bps) {
On 2016/08/25 10:39:56, philipel wrote:
> In order to make the bitrate prober more general I would suggest we move the
> logic to perform exponential probing to the congestion controller (now that
the
> delay based bwe takes a congestion controller instead of a remote bitrate
> observer).

Do you mean that the CongestionController should be calling CreateProbeCluster()
with exponentially increasing bitrates?

I don't want to spread out the logic too much, but at the same time it would be
good to don't make too many assumptions in this class, so if the amount of
probing code needed in CongestionController is small I think that might be a
good idea.

[philipel, 2016-09-02 13:32:50]

https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.cc (right):

https://codereview.webrtc.org/2235373004/diff/60001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:78: void
BitrateProber::SetEstimatedBitrate(int bitrate_bps) {
On 2016/09/02 13:10:22, stefan-webrtc (holmer) wrote:
> On 2016/08/25 10:39:56, philipel wrote:
> > In order to make the bitrate prober more general I would suggest we move the
> > logic to perform exponential probing to the congestion controller (now that
> the
> > delay based bwe takes a congestion controller instead of a remote bitrate
> > observer).
> 
> Do you mean that the CongestionController should be calling
CreateProbeCluster()
> with exponentially increasing bitrates?
> 
> I don't want to spread out the logic too much, but at the same time it would
be
> good to don't make too many assumptions in this class, so if the amount of
> probing code needed in CongestionController is small I think that might be a
> good idea.

To me it looks like this could be implemented either in
CongestionController::Process or in
CongestionController::MaybeTriggerOnNetworkChanged.

I think it's important to keep the "lego blocks" clean so they are both easy to
understand but also to avoid having to refactor even the most basic components
in the case of anyone wanting to change the probing behavior.

[Irfan, 2016-09-08 08:47:55]

PTAL. A couple of follow up fixes needed to make the capacity ramp up:

1. https://codereview.chromium.org/2269873002/  or something along those lines
is needed to make the start up exponential probing work

2. Multiple packets per probe to go > 6 Mbps

[philipel, 2016-09-08 12:52:47]

lg, just a few comments.

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

https://codereview.webrtc.org/2235373004/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:318: void
CongestionController::OnDelayBasedBweChanged(int bitrate_bps) {
Where can I find the changes to the .h file?

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

https://codereview.webrtc.org/2235373004/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/probe_controller.cc:32: constexpr int
kDisableExponentialProbing = 0;
kExponentialProbingDisabled

https://codereview.webrtc.org/2235373004/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/probe_controller.cc:109:
((clock_->TimeInMilliseconds() - time_last_probing_initiated_ms_) >
Remove Process() and add the timeout check to SetEstimatedBitrate.

https://codereview.webrtc.org/2235373004/diff/80001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/probe_controller.h (right):

https://codereview.webrtc.org/2235373004/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/probe_controller.h:45:
kWaitForProbingResult,
kWaitingForProbingResult

[Irfan, 2016-09-09 08:12:53]

Added a few unit tests and reverted using observer on delay_based_bwe to allow
tests to be same

https://codereview.chromium.org/2235373004/diff/80001/webrtc/modules/congesti...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.chromium.org/2235373004/diff/80001/webrtc/modules/congesti...
webrtc/modules/congestion_controller/congestion_controller.cc:318: void
CongestionController::OnDelayBasedBweChanged(int bitrate_bps) {
On 2016/09/08 12:52:46, philipel wrote:
> Where can I find the changes to the .h file?

The .h file is part of this patchset, its in include/

https://codereview.chromium.org/2235373004/diff/80001/webrtc/modules/congesti...
File webrtc/modules/congestion_controller/probe_controller.cc (right):

https://codereview.chromium.org/2235373004/diff/80001/webrtc/modules/congesti...
webrtc/modules/congestion_controller/probe_controller.cc:32: constexpr int
kDisableExponentialProbing = 0;
On 2016/09/08 12:52:47, philipel wrote:
> kExponentialProbingDisabled

Done.

https://codereview.chromium.org/2235373004/diff/80001/webrtc/modules/congesti...
webrtc/modules/congestion_controller/probe_controller.cc:109:
((clock_->TimeInMilliseconds() - time_last_probing_initiated_ms_) >
On 2016/09/08 12:52:47, philipel wrote:
> Remove Process() and add the timeout check to SetEstimatedBitrate.

Done.

https://codereview.chromium.org/2235373004/diff/80001/webrtc/modules/congesti...
File webrtc/modules/congestion_controller/probe_controller.h (right):

https://codereview.chromium.org/2235373004/diff/80001/webrtc/modules/congesti...
webrtc/modules/congestion_controller/probe_controller.h:45:
kWaitForProbingResult,
On 2016/09/08 12:52:47, philipel wrote:
> kWaitingForProbingResult

Done.

[philipel, 2016-09-09 08:59:09]

lgtm with the thread annotation fix.

https://codereview.chromium.org/2235373004/diff/80001/webrtc/modules/congesti...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.chromium.org/2235373004/diff/80001/webrtc/modules/congesti...
webrtc/modules/congestion_controller/congestion_controller.cc:318: void
CongestionController::OnDelayBasedBweChanged(int bitrate_bps) {
On 2016/09/09 08:12:52, Irfan wrote:
> On 2016/09/08 12:52:46, philipel wrote:
> > Where can I find the changes to the .h file?
> 
> The .h file is part of this patchset, its in include/

Oops

https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest...
File webrtc/modules/congestion_controller/delay_based_bwe.h (right):

https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest...
webrtc/modules/congestion_controller/delay_based_bwe.h:64: uint32_t*
target_bitrate_bps);
Add EXCLUSIVE_LOCKS_REQUIRED(crit_) annotation and remove the CritScope.

https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest...
File webrtc/modules/congestion_controller/probe_controller.h (right):

https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest...
webrtc/modules/congestion_controller/probe_controller.h:37: void
InitiateProbing(std::initializer_list<int> bitrates_to_probe,
What is the reason we use an std::initializer_list instead of a std::vector?

[philipel, 2016-09-09 09:05:18]

One more thing. Irfan, can you create a bug so we can track all CLs related to
this feature?

[philipel, 2016-09-09 10:54:41]

Description was changed from

==========
Probing: Add support for exponential startup probing

Adds support for exponentially probing the bandwidth at start-up to allow
ramp-up to real capacity of the network.

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

to

==========
Probing: Add support for exponential startup probing

Adds support for exponentially probing the bandwidth at start-up to allow
ramp-up to real capacity of the network.

BUG=webrtc:6332
==========

[stefan-webrtc, 2016-09-09 11:55:36]

On 2016/09/09 09:05:18, philipel wrote:
> One more thing. Irfan, can you create a bug so we can track all CLs related to
> this feature?

Tracking bug here: https://bugs.chromium.org/p/webrtc/issues/detail?id=6332

Feel free to create blocking bugs for sub tasks as you see fit.

[stefan-webrtc, 2016-09-09 12:03:52]

https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest...
webrtc/modules/congestion_controller/congestion_controller.cc:259:
RemoteBitrateEstimator* rbe = new DelayBasedBwe(this, clock_);
It's not clear to me why we pass in a CongestionController to DelayBasedBwe
instead of a BitrateController. Seems to just pollute the CongestionController
interface.

I think it'd be better to have BitrateController the interface needed.

[Irfan, 2016-09-12 06:40:40]

https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest...
webrtc/modules/congestion_controller/congestion_controller.cc:259:
RemoteBitrateEstimator* rbe = new DelayBasedBwe(this, clock_);
On 2016/09/09 12:03:52, stefan-webrtc (holmer) wrote:
> It's not clear to me why we pass in a CongestionController to DelayBasedBwe
> instead of a BitrateController. Seems to just pollute the CongestionController
> interface.
> 
> I think it'd be better to have BitrateController the interface needed.

I have gone and ahead and taken your suggestion.

There were a couple of things abstracting this out of BitrateController did:
adding a clearer API about it being a delay based estimate and removing the
redundant ssrc list. Nevertheless, I have added comments to clear that up.

https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest...
File webrtc/modules/congestion_controller/delay_based_bwe.h (right):

https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest...
webrtc/modules/congestion_controller/delay_based_bwe.h:64: uint32_t*
target_bitrate_bps);
On 2016/09/09 08:59:09, philipel wrote:
> Add EXCLUSIVE_LOCKS_REQUIRED(crit_) annotation and remove the CritScope.

Done.

https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest...
File webrtc/modules/congestion_controller/probe_controller.h (right):

https://codereview.chromium.org/2235373004/diff/100001/webrtc/modules/congest...
webrtc/modules/congestion_controller/probe_controller.h:37: void
InitiateProbing(std::initializer_list<int> bitrates_to_probe,
On 2016/09/09 08:59:09, philipel wrote:
> What is the reason we use an std::initializer_list instead of a std::vector?

It is a light weight proxy for {} list. std::vector actually takes an
std::initializer_list and constructs a vector. Since we just need an
unmodifiable list, this suffices.

[stefan-webrtc, 2016-09-12 13:58:29]

lgtm % nit

https://codereview.chromium.org/2235373004/diff/120001/webrtc/modules/bitrate...
File webrtc/modules/bitrate_controller/bitrate_controller_impl.cc (right):

https://codereview.chromium.org/2235373004/diff/120001/webrtc/modules/bitrate...
webrtc/modules/bitrate_controller/bitrate_controller_impl.cc:194: // This is
called for BW estimate from a send-side estimator.
Perhaps make this a TODO and mention that we should introduce a new observer for
DelayBasedBwe which is independent of RemoteBitrateEstimator.

[stefan-webrtc, 2016-09-12 13:59:28]

https://codereview.chromium.org/2235373004/diff/120001/webrtc/modules/bitrate...
File webrtc/modules/bitrate_controller/include/bitrate_controller.h (right):

https://codereview.chromium.org/2235373004/diff/120001/webrtc/modules/bitrate...
webrtc/modules/bitrate_controller/include/bitrate_controller.h:23: #include
"webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h"
Include order.

[Irfan, 2016-09-12 14:22:19]

https://codereview.chromium.org/2235373004/diff/120001/webrtc/modules/bitrate...
File webrtc/modules/bitrate_controller/bitrate_controller_impl.cc (right):

https://codereview.chromium.org/2235373004/diff/120001/webrtc/modules/bitrate...
webrtc/modules/bitrate_controller/bitrate_controller_impl.cc:194: // This is
called for BW estimate from a send-side estimator.
On 2016/09/12 13:58:29, stefan-webrtc (holmer) wrote:
> Perhaps make this a TODO and mention that we should introduce a new observer
for
> DelayBasedBwe which is independent of RemoteBitrateEstimator.

Done.

https://codereview.chromium.org/2235373004/diff/120001/webrtc/modules/bitrate...
File webrtc/modules/bitrate_controller/include/bitrate_controller.h (right):

https://codereview.chromium.org/2235373004/diff/120001/webrtc/modules/bitrate...
webrtc/modules/bitrate_controller/include/bitrate_controller.h:23: #include
"webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h"
On 2016/09/12 13:59:28, stefan-webrtc (holmer) wrote:
> Include order.

Done.

[Irfan, 2016-09-12 14:22:30]

The CQ bit was checked by isheriff@chromium.org

[Irfan, 2016-09-12 14:22:30]

The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.chromium.org/2235373004/#ps140001
(title: "Fixed nit")

[commit-bot: I haz the power, 2016-09-12 14:22:39]

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-09-12 14:49:52]

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

[commit-bot: I haz the power, 2016-09-12 14:49:55]

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

[Irfan, 2016-09-12 15:02:09]

The CQ bit was checked by isheriff@chromium.org

[Irfan, 2016-09-12 15:02:10]

The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.chromium.org/2235373004/#ps160001
(title: "lint fix")

[commit-bot: I haz the power, 2016-09-12 15:02:14]

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-09-12 17:03:05]

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

[commit-bot: I haz the power, 2016-09-12 17:03:06]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[commit-bot: I haz the power, 2016-09-12 19:29:08]

Description was changed from

==========
Probing: Add support for exponential startup probing

Adds support for exponentially probing the bandwidth at start-up to allow
ramp-up to real capacity of the network.

BUG=webrtc:6332
==========

to

==========
Probing: Add support for exponential startup probing

Adds support for exponentially probing the bandwidth at start-up to allow
ramp-up to real capacity of the network.

BUG=webrtc:6332
R=philipel@webrtc.org, stefan@webrtc.org

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

[Irfan, 2016-09-12 19:29:09]

Description was changed from

==========
Probing: Add support for exponential startup probing

Adds support for exponentially probing the bandwidth at start-up to allow
ramp-up to real capacity of the network.

BUG=webrtc:6332
R=philipel@webrtc.org, stefan@webrtc.org

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

to

==========
Probing: Add support for exponential startup probing

Adds support for exponentially probing the bandwidth at start-up to allow
ramp-up to real capacity of the network.

BUG=webrtc:6332
R=philipel@webrtc.org, stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/b2540bb99fa974bc8cb66eeff...
==========

[commit-bot: I haz the power, 2016-09-12 19:29:09]

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

[Irfan, 2016-09-12 19:29:10]

Committed patchset #9 (id:160001) manually as
b2540bb99fa974bc8cb66eeff9c6b6d7fd0c36eb (presubmit successful).