Added network thread to rtc::BaseChannel

Adds network thread to rtc::BaseChannel
BaseChannel do calls to transport_channel on network_thread,
while keep calls to media_engine on worker_thread.
It still works when network_thread == worker_thread.

BUG=webrtc:5645
R=pthatcher@webrtc.org

Committed: https://crrev.com/33b01f2162af32457e0a6a6e0eb670254701df61
Cr-Commit-Position: refs/heads/master@{#12690}

[danilchap, 2016-04-22 12:01:25]

Description was changed from

==========
Added network thread to rtc::BaseChannel
BaseChannel calls transport_channel on network_thread,
but keep calling media_engine on worker_thread.
It still works when network_thread=worker_thread.

BUG=webrtc:5645
==========

to

==========
Adds network thread to rtc::BaseChannel
BaseChannel do calls to transport_channel on network_thread,
while keep calls to media_engine on worker_thread.
It still works when network_thread == worker_thread.

BUG=webrtc:5645
==========

[danilchap, 2016-04-22 12:27:14]

danilchap@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[danilchap, 2016-04-22 12:27:15]

This CL is main part of introducing network thread to improve bwe prober.
(https://codereview.webrtc.org/1888903003/).

It use BaseChannel class to draw separation line between new network and old
worker threads.

Tests adjusted: because sending can now be done on a different than worker
thread, return value of SendRtp is almost always true. Tests no longer validate
if packet was sent, but check only if packet was received.

[danilchap, 2016-04-25 09:45:53]

danilchap@webrtc.org changed reviewers:
+ perkj@webrtc.org

[perkj_webrtc, 2016-04-25 14:42:13]

On 2016/04/22 12:27:15, danilchap wrote:
> This CL is main part of introducing network thread to improve bwe prober.
> (https://codereview.webrtc.org/1888903003/).
> 
> It use BaseChannel class to draw separation line between new network and old
> worker threads.
> 
> Tests adjusted: because sending can now be done on a different than worker
> thread, return value of SendRtp is almost always true. Tests no longer
validate
> if packet was sent, but check only if packet was received.

Looks like something for pthatcher to take the first pass at.

[perkj_webrtc, 2016-04-29 07:46:12]

It probably make sense to have a separate network thread. But I don't think that
I am the right person to decide. 
Peter- can you your or your team in kirkland take a look?

https://codereview.webrtc.org/1903393004/diff/100001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/1903393004/diff/100001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:567:
media_channel()->OnNetworkRouteChanged(message->transport_name,
Why not always post this to the worker even if worker_thread_ = network thread.

https://codereview.webrtc.org/1903393004/diff/100001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:576: if (rtcp) {
DCHECK thread please.

https://codereview.webrtc.org/1903393004/diff/100001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:587: if (worker_thread_->IsCurrent()) {
dito- can we always post regardless if network_thread != worker_thread.

https://codereview.webrtc.org/1903393004/diff/100001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:1604: if (worker_thread_->IsCurrent()) {
dcheck thread.  Which thread is this?

[danilchap, 2016-04-29 08:46:59]

danilchap@webrtc.org changed reviewers:
- perkj@webrtc.org

[danilchap, 2016-04-29 08:47:00]

https://codereview.webrtc.org/1903393004/diff/100001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/1903393004/diff/100001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:567:
media_channel()->OnNetworkRouteChanged(message->transport_name,
On 2016/04/29 07:46:12, perkj_webrtc wrote:
> Why not always post this to the worker even if worker_thread_ = network
thread.

The idea was to mimic current behavior as much as possible (i.e. in case
worker_thread_ = network_thread, this CL should be almost noop).
But I guess it is a weak argument against clearer code.

Done.

https://codereview.webrtc.org/1903393004/diff/100001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:576: if (rtcp) {
On 2016/04/29 07:46:12, perkj_webrtc wrote:
> DCHECK thread please. 

Done.

https://codereview.webrtc.org/1903393004/diff/100001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:587: if (worker_thread_->IsCurrent()) {
On 2016/04/29 07:46:12, perkj_webrtc wrote:
> dito- can we always post regardless if network_thread != worker_thread.

Done.

https://codereview.webrtc.org/1903393004/diff/100001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:1604: if (worker_thread_->IsCurrent()) {
On 2016/04/29 07:46:12, perkj_webrtc wrote:
> dcheck thread.  Which thread is this? 

Done.

[pthatcher1, 2016-04-29 23:36:24]

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:212: network_thread_->Invoke<void>([this] {
It seems risky to have the BaseChannel destructor doing a synchronous call on
the worker thread. 

Is the worker thread even allowed to block on the network thread?  Is the
network thread allowed to block on the worker thread?  If they can both block on
each other, that could lead to deadlock.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:212: network_thread_->Invoke<void>([this] {
I don't think we're allowed to invoke lambdas because then they are stored in a
queue, which is against the chrome style guide, last I checked.  That's done
several times in this file.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:234: if (!SetDtlsSrtpCryptoSuites(transport_channel(),
false)) {
I think SetDtlsSrtpCryptoSuites, rtcp_transport_enabled(), and
SetDtlsSrtpCryptoSuites can only be accessed on the signaling thread, not the
network thread.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:447: new ConnectionMonitor(this, network_thread(),
rtc::Thread::Current()));
We should comment that this is because GetConnectionStats will get called on
this thread, which needs to be the network thread.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:505: return channel ? channel->SetOption(opt, value) : -1;
Only this last part should be on the network thread.  The rest should be on the
signaling thread.  The old code is wrong, it appears, since it calls the last
part on the signaling thread, which isn't safe.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:566: worker_thread_->Post(this, MSG_NETWORK_ROUTE_CHANGED,
message);
Can't we do an AsyncInvoke on the worker_thread_ with
media_channel()->OnNetworkRouteChanged and avoid the message?

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:583: this, ready_to_send ? MSG_READY_TO_SEND :
MSG_NOT_READY_TO_SEND);
Same here.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:605: SendingPacketMessageData* data = new
SendingPacketMessageData;
Why is this "Sending" instead of "Send".

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:796: worker_thread_->Post(this, message_id, message_data);
Same here with AsyncInvoke

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:884: RTC_NOTREACHED();
This isn't reached?

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:1006: if (!ShouldSetupDtlsSrtp()) {
This should be renamed to ShouldSetupDtlsSrtp_n.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:1048: }
I think set_secure_required needs to be on the signaling thread.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channelmanager.h
File webrtc/pc/channelmanager.h (right):

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channelmanager...
webrtc/pc/channelmanager.h:63: return false;
{}s please

[danilchap, 2016-05-02 14:50:34]

Thank you for feedback.
Changed all places where I understand the reason, but there were few (mostly
about signaling thread) where I do not understand reason for a change. How can I
get more details?

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:212: network_thread_->Invoke<void>([this] {
On 2016/04/29 23:36:23, pthatcher1 wrote:
> It seems risky to have the BaseChannel destructor doing a synchronous call on
> the worker thread. 
> 
> Is the worker thread even allowed to block on the network thread?  Is the
> network thread allowed to block on the worker thread?  If they can both block
on
> each other, that could lead to deadlock.

Worker can be blocked on network thread, but network thread can't be blocked on
worker thread. ChannelManager was updated to allow blocking calls on worker
thread and disallow them on network thread.

I assumed this is safe because that how ChannelManager is destructed: it does a
synchronous call on signaling thread to worker thread.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:212: network_thread_->Invoke<void>([this] {
On 2016/04/29 23:36:23, pthatcher1 wrote:
> I don't think we're allowed to invoke lambdas because then they are stored in
a
> queue, which is against the chrome style guide, last I checked.  That's done
> several times in this file.

Wasn't sure if storing in sendlist queue was counted as storing: style reason
against lambdas because of object lifetime mistakes. With synchronous calls I
considered it is not an issue.
Lambdas Updated to rtc::Bind everywhere in this file.

Wonder if in unitests it is better to use Bind for consistency or lambdas for
readability.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:234: if (!SetDtlsSrtpCryptoSuites(transport_channel(),
false)) {
On 2016/04/29 23:36:23, pthatcher1 wrote:
> I think SetDtlsSrtpCryptoSuites, rtcp_transport_enabled(), and
> SetDtlsSrtpCryptoSuites can only be accessed on the signaling thread, not the
> network thread.

Old code access them on worker, not signaling thread.
So it seems legal to move this calls to network thread:
rtcp_transport_enabled set and accessed on network thread only (except
BaseChannel contructor)
SetDtlsSrtpCryptoSuites calls TransportChannel, I assume that is better be done
on network thread. SetDtlsSrtpCryptoSuites also call some static functions that
can be be called on any thread.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:447: new ConnectionMonitor(this, network_thread(),
rtc::Thread::Current()));
On 2016/04/29 23:36:23, pthatcher1 wrote:
> We should comment that this is because GetConnectionStats will get called on
> this thread, which needs to be the network thread.

Done.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:505: return channel ? channel->SetOption(opt, value) : -1;
On 2016/04/29 23:36:23, pthatcher1 wrote:
> Only this last part should be on the network thread.  The rest should be on
the
> signaling thread.  The old code is wrong, it appears, since it calls the last
> part on the signaling thread, which isn't safe.

transport_channels created/destroyed/set on network thread,
socket_options are used to set options on TransportChannel that live on network
thread,
why those two pairs of variables should be accessed on signaling thread?

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:566: worker_thread_->Post(this, MSG_NETWORK_ROUTE_CHANGED,
message);
On 2016/04/29 23:36:23, pthatcher1 wrote:
> Can't we do an AsyncInvoke on the worker_thread_ with
> media_channel()->OnNetworkRouteChanged and avoid the message?

Didn't know there was AsyncInvoke.
Done.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:583: this, ready_to_send ? MSG_READY_TO_SEND :
MSG_NOT_READY_TO_SEND);
On 2016/04/29 23:36:23, pthatcher1 wrote:
> Same here.

Done.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:605: SendingPacketMessageData* data = new
SendingPacketMessageData;
On 2016/04/29 23:36:23, pthatcher1 wrote:
> Why is this "Sending" instead of "Send".

For no good reason. Changed.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:796: worker_thread_->Post(this, message_id, message_data);
On 2016/04/29 23:36:23, pthatcher1 wrote:
> Same here with AsyncInvoke

Done.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:884: RTC_NOTREACHED();
On 2016/04/29 23:36:23, pthatcher1 wrote:
> This isn't reached?

In practice it isn't reached.

In theory it can be reached, but then old code would do a disallowed blocking
call on worker thread.
New code does disallowed blocking call on network thread.

Fixed with AsyncInvoke instead of commenting out.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:1006: if (!ShouldSetupDtlsSrtp()) {
On 2016/04/29 23:36:23, pthatcher1 wrote:
> This should be renamed to ShouldSetupDtlsSrtp_n.

Done.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:1048: }
On 2016/04/29 23:36:23, pthatcher1 wrote:
> I think set_secure_required needs to be on the signaling thread.

Why?
secure_required accessor is used in tests only,
secure_required_ variable used on send and receive paths on network thread.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channelmanager.h
File webrtc/pc/channelmanager.h (right):

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channelmanager...
webrtc/pc/channelmanager.h:63: return false;
On 2016/04/29 23:36:23, pthatcher1 wrote:
> {}s please

Done.

[pthatcher1, 2016-05-11 04:50:02]

lgtm

Please make the few naming changes and "big comment".  I'm willing to review
those more, but I think it's OK if you submit without further review from me.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:212: network_thread_->Invoke<void>([this] {
On 2016/05/02 14:50:34, danilchap wrote:
> On 2016/04/29 23:36:23, pthatcher1 wrote:
> > I don't think we're allowed to invoke lambdas because then they are stored
in
> a
> > queue, which is against the chrome style guide, last I checked.  That's done
> > several times in this file.
> 
> Wasn't sure if storing in sendlist queue was counted as storing: style reason
> against lambdas because of object lifetime mistakes. With synchronous calls I
> considered it is not an issue.

That's a good point.  It's probably worth bringing up to webrtc-eng.  If we
decide that it's OK to make a synchronous call with lambdas, then we should do
that everywhere we make a synchronous call (and be consistent).  On the other
hand, some people want to get rid of all synchronous calls.  Would you like to
bring it up on webrtc-eng?

> Lambdas Updated to rtc::Bind everywhere in this file.
> 
> Wonder if in unitests it is better to use Bind for consistency or lambdas for
> readability.

I think for unit tests, we can be a little more loose and use lambdas.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:212: network_thread_->Invoke<void>([this] {
On 2016/05/02 14:50:34, danilchap wrote:
> On 2016/04/29 23:36:23, pthatcher1 wrote:
> > It seems risky to have the BaseChannel destructor doing a synchronous call
on
> > the worker thread. 
> > 
> > Is the worker thread even allowed to block on the network thread?  Is the
> > network thread allowed to block on the worker thread?  If they can both
block
> on
> > each other, that could lead to deadlock.
> 
> Worker can be blocked on network thread, but network thread can't be blocked
on
> worker thread. ChannelManager was updated to allow blocking calls on worker
> thread and disallow them on 
> 

That's probably the only thing that makes sense.  But we should make a big
comment at the top of the file with a big warning about this, explaining what
can block on what and what _w and _n and _s mean.  

> I assumed this is safe because that how ChannelManager is destructed: it does
a
> synchronous call on signaling thread to worker thread.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:234: if (!SetDtlsSrtpCryptoSuites(transport_channel(),
false)) {
On 2016/05/02 14:50:34, danilchap wrote:
> On 2016/04/29 23:36:23, pthatcher1 wrote:
> > I think SetDtlsSrtpCryptoSuites, rtcp_transport_enabled(), and
> > SetDtlsSrtpCryptoSuites can only be accessed on the signaling thread, not
the
> > network thread.
> 
> Old code access them on worker, not signaling thread.

Ah, true.  I was thrown off by Init().  It should be named Init_w().

> So it seems legal to move this calls to network thread:


> rtcp_transport_enabled set and accessed on network thread only (except
> BaseChannel contructor)
> SetDtlsSrtpCryptoSuites calls TransportChannel, I assume that is better be
done
> on network thread. 

Yes, you are correct.   We should call it SetDtlsSrtpCryptoSuites_n, then. 

SetDtlsSrtpCryptoSuites also call some static functions that
> can be be called on any thread.

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:505: return channel ? channel->SetOption(opt, value) : -1;
On 2016/05/02 14:50:34, danilchap wrote:
> On 2016/04/29 23:36:23, pthatcher1 wrote:
> > Only this last part should be on the network thread.  The rest should be on
> the
> > signaling thread.  The old code is wrong, it appears, since it calls the
last
> > part on the signaling thread, which isn't safe.
> 
> transport_channels created/destroyed/set on network thread,
> socket_options are used to set options on TransportChannel that live on
network
> thread,
> why those two pairs of variables should be accessed on signaling thread?

Ah, that's a good point.  transport_channel_ and rtcp_transport_channel_ should
only be accessed on the network thread.   I think the way you did this is the
right way.

https://codereview.webrtc.org/1903393004/diff/240001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/1903393004/diff/240001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:198: void BaseChannel::Destruct_n() {
I think we could call this something more specific, like DeinitNetwork_n

https://codereview.webrtc.org/1903393004/diff/240001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:224: bool BaseChannel::Init_n() {
And this InitNetwork_n.

https://codereview.webrtc.org/1903393004/diff/240001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:436: // would pull stats with
BaseChannel::GetConnectionStats.
Maybe change "would pull stats with BaseChannel::GetConnectionStats" => "will
call BaseChannel::GetConnectionStats, which must be called on the network
thread"

https://codereview.webrtc.org/1903393004/diff/240001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:802: // require non-const pointer to buffer. This doesn't
memcpy the actual data.
require => requires

https://codereview.webrtc.org/1903393004/diff/240001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/1903393004/diff/240001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:325: TransportChannel* transport_channel_;
Can you leave a comment explaining that these should only be accessed on the
network thread?

[danilchap, 2016-05-11 12:19:15]

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

[danilchap, 2016-05-11 12:19:16]

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:212: network_thread_->Invoke<void>([this] {
On 2016/05/11 04:50:01, pthatcher1 wrote:
> On 2016/05/02 14:50:34, danilchap wrote:
> > On 2016/04/29 23:36:23, pthatcher1 wrote:
> > > It seems risky to have the BaseChannel destructor doing a synchronous call
> on
> > > the worker thread. 
> > > 
> > > Is the worker thread even allowed to block on the network thread?  Is the
> > > network thread allowed to block on the worker thread?  If they can both
> block
> > on
> > > each other, that could lead to deadlock.
> > 
> > Worker can be blocked on network thread, but network thread can't be blocked
> on
> > worker thread. ChannelManager was updated to allow blocking calls on worker
> > thread and disallow them on 
> > 
> 
> That's probably the only thing that makes sense.  But we should make a big
> comment at the top of the file with a big warning about this, explaining what
> can block on what and what _w and _n and _s mean.  
> 
> > I assumed this is safe because that how ChannelManager is destructed: it
does
> a
> > synchronous call on signaling thread to worker thread.
> 
> 
Expanded comment above BaseChannel definition

https://codereview.webrtc.org/1903393004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:234: if (!SetDtlsSrtpCryptoSuites(transport_channel(),
false)) {
On 2016/05/11 04:50:01, pthatcher1 wrote:
> On 2016/05/02 14:50:34, danilchap wrote:
> > On 2016/04/29 23:36:23, pthatcher1 wrote:
> > > I think SetDtlsSrtpCryptoSuites, rtcp_transport_enabled(), and
> > > SetDtlsSrtpCryptoSuites can only be accessed on the signaling thread, not
> the
> > > network thread.
> > 
> > Old code access them on worker, not signaling thread.
> 
> Ah, true.  I was thrown off by Init().  It should be named Init_w().
> 
> > So it seems legal to move this calls to network thread:
> 
> 
> > rtcp_transport_enabled set and accessed on network thread only (except
> > BaseChannel contructor)
> > SetDtlsSrtpCryptoSuites calls TransportChannel, I assume that is better be
> done
> > on network thread. 
> 
> Yes, you are correct.   We should call it SetDtlsSrtpCryptoSuites_n, then. 
> 
> SetDtlsSrtpCryptoSuites also call some static functions that
> > can be be called on any thread.
> 

Init renamed to Init_w, few more function got thread suffixes too.
Discovered few member accesses on wrong threads, added Invokes.

https://codereview.webrtc.org/1903393004/diff/240001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/1903393004/diff/240001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:198: void BaseChannel::Destruct_n() {
On 2016/05/11 04:50:01, pthatcher1 wrote:
> I think we could call this something more specific, like DeinitNetwork_n

Done.

https://codereview.webrtc.org/1903393004/diff/240001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:224: bool BaseChannel::Init_n() {
On 2016/05/11 04:50:01, pthatcher1 wrote:
> And this InitNetwork_n.

Done.

https://codereview.webrtc.org/1903393004/diff/240001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:436: // would pull stats with
BaseChannel::GetConnectionStats.
On 2016/05/11 04:50:01, pthatcher1 wrote:
> Maybe change "would pull stats with BaseChannel::GetConnectionStats" => "will
> call BaseChannel::GetConnectionStats, which must be called on the network
> thread"

Done.

https://codereview.webrtc.org/1903393004/diff/240001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:802: // require non-const pointer to buffer. This doesn't
memcpy the actual data.
On 2016/05/11 04:50:01, pthatcher1 wrote:
> require => requires

Done.

https://codereview.webrtc.org/1903393004/diff/240001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/1903393004/diff/240001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:325: TransportChannel* transport_channel_;
On 2016/05/11 04:50:01, pthatcher1 wrote:
> Can you leave a comment explaining that these should only be accessed on the
> network thread? 

reorgonized members and separated two groups: one to be access by network
thread, another - by worker thread (with a small commented exception)

[stefan-webrtc, 2016-05-11 13:10:26]

https://codereview.webrtc.org/1903393004/diff/260001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/1903393004/diff/260001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:54: // BaseChannel assumes signaling and other threads
allowed to make synchronius
"...are allowed to make synchronous calls to the worker thread, the worker
thread makes synchronous calls only to the network thread, and the network
thread can't be blocked by other threads."

[danilchap, 2016-05-11 14:15:30]

https://codereview.webrtc.org/1903393004/diff/260001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/1903393004/diff/260001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:54: // BaseChannel assumes signaling and other threads
allowed to make synchronius
On 2016/05/11 13:10:26, stefan-webrtc (holmer) wrote:
> "...are allowed to make synchronous calls to the worker thread, the worker
> thread makes synchronous calls only to the network thread, and the network
> thread can't be blocked by other threads."

Thank you!

[danilchap, 2016-05-11 17:55:46]

Description was changed from

==========
Adds network thread to rtc::BaseChannel
BaseChannel do calls to transport_channel on network_thread,
while keep calls to media_engine on worker_thread.
It still works when network_thread == worker_thread.

BUG=webrtc:5645
==========

to

==========
Adds network thread to rtc::BaseChannel
BaseChannel do calls to transport_channel on network_thread,
while keep calls to media_engine on worker_thread.
It still works when network_thread == worker_thread.

BUG=webrtc:5645
R=pthatcher@webrtc.org

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

[commit-bot: I haz the power, 2016-05-11 17:55:46]

Description was changed from

==========
Adds network thread to rtc::BaseChannel
BaseChannel do calls to transport_channel on network_thread,
while keep calls to media_engine on worker_thread.
It still works when network_thread == worker_thread.

BUG=webrtc:5645
==========

to

==========
Adds network thread to rtc::BaseChannel
BaseChannel do calls to transport_channel on network_thread,
while keep calls to media_engine on worker_thread.
It still works when network_thread == worker_thread.

BUG=webrtc:5645
R=pthatcher@webrtc.org

Committed: https://crrev.com/33b01f2162af32457e0a6a6e0eb670254701df61
Cr-Commit-Position: refs/heads/master@{#12690}
==========

[danilchap, 2016-05-11 17:55:47]

Committed patchset #17 (id:320001) manually as
33b01f2162af32457e0a6a6e0eb670254701df61 (presubmit successful).

[commit-bot: I haz the power, 2016-05-11 17:55:48]

Patchset 17 (id:??) landed as
https://crrev.com/33b01f2162af32457e0a6a6e0eb670254701df61
Cr-Commit-Position: refs/heads/master@{#12690}

[juberti2, 2016-05-12 03:21:45]

Description was changed from

==========
Adds network thread to rtc::BaseChannel
BaseChannel do calls to transport_channel on network_thread,
while keep calls to media_engine on worker_thread.
It still works when network_thread == worker_thread.

BUG=webrtc:5645
R=pthatcher@webrtc.org

Committed: https://crrev.com/33b01f2162af32457e0a6a6e0eb670254701df61
Cr-Commit-Position: refs/heads/master@{#12690}
==========

to

==========
Adds network thread to rtc::BaseChannel
BaseChannel do calls to transport_channel on network_thread,
while keep calls to media_engine on worker_thread.
It still works when network_thread == worker_thread.

BUG=webrtc:5645
R=pthatcher@webrtc.org

Committed: https://crrev.com/33b01f2162af32457e0a6a6e0eb670254701df61
Cr-Commit-Position: refs/heads/master@{#12690}
==========