Reland of: Separating SCTP code from BaseChannel/MediaChannel.

webrtc/api/webrtcsession.h

Index: webrtc/api/webrtcsession.h
diff --git a/webrtc/api/webrtcsession.h b/webrtc/api/webrtcsession.h
index ef31560509879ec3abeb4705ee1c20a0ecb1cc72..66cdf45cd0f1099b6c85611c572c33f184657e0e 100644
--- a/webrtc/api/webrtcsession.h
+++ b/webrtc/api/webrtcsession.h
@@ -37,7 +37,9 @@
 namespace cricket {
 
 class ChannelManager;
-class DataChannel;
+class RtpDataChannel;
+class SctpTransportInternal;
+class SctpTransportInternalFactory;
 class StatsReport;
 class VideoChannel;
 class VoiceChannel;
@@ -67,8 +69,8 @@ extern const char kSdpWithoutIceUfragPwd[];
 extern const char kSdpWithoutSdesAndDtlsDisabled[];
 extern const char kSessionError[];
 extern const char kSessionErrorDesc[];
-extern const char kDtlsSetupFailureRtp[];
-extern const char kDtlsSetupFailureRtcp[];
+extern const char kDtlsSrtpSetupFailureRtp[];
+extern const char kDtlsSrtpSetupFailureRtcp[];
 extern const char kEnableBundleFailed[];
 
 // Maximum number of received video streams that will be processed by webrtc
@@ -158,13 +160,15 @@ class WebRtcSession :
     ERROR_TRANSPORT = 2,  // transport error of some kind
   };
 
+  // |sctp_factory| may be null, in which case SCTP is treated as unsupported.
   WebRtcSession(
       webrtc::MediaControllerInterface* media_controller,
       rtc::Thread* network_thread,
       rtc::Thread* worker_thread,
       rtc::Thread* signaling_thread,
       cricket::PortAllocator* port_allocator,
-      std::unique_ptr<cricket::TransportController> transport_controller);
+      std::unique_ptr<cricket::TransportController> transport_controller,
+      std::unique_ptr<cricket::SctpTransportInternalFactory> sctp_factory);
   virtual ~WebRtcSession();
 
   // These are const to allow them to be called from const methods.
@@ -199,26 +203,34 @@ class WebRtcSession :
     ice_observer_ = observer;
   }
 
+  // Exposed for stats collecting.
   virtual cricket::VoiceChannel* voice_channel() {
     return voice_channel_.get();
   }
   virtual cricket::VideoChannel* video_channel() {
     return video_channel_.get();
   }
-  virtual cricket::DataChannel* data_channel() {
-    return data_channel_.get();
+  // Only valid when using deprecated RTP data channels.
+  virtual cricket::RtpDataChannel* rtp_data_channel() {
+    return rtp_data_channel_.get();
+  }
+  virtual std::string sctp_content_name() const {
+    return sctp_transport_ ? sctp_content_name_ : std::string();
+  }
+  virtual std::string sctp_transport_name() const {
+    return sctp_transport_ ? sctp_transport_name_ : std::string();

[pthatcher1, 2016/12/23 01:39:31]

I think it would be slightly better to have these be optional<std::string>.  

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

Done.

   }
 
   cricket::BaseChannel* GetChannel(const std::string& content_name);
 
   cricket::SecurePolicy SdesPolicy() const;
 
-  // Get current ssl role from transport.
-  bool GetSslRole(const std::string& transport_name, rtc::SSLRole* role);
-
-  // Get current SSL role for this channel's transport.
-  // If |transport| is null, returns false.
-  bool GetSslRole(const cricket::BaseChannel* channel, rtc::SSLRole* role);
+  // Get current SSL role used by SCTP's underlying transport.
+  bool GetSctpSslRole(rtc::SSLRole* role);

[pthatcher1, 2016/12/23 01:39:31]

Why do we need this instead of just calling GetSslRole(sctp_content_name(),
&role))?

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

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.

