Added send-thresholding and fixed text packet dumping.

talk/media/sctp/sctpdataengine.cc

Index: talk/media/sctp/sctpdataengine.cc
diff --git a/talk/media/sctp/sctpdataengine.cc b/talk/media/sctp/sctpdataengine.cc
index 8c8a6a192fa25833c3de66c0587b9ed7ed3bccb3..26513cb49075c34ec49685b2e8e708131fec7ad9 100644
--- a/talk/media/sctp/sctpdataengine.cc
+++ b/talk/media/sctp/sctpdataengine.cc
@@ -177,11 +177,11 @@ static bool GetDataMediaType(
 }
 
 // Log the packet in text2pcap format, if log level is at LS_VERBOSE.
-static void VerboseLogPacket(void *addr, size_t length, int direction) {
+static void VerboseLogPacket(void *data, size_t length, int direction) {
   if (LOG_CHECK_LEVEL(LS_VERBOSE) && length > 0) {
     char *dump_buf;
     if ((dump_buf = usrsctp_dumppacket(
-             addr, length, direction)) != NULL) {
+             data, length, direction)) != NULL) {
       LOG(LS_VERBOSE) << dump_buf;
       usrsctp_freedumpbuffer(dump_buf);
     }
@@ -198,7 +198,7 @@ static int OnSctpOutboundPacket(void* addr, void* data, size_t length,
                   << "; tos: " << std::hex << static_cast<int>(tos)
                   << "; set_df: " << std::hex << static_cast<int>(set_df);
 
-  VerboseLogPacket(addr, length, SCTP_DUMP_OUTBOUND);
+  VerboseLogPacket(data, length, SCTP_DUMP_OUTBOUND);
   // Note: We have to copy the data; the caller will delete it.
   auto* msg = new OutboundPacketMessage(
       new rtc::Buffer(reinterpret_cast<uint8_t*>(data), length));
@@ -315,6 +315,9 @@ DataMediaChannel* SctpDataEngine::CreateChannel(
   return new SctpDataMediaChannel(rtc::Thread::Current());
 }
 
+// Our registrar of sockets to their channels.  Used for callbacks.
+SctpDataMediaChannel::SocketChannelMap SctpDataMediaChannel::sock_channel_map_;
+
 SctpDataMediaChannel::SctpDataMediaChannel(rtc::Thread* thread)
     : worker_thread_(thread),
       local_port_(kSctpDefaultPort),
@@ -341,19 +344,40 @@ sockaddr_conn SctpDataMediaChannel::GetSctpSockAddr(int port) {
   return sconn;
 }
 
+// static
+int SctpDataMediaChannel::SendThresholdCallback(struct socket* sock,

[pthatcher1, 2015/08/05 21:42:55]

I think it would make more sense to put this callback on the engine as a
non-static method.  

[lally1, 2015/08/18 20:38:56]

I don't know where I'd get an instance pointer.

[pthatcher1, 2015/08/18 21:51:52]

The instance pointer to the engine?  Could you use rtc::Bind to get a callback
that has a pointer to the engine?

+                                                uint32_t sb_free) {

[pthatcher1, 2015/08/05 21:42:55]

No ulp_info, like with reading a packet?  That's lame.

[lally1, 2015/08/18 20:38:56]

Yeah, that's the real issue here.  I had a prior version of this CL that used
usrsctp_getladdrs(sock,..)+usrsctp_freeladdrs to get the full sockaddr_conn, but
that's a full malloc/free cycle.

+  SocketChannelMap::iterator it = sock_channel_map_.find(sock);

[pthatcher1, 2015/08/05 21:42:55]

You can use 

auto it = sock_channel_map_.find(sock);


And then you don't need to have the SocketChannelMap typedef.

[lally1, 2015/08/18 20:38:56]

Thanks!

+  if (it == sock_channel_map_.end()) {

[pthatcher1, 2015/08/05 21:42:55]

Since there are almost always a very low number of SctpDataMediaChannels at one
time (1 or 2), it would be usually be more efficient and perhaps easier to
manager to just iterate over a vector of channels to find the one that has the
matching socket.

[lally1, 2015/08/18 20:38:56]

Done.

+    // Socket is in the process of shutdown.
+    LOG(LS_ERROR) << "Failed to get SctpDataMediaChannel for socket " << sock;
+    return 0;
+  }
+
+  it->second->SignalReadyToSend(true);
+  return 0;
+}
+
 bool SctpDataMediaChannel::OpenSctpSocket() {
   if (sock_) {
     LOG(LS_VERBOSE) << debug_name_
                     << "->Ignoring attempt to re-create existing socket.";
     return false;
   }
+
+  const int send_threshold = usrsctp_sysctl_get_sctp_sendspace() / 2;
+
   sock_ = usrsctp_socket(AF_CONN, SOCK_STREAM, IPPROTO_SCTP,
-                         cricket::OnSctpInboundPacket, NULL, 0, this);
+                         cricket::OnSctpInboundPacket,
+                         &SctpDataMediaChannel::SendThresholdCallback,
+                         send_threshold, this);
   if (!sock_) {
     LOG_ERRNO(LS_ERROR) << debug_name_ << "Failed to create SCTP socket.";
     return false;
   }
 
+  sock_channel_map_[sock_] = this;
+
   // Make the socket non-blocking. Connect, close, shutdown etc will not block
   // the thread waiting for the socket operation to complete.
   if (usrsctp_set_non_blocking(sock_, 1) < 0) {
@@ -393,7 +417,8 @@ bool SctpDataMediaChannel::OpenSctpSocket() {
   }
 
   // Disable MTU discovery
-  struct sctp_paddrparams params = {{0}};
+  struct sctp_paddrparams params;
+  memset(&params, 0, sizeof(params));
   params.spp_assoc_id = 0;
   params.spp_flags = SPP_PMTUD_DISABLE;
   params.spp_pathmtu = kSctpMtu;
@@ -437,6 +462,12 @@ void SctpDataMediaChannel::CloseSctpSocket() {
     // close is called. This means that any pending packets in usrsctp will be
     // discarded instead of being sent.
     usrsctp_close(sock_);
+    SocketChannelMap::iterator it = sock_channel_map_.find(sock_);
+    if (it != sock_channel_map_.end()) {
+      sock_channel_map_.erase(it);
+    } else {
+      LOG(LS_ERROR) << "CloseSctpSocket: the socket wasn't registered.";
+    }

[pthatcher1, 2015/08/05 21:42:55]

I think this works, and it shorter and only does one lookup:

if (!sock_channel_map_.erase(sock_)) {
  LOG(LS_ERROR) << "CloseSctpSocket: the socket wasn't registered.";
}

[lally1, 2015/08/18 20:38:56]

Thanks.  Moved above into ~SctpDataMediaChannel(), but as it's a vector<>, we
can't use this form of erase().

     sock_ = NULL;
     usrsctp_deregister_address(this);
   }
@@ -904,10 +935,13 @@ bool SctpDataMediaChannel::SetRecvCodecs(const std::vector<DataCodec>& codecs) {
 
 void SctpDataMediaChannel::OnPacketFromSctpToNetwork(
     rtc::Buffer* buffer) {
-  if (buffer->size() > kSctpMtu) {
+  // this is completely made up.
+  const int kSctpOverhead = 76;

[pthatcher1, 2015/08/05 21:42:55]

This could use a little more documentation, even if to explain why it can't be
0.  

[lally1, 2015/08/18 20:38:56]

Done.

+  if (buffer->size() > (kSctpOverhead + kSctpMtu)) {
     LOG(LS_ERROR) << debug_name_ << "->OnPacketFromSctpToNetwork(...): "
                   << "SCTP seems to have made a packet that is bigger "
-                     "than its official MTU.";
+                  << "than its official MTU: " << buffer->size()
+                  << " vs max of " << kSctpMtu;

[pthatcher1, 2015/08/05 21:42:55]

Should probably include something like "assuming X bytes of SCTP overhead".

[lally1, 2015/08/18 20:38:56]

Done.

   }
   MediaChannel::SendPacket(buffer);
 }