Move ready to send logic from BaseChannel to RtpTransport.

Move ready to send logic from BaseChannel to RtpTransport.

BUG=webrtc:7013
Review-Url: https://codereview.webrtc.org/2812243005
Cr-Commit-Position: refs/heads/master@{#17853}
Committed: https://chromium.googlesource.com/external/webrtc/+/56162b9f675d6b55ebb7e7d682ea54a2a2e7e672

[Zach Stein, 2017-04-13 18:24:48]

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

[Zach Stein, 2017-04-13 18:24:49]

PTAL

[Taylor Brandstetter, 2017-04-14 18:05:33]

The additions of "Connect", "Disconnect" and "SetReadyToSend" seem to be making
RtpTransport and BaseChannel more tightly coupled. I think we can avoid this by
moving a few more responsibilities to RtpTransport; see comments.

https://codereview.webrtc.org/2812243005/diff/1/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2812243005/diff/1/webrtc/pc/channel.cc#newcode385
webrtc/pc/channel.cc:385: rtp_transport_.Disconnect(rtcp);
Could the "disconnect"/"connect" happen implicitly in
"set_rt[c]p_packet_transport"? If you do this, they should be changed to use
camel case naming, since they'd no longer be trivial setters.

https://codereview.webrtc.org/2812243005/diff/1/webrtc/pc/channel.cc#newcode667
webrtc/pc/channel.cc:667: void BaseChannel::SetTransportChannelReadyToSend(bool
rtcp, bool ready) {
Can this method be removed, and the necessary logic moved to RtpTransport?
Currently this is called either:

1. When the transport channel changes
2. When sending a packet fails

RtpTransport already knows about #1. So you'd just need to move the
responsibility of sending packets to RtpTransport to handle #2. Meaning, add
"SendRtpPacket"/"SendRtcpPacket" methods that take an RTP or SRTP packet as
input and just send it on the right transport.

https://codereview.webrtc.org/2812243005/diff/1/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2812243005/diff/1/webrtc/pc/channel.h#newcode266
webrtc/pc/channel.h:266: void OnTransportReadyToSend(bool ready);
Could you add an "// From RtpTransport." comment above, so it's clear what
object this slot is connected to?

https://codereview.webrtc.org/2812243005/diff/1/webrtc/pc/rtptransport.h
File webrtc/pc/rtptransport.h (right):

https://codereview.webrtc.org/2812243005/diff/1/webrtc/pc/rtptransport.h#newc...
webrtc/pc/rtptransport.h:48: void OnReadyToSend(rtc::PacketTransportInternal*
transport);
This should be private.

[Zach Stein, 2017-04-18 23:39:29]

I think I have addressed everything, except for updating the newly introduced
rtcp_mux_enabled_. The existing code checks if rtcp_packet_transport is null,
but I don't think that is safe generally. The open question for me is whether
BaseChannel should call rtp_transport_.set_rtcp_mux_enabled or if RtcpMuxFilter
should signal changes. Let me know what you think!

https://codereview.webrtc.org/2812243005/diff/1/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2812243005/diff/1/webrtc/pc/channel.cc#newcode385
webrtc/pc/channel.cc:385: rtp_transport_.Disconnect(rtcp);
On 2017/04/14 18:05:33, Taylor Brandstetter wrote:
> Could the "disconnect"/"connect" happen implicitly in
> "set_rt[c]p_packet_transport"? If you do this, they should be changed to use
> camel case naming, since they'd no longer be trivial setters.

Done.

https://codereview.webrtc.org/2812243005/diff/1/webrtc/pc/channel.cc#newcode667
webrtc/pc/channel.cc:667: void BaseChannel::SetTransportChannelReadyToSend(bool
rtcp, bool ready) {
On 2017/04/14 18:05:33, Taylor Brandstetter wrote:
> Can this method be removed, and the necessary logic moved to RtpTransport?
> Currently this is called either:
> 
> 1. When the transport channel changes
> 2. When sending a packet fails
> 
> RtpTransport already knows about #1. So you'd just need to move the
> responsibility of sending packets to RtpTransport to handle #2. Meaning, add
> "SendRtpPacket"/"SendRtcpPacket" methods that take an RTP or SRTP packet as
> input and just send it on the right transport.

Done.

https://codereview.webrtc.org/2812243005/diff/1/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2812243005/diff/1/webrtc/pc/channel.h#newcode266
webrtc/pc/channel.h:266: void OnTransportReadyToSend(bool ready);
On 2017/04/14 18:05:33, Taylor Brandstetter wrote:
> Could you add an "// From RtpTransport." comment above, so it's clear what
> object this slot is connected to?

Done.

https://codereview.webrtc.org/2812243005/diff/1/webrtc/pc/rtptransport.h
File webrtc/pc/rtptransport.h (right):

https://codereview.webrtc.org/2812243005/diff/1/webrtc/pc/rtptransport.h#newc...
webrtc/pc/rtptransport.h:48: void OnReadyToSend(rtc::PacketTransportInternal*
transport);
On 2017/04/14 18:05:33, Taylor Brandstetter wrote:
> This should be private.

Done.

[Taylor Brandstetter, 2017-04-19 05:56:33]

Aside from the "send_on_rtcp"/"rtcp_mux_enabled_" comment, just nits; looks
good.

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:689: const bool send_on_rtcp = rtcp &&
!rtcp_mux_filter_.IsActive();
This could be moved into RtpTransport, if rtcp_mux_filter_.IsActive() always
matched "rtcp_mux_enabled()"

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1230: // TODO(zstein): rtp_transport_.set_rtcp_mux_enabled
here?
I think you could do
"rtp_transport_.set_rtcp_mux_enabled(rtcp_mux_filter_.IsActive());" after the
switch statement, that seems simplest.

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1255: // TODO(zstein): Maybe rtcp_mux_filter should signal
these changes.
Since its state will always change synchronously, I don't think that's necessary

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/channel_unittes...
File webrtc/pc/channel_unittest.cc (left):

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/channel_unittes...
webrtc/pc/channel_unittest.cc:1881: void TestOnReadyToSend() {
Why does this test need to change? It seems correct before, and the new version
calls a "for testing" method which we should avoid when possible.

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/channel_unittes...
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/channel_unittes...
webrtc/pc/channel_unittest.cc:1914: RTC_FROM_HERE, [this] {
channel1_->OnTransportReadyToSend(false); });
Can you add a TODO to not call a "for testing" method, and instead make the
transport become "not ready to send" by having it simulate ENOTCONN?

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/rtptransport.cc
File webrtc/pc/rtptransport.cc (right):

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:62: // TODO(zstein): Use rtcp_mux_enabled_ instead of
send_on_rtcp
Could this be done in this CL?

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:132: SignalReadyToSend(ready_to_send);
nit: Would be preferable if this signal only fired when the state changes. We
could end up with log spam on accident.

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/rtptransport.h
File webrtc/pc/rtptransport.h (right):

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/rtptransport.h#...
webrtc/pc/rtptransport.h:60: bool IsWritable(bool send_on_rtcp) const;
As mentioned in other comments, I think "send_on_rtcp" can change to just "rtcp"
(meaning, whether an RTP or RTCP packet needs to be sent).

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/rtptransport_un...
File webrtc/pc/rtptransport_unittest.cc (right):

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/rtptransport_un...
webrtc/pc/rtptransport_unittest.cc:22: RtpTransport transport(false);
nit: Would be nice to have a way to know what the bool param means. Such as:

constexpr bool kMuxDisabled = false;
constexpr bool kMuxEnabled = true;
// ...
RtpTransport transport(kMuxDisabled);

Or:

RtpTransport transport(/*rtcp_mux_enabled=*/false);

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/rtptransport_un...
webrtc/pc/rtptransport_unittest.cc:50: SignalObserver observer;
nit: SignalObserver could take a pointer to the transport in its constructor and
hook up the signal there, just to make the test cases a little cleaner.

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/rtptransport_un...
webrtc/pc/rtptransport_unittest.cc:91: // TODO(zstein): disabling mux should
signal not ready?
Disabling mux when the RTCP transport is unset isn't something we support, or
need to support (to my knowledge).

[Zach Stein, 2017-04-20 19:59:11]

I think I got everything :)

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:689: const bool send_on_rtcp = rtcp &&
!rtcp_mux_filter_.IsActive();
On 2017/04/19 05:56:33, Taylor Brandstetter wrote:
> This could be moved into RtpTransport, if rtcp_mux_filter_.IsActive() always
> matched "rtcp_mux_enabled()"

Done.

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1230: // TODO(zstein): rtp_transport_.set_rtcp_mux_enabled
here?
On 2017/04/19 05:56:33, Taylor Brandstetter wrote:
> I think you could do
> "rtp_transport_.set_rtcp_mux_enabled(rtcp_mux_filter_.IsActive());" after the
> switch statement, that seems simplest.

I think that is just what I want - thanks!

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1255: // TODO(zstein): Maybe rtcp_mux_filter should signal
these changes.
On 2017/04/19 05:56:33, Taylor Brandstetter wrote:
> Since its state will always change synchronously, I don't think that's
necessary

Acknowledged.

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/channel_unittes...
File webrtc/pc/channel_unittest.cc (left):

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/channel_unittes...
webrtc/pc/channel_unittest.cc:1881: void TestOnReadyToSend() {
On 2017/04/19 05:56:33, Taylor Brandstetter wrote:
> Why does this test need to change? It seems correct before, and the new
version
> calls a "for testing" method which we should avoid when possible.

This test called SetTransportChannelReadyToSend, which this change removes.
OnTransportReadyToSend is public just for testing, but so was
SetTransportChannelReadyToSend.

The new version of this test is just checking that OnTranpsortReadyToSend is
hooked up to the media channel (pretty low value). The testing of the
aggregation logic moves to the RtpTransport tests.

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/channel_unittes...
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/channel_unittes...
webrtc/pc/channel_unittest.cc:1914: RTC_FROM_HERE, [this] {
channel1_->OnTransportReadyToSend(false); });
On 2017/04/19 05:56:33, Taylor Brandstetter wrote:
> Can you add a TODO to not call a "for testing" method, and instead make the
> transport become "not ready to send" by having it simulate ENOTCONN?

Done.

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/rtptransport.cc
File webrtc/pc/rtptransport.cc (right):

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:62: // TODO(zstein): Use rtcp_mux_enabled_ instead of
send_on_rtcp
On 2017/04/19 05:56:33, Taylor Brandstetter wrote:
> Could this be done in this CL?

Yup, I was just looking for advice on when to set it which you gave :)

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:132: SignalReadyToSend(ready_to_send);
On 2017/04/19 05:56:33, Taylor Brandstetter wrote:
> nit: Would be preferable if this signal only fired when the state changes. We
> could end up with log spam on accident.

Done.

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/rtptransport.h
File webrtc/pc/rtptransport.h (right):

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/rtptransport.h#...
webrtc/pc/rtptransport.h:60: bool IsWritable(bool send_on_rtcp) const;
On 2017/04/19 05:56:33, Taylor Brandstetter wrote:
> As mentioned in other comments, I think "send_on_rtcp" can change to just
"rtcp"
> (meaning, whether an RTP or RTCP packet needs to be sent).

Done.

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/rtptransport_un...
File webrtc/pc/rtptransport_unittest.cc (right):

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/rtptransport_un...
webrtc/pc/rtptransport_unittest.cc:22: RtpTransport transport(false);
On 2017/04/19 05:56:33, Taylor Brandstetter wrote:
> nit: Would be nice to have a way to know what the bool param means. Such as:
> 
> constexpr bool kMuxDisabled = false;
> constexpr bool kMuxEnabled = true;
> // ...
> RtpTransport transport(kMuxDisabled);
> 
> Or:
> 
> RtpTransport transport(/*rtcp_mux_enabled=*/false);

Done.

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/rtptransport_un...
webrtc/pc/rtptransport_unittest.cc:50: SignalObserver observer;
On 2017/04/19 05:56:33, Taylor Brandstetter wrote:
> nit: SignalObserver could take a pointer to the transport in its constructor
and
> hook up the signal there, just to make the test cases a little cleaner.

Done.

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/rtptransport_un...
webrtc/pc/rtptransport_unittest.cc:91: // TODO(zstein): disabling mux should
signal not ready?
On 2017/04/19 05:56:33, Taylor Brandstetter wrote:
> Disabling mux when the RTCP transport is unset isn't something we support, or
> need to support (to my knowledge).

Acknowledged.

[Taylor Brandstetter, 2017-04-21 09:12:38]

lgtm

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/channel_unittes...
File webrtc/pc/channel_unittest.cc (left):

https://codereview.webrtc.org/2812243005/diff/20001/webrtc/pc/channel_unittes...
webrtc/pc/channel_unittest.cc:1881: void TestOnReadyToSend() {
On 2017/04/20 19:59:10, Zach Stein wrote:
> On 2017/04/19 05:56:33, Taylor Brandstetter wrote:
> > Why does this test need to change? It seems correct before, and the new
> version
> > calls a "for testing" method which we should avoid when possible.
> 
> This test called SetTransportChannelReadyToSend, which this change removes.
> OnTransportReadyToSend is public just for testing, but so was
> SetTransportChannelReadyToSend.
> 
> The new version of this test is just checking that OnTranpsortReadyToSend is
> hooked up to the media channel (pretty low value). The testing of the
> aggregation logic moves to the RtpTransport tests.

Yeah, I see that now. So, with the TODO to not use OnTransportReadyToSend, I
think it's fine to land this.

[Zach Stein, 2017-04-24 18:44:51]

The CQ bit was checked by zstein@webrtc.org

[commit-bot: I haz the power, 2017-04-24 18:44:55]

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-04-24 18:46:23]

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

[commit-bot: I haz the power, 2017-04-24 18:46:24]

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...)
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/19757)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/25066)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/23920)