+  // Get SSL role for an arbitrary m= section (handles bundling correctly).
+  // TODO(deadbeef): This is only used internally by the session description
+  // factory, it shouldn't really be public).
+  bool GetSslRole(const std::string& content_name, rtc::SSLRole* role);
 
   void CreateOffer(
       CreateSessionDescriptionObserver* observer,
@@ -232,6 +244,7 @@ class WebRtcSession :
   // The ownership of |desc| will be transferred after this call.
   bool SetRemoteDescription(SessionDescriptionInterface* desc,
                             std::string* err_desc);
+
   bool ProcessIceMessage(const IceCandidateInterface* ice_candidate);
 
   bool RemoveRemoteIceCandidates(
@@ -326,7 +339,7 @@ class WebRtcSession :
   // WebRTCSessionDescriptionFactory. Should happen before setLocalDescription.
   void OnCertificateReady(
       const rtc::scoped_refptr<rtc::RTCCertificate>& certificate);
-  void OnDtlsSetupFailure(cricket::BaseChannel*, bool rtcp);
+  void OnDtlsSrtpSetupFailure(cricket::BaseChannel*, bool rtcp);
 
   // For unit test.
   bool waiting_for_certificate_for_testing() const;
@@ -338,8 +351,9 @@ class WebRtcSession :
     transport_controller_->SetMetricsObserver(metrics_observer);
   }
 
-  // Called when voice_channel_, video_channel_ and data_channel_ are created
-  // and destroyed. As a result of, for example, setting a new description.
+  // Called when voice_channel_, video_channel_ and
+  // rtp_data_channel_/sctp_transport_ are created and destroyed. As a result
+  // of, for example, setting a new description.
   sigslot::signal0<> SignalVoiceChannelCreated;
   sigslot::signal0<> SignalVoiceChannelDestroyed;
   sigslot::signal0<> SignalVideoChannelCreated;
@@ -397,6 +411,7 @@ class WebRtcSession :
   bool PushdownMediaDescription(cricket::ContentAction action,
                                 cricket::ContentSource source,
                                 std::string* error_desc);
+  bool PushdownSctpParameters_n(cricket::ContentSource source);
 
   bool PushdownTransportDescription(cricket::ContentSource source,
                                     cricket::ContentAction action,
@@ -461,11 +476,24 @@ class WebRtcSession :
   std::unique_ptr<SessionStats> GetStats_n(
       const ChannelNamePairs& channel_name_pairs);
 
-  // Listens to SCTP CONTROL messages on unused SIDs and process them as OPEN
-  // messages.
-  void OnDataChannelMessageReceived(cricket::DataChannel* channel,
-                                    const cricket::ReceiveDataParams& params,
-                                    const rtc::CopyOnWriteBuffer& payload);
+  bool CreateSctpTransport_n(const std::string& content_name,
+                             const std::string& transport_name);
+  // For bundling.
+  void ChangeSctpTransport_n(const std::string& transport_name);
+  void DestroySctpTransport_n();
+  // SctpTransport signal handlers. Needed to marshal signals from the network
+  // to signaling thread.
+  void OnSctpTransportReadyToSendData_n();
+  // This may be called with "false" if the direction of the m= section causes
+  // us to tear down the SCTP connection.
+  void OnSctpTransportReadyToSendData_s(bool ready);
+  void OnSctpTransportDataReceived_n(const cricket::ReceiveDataParams& params,
+                                     const rtc::CopyOnWriteBuffer& payload);
+  // Beyond just firing the signal to the signaling thread, listens to SCTP
+  // CONTROL messages on unused SIDs and processes them as OPEN messages.
+  void OnSctpTransportDataReceived_s(const cricket::ReceiveDataParams& params,
+                                     const rtc::CopyOnWriteBuffer& payload);
+  void OnSctpStreamClosedRemotely_n(int sid);
 
   std::string BadStateErrMsg(State state);
   void SetIceConnectionState(PeerConnectionInterface::IceConnectionState state);
@@ -498,6 +526,7 @@ class WebRtcSession :
   // this session.
   bool SrtpRequired() const;
 
+  // TransportController signal handlers.
   void OnTransportControllerConnectionState(cricket::IceConnectionState state);
   void OnTransportControllerReceiving(bool receiving);
   void OnTransportControllerGatheringState(cricket::IceGatheringState state);
@@ -506,6 +535,7 @@ class WebRtcSession :
       const std::vector<cricket::Candidate>& candidates);
   void OnTransportControllerCandidatesRemoved(
       const std::vector<cricket::Candidate>& candidates);
+  void OnTransportControllerDtlsHandshakeError(rtc::SSLHandshakeError error);
 
   std::string GetSessionErrorMsg();
 
@@ -522,8 +552,6 @@ class WebRtcSession :
 
   const std::string GetTransportName(const std::string& content_name);
 
-  void OnDtlsHandshakeError(rtc::SSLHandshakeError error);
-
   rtc::Thread* const network_thread_;
   rtc::Thread* const worker_thread_;
   rtc::Thread* const signaling_thread_;
@@ -536,10 +564,36 @@ class WebRtcSession :
   bool initial_offerer_ = false;
 
   const std::unique_ptr<cricket::TransportController> transport_controller_;
+  const std::unique_ptr<cricket::SctpTransportInternalFactory> sctp_factory_;
   MediaControllerInterface* media_controller_;
   std::unique_ptr<cricket::VoiceChannel> voice_channel_;
   std::unique_ptr<cricket::VideoChannel> video_channel_;
-  std::unique_ptr<cricket::DataChannel> data_channel_;
+  // |rtp_data_channel_| is used if in RTP data channel mode, |sctp_transport_|
+  // when using SCTP.
+  std::unique_ptr<cricket::RtpDataChannel> rtp_data_channel_;
+
+  std::unique_ptr<cricket::SctpTransportInternal> sctp_transport_;
+  // |sctp_transport_name_| keeps track of what DTLS transport the SCTP
+  // transport is using (which can change due to bundling).
+  std::string sctp_transport_name_;
+  // |sctp_content_name_| is the content name (MID) in SDP.
+  std::string sctp_content_name_;
+  // Value cached on signaling thread. Only updated when SctpReadyToSendData
+  // fires on the signaling thread.
+  bool sctp_ready_to_send_data_ = false;
+  // Same as signals provided by SctpTransport, but these are guaranteed to
+  // fire on the signaling thread, whereas SctpTransport fires on the networking
+  // thread.

[pthatcher1, 2016/12/23 01:39:31]

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.

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

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.

+  // |sctp_invoker_| is used so that any signals queued on the signaling thread
+  // from the network thread are immediately discarded if the SctpTransport is
+  // destroyed (due to m= section being rejected).
+  std::unique_ptr<rtc::AsyncInvoker> sctp_invoker_;
+  sigslot::signal1<bool> SignalSctpReadyToSendData;
+  sigslot::signal2<const cricket::ReceiveDataParams&,
+                   const rtc::CopyOnWriteBuffer&>
+      SignalSctpDataReceived;
+  sigslot::signal1<int> SignalSctpStreamClosedRemotely;
+
   cricket::ChannelManager* channel_manager_;
   IceObserver* ice_observer_;
   PeerConnectionInterface::IceConnectionState ice_connection_state_;