Update the call when the network route changes

Update the call when the network route changes
so that BWE can be updated promptly.

BUG=webrtc:5726
R=mflodman@webrtc.org, pbos@webrtc.org, pthatcher@google.com, pthatcher@webrtc.org, stefan@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/0e533ef487f4851d4c6e730bd2d93c9449578c3a

[honghaiz3, 2016-03-30 00:37:05]

honghaiz@webrtc.org changed reviewers:
+ pthatcher@webrtc.org, stefan@webrtc.org

[honghaiz3, 2016-03-30 00:39:20]

Patchset #1 (id:1) has been deleted

[pthatcher1, 2016-03-30 17:01:42]

Would it make sense to land this without the ResetBwe method, or would it make
more sense to wait until that's implement, and then include this CL with that
one?

[honghaiz3, 2016-03-30 18:13:30]

Patchset #2 (id:40001) has been deleted

[honghaiz3, 2016-03-30 18:14:34]

On 2016/03/30 17:01:42, pthatcher1 wrote:
> Would it make sense to land this without the ResetBwe method, or would it make
> more sense to wait until that's implement, and then include this CL with that
> one?
Done.

[pthatcher, 2016-03-30 18:41:14]

pthatcher@google.com changed reviewers:
+ pthatcher@google.com

[pthatcher, 2016-03-30 18:41:14]

lgtm

[honghaiz3, 2016-03-30 19:51:53]

On 2016/03/30 18:41:14, pthatcher wrote:
> lgtm

Can you lgtm using your webrtc account? I cannot land it otherwise.

[honghaiz3, 2016-03-30 23:09:51]

On 2016/03/30 19:51:53, honghaiz3 wrote:
> On 2016/03/30 18:41:14, pthatcher wrote:
> > lgtm
> 
> Can you lgtm using your webrtc account? I cannot land it otherwise.

Please also lgtm this using your webrtc account.

[pthatcher1, 2016-03-30 23:16:09]

lgtm

[stefan-webrtc, 2016-03-31 07:17:05]

https://codereview.webrtc.org/1844773002/diff/20001/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/1844773002/diff/20001/webrtc/call.h#newcode146
webrtc/call.h:146: const cricket::NetworkRoute& network_route) = 0;
Should NetworkRoute be under the cricket namespace? Wouldn't rtc be better?

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

https://codereview.webrtc.org/1844773002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:615: bool inserted = result.second;
bool inserted = result.first.second?

https://codereview.webrtc.org/1844773002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:621: if (changed) {
if (kv->second != network_route) {

https://codereview.webrtc.org/1844773002/diff/60001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1844773002/diff/60001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:607: // established.
This makes me wonder if this method should replace SignalChannelNetworkState
too? Or should we differentiate between a NetworkRoute which isn't .connected
and one which is "down" (as set by SignalChannelNetworkState)?

https://codereview.webrtc.org/1844773002/diff/60001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:627: // TODO(stefan@): Update the BWE bitrates.
TODO(holmer) preferred

[honghaiz3, 2016-03-31 17:42:53]

Thanks! PTAL.

https://codereview.webrtc.org/1844773002/diff/20001/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/1844773002/diff/20001/webrtc/call.h#newcode146
webrtc/call.h:146: const cricket::NetworkRoute& network_route) = 0;
On 2016/03/31 07:17:05, stefan-webrtc (holmer) wrote:
> Should NetworkRoute be under the cricket namespace? Wouldn't rtc be better?

Good point! Done.

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

https://codereview.webrtc.org/1844773002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:615: bool inserted = result.second;
On 2016/03/31 07:17:05, stefan-webrtc (holmer) wrote:
> bool inserted = result.first.second?
No. The return value of map::insert:
The single element versions (1) return a pair, with its member pair::first set
to an iterator pointing to either the newly inserted element or to the element
with an equivalent key in the map. The pair::second element in the pair is set
to true if a new element was inserted or false if an equivalent key already
existed.

