From 7391881f9762ccadeeb0249560b33cf2bcfaf7f9 Mon Sep 17 00:00:00 2001 From: tommi Date: Thu, 27 Aug 2015 04:29:58 -0700 Subject: [PATCH] Revert of Added send-thresholding and fixed text packet dumping. (patchset #4 id:160001 of https://codereview.webrtc.org/1266033005/ ) Reason for revert: The CL adds a global variable. Original issue's description: > Added send-thresholding and fixed text packet dumping. Also a little squelch for the over-max-MTU log spam we see in there. > > BUG=https://code.google.com/p/webrtc/issues/detail?id=4468 > R=pthatcher@chromium.org, pthatcher@webrtc.org > > Committed: https://chromium.googlesource.com/external/webrtc/+/d838d2791979bb50f464a61c557d55c6a324621e TBR=pthatcher@webrtc.org,bemasc@webrtc.org,pthatcher@chromium.org,thakis@chromium.org,lally@webrtc.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=https://code.google.com/p/webrtc/issues/detail?id=4468 Review URL: https://codereview.webrtc.org/1315923003 Cr-Commit-Position: refs/heads/master@{#9796} --- talk/media/sctp/sctpdataengine.cc | 81 ++-------------------- talk/media/sctp/sctpdataengine.h | 13 +--- talk/media/sctp/sctpdataengine_unittest.cc | 23 ------ 3 files changed, 8 insertions(+), 109 deletions(-) diff --git a/talk/media/sctp/sctpdataengine.cc b/talk/media/sctp/sctpdataengine.cc index 19cdddc426..8c8a6a192f 100644 --- a/talk/media/sctp/sctpdataengine.cc +++ b/talk/media/sctp/sctpdataengine.cc @@ -109,8 +109,6 @@ typedef rtc::ScopedMessageData 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 @@ -179,11 +177,11 @@ static bool GetDataMediaType( } // Log the packet in text2pcap format, if log level is at LS_VERBOSE. -static void VerboseLogPacket(void *data, size_t length, int direction) { +static void VerboseLogPacket(void *addr, size_t length, int direction) { if (LOG_CHECK_LEVEL(LS_VERBOSE) && length > 0) { char *dump_buf; if ((dump_buf = usrsctp_dumppacket( - data, length, direction)) != NULL) { + addr, length, direction)) != NULL) { LOG(LS_VERBOSE) << dump_buf; usrsctp_freedumpbuffer(dump_buf); } @@ -246,10 +244,6 @@ static int OnSctpInboundPacket(struct socket* sock, union sctp_sockstore addr, // Set the initial value of the static SCTP Data Engines reference count. int SctpDataEngine::usrsctp_engines_count = 0; -// All the channels created by this engine, used for callbacks from -// usrsctplib that only contain socket pointers. Channels are removed when -// SignalDestroyed is fired. -std::vector SctpDataEngine::channels_; SctpDataEngine::SctpDataEngine() { if (usrsctp_engines_count == 0) { @@ -264,11 +258,6 @@ SctpDataEngine::SctpDataEngine() { // TODO(ldixon): Consider turning this on/off. usrsctp_sysctl_set_sctp_ecn_enable(0); - 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 @@ -323,48 +312,9 @@ DataMediaChannel* SctpDataEngine::CreateChannel( if (data_channel_type != DCT_SCTP) { return NULL; } - SctpDataMediaChannel *channel = new SctpDataMediaChannel( - rtc::Thread::Current()); - channels_.push_back(channel); - channel->SignalDestroyed.connect(this, &SctpDataEngine::OnChannelDestroyed); - return channel; + return new SctpDataMediaChannel(rtc::Thread::Current()); } -// static -SctpDataMediaChannel* SctpDataEngine::GetChannelFromSocket( - struct socket* sock) { - for (auto p:channels_) { - if (p->socket() == sock) { - return p; - } - } - return 0; -} - - -void SctpDataEngine::OnChannelDestroyed(SctpDataMediaChannel* channel) { - auto it = std::find(channels_.begin(), channels_.end(), channel); - if (it == channels_.end()) { - LOG(LS_ERROR) << "OnChannelDestroyed: the channel wasn't registered."; - return; - } - channels_.erase(it); -} - -// static -int SctpDataEngine::SendThresholdCallback(struct socket* sock, - uint32_t sb_free) { - SctpDataMediaChannel *channel = GetChannelFromSocket(sock); - if (!channel) { - LOG(LS_ERROR) << "SendThresholdCallback: Failed to get channel for socket " - << sock; - return 0; - } - channel->SignalReadyToSend(true); - return 0; -} - - SctpDataMediaChannel::SctpDataMediaChannel(rtc::Thread* thread) : worker_thread_(thread), local_port_(kSctpDefaultPort), @@ -377,7 +327,6 @@ SctpDataMediaChannel::SctpDataMediaChannel(rtc::Thread* thread) SctpDataMediaChannel::~SctpDataMediaChannel() { CloseSctpSocket(); - SignalDestroyed(this); } sockaddr_conn SctpDataMediaChannel::GetSctpSockAddr(int port) { @@ -398,16 +347,8 @@ 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 send_threshold = usrsctp_sysctl_get_sctp_sendspace() / 2; - sock_ = usrsctp_socket(AF_CONN, SOCK_STREAM, IPPROTO_SCTP, - cricket::OnSctpInboundPacket, - &SctpDataEngine::SendThresholdCallback, - send_threshold, this); + cricket::OnSctpInboundPacket, NULL, 0, this); if (!sock_) { LOG_ERRNO(LS_ERROR) << debug_name_ << "Failed to create SCTP socket."; return false; @@ -452,8 +393,7 @@ bool SctpDataMediaChannel::OpenSctpSocket() { } // Disable MTU discovery - struct sctp_paddrparams params; - memset(¶ms, 0, sizeof(params)); + struct sctp_paddrparams params = {{0}}; params.spp_assoc_id = 0; params.spp_flags = SPP_PMTUD_DISABLE; params.spp_pathmtu = kSctpMtu; @@ -964,17 +904,10 @@ bool SctpDataMediaChannel::SetRecvCodecs(const std::vector& codecs) { void SctpDataMediaChannel::OnPacketFromSctpToNetwork( rtc::Buffer* buffer) { - // 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)) { + if (buffer->size() > kSctpMtu) { LOG(LS_ERROR) << debug_name_ << "->OnPacketFromSctpToNetwork(...): " << "SCTP seems to have made a packet that is bigger " - << "than its official MTU: " << buffer->size() - << " vs max of " << kSctpMtu - << " even after adding " << kSctpOverhead - << " extra SCTP overhead"; + "than its official MTU."; } MediaChannel::SendPacket(buffer); } diff --git a/talk/media/sctp/sctpdataengine.h b/talk/media/sctp/sctpdataengine.h index a591542bd6..86bfa37a48 100644 --- a/talk/media/sctp/sctpdataengine.h +++ b/talk/media/sctp/sctpdataengine.h @@ -64,8 +64,6 @@ const uint32 kMaxSctpSid = 1023; // usrsctp.h) const int kSctpDefaultPort = 5000; -class SctpDataMediaChannel; - // A DataEngine that interacts with usrsctp. // // From channel calls, data flows like this: @@ -90,7 +88,7 @@ class SctpDataMediaChannel; // 14. SctpDataMediaChannel::SignalDataReceived(data) // [from the same thread, methods registered/connected to // SctpDataMediaChannel are called with the recieved data] -class SctpDataEngine : public DataEngineInterface, public sigslot::has_slots<> { +class SctpDataEngine : public DataEngineInterface { public: SctpDataEngine(); virtual ~SctpDataEngine(); @@ -99,15 +97,9 @@ class SctpDataEngine : public DataEngineInterface, public sigslot::has_slots<> { virtual const std::vector& data_codecs() { return codecs_; } - static int SendThresholdCallback(struct socket* sock, uint32_t sb_free); - private: - static std::vector channels_; static int usrsctp_engines_count; std::vector codecs_; - - static SctpDataMediaChannel* GetChannelFromSocket(struct socket* sock); - void OnChannelDestroyed(SctpDataMediaChannel *channel); }; // TODO(ldixon): Make into a special type of TypedMessageData. @@ -196,9 +188,6 @@ class SctpDataMediaChannel : public DataMediaChannel, debug_name_ = debug_name; } const std::string& debug_name() const { return debug_name_; } - const struct socket* socket() { return sock_; } - - sigslot::signal1 SignalDestroyed; private: sockaddr_conn GetSctpSockAddr(int port); diff --git a/talk/media/sctp/sctpdataengine_unittest.cc b/talk/media/sctp/sctpdataengine_unittest.cc index 437253087d..5b4c09e6a7 100644 --- a/talk/media/sctp/sctpdataengine_unittest.cc +++ b/talk/media/sctp/sctpdataengine_unittest.cc @@ -240,16 +240,10 @@ class SctpDataMediaChannelTest : public testing::Test, net2_.reset(new SctpFakeNetworkInterface(rtc::Thread::Current())); recv1_.reset(new SctpFakeDataReceiver()); recv2_.reset(new SctpFakeDataReceiver()); - chan1_ready_to_send_count_ = 0; - chan2_ready_to_send_count_ = 0; chan1_.reset(CreateChannel(net1_.get(), recv1_.get())); chan1_->set_debug_name("chan1/connector"); - chan1_->SignalReadyToSend.connect( - this, &SctpDataMediaChannelTest::OnChan1ReadyToSend); chan2_.reset(CreateChannel(net2_.get(), recv2_.get())); chan2_->set_debug_name("chan2/listener"); - chan2_->SignalReadyToSend.connect( - this, &SctpDataMediaChannelTest::OnChan2ReadyToSend); // Setup two connected channels ready to send and receive. net1_->SetDestination(chan2_.get()); net2_->SetDestination(chan1_.get()); @@ -336,8 +330,6 @@ class SctpDataMediaChannelTest : public testing::Test, SctpFakeDataReceiver* receiver1() { return recv1_.get(); } SctpFakeDataReceiver* receiver2() { return recv2_.get(); } - int channel1_ready_to_send_count() { return chan1_ready_to_send_count_; } - int channel2_ready_to_send_count() { return chan2_ready_to_send_count_; } private: rtc::scoped_ptr engine_; rtc::scoped_ptr net1_; @@ -346,12 +338,6 @@ class SctpDataMediaChannelTest : public testing::Test, rtc::scoped_ptr recv2_; rtc::scoped_ptr chan1_; rtc::scoped_ptr chan2_; - - int chan1_ready_to_send_count_; - int chan2_ready_to_send_count_; - - void OnChan1ReadyToSend(bool send) { if (send) chan1_ready_to_send_count_++; } - void OnChan2ReadyToSend(bool send) { if (send) chan2_ready_to_send_count_++; } }; // Verifies that SignalReadyToSend is fired. @@ -500,15 +486,6 @@ TEST_F(SctpDataMediaChannelTest, ClosesStreamsOnBothSides) { EXPECT_TRUE_WAIT(chan_1_sig_receiver.WasStreamClosed(4), 1000); } -TEST_F(SctpDataMediaChannelTest, EngineSignalsRightChannel) { - SetupConnectedChannels(); - EXPECT_TRUE_WAIT(channel1()->socket() != NULL, 1000); - struct socket *sock = const_cast(channel1()->socket()); - int prior_count = channel1_ready_to_send_count(); - cricket::SctpDataEngine::SendThresholdCallback(sock, 0); - EXPECT_GT(channel1_ready_to_send_count(), prior_count); -} - // Flaky on Linux and Windows. See webrtc:4453. #if defined(WEBRTC_WIN) || defined(WEBRTC_LINUX) #define MAYBE_ReusesAStream DISABLED_ReusesAStream