Remove BaseChannel's dependency on TransportController.

Remove BaseChannel's dependency on TransportController.

The BaseChannel can set the transport directly without depending on
TransportController.

When initializing the network of the BaseChannel, the ChannelManager will
create TransportChannels with the TransportController.
When enabling bundling, WebRtcSession will get or create TransportChannels
with the TransportController.

When a TransportChannel of the BaseChannel needs to be destroyed, it will
fire a signal to notify the WebRtcSession.

BUG=none.

Review-Url: https://codereview.webrtc.org/2614263002
Cr-Commit-Position: refs/heads/master@{#16043}
Committed: https://chromium.googlesource.com/external/webrtc/+/f5b251b816ca9ad7579321a4d94535c623618dbb

[Zhi Huang, 2017-01-06 23:05:31]

Description was changed from

==========
Remove the TransportController from BaseChannel.

The BaseChannel can set transport directly without depending on
TransportController.

When initilizing the network of the BaseChannel, the ChannelManager will
create TransportChannels with the TransportController.
When enabling the max bundle, WebRtcSession will get or create TransportChannels
with the TransportController.

When a TransportChannel of the BaseChannel  needs to be destoryed,it will
fire a signal to notify the WebRtcSession.

BUG=none.
==========

to

==========
Remove the TransportController from BaseChannel.

The BaseChannel can set transport directly without depending on
TransportController.

When initilizing the network of the BaseChannel, the ChannelManager will
create TransportChannels with the TransportController.
When enabling the max bundle, WebRtcSession will get or create TransportChannels
with the TransportController.

When a TransportChannel of the BaseChannel  needs to be destoryed,it will
fire a signal to notify the WebRtcSession.

BUG=none.
==========

[Zhi Huang, 2017-01-06 23:17:50]

Patchset #1 (id:1) has been deleted

[Zhi Huang, 2017-01-06 23:18:06]

The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 23:18:09]

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

[commit-bot: I haz the power, 2017-01-06 23:23:39]

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

[commit-bot: I haz the power, 2017-01-06 23:23:39]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)

[Zhi Huang, 2017-01-06 23:41:30]

The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 23:41:34]

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

[commit-bot: I haz the power, 2017-01-07 00:54:08]

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

[commit-bot: I haz the power, 2017-01-07 00:54:09]

Dry run: This issue passed the CQ dry run.

[Taylor Brandstetter, 2017-01-09 22:50:41]

deadbeef@webrtc.org changed reviewers:
+ deadbeef@webrtc.org

[Taylor Brandstetter, 2017-01-09 22:50:43]