https://codereview.webrtc.org/1844773002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:621: if (changed) {
On 2016/03/31 07:17:05, stefan-webrtc (holmer) wrote:
> if (kv->second != network_route) {

Done.

https://codereview.webrtc.org/1844773002/diff/60001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1844773002/diff/60001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:607: // established.
On 2016/03/31 07:17:05, stefan-webrtc (holmer) wrote:
> This makes me wonder if this method should replace SignalChannelNetworkState
> too? Or should we differentiate between a NetworkRoute which isn't .connected
> and one which is "down" (as set by SignalChannelNetworkState)?
Currently SignalChannelNetworkState is triggered by
transport_channel::writable() and EWOULDBLOCK, but this is signalled by selected
candidate pair changed. 
So I think this should not replace SignalChannelNetworkState. Perhaps this
(network disconnect) need not be handled here. Instead, it should be handled in
the SignalChannelNetworkState (network up and down).
It is also possible to merge them together, but this will need some more
discussion. Peter and I discussed the plan to refactor the SignalReadyToSend
(which fires SignalChannelNetworkState). We can consider merging these two
methods along with that work. For now I will just leave a TODO here.

https://codereview.webrtc.org/1844773002/diff/60001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:627: // TODO(stefan@): Update the BWE bitrates.
On 2016/03/31 07:17:05, stefan-webrtc (holmer) wrote:
> TODO(holmer) preferred

Done.

[stefan-webrtc, 2016-04-01 08:08:38]

lgtm

https://codereview.webrtc.org/1844773002/diff/60001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1844773002/diff/60001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:607: // established.
On 2016/03/31 17:42:52, honghaiz3 wrote:
> On 2016/03/31 07:17:05, stefan-webrtc (holmer) wrote:
> > This makes me wonder if this method should replace SignalChannelNetworkState
> > too? Or should we differentiate between a NetworkRoute which isn't
.connected
> > and one which is "down" (as set by SignalChannelNetworkState)?
> Currently SignalChannelNetworkState is triggered by
> transport_channel::writable() and EWOULDBLOCK, but this is signalled by
selected
> candidate pair changed. 
> So I think this should not replace SignalChannelNetworkState. Perhaps this
> (network disconnect) need not be handled here. Instead, it should be handled
in
> the SignalChannelNetworkState (network up and down).
> It is also possible to merge them together, but this will need some more
> discussion. Peter and I discussed the plan to refactor the SignalReadyToSend
> (which fires SignalChannelNetworkState). We can consider merging these two
> methods along with that work. For now I will just leave a TODO here.

Sounds good, it would be nice to keep the number of "network has changed in some
way" signals down...

[stefan-webrtc, 2016-04-01 08:09:25]

Please associate it with a bug so that we can track all the changes we're making
there.

[honghaiz3, 2016-04-01 22:19:03]

Description was changed from

==========
Update the call when the network route changes
so that BWE can be updated promptly.

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

to

==========
Update the call when the network route changes
so that BWE can be updated promptly.

BUG=webrtc:5726
==========

[honghaiz3, 2016-04-01 22:20:55]

On 2016/04/01 08:09:25, stefan-webrtc (holmer) wrote:
> Please associate it with a bug so that we can track all the changes we're
making
> there.

Done.

[honghaiz3, 2016-04-01 22:21:07]

Thanks!

https://codereview.webrtc.org/1844773002/diff/60001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1844773002/diff/60001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:607: // established.
On 2016/04/01 08:08:38, stefan-webrtc (holmer) wrote:
> On 2016/03/31 17:42:52, honghaiz3 wrote:
> > On 2016/03/31 07:17:05, stefan-webrtc (holmer) wrote:
> > > This makes me wonder if this method should replace
SignalChannelNetworkState
> > > too? Or should we differentiate between a NetworkRoute which isn't
> .connected
> > > and one which is "down" (as set by SignalChannelNetworkState)?
> > Currently SignalChannelNetworkState is triggered by
> > transport_channel::writable() and EWOULDBLOCK, but this is signalled by
> selected
> > candidate pair changed. 
> > So I think this should not replace SignalChannelNetworkState. Perhaps this
> > (network disconnect) need not be handled here. Instead, it should be handled
> in
> > the SignalChannelNetworkState (network up and down).
> > It is also possible to merge them together, but this will need some more
> > discussion. Peter and I discussed the plan to refactor the SignalReadyToSend
> > (which fires SignalChannelNetworkState). We can consider merging these two
> > methods along with that work. For now I will just leave a TODO here.
> 
> Sounds good, it would be nice to keep the number of "network has changed in
some
> way" signals down...
Not sure what you mean by that.

[honghaiz3, 2016-04-01 22:21:13]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-04-01 22:21:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@webrtc.org, pthatcher@google.com
Link to the patchset: https://codereview.webrtc.org/1844773002/#ps100001 (title:
"Merge (with adding sent_packet_id)")

