Reland of: Separating SCTP code from BaseChannel/MediaChannel.

Reland of: Separating SCTP code from BaseChannel/MediaChannel.

The BaseChannel code is geared around RTP; the presence of media engines,
send and receive streams, SRTP, SDP directional attribute negotiation, etc.
It doesn't make sense to use it for SCTP as well. This separation should make
future work both on BaseChannel and the SCTP code paths easier.

SctpDataEngine now becomes SctpTransport, and is used by WebRtcSession
directly. cricket::DataChannel is also renamed, to RtpDataChannel, so it
doesn't get confused with webrtc::DataChannel any more.

Beyond just moving code around, some consequences of this CL:
- We'll now stop using the worker thread for SCTP. Packets will be
  processed right on the network thread instead.
- The SDP directional attribute is ignored, as it's supposed to be.

BUG=None

Review-Url: https://codereview.webrtc.org/2564333002
Cr-Original-Commit-Position: refs/heads/master@{#15906}
Committed: https://chromium.googlesource.com/external/webrtc/+/67b3bbe639645ab719972682359acda303d94454
Review-Url: https://codereview.webrtc.org/2564333002
Cr-Commit-Position: refs/heads/master@{#15973}
Committed: https://chromium.googlesource.com/external/webrtc/+/953c2cea5ef35e2278a512dfd466c47cca2203a6

[Taylor Brandstetter, 2016-12-22 22:53:15]

Description was changed from

==========
(Work in progress) Separating SCTP code from BaseChannel/MediaChannel.

The BaseChannel code is geared around RTP; the presence of media engines, etc.;
it doesn't make sense to use it for SCTP as well. This separation should make
future work both on BaseChannel and the SCTP code paths easier.

The only notable consequence of this CL is that we'll now stop using the worker
thread for SCTP. Packets will be processed right on the network thread
instead.

BUG=None
==========

to

==========
Separating SCTP code from BaseChannel/MediaChannel.

The BaseChannel code is geared around RTP; the presence of media engines,
send and receive streams, SRTP, SDP directional attribute negotiation, etc.
It doesn't make sense to use it for SCTP as well. This separation should make
future work both on BaseChannel and the SCTP code paths easier.

SctpDataEngine now becomes SctpTransport, and is used by WebRtcSession
directly. cricket::DataChannel is also renamed, to RtpDataChannel, so it
doesn't get confused with webrtc::DataChannel any more.

Beyond just moving code around, some consequences of this CL:
- We'll now stop using the worker thread for SCTP. Packets will be
  processed right on the network thread instead.
- The SDP directional attribute is ignored, as it's supposed to be.

BUG=None
==========

[Taylor Brandstetter, 2016-12-22 22:59:47]

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

[Taylor Brandstetter, 2016-12-22 22:59:48]

Hi Peter; I think this CL is finally ready for review. I think this will make it
a lot easier to split BaseChannel into "RtpTransceiver"/"RtpTransport", since
there won't be hacky SCTP baggage to maintain.

https://codereview.webrtc.org/2564333002/diff/80001/webrtc/media/sctp/sctptra...
File webrtc/media/sctp/sctptransport_unittest.cc (right):

https://codereview.webrtc.org/2564333002/diff/80001/webrtc/media/sctp/sctptra...
webrtc/media/sctp/sctptransport_unittest.cc:1: /*
This isn't just a rename; I added a bunch of missing tests in this file, so
don't skip reviewing it.

https://codereview.webrtc.org/2564333002/diff/80001/webrtc/pc/channel.h
File webrtc/pc/channel.h (left):

https://codereview.webrtc.org/2564333002/diff/80001/webrtc/pc/channel.h#oldco...
webrtc/pc/channel.h:730: // RtpDataChannel instead of using this.
Hey, I'm fixing your TODO!

[Taylor Brandstetter, 2016-12-22 23:00:09]

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

[commit-bot: I haz the power, 2016-12-22 23:00:13]

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, 2016-12-22 23:01:44]

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

[commit-bot: I haz the power, 2016-12-22 23:01:45]

Dry run: Try jobs failed on following builders:
  ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/13654)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/16007)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/15863)

[Taylor Brandstetter, 2016-12-22 23:20:09]

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

[commit-bot: I haz the power, 2016-12-22 23:20:16]

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, 2016-12-22 23:39:41]

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

[commit-bot: I haz the power, 2016-12-22 23:39:42]

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

[pthatcher1, 2016-12-23 01:39:32]

Impressive refactoring.

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:521: SignalDataChannelDestroyed();
Should this be SignalSctpTransportDestroyed()?

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:1292: sctp_transport_.get(), params, payload,
result));
Similar to passing a "fire events on this thread" being passed to SctpTransport,
if the SctpTransport automatically hops to the network thread when it's called
from a thread other than the network thread, then would be a nice safety that
would work for both ORTC and WebRtcSession.

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/api/webrtcsession.h
File webrtc/api/webrtcsession.h (right):

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.h:221: return sctp_transport_ ? sctp_transport_name_ :
std::string();
I think it would be slightly better to have these be optional<std::string>.

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.h:229: bool GetSctpSslRole(rtc::SSLRole* role);
Why do we need this instead of just calling GetSslRole(sctp_content_name(),
&role))?

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.h:586: // thread.
Why don't we just pass a thread to the SctpTransport that is the "fire events on
this thread" thread?  If that thread is the network thread, it does no hops.  If
it is different, then it hops to that thread?   That would be easier for use as
an ORTC object, since then it could be threadsafe easily outside of a
WebRtcSession.

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
File webrtc/media/sctp/sctptransport.cc (right):

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransport.cc:488: return true;
Which parts of this file are significantly different than sctpdataengine so that
I can focus the review on those parts?

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
File webrtc/media/sctp/sctptransport.h (right):

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransport.h:29: #include
"webrtc/media/base/mediachannel.h"
Just go ahead  and rename it to .sid and make the rtp data channel have the
oddity that a SID turns into an SSRC.  And you can probably move
SendDataParams/ReceiveDataParams out of mediachannel.h.

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
File webrtc/media/sctp/sctptransport_unittest.cc (right):

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransport_unittest.cc:537: // Flaky, see webrtc:4453.
Which tests are the new ones so I can focus on those?

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
File webrtc/media/sctp/sctptransportinternal.h (right):

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransportinternal.h:74: // support though. See:
https://github.com/w3c/webrtc-pc/issues/979
I don't think we'll ever need that.

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransportinternal.h:75: virtual bool Start(int
local_sctp_port, int remote_sctp_port) = 0;
We could model this as the ORTC object:

- A port is passed into the constructor (with default of 5000)
- The thing passed into Start is the remote capabilities (max message size)

Do we even need the remote port?  I don't see it in the ORTC API.

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransportinternal.h:86: virtual bool OpenStream(int sid) =
0;
It would be nice if we instead had a CreateDataChannel method here that returned
a data channel that's tied to a SID.  But that can come later, I guess.

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransportinternal.h:91: virtual bool ResetStream(int sid)
= 0;
Why not CloseStream (to go along with DataChannel.close() and the opposite of
Open()).

[Taylor Brandstetter, 2016-12-23 06:29:05]

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:521: SignalDataChannelDestroyed();
On 2016/12/23 01:39:31, pthatcher1 wrote:
> Should this be SignalSctpTransportDestroyed()?

It could also be an RtpDataChannel though.

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/api/webrtcsession.h
File webrtc/api/webrtcsession.h (right):

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.h:221: return sctp_transport_ ? sctp_transport_name_ :
std::string();
On 2016/12/23 01:39:31, pthatcher1 wrote:
> I think it would be slightly better to have these be optional<std::string>.  

Done.

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.h:229: bool GetSctpSslRole(rtc::SSLRole* role);
On 2016/12/23 01:39:31, pthatcher1 wrote:
> Why do we need this instead of just calling GetSslRole(sctp_content_name(),
> &role))?

Because I want to remove GetSslRole (see below TODO comment). The SSL role is
only really needed to figure out what SIDs to use for SCTP data channels.

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.h:586: // thread.
On 2016/12/23 01:39:31, pthatcher1 wrote:
> Why don't we just pass a thread to the SctpTransport that is the "fire events
on
> this thread" thread?  If that thread is the network thread, it does no hops. 
If
> it is different, then it hops to that thread?   That would be easier for use
as
> an ORTC object, since then it could be threadsafe easily outside of a
> WebRtcSession.

Because that's giving SctpTransport more responsibilities than I think it should
have. I'd be open to having a proxy object do that (using proxy.h), but proxy.h
doesn't currently support proxying sigslot signals to the correct thread. So I
planned to leave that as a future exercise. I'll leave a TODO describing this.

That reminds me though, I need to add a thread checker to SctpTransport.

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
File webrtc/media/sctp/sctptransport.cc (right):

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransport.cc:488: return true;
On 2016/12/23 01:39:31, pthatcher1 wrote:
> Which parts of this file are significantly different than sctpdataengine so
that
> I can focus the review on those parts?

The public methods (SetTransportChannel, Start, OpenStream, etc.). The rest is
mostly the same, though there are some differences. For example, I switched to
using AsyncInvoker instead of using Post/OnMessage directly.

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
File webrtc/media/sctp/sctptransport.h (right):

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransport.h:29: #include
"webrtc/media/base/mediachannel.h"
On 2016/12/23 01:39:31, pthatcher1 wrote:
> Just go ahead  and rename it to .sid and make the rtp data channel have the
> oddity that a SID turns into an SSRC.  And you can probably move
> SendDataParams/ReceiveDataParams out of mediachannel.h.

I changed it to a union type. I'll leave moving the declarations as a TODO (we
should also probably move the sctptransport files out of media/)

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
File webrtc/media/sctp/sctptransport_unittest.cc (right):

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransport_unittest.cc:537: // Flaky, see webrtc:4453.
On 2016/12/23 01:39:31, pthatcher1 wrote:
> Which tests are the new ones so I can focus on those?
The ones that are new or fundamentally different from the old versions are:

SwitchTransportChannel
DuplicateStartCallsIgnored
CallingStartWithDifferentPortFails
NegativeOnePortTreatedAsDefault
OpenStreamWithAlreadyOpenedStreamFails
ResetStreamWithAlreadyResetStreamFails
SignalReadyToSendDataAfterDtlsWritable
SignalReadyToSendDataAfterBlocked
SendDataWithNonexistentStreamFails

And the code in the SctpTransportTest base class.

The rest of the tests (6 I think?) are mostly the same.

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
File webrtc/media/sctp/sctptransportinternal.h (right):

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransportinternal.h:74: // support though. See:
https://github.com/w3c/webrtc-pc/issues/979
On 2016/12/23 01:39:31, pthatcher1 wrote:
> I don't think we'll ever need that.

Can you comment on the github issue?

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransportinternal.h:75: virtual bool Start(int
local_sctp_port, int remote_sctp_port) = 0;
On 2016/12/23 01:39:31, pthatcher1 wrote:
> We could model this as the ORTC object:
> 
> - A port is passed into the constructor (with default of 5000)
> - The thing passed into Start is the remote capabilities (max message size)
> 
> Do we even need the remote port?  I don't see it in the ORTC API.

There's an issue filed for ORTC for that. There was discussion about it this
week, coincidentally; I'd recommend joining the mailing list if you haven't.

I agree that we may end up putting the local port in the constructor, but that
would require more changes in WebRtcSession than I wanted to do now. But we
should do that by the time we have an ORTC version of SctpTransport in our
public API, assuming that's how the ORTC API settles out.

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransportinternal.h:86: virtual bool OpenStream(int sid) =
0;
On 2016/12/23 01:39:31, pthatcher1 wrote:
> It would be nice if we instead had a CreateDataChannel method here that
returned
> a data channel that's tied to a SID.  But that can come later, I guess. 

I disagree; that would create a circular dependency between DataChannel and
SctpTransport that we don't really need.

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransportinternal.h:91: virtual bool ResetStream(int sid)
= 0;
On 2016/12/23 01:39:31, pthatcher1 wrote:
> Why not CloseStream (to go along with DataChannel.close() and the opposite of
> Open()).

Because streams aren't closed, they're reset; that's just how SCTP works. See:
https://tools.ietf.org/html/rfc6525

If anything, OpenStream should be removed; calling OpenStream doesn't actually
do anything.

[pthatcher1, 2017-01-03 18:31:26]

I'm still looking into the areas you pointed out that needed more review.

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
File webrtc/media/sctp/sctptransportinternal.h (right):

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransportinternal.h:74: // support though. See:
https://github.com/w3c/webrtc-pc/issues/979
On 2016/12/23 06:29:05, Taylor Brandstetter (OOO) wrote:
> On 2016/12/23 01:39:31, pthatcher1 wrote:
> > I don't think we'll ever need that.
> 
> Can you comment on the github issue?

I commented on github.  It seems like Bernard agrees that we won't need this.

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransportinternal.h:75: virtual bool Start(int
local_sctp_port, int remote_sctp_port) = 0;
On 2016/12/23 06:29:05, Taylor Brandstetter (OOO) wrote:
> On 2016/12/23 01:39:31, pthatcher1 wrote:
> > We could model this as the ORTC object:
> > 
> > - A port is passed into the constructor (with default of 5000)
> > - The thing passed into Start is the remote capabilities (max message size)
> > 
> > Do we even need the remote port?  I don't see it in the ORTC API.
> 
> There's an issue filed for ORTC for that. There was discussion about it this
> week, coincidentally; I'd recommend joining the mailing list if you haven't.
> 
> I agree that we may end up putting the local port in the constructor, but that
> would require more changes in WebRtcSession than I wanted to do now. But we
> should do that by the time we have an ORTC version of SctpTransport in our
> public API, assuming that's how the ORTC API settles out.

Sure, we can always refactor more later.  

I just responded to the mailing list.  JSEP (via draft-ietf-mmusic-sctp-sdp)
requires the ports to be the same, so could simplify this start method to only
taking one port value.

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransportinternal.h:86: virtual bool OpenStream(int sid) =
0;
On 2016/12/23 06:29:05, Taylor Brandstetter (OOO) wrote:
> On 2016/12/23 01:39:31, pthatcher1 wrote:
> > It would be nice if we instead had a CreateDataChannel method here that
> returned
> > a data channel that's tied to a SID.  But that can come later, I guess. 
> 
> I disagree; that would create a circular dependency between DataChannel and
> SctpTransport that we don't really need.

Sorry, I meant return a DataChannelInterface, which would avoid a circular
dependency.

https://codereview.webrtc.org/2564333002/diff/60002/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2564333002/diff/60002/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1105: bundle.HasContentName(*sctp_content_name_)) {
Would it be wise to add an "&& sctp_transport_name_" in there also?

https://codereview.webrtc.org/2564333002/diff/60002/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1911: std::string old_sctp_transport_name =
*sctp_transport_name_;
Would it be wise to add an RTC_DCHECK(sctp_transport_name_) there too?

https://codereview.webrtc.org/2564333002/diff/60002/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:2299: if (sctp_transport_ && content_name ==
*sctp_content_name_) {
And "&& sctp_transport_name_" here as well?

[pthatcher1, 2017-01-03 18:39:39]

Everything else looks good.

[pthatcher1, 2017-01-03 18:39:53]

lgtm

[pthatcher1, 2017-01-03 20:45:00]

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
File webrtc/media/sctp/sctptransportinternal.h (right):

https://codereview.webrtc.org/2564333002/diff/160001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransportinternal.h:75: virtual bool Start(int
local_sctp_port, int remote_sctp_port) = 0;
On 2017/01/03 18:31:26, pthatcher1 wrote:
> On 2016/12/23 06:29:05, Taylor Brandstetter (OOO) wrote:
> > On 2016/12/23 01:39:31, pthatcher1 wrote:
> > > We could model this as the ORTC object:
> > > 
> > > - A port is passed into the constructor (with default of 5000)
> > > - The thing passed into Start is the remote capabilities (max message
size)
> > > 
> > > Do we even need the remote port?  I don't see it in the ORTC API.
> > 
> > There's an issue filed for ORTC for that. There was discussion about it this
> > week, coincidentally; I'd recommend joining the mailing list if you haven't.
> > 
> > I agree that we may end up putting the local port in the constructor, but
that
> > would require more changes in WebRtcSession than I wanted to do now. But we
> > should do that by the time we have an ORTC version of SctpTransport in our
> > public API, assuming that's how the ORTC API settles out.
> 
> Sure, we can always refactor more later.  
> 
> I just responded to the mailing list.  JSEP (via draft-ietf-mmusic-sctp-sdp)
> requires the ports to be the same, so could simplify this start method to only
> taking one port value.

Nevermind, I was wrong about that :).

[Taylor Brandstetter, 2017-01-03 23:45:02]

https://codereview.webrtc.org/2564333002/diff/60002/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2564333002/diff/60002/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1105: bundle.HasContentName(*sctp_content_name_)) {
On 2017/01/03 18:31:26, pthatcher1 wrote:
> Would it be wise to add an "&& sctp_transport_name_" in there also?

I ended up adding a DCHECK instead, since sctp_content_name_ is also
dereferenced.

https://codereview.webrtc.org/2564333002/diff/60002/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1911: std::string old_sctp_transport_name =
*sctp_transport_name_;
On 2017/01/03 18:31:26, pthatcher1 wrote:
> Would it be wise to add an RTC_DCHECK(sctp_transport_name_) there too?

Done.

https://codereview.webrtc.org/2564333002/diff/60002/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:2299: if (sctp_transport_ && content_name ==
*sctp_content_name_) {
On 2017/01/03 18:31:26, pthatcher1 wrote:
> And "&& sctp_transport_name_" here as well?

Again, went with DCHECKs

[Taylor Brandstetter, 2017-01-03 23:52:25]

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

[commit-bot: I haz the power, 2017-01-03 23:52:33]

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-04 00:22:20]

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

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

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

[Taylor Brandstetter, 2017-01-05 01:27:16]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2017-01-05 01:27:17]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2564333002/#ps240001 (title:
"Merge with master")

[commit-bot: I haz the power, 2017-01-05 01:27:27]

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-05 02:38:03]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1483579636751300,
"parent_rev": "d9762e793897d3fe5667fa107027ecea82ac574a", "commit_rev":
"67b3bbe639645ab719972682359acda303d94454"}

[commit-bot: I haz the power, 2017-01-05 02:38:08]

Description was changed from

==========
Separating SCTP code from BaseChannel/MediaChannel.

The BaseChannel code is geared around RTP; the presence of media engines,
send and receive streams, SRTP, SDP directional attribute negotiation, etc.
It doesn't make sense to use it for SCTP as well. This separation should make
future work both on BaseChannel and the SCTP code paths easier.

SctpDataEngine now becomes SctpTransport, and is used by WebRtcSession
directly. cricket::DataChannel is also renamed, to RtpDataChannel, so it
doesn't get confused with webrtc::DataChannel any more.

Beyond just moving code around, some consequences of this CL:
- We'll now stop using the worker thread for SCTP. Packets will be
  processed right on the network thread instead.
- The SDP directional attribute is ignored, as it's supposed to be.

BUG=None
==========

to

==========
Separating SCTP code from BaseChannel/MediaChannel.

The BaseChannel code is geared around RTP; the presence of media engines,
send and receive streams, SRTP, SDP directional attribute negotiation, etc.
It doesn't make sense to use it for SCTP as well. This separation should make
future work both on BaseChannel and the SCTP code paths easier.

SctpDataEngine now becomes SctpTransport, and is used by WebRtcSession
directly. cricket::DataChannel is also renamed, to RtpDataChannel, so it
doesn't get confused with webrtc::DataChannel any more.

Beyond just moving code around, some consequences of this CL:
- We'll now stop using the worker thread for SCTP. Packets will be
  processed right on the network thread instead.
- The SDP directional attribute is ignored, as it's supposed to be.

BUG=None

Review-Url: https://codereview.webrtc.org/2564333002
Cr-Commit-Position: refs/heads/master@{#15906}
Committed:
https://chromium.googlesource.com/external/webrtc/+/67b3bbe639645ab7199726823...
==========

[commit-bot: I haz the power, 2017-01-05 02:38:10]

Committed patchset #14 (id:240001) as
https://chromium.googlesource.com/external/webrtc/+/67b3bbe639645ab7199726823...

[Taylor Brandstetter, 2017-01-05 04:24:29]

A revert of this CL (patchset #14 id:240001) has been created in
https://codereview.webrtc.org/2614813003/ by deadbeef@webrtc.org.

The reason for reverting is: Hitting DCHECK in chromium's
WebrtcTransportTest.TerminateDataChannel and WebrtcTransportTest.DataStreamLate.
Will investigate and reland..

[Taylor Brandstetter, 2017-01-05 04:47:39]

Description was changed from

==========
Separating SCTP code from BaseChannel/MediaChannel.

The BaseChannel code is geared around RTP; the presence of media engines,
send and receive streams, SRTP, SDP directional attribute negotiation, etc.
It doesn't make sense to use it for SCTP as well. This separation should make
future work both on BaseChannel and the SCTP code paths easier.

SctpDataEngine now becomes SctpTransport, and is used by WebRtcSession
directly. cricket::DataChannel is also renamed, to RtpDataChannel, so it
doesn't get confused with webrtc::DataChannel any more.

Beyond just moving code around, some consequences of this CL:
- We'll now stop using the worker thread for SCTP. Packets will be
  processed right on the network thread instead.
- The SDP directional attribute is ignored, as it's supposed to be.

BUG=None

Review-Url: https://codereview.webrtc.org/2564333002
Cr-Commit-Position: refs/heads/master@{#15906}
Committed:
https://chromium.googlesource.com/external/webrtc/+/67b3bbe639645ab7199726823...
==========

to

==========
Reland of: Separating SCTP code from BaseChannel/MediaChannel.

The BaseChannel code is geared around RTP; the presence of media engines,
send and receive streams, SRTP, SDP directional attribute negotiation, etc.
It doesn't make sense to use it for SCTP as well. This separation should make
future work both on BaseChannel and the SCTP code paths easier.

SctpDataEngine now becomes SctpTransport, and is used by WebRtcSession
directly. cricket::DataChannel is also renamed, to RtpDataChannel, so it
doesn't get confused with webrtc::DataChannel any more.

Beyond just moving code around, some consequences of this CL:
- We'll now stop using the worker thread for SCTP. Packets will be
  processed right on the network thread instead.
- The SDP directional attribute is ignored, as it's supposed to be.

BUG=None

Review-Url: https://codereview.webrtc.org/2564333002
Cr-Commit-Position: refs/heads/master@{#15906}
Committed:
https://chromium.googlesource.com/external/webrtc/+/67b3bbe639645ab7199726823...
==========

[Taylor Brandstetter, 2017-01-05 05:15:24]

Hi Peter, can you take a look at just the last patchset (16)? I was hitting a
DCHECK when a data m= section was negotiated on a subsequent offer, because I
was prematurely calling PushdownSctpParameters when only the offer was applied.

[pthatcher1, 2017-01-06 23:43:46]

lgtm

[Taylor Brandstetter, 2017-01-09 21:53:34]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2017-01-09 21:53:34]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2564333002/#ps300001 (title:
"Merge with master.")

[commit-bot: I haz the power, 2017-01-09 21:53:44]

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-09 22:53:42]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1483998814146080,
"parent_rev": "1f035c0f2a872fc361295a754bbe85515ef99597", "commit_rev":
"953c2cea5ef35e2278a512dfd466c47cca2203a6"}

[commit-bot: I haz the power, 2017-01-09 22:53:45]

Description was changed from

==========
Reland of: Separating SCTP code from BaseChannel/MediaChannel.

The BaseChannel code is geared around RTP; the presence of media engines,
send and receive streams, SRTP, SDP directional attribute negotiation, etc.
It doesn't make sense to use it for SCTP as well. This separation should make
future work both on BaseChannel and the SCTP code paths easier.

SctpDataEngine now becomes SctpTransport, and is used by WebRtcSession
directly. cricket::DataChannel is also renamed, to RtpDataChannel, so it
doesn't get confused with webrtc::DataChannel any more.

Beyond just moving code around, some consequences of this CL:
- We'll now stop using the worker thread for SCTP. Packets will be
  processed right on the network thread instead.
- The SDP directional attribute is ignored, as it's supposed to be.

BUG=None

Review-Url: https://codereview.webrtc.org/2564333002
Cr-Commit-Position: refs/heads/master@{#15906}
Committed:
https://chromium.googlesource.com/external/webrtc/+/67b3bbe639645ab7199726823...
==========

to

==========
Reland of: Separating SCTP code from BaseChannel/MediaChannel.

The BaseChannel code is geared around RTP; the presence of media engines,
send and receive streams, SRTP, SDP directional attribute negotiation, etc.
It doesn't make sense to use it for SCTP as well. This separation should make
future work both on BaseChannel and the SCTP code paths easier.

SctpDataEngine now becomes SctpTransport, and is used by WebRtcSession
directly. cricket::DataChannel is also renamed, to RtpDataChannel, so it
doesn't get confused with webrtc::DataChannel any more.

Beyond just moving code around, some consequences of this CL:
- We'll now stop using the worker thread for SCTP. Packets will be
  processed right on the network thread instead.
- The SDP directional attribute is ignored, as it's supposed to be.

BUG=None

Review-Url: https://codereview.webrtc.org/2564333002
Cr-Original-Commit-Position: refs/heads/master@{#15906}
Committed:
https://chromium.googlesource.com/external/webrtc/+/67b3bbe639645ab7199726823...
Review-Url: https://codereview.webrtc.org/2564333002
Cr-Commit-Position: refs/heads/master@{#15973}
Committed:
https://chromium.googlesource.com/external/webrtc/+/953c2cea5ef35e2278a512dfd...
==========

[commit-bot: I haz the power, 2017-01-09 22:53:46]

Committed patchset #17 (id:300001) as
https://chromium.googlesource.com/external/webrtc/+/953c2cea5ef35e2278a512dfd...