Looks good so far. I'm just wondering if we could go a couple steps further,
moving the initial channel creation and some of the deletion logic to
WebRtcSession. We could also reduce some thread hopping while we're at it
(though that's more a nice-to-have).

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1072: if (!ch->SetTransport(rtp_transport,
rtcp_transport)) {
I think there's an opportunity here to reduce some thread hops. Currently each
"SetTransport" will hop to the network thread, but instead, we could invoke
"EnableBundle" on the network thread.

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1681: this, &WebRtcSession::OnDestroyTransport);
It would be nice if the creation *and* destruction were both handled in
WebRtcSession. Would that be feasible? The cases where DestroyTransportChannel
should be called are:

* When the BaseChannel is destroyed
* When SetTransport is called
* When ActivateRtcpMux is called

This sounds reasonable to handle here, and that would avoid needing to have
"SignalDestroyTransport".

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportcontroller.h (right):

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportcontroller.h:116: TransportChannel*
CreateTransportChannel(const std::string& transport_name,
If EnableBundle were called on the networking thread, we could avoid the
signaling thread versions of these methods.

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:286: LOG(LS_WARNING) << "Setting nullptr to RTP Transport
and RTCP Transport.";
It seems like this should never happen. Change log level to error and return
false?

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:292: rtcp_transport->transport_name());
Could also RTC_DCHECK that ShouldCreateRtcpTransport returns true.

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:313: LOG(LS_INFO) << "Create RTCP Transport for " <<
content_name() << " on "
Should update log statements; is now "setting" instead of "creating"

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:477: bool BaseChannel::ShouldCreateRtcpTransport() {
nit: Would "NeedsRtcpTransport" be a more fitting name, since it isn't actually
created here? Just a thought.

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/pc/channelmanager.cc
File webrtc/pc/channelmanager.cc (right):

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/pc/channelmanager....
webrtc/pc/channelmanager.cc:261: : nullptr;
Could this code be moved to WebRtcSession? That would make WebRtcSession
responsible for all calls to "CreateTransportChannel", and would allow the
"transport_controller" argument to be removed from these
"Create[Voice/Video/Data]Channel" methods.

[Zhi Huang, 2017-01-12 00:16:14]

Patchset #3 (id:60001) has been deleted

[Zhi Huang, 2017-01-12 00:16:21]

Patchset #3 (id:80001) has been deleted

[Zhi Huang, 2017-01-12 00:17:48]

Patchset #3 (id:100001) has been deleted

[Zhi Huang, 2017-01-12 00:28:38]

Patchset #4 (id:140001) has been deleted

[Zhi Huang, 2017-01-12 00:28:50]

Patchset #3 (id:120001) has been deleted

[Zhi Huang, 2017-01-12 00:46:39]

Patchset #3 (id:160001) has been deleted

[Zhi Huang, 2017-01-12 01:53:13]

Patchset #3 (id:180001) has been deleted

[Zhi Huang, 2017-01-12 03:47:47]

Please take another look.
Thanks.

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1681: this, &WebRtcSession::OnDestroyTransport);
On 2017/01/09 22:50:42, Taylor Brandstetter wrote:
> It would be nice if the creation *and* destruction were both handled in
> WebRtcSession. Would that be feasible? The cases where DestroyTransportChannel
> should be called are:
> 
> * When the BaseChannel is destroyed
> * When SetTransport is called
> * When ActivateRtcpMux is called
> 
> This sounds reasonable to handle here, and that would avoid needing to have
> "SignalDestroyTransport".

Done.

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportcontroller.h (right):

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportcontroller.h:116: TransportChannel*
CreateTransportChannel(const std::string& transport_name,
On 2017/01/09 22:50:42, Taylor Brandstetter wrote:
> If EnableBundle were called on the networking thread, we could avoid the
> signaling thread versions of these methods.

This method could be called on other threads in WebRtcSession.

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:286: LOG(LS_WARNING) << "Setting nullptr to RTP Transport
and RTCP Transport.";
On 2017/01/09 22:50:42, Taylor Brandstetter wrote:
> It seems like this should never happen. Change log level to error and return
> false?

Done.

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:292: rtcp_transport->transport_name());
On 2017/01/09 22:50:42, Taylor Brandstetter wrote:
> Could also RTC_DCHECK that ShouldCreateRtcpTransport returns true.

Done.

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:313: LOG(LS_INFO) << "Create RTCP Transport for " <<
content_name() << " on "
On 2017/01/09 22:50:42, Taylor Brandstetter wrote:
> Should update log statements; is now "setting" instead of "creating"

Done.

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:477: bool BaseChannel::ShouldCreateRtcpTransport() {
On 2017/01/09 22:50:42, Taylor Brandstetter wrote:
> nit: Would "NeedsRtcpTransport" be a more fitting name, since it isn't
actually
> created here? Just a thought.

Done.

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/pc/channelmanager.cc
File webrtc/pc/channelmanager.cc (right):

https://codereview.webrtc.org/2614263002/diff/40001/webrtc/pc/channelmanager....
webrtc/pc/channelmanager.cc:261: : nullptr;
On 2017/01/09 22:50:42, Taylor Brandstetter wrote:
> Could this code be moved to WebRtcSession? That would make WebRtcSession
> responsible for all calls to "CreateTransportChannel", and would allow the
> "transport_controller" argument to be removed from these
> "Create[Voice/Video/Data]Channel" methods.

Done.

[Taylor Brandstetter, 2017-01-12 22:39:02]

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:2404: DestroyTransport(transport_name,
cricket::ICE_CANDIDATE_COMPONENT_RTCP);
Do you need to do the "need_to_delete_rtcp" thing here as well?

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/p2p/base/faketran...
File webrtc/p2p/base/faketransportcontroller.h (right):

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/p2p/base/faketran...
webrtc/p2p/base/faketransportcontroller.h:446: void OnDestroyRtcpTransport(const
std::string& transport_name) {
Can this slot either be moved to the test where it's used, or remove the "On"
from the name? Usually, methods named like "OnFoo" are private.

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:170: signaling_thread_(signaling_thread),
It doesn't look like this is used anywhere

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:196: LOG(LS_INFO) << "Destroyed channel:" << content_name_;
nit: space after colon

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:217: void BaseChannel::DestroyTransportChannels_n() {
This can be removed now. Deinit already calls DisconnectTransportChannels_n,
which does this.

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:164: // Emitted whenever the rtcp-mux is active and the
rtcp-transport needs to be
"needs to be destroyed" -> "can be destroyed"; another channel may still be
using the RTCP transports, so it's not always destroyed when this is fired.

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:171: bool NeedsRtcpTransport();
This can be private; the use in "EnableBundle" can be replaced by just
"rtcp_transport() != nullptr"

[Zhi Huang, 2017-01-13 00:12:47]

Please take anther look. Thanks.

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:2404: DestroyTransport(transport_name,
cricket::ICE_CANDIDATE_COMPONENT_RTCP);
On 2017/01/12 22:39:02, Taylor Brandstetter wrote:
> Do you need to do the "need_to_delete_rtcp" thing here as well?

Yes. This is needed.

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/p2p/base/faketran...
File webrtc/p2p/base/faketransportcontroller.h (right):

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/p2p/base/faketran...
webrtc/p2p/base/faketransportcontroller.h:446: void OnDestroyRtcpTransport(const
std::string& transport_name) {
On 2017/01/12 22:39:02, Taylor Brandstetter wrote:
> Can this slot either be moved to the test where it's used, or remove the "On"
> from the name? Usually, methods named like "OnFoo" are private.

I'll go with the simpler one.

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:170: signaling_thread_(signaling_thread),
On 2017/01/12 22:39:02, Taylor Brandstetter wrote:
> It doesn't look like this is used anywhere

"signaling_thread()" used to return 

"transport_controller_->signaling_thread()"

now it will return "signaling_thread_"

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:196: LOG(LS_INFO) << "Destroyed channel:" << content_name_;
On 2017/01/12 22:39:02, Taylor Brandstetter wrote:
> nit: space after colon

Done.

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:217: void BaseChannel::DestroyTransportChannels_n() {
On 2017/01/12 22:39:02, Taylor Brandstetter wrote:
> This can be removed now. Deinit already calls DisconnectTransportChannels_n,
> which does this.

You are right.

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:164: // Emitted whenever the rtcp-mux is active and the
rtcp-transport needs to be
On 2017/01/12 22:39:02, Taylor Brandstetter wrote:
> "needs to be destroyed" -> "can be destroyed"; another channel may still be
> using the RTCP transports, so it's not always destroyed when this is fired.

Done.

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:171: bool NeedsRtcpTransport();
On 2017/01/12 22:39:02, Taylor Brandstetter wrote:
> This can be private; the use in "EnableBundle" can be replaced by just
> "rtcp_transport() != nullptr"

I think the channel_unittests.cc will need to know if rtcp transport are needed
when creating channels.

[Taylor Brandstetter, 2017-01-13 01:37:26]

Description was changed from

==========
Remove the TransportController from BaseChannel.

The BaseChannel can set transport directly without depending on
TransportController.

When initilizing the network of the BaseChannel, the ChannelManager will
create TransportChannels with the TransportController.
When enabling the max bundle, WebRtcSession will get or create TransportChannels
with the TransportController.

When a TransportChannel of the BaseChannel  needs to be destoryed,it will
fire a signal to notify the WebRtcSession.

BUG=none.
==========

to

==========
Remove the TransportController from BaseChannel.

The BaseChannel can set transport directly without depending on
TransportController.

When initializing the network of the BaseChannel, the ChannelManager will
create TransportChannels with the TransportController.
When enabling bundling, WebRtcSession will get or create TransportChannels
with the TransportController.

When a TransportChannel of the BaseChannel needs to be destroyed, it will
fire a signal to notify the WebRtcSession.

BUG=none.
==========

[Taylor Brandstetter, 2017-01-13 01:37:46]

Description was changed from

==========
Remove the TransportController from BaseChannel.

The BaseChannel can set transport directly without depending on
TransportController.

When initializing the network of the BaseChannel, the ChannelManager will
create TransportChannels with the TransportController.
When enabling bundling, WebRtcSession will get or create TransportChannels
with the TransportController.

When a TransportChannel of the BaseChannel needs to be destroyed, it will
fire a signal to notify the WebRtcSession.

BUG=none.
==========

to

==========
Remove BaseChannel's dependency on TransportController.

The BaseChannel can set transport directly without depending on
TransportController.

When initializing the network of the BaseChannel, the ChannelManager will
create TransportChannels with the TransportController.
When enabling bundling, WebRtcSession will get or create TransportChannels
with the TransportController.

When a TransportChannel of the BaseChannel needs to be destroyed, it will
fire a signal to notify the WebRtcSession.

BUG=none.
==========

[Taylor Brandstetter, 2017-01-13 01:37:57]

Description was changed from

==========
Remove BaseChannel's dependency on TransportController.

The BaseChannel can set transport directly without depending on
TransportController.

When initializing the network of the BaseChannel, the ChannelManager will
create TransportChannels with the TransportController.
When enabling bundling, WebRtcSession will get or create TransportChannels
with the TransportController.

When a TransportChannel of the BaseChannel needs to be destroyed, it will
fire a signal to notify the WebRtcSession.

BUG=none.
==========

to

==========
Remove BaseChannel's dependency on TransportController.

The BaseChannel can set the transport directly without depending on
TransportController.

When initializing the network of the BaseChannel, the ChannelManager will
create TransportChannels with the TransportController.
When enabling bundling, WebRtcSession will get or create TransportChannels
with the TransportController.

When a TransportChannel of the BaseChannel needs to be destroyed, it will
fire a signal to notify the WebRtcSession.

BUG=none.
==========

[Taylor Brandstetter, 2017-01-13 01:39:49]

lgtm. I also modified the CL description a bit.

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:170: signaling_thread_(signaling_thread),
On 2017/01/13 00:12:47, Zhi Huang wrote:
> On 2017/01/12 22:39:02, Taylor Brandstetter wrote:
> > It doesn't look like this is used anywhere
> 
> "signaling_thread()" used to return 
> 
> "transport_controller_->signaling_thread()"
> 
> now it will return "signaling_thread_"

Oh, I didn't realize it was used in the header. My bad.

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2614263002/diff/200001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:171: bool NeedsRtcpTransport();
On 2017/01/13 00:12:47, Zhi Huang wrote:
> On 2017/01/12 22:39:02, Taylor Brandstetter wrote:
> > This can be private; the use in "EnableBundle" can be replaced by just
> > "rtcp_transport() != nullptr"
> 
> I think the channel_unittests.cc will need to know if rtcp transport are
needed
> when creating channels.

Acknowledged.

[Taylor Brandstetter, 2017-01-13 01:41:21]

One last small thing I noticed

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:165: // destroyed.
Would be helpful to comment that this is fired on the network thread.

[Taylor Brandstetter, 2017-01-13 01:42:01]

The CQ bit was checked by deadbeef@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 01:42:09]

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

[commit-bot: I haz the power, 2017-01-13 02:32:29]

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

[commit-bot: I haz the power, 2017-01-13 02:32:30]

Dry run: This issue passed the CQ dry run.

[Taylor Brandstetter, 2017-01-13 03:34:56]

I'm going to go ahead and try to land this, since I have a CL that would benefit
from it.

[Taylor Brandstetter, 2017-01-13 03:35:03]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2017-01-13 03:35:10]

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

[commit-bot: I haz the power, 2017-01-13 03:37:49]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1484278503211840,
"parent_rev": "ac1f348f53f9b2d2033cd96074d15d7bcd3f28c5", "commit_rev":
"f5b251b816ca9ad7579321a4d94535c623618dbb"}

[commit-bot: I haz the power, 2017-01-13 03:37:53]

Description was changed from

==========
Remove BaseChannel's dependency on TransportController.

The BaseChannel can set the transport directly without depending on
TransportController.

When initializing the network of the BaseChannel, the ChannelManager will
create TransportChannels with the TransportController.
When enabling bundling, WebRtcSession will get or create TransportChannels
with the TransportController.

When a TransportChannel of the BaseChannel needs to be destroyed, it will
fire a signal to notify the WebRtcSession.

BUG=none.
==========

to

==========
Remove BaseChannel's dependency on TransportController.

The BaseChannel can set the transport directly without depending on
TransportController.

When initializing the network of the BaseChannel, the ChannelManager will
create TransportChannels with the TransportController.
When enabling bundling, WebRtcSession will get or create TransportChannels
with the TransportController.

When a TransportChannel of the BaseChannel needs to be destroyed, it will
fire a signal to notify the WebRtcSession.

BUG=none.

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

[commit-bot: I haz the power, 2017-01-13 03:37:54]

Committed patchset #4 (id:220001) as
https://chromium.googlesource.com/external/webrtc/+/f5b251b816ca9ad7579321a4d...

[Zhi Huang, 2017-01-13 03:45:06]

On 2017/01/13 03:34:56, Taylor Brandstetter wrote:
> I'm going to go ahead and try to land this, since I have a CL that would
benefit
> from it.

Alright, go ahead. Thanks.

[pthatcher1, 2017-01-13 21:51:06]

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

[pthatcher1, 2017-01-13 21:51:07]

Sorry for being late to the review, but I think we could have made this quite a
bit cleaner.  See my idea.

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/api/rtcstatscolle...
File webrtc/api/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector_unittest.cc:599: test_->worker_thread(),
test_->network_thread(), nullptr,
I'd prefer we define a signaling thread for all of these rather than making it
null.  We don't use the signaling thread right now in these tests, but it's
risky to assume that passing nullptr here is safe, when generally it isn't.   
Same for all the tests.

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:1097: return false;
If it failed, do you need to destroy the new transports?

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:1810: return false;
If the constructor failed, do we have to destroy the transports?

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:1814: this,
&WebRtcSession::OnDestroyRtcpTransport_n);
This is funky.  I think it would be better to pass the BaseChannel something it
can call rather than have a callback like this.  See my
RtpTransportPairInterface idea below.

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:1844: }
To make it easier to construct and destroy and pass around, would it make sense
to do something like this?

class RtpTransportPairInterface {
  virtual ~RtpTransportPairInterface();
  virtual TransportChannel* rtp_transport() = 0;
  virtual TransportChannel* rtcp_transport() = 0;
  virtual void DestroyRtcpTransport();
}

class WebRtcSession::RtpTransportPair : RtpTransportPairInterface {
  RtpTransportPair(TransportController* troller, const std::string&
transport_name, create_rtcp) : transport_controller_(troller),
transport_name_(transport_name) {
    rtp_transport_ =
transport_controller_->CreateTransportChannel(transport_name_,
cricket::ICE_CANDIDATE_COMPONENT_RTP);
    if(create_rtcp) {
      rtcp_transport_ =
transport_controller_->CreateTransportChannel(transport_name_,
cricket::ICE_CANDIDATE_COMPONENT_RTCP);
    }    
  }

  ~RtpTransportPair() { 
    transport_controller_->DestroyTransportChannel(transport_name_,
cricket::ICE_CANDIDATE_COMPONENT_RTP);
    if(rtcp_transport_) {
      transport_controller_->DestroyTransportChannel(transport_name_,
cricket::ICE_CANDIDATE_COMPONENT_RTCP);
    }
  }

  TransportChannel* rtp_transport() override {
    return rtp_transport_;
  }
  TransportChannel* rtcp_transport() override {
    return rtcp_transport_;
  }

  void DestroyRtcpTransport() override {
  }

  TransportController* transport_controller_ = nullptr;
  std::string transport_name_;
  TransportChannel* rtp_transport_ = nullptr;
  TransportChannel* rtcp_transport_ = nullptr;
}

std::unique_ptr<RtpTransportPairInterface> transports(new
WebRtcSession::RtpTransportPair(transport_controller_, transport_name));
video_channel_.reset(channel_manager->CreateVideoChannel(media_controller_,
std::move(transport)));


Now:
1.  You only have to pass in one thing, not two.
2.  That thing is clearly owned by BaseChannel, and once deleted cleans up the
transports.
3.  If video_channel_ fails to be created, the transports are automatically
deleted.
4.  There's no code duplication.
5.  You don't have this weird "SignalDestroyRtcpTransport".  The BaseChannel can
just call that on the RtpTransportPairInterface that it owns.

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:2389: transport_controller_.get(), transport_name,
component));
You added code to TransportController to do this thread hop.  You might as well
add DestroyTransportChannel as well.  Then they are both in the same place.  Or
remove both and pass the network_thread_ into the RtpTransportPair and let it do
the thread hop.

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:2435: }
I think most of this goes away if the BaseChannel just deletes the thing you
passed it, and that deletes the transports (such as my RtpTransporPair idea).

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:257: TransportChannel* rtcp_transport) {
Shouldn't this be called SetTransports?

[Taylor Brandstetter, 2017-01-13 23:46:48]

Addressed your concerns in https://codereview.webrtc.org/2637503003

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/api/rtcstatscolle...
File webrtc/api/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector_unittest.cc:599: test_->worker_thread(),
test_->network_thread(), nullptr,
On 2017/01/13 21:51:07, pthatcher1 wrote:
> I'd prefer we define a signaling thread for all of these rather than making it
> null.  We don't use the signaling thread right now in these tests, but it's
> risky to assume that passing nullptr here is safe, when generally it isn't.   
> Same for all the tests.

Fixed in new CL

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:1097: return false;
On 2017/01/13 21:51:07, pthatcher1 wrote:
> If it failed, do you need to destroy the new transports?  

It won't ever fail unless invalid arguments are supplied. I'll change it from a
method that returns false to one that just DCHECKs.

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:1810: return false;
On 2017/01/13 21:51:07, pthatcher1 wrote:
> If the constructor failed, do we have to destroy the transports?

Done in follow up CL.

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:1814: this,
&WebRtcSession::OnDestroyRtcpTransport_n);
On 2017/01/13 21:51:07, pthatcher1 wrote:
> This is funky.  I think it would be better to pass the BaseChannel something
it
> can call rather than have a callback like this.  See my
> RtpTransportPairInterface idea below.

