Perform probing on network route change.

Perform probing on network route change.

BUG=webrtc:7208
Review-Url: https://codereview.webrtc.org/2714503002
Cr-Commit-Position: refs/heads/master@{#17096}
Committed: https://chromium.googlesource.com/external/webrtc/+/cb9ba301f0651a58dfa20c9fea0333d69f5b2fbf

[philipel, 2017-02-22 13:44:38]

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

[philipel, 2017-02-22 13:44:58]

Description was changed from

==========
Peform probing on network route change.

BUG=webrtc:7208
==========

to

==========
Perform probing on network route change.

BUG=webrtc:7208
==========

[stefan-webrtc, 2017-02-22 14:38:00]

Looks good, but I have a question...

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

https://codereview.webrtc.org/2714503002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/congestion_controller.cc:235:
transport_feedback_adapter_.SetMinBitrate(min_bitrate_bps);
Reading this code makes me think... What happens if we get acks from packets
sent on the previous transport after this? Should we reset the SendTimeHistory
here as well, or at least remove instances which are from the old transport
pair?

[philipel, 2017-02-23 11:57:02]

I realized that reconstructing the ProbeController was not thread safe since
there is no lock guarding it, so I implemeted a ProbeController::Reset function
instead.

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

https://codereview.webrtc.org/2714503002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/congestion_controller.cc:235:
transport_feedback_adapter_.SetMinBitrate(min_bitrate_bps);
On 2017/02/22 14:38:00, stefan-webrtc wrote:
> Reading this code makes me think... What happens if we get acks from packets
> sent on the previous transport after this? Should we reset the SendTimeHistory
> here as well, or at least remove instances which are from the old transport
> pair?

SendTimeHistory is now also cleared when ResetBweAndBitrates is called.

[philipel, 2017-03-07 10:33:14]

PTAL

[stefan-webrtc, 2017-03-07 12:16:55]

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

https://codereview.webrtc.org/2714503002/diff/60001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/probe_controller.cc:54: Reset();
I think indentation is wrong here.

https://codereview.webrtc.org/2714503002/diff/60001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/probe_controller.cc:192:
mid_call_probing_waiting_for_result_ = false;
Can we do something like this instead:
*this = ProbeController(pacer_, clock_);

and keep the initializer list? That reduces the risk of this becoming out of
sync with the members, I think...

[philipel, 2017-03-07 13:09:13]

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

https://codereview.webrtc.org/2714503002/diff/60001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/probe_controller.cc:192:
mid_call_probing_waiting_for_result_ = false;
On 2017/03/07 12:16:55, stefan-webrtc wrote:
> Can we do something like this instead:
> *this = ProbeController(pacer_, clock_);
> 
> and keep the initializer list? That reduces the risk of this becoming out of
> sync with the members, I think...

I can see two problems with that. First, what will happen to the pointer holding
the ProbeController? You might rewrite your local this pointer but the
CongestionController won't know about that. Second, what happens when we destroy
a lock other threads might potentially be waiting to acquire?

[stefan-webrtc, 2017-03-07 13:11:06]

lgtm

[philipel, 2017-03-07 13:11:21]

The CQ bit was checked by philipel@webrtc.org

[stefan-webrtc, 2017-03-07 13:11:26]

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

https://codereview.webrtc.org/2714503002/diff/60001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/probe_controller.cc:192:
mid_call_probing_waiting_for_result_ = false;
On 2017/03/07 13:09:13, philipel wrote:
> On 2017/03/07 12:16:55, stefan-webrtc wrote:
> > Can we do something like this instead:
> > *this = ProbeController(pacer_, clock_);
> > 
> > and keep the initializer list? That reduces the risk of this becoming out of
> > sync with the members, I think...
> 
> I can see two problems with that. First, what will happen to the pointer
holding
> the ProbeController? You might rewrite your local this pointer but the
> CongestionController won't know about that. Second, what happens when we
destroy
> a lock other threads might potentially be waiting to acquire?

Agree, that seems very risky. :)

[commit-bot: I haz the power, 2017-03-07 13:11:27]

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

[philipel, 2017-03-07 13:11:46]

The CQ bit was unchecked by philipel@webrtc.org

[philipel, 2017-03-07 13:15:53]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-03-07 13:15:53]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2714503002/#ps80001 (title:
"Rebase   Format")

[commit-bot: I haz the power, 2017-03-07 13:15:56]

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-07 13:18:45]

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

[commit-bot: I haz the power, 2017-03-07 13:18:46]

Try jobs failed on following builders:
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/18319)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)

[philipel, 2017-03-07 13:31:09]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-03-07 13:31:09]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2714503002/#ps100001 (title:
"Rebase")

[commit-bot: I haz the power, 2017-03-07 13:31:16]

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-07 13:34:41]

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

[commit-bot: I haz the power, 2017-03-07 13:34:42]

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

[philipel, 2017-03-07 14:02:57]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-03-07 14:02:58]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2714503002/#ps120001 (title:
"SendTimeHistoryTest.Clear fix.")

[commit-bot: I haz the power, 2017-03-07 14:03:02]

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-07 14:30:59]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488895377527840,
"parent_rev": "f89a738626b572eb4ec06218d3bc545d8b11fb59", "commit_rev":
"cb9ba301f0651a58dfa20c9fea0333d69f5b2fbf"}

[commit-bot: I haz the power, 2017-03-07 14:31:02]

Description was changed from

==========
Perform probing on network route change.

BUG=webrtc:7208
==========

to

==========
Perform probing on network route change.

BUG=webrtc:7208

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

[commit-bot: I haz the power, 2017-03-07 14:31:04]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/external/webrtc/+/cb9ba301f0651a58dfa20c9fe...