Fix issue with send-side bandwidth estimation over TURN TCP connections.

webrtc/p2p/base/asyncstuntcpsocket.cc

 /*
  *  Copyright 2013 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 61 matching lines @@
   char padding[4] = {0};
   AppendToOutBuffer(padding, pad_bytes);
 
   int res = FlushOutBuffer();
   if (res <= 0) {
     // drop packet if we made no progress
     ClearOutBuffer();
     return res;
   }
 
+  rtc::SentPacket sent_packet(options.packet_id, rtc::TimeMillis());

[juberti1, 2017/05/27 23:17:21]

This signal should probably only be fired when the packet has been fully
serialized. 

That said, this change will still be an improvement.

[holmer, 2017/06/02 07:08:00]

Yes, I agree. If I correctly understand how the TCP socket classes interact, I'd
say the signal should be fired in the implementation of socket_:
https://cs.chromium.org/chromium/src/third_party/webrtc/base/asynctcpsocket.c...

which should be where we send the packet to the real socket.


When we run in Chrome, I assume that socket may be the IpcPacketSocket? In that
case we really want the signal fired from the browser process, which maybe this
signal now overrides?

If I've understood correctly, I think we may want to connect to this signal
instead:
https://cs.chromium.org/chromium/src/content/renderer/p2p/ipc_socket_factory....

[Taylor Brandstetter, 2017/06/02 17:42:43]

I guess I don't see how filling a buffer in userspace differs fundamentally from
filling a buffer in kernel space. I thought the main purpose of SignalSentPacket
was to eliminate jitter due to IPC between the browser/renderer processes.

But anyway: this is copying the existing behavior of AsyncTcpSocket, so if this
is an issue it's one that already existed.
In chromium, we connect to that signal; in native apps we connect to this one.
This class is only created by BasicPacketSocketFactory, but chromium uses
IpcPacketSocketFactory as you mentioned.

+  SignalSentPacket(this, sent_packet);
+
   // We claim to have sent the whole thing, even if we only sent partial
   return static_cast<int>(cb);
 }
 
 void AsyncStunTCPSocket::ProcessInput(char* data, size_t* len) {
   rtc::SocketAddress remote_addr(GetRemoteAddress());
   // STUN packet - First 4 bytes. Total header size is 20 bytes.
   // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   // |0 0|     STUN Message Type     |         Message Length        |
   // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
@@ skipping 52 matching lines @@
     // message (including padding) is (4 + Length) rounded up to the nearest
     // multiple of 4.  Over UDP, the padding is not required but MAY be
     // included.
     if (expected_pkt_len % 4)
       *pad_bytes = 4 - (expected_pkt_len % 4);
   }
   return expected_pkt_len;
 }
 
 }  // namespace cricket