(try 2) 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..ab6e2ee5007a302e74dc30ce81b31ea01bfbdd4a 100644
--- a/talk/media/sctp/sctpdataengine.cc
+++ b/talk/media/sctp/sctpdataengine.cc
@@ -109,6 +109,8 @@ typedef rtc::ScopedMessageData<rtc::Buffer> OutboundPacketMessage;
 // take off 80 bytes for DTLS/TURN/TCP/IP overhead.
 static const size_t kSctpMtu = 1200;
 
+// The size of the SCTP association send buffer.  256kB, the usrsctp default.
+static const int kSendBufferSize = 262144;
 enum {
   MSG_SCTPINBOUNDPACKET = 1,   // MessageData is SctpInboundPacket
   MSG_SCTPOUTBOUNDPACKET = 2,  // MessageData is rtc:Buffer
@@ -177,11 +179,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);
     }
@@ -258,6 +260,13 @@ SctpDataEngine::SctpDataEngine() {
     // TODO(ldixon): Consider turning this on/off.
     usrsctp_sysctl_set_sctp_ecn_enable(0);
 
+    // This is harmless, but we should find out when the library default

[tommi, 2015/08/28 18:12:32]

move to error scope?

[lally1, 2015/08/28 18:53:11]

I changed the comment a little (s/happens/changes/), but don't know what you
mean here.  The log already LS_ERROR, is that what you meant?  

+    // happens.
+    int send_size = usrsctp_sysctl_get_sctp_sendspace();
+    if (send_size != kSendBufferSize) {
+      LOG(LS_ERROR) << "Got different send size than expected: " << send_size;
+    }
+
     // TODO(ldixon): Consider turning this on/off.
     // This is not needed right now (we don't do dynamic address changes):
     // If SCTP Auto-ASCONF is enabled, the peer is informed automatically
@@ -315,6 +324,48 @@ DataMediaChannel* SctpDataEngine::CreateChannel(
   return new SctpDataMediaChannel(rtc::Thread::Current());
 }
 
+// static
+SctpDataMediaChannel* SctpDataEngine::GetChannelFromSocket(
+    struct socket* sock) {
+  struct sockaddr* addrs = nullptr;
+  int naddrs = usrsctp_getladdrs(sock, 0, &addrs);
+  DCHECK(naddrs > 0);

[tommi, 2015/08/28 18:12:32]

Generally in chromium (the guidelines webrtc follows), a DCHECK is used to
assert and document expectations.  The condition in the DCHECK is then not
repeated in the production code.  The reason for this is that the DCHECK should
have caught unexpected situations and if we encounter those unexpected
situations out in the field, the code should just crash.

So, should we move this DCHECK down below the check in line 333 and remove the
if (naddres > 0)?

It's up to you, I'm not hung up on it.

[pthatcher2, 2015/08/28 18:36:59]

if (naddrs < 0) {
  ASSERT(false);
  return nullptr;
}

And DCHECK is just the rename ASSERT, and so there is already much code that
looks like this:


if (naddrs < 0) {
  DCHECK(false);
  return nullptr;
}

Peronally, I like it that way.

[pthatcher2, 2015/08/28 18:38:15]

Actually, in this case, I'd rather not have GetChannelFromSocket have a DCHECK
at all.  Just return null and let the caller do a DCHECK against being non-null.

[lally1, 2015/08/28 18:53:11]

I took out the DCHECKs and merged the if()s a bit.

+  if (naddrs < 0) {
+    return nullptr;
+  }
+  SctpDataMediaChannel* channel = nullptr;
+  if (naddrs > 0) {

[pthatcher2, 2015/08/28 18:40:44]

Huh? Why check naddrs so many times?

if (naddrs < 0) {
  return nullptr;
}
SctpDataMediaChannel* channel = nullptr;
if (naddrs > 0) {
  // ...
}
return channel;


Then it's shorter and less indented.

Why not do this?


if (naddrs <= 0) {
  return nullptr;
}
SctpDataMediaChannel* channel = nullptr;
// ...
return channel;

[lally1, 2015/08/28 18:53:11]

Acknowledged.

+    // We only open AF_CONN sockets, and they should all have the
+    // sconn_addr set to the pointer that created them, so [0] is as good
+    // as any other.
+    DCHECK(addrs[0].sa_family == AF_CONN);
+    if (addrs[0].sa_family == AF_CONN) {

[pthatcher2, 2015/08/28 18:44:49]

In this case, I'm fine with a DCHECK with no if statement.

[lally1, 2015/08/28 18:53:11]

Acknowledged.

+      struct sockaddr_conn* sconn =
+        reinterpret_cast<struct sockaddr_conn*>(&addrs[0]);

[tommi, 2015/08/28 18:12:32]

nit: 4 space indent

[lally1, 2015/08/28 18:53:11]

Acknowledged.

+      channel = reinterpret_cast<SctpDataMediaChannel*>(sconn->sconn_addr);
+    }
+    usrsctp_freeladdrs(addrs);

[pthatcher2, 2015/08/28 18:44:49]

Can you put a little comment about what this does?

+  }
+
+  return channel;
+}
+
+// static
+int SctpDataEngine::SendThresholdCallback(struct socket* sock,
+                                          uint32_t sb_free) {
+  // Fired on our I/O thread.  SctpDataMediaChannel::OnPacketReceived() gets
+  // a packet containing acknowledgments, which goes into usrsctp_conninput,
+  // and then back here.

[tommi, 2015/08/28 18:12:32]

nice

+  SctpDataMediaChannel* channel = GetChannelFromSocket(sock);
+  if (!channel) {
+    LOG(LS_ERROR) << "SendThresholdCallback: Failed to get channel for socket "
+                  << sock;
+    return 0;
+  }
+  channel->OnSendThresholdCallback();
+  return 0;
+}
+
 SctpDataMediaChannel::SctpDataMediaChannel(rtc::Thread* thread)
     : worker_thread_(thread),
       local_port_(kSctpDefaultPort),
@@ -329,6 +380,11 @@ SctpDataMediaChannel::~SctpDataMediaChannel() {
   CloseSctpSocket();
 }
 
+void SctpDataMediaChannel::OnSendThresholdCallback() {
+  DCHECK(rtc::Thread::Current() == worker_thread_);

[tommi, 2015/08/28 18:12:32]

thanks!

+  SignalReadyToSend(true);
+}
+
 sockaddr_conn SctpDataMediaChannel::GetSctpSockAddr(int port) {
   sockaddr_conn sconn = {0};
   sconn.sconn_family = AF_CONN;
@@ -347,8 +403,16 @@ bool SctpDataMediaChannel::OpenSctpSocket() {
                     << "->Ignoring attempt to re-create existing socket.";
     return false;
   }
+
+  // If kSendBufferSize isn't reflective of reality, we log an error, but we
+  // still have to do something reasonable here.  Look up what the buffer's
+  // real size is and set our threshold to something reasonable.
+  const static int kSendThreshold = usrsctp_sysctl_get_sctp_sendspace() / 2;
+
   sock_ = usrsctp_socket(AF_CONN, SOCK_STREAM, IPPROTO_SCTP,
-                         cricket::OnSctpInboundPacket, NULL, 0, this);
+                         cricket::OnSctpInboundPacket,
+                         &SctpDataEngine::SendThresholdCallback,
+                         kSendThreshold, this);
   if (!sock_) {
     LOG_ERRNO(LS_ERROR) << debug_name_ << "Failed to create SCTP socket.";
     return false;
@@ -393,7 +457,7 @@ bool SctpDataMediaChannel::OpenSctpSocket() {
   }
 
   // Disable MTU discovery
-  struct sctp_paddrparams params = {{0}};
+  sctp_paddrparams params = {{0}};
   params.spp_assoc_id = 0;
   params.spp_flags = SPP_PMTUD_DISABLE;
   params.spp_pathmtu = kSctpMtu;
@@ -608,7 +672,7 @@ void SctpDataMediaChannel::OnPacketReceived(
     // Pass received packet to SCTP stack. Once processed by usrsctp, the data
     // will be will be given to the global OnSctpInboundData, and then,
     // marshalled by a Post and handled with OnMessage.
-
+    DCHECK(rtc::Thread::Current() == worker_thread_);

[pthatcher2, 2015/08/28 18:44:49]

Why not just put this at the top of the method?

[lally1, 2015/08/28 18:53:11]

Done.

     VerboseLogPacket(packet->data(), packet->size(), SCTP_DUMP_INBOUND);
     usrsctp_conninput(this, packet->data(), packet->size(), 0);
   } else {
@@ -904,10 +968,17 @@ bool SctpDataMediaChannel::SetRecvCodecs(const std::vector<DataCodec>& codecs) {
 
 void SctpDataMediaChannel::OnPacketFromSctpToNetwork(
     rtc::Buffer* buffer) {
-  if (buffer->size() > kSctpMtu) {
+  // usrsctp seems to interpret the MTU we give it strangely -- it seems to
+  // give us back packets bigger than that MTU, if only by a fixed amount.
+  // This is that amount that we've observed.
+  const int kSctpOverhead = 76;
+  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
+                  << " even after adding " << kSctpOverhead
+                  << " extra SCTP overhead";
   }
   MediaChannel::SendPacket(buffer);
 }