[commit-bot: I haz the power, 2016-04-01 22:21:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1844773002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1844773002/100001

[commit-bot: I haz the power, 2016-04-01 22:26:50]

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

[commit-bot: I haz the power, 2016-04-01 22:26:51]

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

[honghaiz3, 2016-04-01 22:35:24]

honghaiz@webrtc.org changed reviewers:
+ pbos@webrtc.org

[honghaiz3, 2016-04-01 22:35:25]

pbos@, can you take a look at the CL?
It needs an owner of webrtc/call.h to review this CL.

[pbos-webrtc, 2016-04-04 14:24:51]

lgtm for webrtc/call*

[honghaiz3, 2016-04-18 17:54:51]

The CQ bit was checked by honghaiz@webrtc.org

[commit-bot: I haz the power, 2016-04-18 17:54:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1844773002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1844773002/100001

[honghaiz3, 2016-04-18 17:58:28]

honghaiz@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[honghaiz3, 2016-04-18 17:58:29]

kjellander@, 

Can you take a look at the CL?
The change to webrtc/call.h still need an owner review.

[commit-bot: I haz the power, 2016-04-18 18:00:57]

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

[commit-bot: I haz the power, 2016-04-18 18:00:59]

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

[kjellander_webrtc, 2016-04-18 19:20:50]

On 2016/04/18 17:58:29, honghaiz3 wrote:
> kjellander@, 
> 
> Can you take a look at the CL?
> The change to webrtc/call.h still need an owner review.

Right, since that sits in the root (which it really shouldn't, I'm expecting
pbos@ to move it), it needs a root OWNERS approval. Neither me, pbos or holmer
is root owner.

[honghaiz3, 2016-04-18 21:37:13]

honghaiz@webrtc.org changed reviewers:
+ juberti@webrtc.org
- kjellander@webrtc.org, pbos@webrtc.org

[honghaiz3, 2016-04-18 21:37:14]

Justin, 

Can you provide a review for this CL?
Looks like that very few people can approve a change to the file webrtc/call.h.

[pbos-webrtc, 2016-04-18 22:43:34]

pbos@webrtc.org changed reviewers:
+ mflodman@webrtc.org, pbos@webrtc.org

[pbos-webrtc, 2016-04-18 22:43:34]

+r mflodman@ for webrtc/call.h

[mflodman, 2016-04-19 06:54:25]

call.h LGTM

[honghaiz3, 2016-04-19 20:49:44]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-04-19 20:49:45]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org,
pthatcher@webrtc.org, pbos@webrtc.org, mflodman@webrtc.org, pthatcher@google.com
Link to the patchset: https://codereview.chromium.org/1844773002/#ps120001
(title: "Merge with head")

[commit-bot: I haz the power, 2016-04-19 20:49:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1844773002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1844773002/120001

[commit-bot: I haz the power, 2016-04-19 22:41:57]

Description was changed from

==========
Update the call when the network route changes
so that BWE can be updated promptly.

BUG=webrtc:5726
==========

to

==========
Update the call when the network route changes
so that BWE can be updated promptly.

BUG=webrtc:5726
R=mflodman@webrtc.org, pbos@webrtc.org, pthatcher@google.com,
pthatcher@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/0e533ef487f4851d4c6e730bd2d93c9449578c3a
Cr-Commit-Position: refs/heads/master@{#12432}
==========

[honghaiz3, 2016-04-19 22:41:57]

Description was changed from

==========
Update the call when the network route changes
so that BWE can be updated promptly.

BUG=webrtc:5726
==========

to

==========
Update the call when the network route changes
so that BWE can be updated promptly.

BUG=webrtc:5726
R=mflodman@webrtc.org, pbos@webrtc.org, pthatcher@google.com,
pthatcher@webrtc.org, stefan@webrtc.org

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

[commit-bot: I haz the power, 2016-04-19 22:41:58]

Patchset 5 (id:??) landed as
https://crrev.com/0e533ef487f4851d4c6e730bd2d93c9449578c3a
Cr-Commit-Position: refs/heads/master@{#12432}

[honghaiz3, 2016-04-19 22:41:59]

Committed patchset #5 (id:120001) manually as
0e533ef487f4851d4c6e730bd2d93c9449578c3a (presubmit successful).

[pthatcher1, 2016-04-29 22:34:43]

lgtm