I think a signal is the right approach here for minimizing interdependencies.
But I changed the signal name to "SignalRtcpMuxFullyActive", which makes this
code make more sense.

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:1844: }
On 2017/01/13 21:51:07, pthatcher1 wrote:
> To make it easier to construct and destroy and pass around, would it make
sense
> to do something like this?
> 
> class RtpTransportPairInterface {
>   virtual ~RtpTransportPairInterface();
>   virtual TransportChannel* rtp_transport() = 0;
>   virtual TransportChannel* rtcp_transport() = 0;
>   virtual void DestroyRtcpTransport();
> }
> 
> class WebRtcSession::RtpTransportPair : RtpTransportPairInterface {
>   RtpTransportPair(TransportController* troller, const std::string&
> transport_name, create_rtcp) : transport_controller_(troller),
> transport_name_(transport_name) {
>     rtp_transport_ =
> transport_controller_->CreateTransportChannel(transport_name_,
> cricket::ICE_CANDIDATE_COMPONENT_RTP);
>     if(create_rtcp) {
>       rtcp_transport_ =
> transport_controller_->CreateTransportChannel(transport_name_,
> cricket::ICE_CANDIDATE_COMPONENT_RTCP);
>     }    
>   }
> 
>   ~RtpTransportPair() { 
>     transport_controller_->DestroyTransportChannel(transport_name_,
> cricket::ICE_CANDIDATE_COMPONENT_RTP);
>     if(rtcp_transport_) {
>       transport_controller_->DestroyTransportChannel(transport_name_,
> cricket::ICE_CANDIDATE_COMPONENT_RTCP);
>     }
>   }
> 
>   TransportChannel* rtp_transport() override {
>     return rtp_transport_;
>   }
>   TransportChannel* rtcp_transport() override {
>     return rtcp_transport_;
>   }
> 
>   void DestroyRtcpTransport() override {
>   }
> 
>   TransportController* transport_controller_ = nullptr;
>   std::string transport_name_;
>   TransportChannel* rtp_transport_ = nullptr;
>   TransportChannel* rtcp_transport_ = nullptr;
> }
> 
> std::unique_ptr<RtpTransportPairInterface> transports(new
> WebRtcSession::RtpTransportPair(transport_controller_, transport_name));
> video_channel_.reset(channel_manager->CreateVideoChannel(media_controller_,
> std::move(transport)));
> 
> 
> Now:
> 1.  You only have to pass in one thing, not two.
> 2.  That thing is clearly owned by BaseChannel, and once deleted cleans up the
> transports.
> 3.  If video_channel_ fails to be created, the transports are automatically
> deleted.
> 4.  There's no code duplication.
> 5.  You don't have this weird "SignalDestroyRtcpTransport".  The BaseChannel
can
> just call that on the RtpTransportPairInterface that it owns.

This is something that would be handled elegantly when BaseChannel is refactored
into RtpTransceiver/RtpTransport, but for now I thought we just wanted to do the
minimum changes that will let us implement the shim objects.

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:2389: transport_controller_.get(), transport_name,
component));
On 2017/01/13 21:51:07, pthatcher1 wrote:
> You added code to TransportController to do this thread hop.  You might as
well
> add DestroyTransportChannel as well.  Then they are both in the same place. 
Or
> remove both and pass the network_thread_ into the RtpTransportPair and let it
do
> the thread hop. 

I think this was an artifact of changing things back and forth, I'll make it
consistent with CreateTransportChannel.

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2614263002/diff/220001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:257: TransportChannel* rtcp_transport) {
On 2017/01/13 21:51:07, pthatcher1 wrote:
> Shouldn't this be called SetTransports?

Sure, done in follow-up CL.