[Zach Stein, 2017-04-24 19:21:32]

The CQ bit was checked by zstein@webrtc.org

[Zach Stein, 2017-04-24 19:21:32]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2812243005/#ps60001 (title:
"Rebased")

[commit-bot: I haz the power, 2017-04-24 19:21:34]

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-04-24 19:33:38]

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

[commit-bot: I haz the power, 2017-04-24 19:33:39]

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

[Zach Stein, 2017-04-24 23:32:20]

The CQ bit was checked by zstein@webrtc.org

[Zach Stein, 2017-04-24 23:32:20]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2812243005/#ps80001 (title:
"Remove dcheck that does not currently hold.")

[commit-bot: I haz the power, 2017-04-24 23:32:25]

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-04-24 23:54:35]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493076740178570,
"parent_rev": "2784b69401cf35ab489325e3972179e6e9fa2162", "commit_rev":
"56162b9f675d6b55ebb7e7d682ea54a2a2e7e672"}

[commit-bot: I haz the power, 2017-04-24 23:54:39]

Description was changed from

==========
Move ready to send logic from BaseChannel to RtpTransport.

BUG=webrtc:7013
==========

to

==========
Move ready to send logic from BaseChannel to RtpTransport.

BUG=webrtc:7013

Review-Url: https://codereview.webrtc.org/2812243005
Cr-Commit-Position: refs/heads/master@{#17853}
Committed:
https://chromium.googlesource.com/external/webrtc/+/56162b9f675d6b55ebb7e7d68...
==========

[commit-bot: I haz the power, 2017-04-24 23:54:40]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/56162b9f675d6b55ebb7e7d68...