From fbb45bd02f8ce33b24bbd5bc1c16aa5c22571729 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Wed, 15 May 2019 08:07:47 +0200 Subject: [PATCH] Send and parse SCTP max-message-size in SDP This also changes the default when no max-message-size is set to the protocol defined value of 64K, and prevents messages from being sent when they are too large to send. Bug: webrtc:10358 Change-Id: Iacc1dd774d1554d9f27315378fbea6351300b5cc Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/135948 Commit-Queue: Harald Alvestrand Reviewed-by: Steve Anton Cr-Commit-Position: refs/heads/master@{#27945} --- media/sctp/sctp_transport.cc | 36 ++++++++++++----- media/sctp/sctp_transport.h | 4 +- media/sctp/sctp_transport_internal.h | 15 +++++-- media/sctp/sctp_transport_unittest.cc | 48 +++++++++++++++++----- pc/media_session.cc | 7 ++++ pc/peer_connection.cc | 47 ++++++++++------------ pc/peer_connection.h | 3 +- pc/sctp_transport.cc | 10 ++++- pc/sctp_transport_unittest.cc | 5 ++- pc/session_description.h | 3 +- pc/test/fake_sctp_transport.h | 5 ++- pc/webrtc_sdp.cc | 58 +++++++++++++++------------ pc/webrtc_sdp_unittest.cc | 28 +++++++++++++ 13 files changed, 189 insertions(+), 80 deletions(-) diff --git a/media/sctp/sctp_transport.cc b/media/sctp/sctp_transport.cc index 8bcec41a28..b99a55b7f8 100644 --- a/media/sctp/sctp_transport.cc +++ b/media/sctp/sctp_transport.cc @@ -46,9 +46,6 @@ namespace { // take off 80 bytes for DTLS/TURN/TCP/IP overhead. static constexpr size_t kSctpMtu = 1200; -// The size of the SCTP association send buffer. 256kB, the usrsctp default. -static constexpr int kSendBufferSize = 256 * 1024; - // Set the initial value of the static SCTP Data Engines reference count. int g_usrsctp_usage_count = 0; rtc::GlobalLockPod g_usrsctp_lock_; @@ -185,7 +182,7 @@ class SctpTransport::UsrSctpWrapper { // This is harmless, but we should find out when the library default // changes. int send_size = usrsctp_sysctl_get_sctp_sendspace(); - if (send_size != kSendBufferSize) { + if (send_size != kSctpSendBufferSize) { RTC_LOG(LS_ERROR) << "Got different send size than expected: " << send_size; } @@ -327,7 +324,7 @@ class SctpTransport::UsrSctpWrapper { // callback. Larger messages (originating from other implementations) will // still be delivered in chunks. if (!(flags & MSG_EOR) && - (transport->partial_message_.size() < kSendBufferSize)) { + (transport->partial_message_.size() < kSctpSendBufferSize)) { return 1; } @@ -410,7 +407,9 @@ void SctpTransport::SetDtlsTransport(rtc::PacketTransportInternal* transport) { } } -bool SctpTransport::Start(int local_sctp_port, int remote_sctp_port) { +bool SctpTransport::Start(int local_sctp_port, + int remote_sctp_port, + int max_message_size) { RTC_DCHECK_RUN_ON(network_thread_); if (local_sctp_port == -1) { local_sctp_port = kSctpDefaultPort; @@ -418,6 +417,20 @@ bool SctpTransport::Start(int local_sctp_port, int remote_sctp_port) { if (remote_sctp_port == -1) { remote_sctp_port = kSctpDefaultPort; } + if (max_message_size > kSctpSendBufferSize) { + RTC_LOG(LS_ERROR) << "Max message size of " << max_message_size + << " is larger than send bufffer size " + << kSctpSendBufferSize; + return false; + } + if (max_message_size < 1) { + RTC_LOG(LS_ERROR) << "Max message size of " << max_message_size + << " is too small"; + return false; + } + // We allow changing max_message_size with a second Start() call, + // but not changing the port numbers. + max_message_size_ = max_message_size; if (started_) { if (local_sctp_port != local_port_ || remote_sctp_port != remote_port_) { RTC_LOG(LS_ERROR) @@ -537,6 +550,11 @@ bool SctpTransport::SendData(const SendDataParams& params, } } + if (payload.size() > static_cast(max_message_size_)) { + RTC_LOG(LS_ERROR) << "Attempting to send message of size " << payload.size() + << " which is larger than limit " << max_message_size_; + return false; + } // We don't fragment. send_res = usrsctp_sendv( sock_, payload.data(), static_cast(payload.size()), NULL, 0, &spa, @@ -657,9 +675,9 @@ bool SctpTransport::OpenSctpSocket() { UsrSctpWrapper::IncrementUsrSctpUsageCount(); - // 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. + // If kSctpSendBufferSize 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. static const int kSendThreshold = usrsctp_sysctl_get_sctp_sendspace() / 2; sock_ = usrsctp_socket( diff --git a/media/sctp/sctp_transport.h b/media/sctp/sctp_transport.h index dccecd86fb..554407b15d 100644 --- a/media/sctp/sctp_transport.h +++ b/media/sctp/sctp_transport.h @@ -72,13 +72,14 @@ class SctpTransport : public SctpTransportInternal, // SctpTransportInternal overrides (see sctptransportinternal.h for comments). void SetDtlsTransport(rtc::PacketTransportInternal* transport) override; - bool Start(int local_port, int remote_port) override; + bool Start(int local_port, int remote_port, int max_message_size) override; bool OpenStream(int sid) override; bool ResetStream(int sid) override; bool SendData(const SendDataParams& params, const rtc::CopyOnWriteBuffer& payload, SendDataResult* result = nullptr) override; bool ReadyToSendData() override; + int max_message_size() const override { return max_message_size_; } void set_debug_name_for_testing(const char* debug_name) override { debug_name_ = debug_name; } @@ -151,6 +152,7 @@ class SctpTransport : public SctpTransportInternal, bool was_ever_writable_ = false; int local_port_ = kSctpDefaultPort; int remote_port_ = kSctpDefaultPort; + int max_message_size_ = kSctpSendBufferSize; struct socket* sock_ = nullptr; // The socket created by usrsctp_socket(...). // Has Start been called? Don't create SCTP socket until it has. diff --git a/media/sctp/sctp_transport_internal.h b/media/sctp/sctp_transport_internal.h index 12f1782783..c08c41483d 100644 --- a/media/sctp/sctp_transport_internal.h +++ b/media/sctp/sctp_transport_internal.h @@ -28,6 +28,10 @@ namespace cricket { +// Constants that are important to API users +// The size of the SCTP association send buffer. 256kB, the usrsctp default. +constexpr int kSctpSendBufferSize = 256 * 1024; + // The number of outgoing streams that we'll negotiate. Since stream IDs (SIDs) // are 0-based, the highest usable SID is 1023. // @@ -67,13 +71,16 @@ class SctpTransportInternal { // listener and connector must be using the same port. They are not related // to the ports at the IP level. If set to -1, we default to // kSctpDefaultPort. + // |max_message_size_| sets the max message size on the connection. + // It must be smaller than or equal to kSctpSendBufferSize. + // It can be changed by a secons Start() call. // - // TODO(deadbeef): Add remote max message size as parameter to Start, once we - // start supporting it. // TODO(deadbeef): Support calling Start with different local/remote ports // and create a new association? Not clear if this is something we need to // support though. See: https://github.com/w3c/webrtc-pc/issues/979 - virtual bool Start(int local_sctp_port, int remote_sctp_port) = 0; + virtual bool Start(int local_sctp_port, + int remote_sctp_port, + int max_message_size) = 0; // NOTE: Initially there was a "Stop" method here, but it was never used, so // it was removed. @@ -105,6 +112,8 @@ class SctpTransportInternal { // ICE channels may be unwritable while ReadyToSendData is true, because data // can still be queued in usrsctp. virtual bool ReadyToSendData() = 0; + // Returns the current max message size, set with Start(). + virtual int max_message_size() const = 0; sigslot::signal0<> SignalReadyToSendData; // ReceiveDataParams includes SID, seq num, timestamp, etc. CopyOnWriteBuffer diff --git a/media/sctp/sctp_transport_unittest.cc b/media/sctp/sctp_transport_unittest.cc index 86cb416e22..d7ccd584c1 100644 --- a/media/sctp/sctp_transport_unittest.cc +++ b/media/sctp/sctp_transport_unittest.cc @@ -151,8 +151,8 @@ class SctpTransportTest : public ::testing::Test, public sigslot::has_slots<> { << "Connect the transports -----------------------------"; // Both transports need to have started (with matching ports) for an // association to be formed. - transport1_->Start(port1, port2); - transport2_->Start(port2, port1); + transport1_->Start(port1, port2, kSctpSendBufferSize); + transport2_->Start(port2, port1, kSctpSendBufferSize); } bool AddStream(int sid) { @@ -251,8 +251,8 @@ TEST_F(SctpTransportTest, SwitchDtlsTransport) { transport2->OpenStream(1); // Tell them both to start (though transport1_ is connected to black_hole). - transport1->Start(kTransport1Port, kTransport2Port); - transport2->Start(kTransport2Port, kTransport1Port); + transport1->Start(kTransport1Port, kTransport2Port, kSctpSendBufferSize); + transport2->Start(kTransport2Port, kTransport1Port, kSctpSendBufferSize); // Switch transport1_ to the normal fake_dtls1_ transport. transport1->SetDtlsTransport(&fake_dtls1); @@ -276,7 +276,8 @@ TEST_F(SctpTransportTest, SwitchDtlsTransport) { // Calling Start twice shouldn't do anything bad, if with the same parameters. TEST_F(SctpTransportTest, DuplicateStartCallsIgnored) { SetupConnectedTransportsWithTwoStreams(); - EXPECT_TRUE(transport1()->Start(kTransport1Port, kTransport2Port)); + EXPECT_TRUE(transport1()->Start(kTransport1Port, kTransport2Port, + kSctpSendBufferSize)); // Make sure we can still send/recv data. SendDataResult result; @@ -289,8 +290,8 @@ TEST_F(SctpTransportTest, DuplicateStartCallsIgnored) { // Calling Start a second time with a different port should fail. TEST_F(SctpTransportTest, CallingStartWithDifferentPortFails) { SetupConnectedTransportsWithTwoStreams(); - EXPECT_FALSE(transport1()->Start(kTransport1Port, 1234)); - EXPECT_FALSE(transport1()->Start(1234, kTransport2Port)); + EXPECT_FALSE(transport1()->Start(kTransport1Port, 1234, kSctpSendBufferSize)); + EXPECT_FALSE(transport1()->Start(1234, kTransport2Port, kSctpSendBufferSize)); } // A value of -1 for the local/remote port should be treated as the default @@ -311,8 +312,8 @@ TEST_F(SctpTransportTest, NegativeOnePortTreatedAsDefault) { // Tell them both to start, giving one transport the default port and the // other transport -1. - transport1->Start(kSctpDefaultPort, kSctpDefaultPort); - transport2->Start(-1, -1); + transport1->Start(kSctpDefaultPort, kSctpDefaultPort, kSctpSendBufferSize); + transport2->Start(-1, -1, kSctpSendBufferSize); // Connect the two fake DTLS transports. bool asymmetric = false; @@ -351,7 +352,7 @@ TEST_F(SctpTransportTest, SignalReadyToSendDataAfterDtlsWritable) { std::unique_ptr transport(CreateTransport(&fake_dtls, &recv)); SctpTransportObserver observer(transport.get()); - transport->Start(kSctpDefaultPort, kSctpDefaultPort); + transport->Start(kSctpDefaultPort, kSctpDefaultPort, kSctpSendBufferSize); fake_dtls.SetWritable(true); EXPECT_TRUE_WAIT(observer.ReadyToSend(), kDefaultTimeout); } @@ -564,4 +565,31 @@ TEST_F(SctpTransportTest, ReusesAStream) { EXPECT_EQ_WAIT(2, transport2_observer.StreamCloseCount(1), kDefaultTimeout); } +TEST_F(SctpTransportTest, RejectsTooLargeMessageSize) { + FakeDtlsTransport fake_dtls("fake dtls", 0); + SctpFakeDataReceiver recv; + std::unique_ptr transport(CreateTransport(&fake_dtls, &recv)); + + EXPECT_FALSE(transport->Start(kSctpDefaultPort, kSctpDefaultPort, + kSctpSendBufferSize + 1)); +} + +TEST_F(SctpTransportTest, RejectsTooSmallMessageSize) { + FakeDtlsTransport fake_dtls("fake dtls", 0); + SctpFakeDataReceiver recv; + std::unique_ptr transport(CreateTransport(&fake_dtls, &recv)); + + EXPECT_FALSE(transport->Start(kSctpDefaultPort, kSctpDefaultPort, 0)); +} + +TEST_F(SctpTransportTest, RejectsSendTooLargeMessages) { + SetupConnectedTransportsWithTwoStreams(); + // Use "Start" to reduce the max message size + transport1()->Start(kTransport1Port, kTransport2Port, 10); + EXPECT_EQ(10, transport1()->max_message_size()); + const char eleven_characters[] = "12345678901"; + SendDataResult result; + EXPECT_FALSE(SendData(transport1(), 1, eleven_characters, &result)); +} + } // namespace cricket diff --git a/pc/media_session.cc b/pc/media_session.cc index 69ebb841e6..0924e6da91 100644 --- a/pc/media_session.cc +++ b/pc/media_session.cc @@ -25,6 +25,7 @@ #include "api/crypto_params.h" #include "media/base/h264_profile_level_id.h" #include "media/base/media_constants.h" +#include "media/sctp/sctp_transport_internal.h" #include "p2p/base/p2p_constants.h" #include "pc/channel_manager.h" #include "pc/media_protocol_names.h" @@ -2238,6 +2239,8 @@ bool MediaSessionDescriptionFactory::AddSctpDataContentForOffer( data->set_protocol(secure_transport ? kMediaProtocolUdpDtlsSctp : kMediaProtocolSctp); data->set_use_sctpmap(session_options.use_obsolete_sctp_sdp); + data->set_max_message_size(kSctpSendBufferSize); + if (!CreateContentOffer(media_description_options, session_options, sdes_policy, GetCryptos(current_content), crypto_suites, RtpHeaderExtensions(), ssrc_generator_, @@ -2579,6 +2582,10 @@ bool MediaSessionDescriptionFactory::AddDataContentForAnswer( offer_content->media_description()->as_sctp(); // Respond with the offerer's proto, whatever it is. data_answer->as_sctp()->set_protocol(offer_data_description->protocol()); + // Respond with our max message size or the remote max messsage size, + // whichever is smaller. + data_answer->as_sctp()->set_max_message_size(std::min( + offer_data_description->max_message_size(), kSctpSendBufferSize)); if (!CreateMediaContentAnswer( offer_data_description, media_description_options, session_options, sdes_policy, GetCryptos(current_content), RtpHeaderExtensions(), diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index fcd1a2a117..e1cc6ef7a9 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -556,18 +556,6 @@ bool VerifyIceUfragPwdPresent(const SessionDescription* desc) { return true; } -// Get the SCTP port out of a SessionDescription. -// Return -1 if not found. -int GetSctpPort(const SessionDescription* session_description) { - const cricket::SctpDataContentDescription* data_desc = - GetFirstSctpDataContentDescription(session_description); - RTC_DCHECK(data_desc); - if (!data_desc) { - return -1; - } - return data_desc->port(); -} - // Returns true if |new_desc| requests an ICE restart (i.e., new ufrag/pwd). bool CheckForRemoteIceRestart(const SessionDescriptionInterface* old_desc, const SessionDescriptionInterface* new_desc, @@ -5663,17 +5651,24 @@ RTCError PeerConnection::PushdownMediaDescription( // Need complete offer/answer with an SCTP m= section before starting SCTP, // according to https://tools.ietf.org/html/draft-ietf-mmusic-sctp-sdp-19 - if (sctp_transport_ && local_description() && remote_description() && - cricket::GetFirstDataContent(local_description()->description()) && - cricket::GetFirstDataContent(remote_description()->description())) { - bool success = network_thread()->Invoke( - RTC_FROM_HERE, - rtc::Bind(&PeerConnection::PushdownSctpParameters_n, this, source, - GetSctpPort(local_description()->description()), - GetSctpPort(remote_description()->description()))); - if (!success) { - LOG_AND_RETURN_ERROR(RTCErrorType::INTERNAL_ERROR, - "Failed to push down SCTP parameters."); + if (sctp_transport_ && local_description() && remote_description()) { + auto local_sctp_description = cricket::GetFirstSctpDataContentDescription( + local_description()->description()); + auto remote_sctp_description = cricket::GetFirstSctpDataContentDescription( + remote_description()->description()); + if (local_sctp_description && remote_sctp_description) { + int max_message_size = + std::min(local_sctp_description->max_message_size(), + remote_sctp_description->max_message_size()); + bool success = network_thread()->Invoke( + RTC_FROM_HERE, + rtc::Bind(&PeerConnection::PushdownSctpParameters_n, this, source, + local_sctp_description->port(), + remote_sctp_description->port(), max_message_size)); + if (!success) { + LOG_AND_RETURN_ERROR(RTCErrorType::INTERNAL_ERROR, + "Failed to push down SCTP parameters."); + } } } @@ -5682,11 +5677,13 @@ RTCError PeerConnection::PushdownMediaDescription( bool PeerConnection::PushdownSctpParameters_n(cricket::ContentSource source, int local_sctp_port, - int remote_sctp_port) { + int remote_sctp_port, + int max_message_size) { RTC_DCHECK_RUN_ON(network_thread()); // Apply the SCTP port (which is hidden inside a DataCodec structure...) // When we support "max-message-size", that would also be pushed down here. - return cricket_sctp_transport()->Start(local_sctp_port, remote_sctp_port); + return cricket_sctp_transport()->Start(local_sctp_port, remote_sctp_port, + max_message_size); } RTCError PeerConnection::PushdownTransportDescription( diff --git a/pc/peer_connection.h b/pc/peer_connection.h index be46980ac4..e83ef1eb12 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -879,7 +879,8 @@ class PeerConnection : public PeerConnectionInternal, RTC_RUN_ON(signaling_thread()); bool PushdownSctpParameters_n(cricket::ContentSource source, int local_sctp_port, - int remote_sctp_port); + int remote_sctp_port, + int max_message_size); RTCError PushdownTransportDescription(cricket::ContentSource source, SdpType type); diff --git a/pc/sctp_transport.cc b/pc/sctp_transport.cc index 1db149f18b..d23d619be3 100644 --- a/pc/sctp_transport.cc +++ b/pc/sctp_transport.cc @@ -105,8 +105,14 @@ void SctpTransport::UpdateInformation(SctpTransportState state) { must_send_update = (state != info_.state()); // TODO(https://bugs.webrtc.org/10358): Update max message size and // max channels from internal SCTP transport when available. - info_ = SctpTransportInformation( - state, dtls_transport_, info_.MaxMessageSize(), info_.MaxChannels()); + if (internal_sctp_transport_) { + info_ = SctpTransportInformation( + state, dtls_transport_, internal_sctp_transport_->max_message_size(), + info_.MaxChannels()); + } else { + info_ = SctpTransportInformation( + state, dtls_transport_, info_.MaxMessageSize(), info_.MaxChannels()); + } if (observer_ && must_send_update) { info_copy = info_; } diff --git a/pc/sctp_transport_unittest.cc b/pc/sctp_transport_unittest.cc index 3771b2c37b..3438b17e39 100644 --- a/pc/sctp_transport_unittest.cc +++ b/pc/sctp_transport_unittest.cc @@ -32,7 +32,9 @@ namespace { class FakeCricketSctpTransport : public cricket::SctpTransportInternal { public: void SetDtlsTransport(rtc::PacketTransportInternal* transport) override {} - bool Start(int local_port, int remote_port) override { return true; } + bool Start(int local_port, int remote_port, int max_message_size) override { + return true; + } bool OpenStream(int sid) override { return true; } bool ResetStream(int sid) override { return true; } bool SendData(const cricket::SendDataParams& params, @@ -42,6 +44,7 @@ class FakeCricketSctpTransport : public cricket::SctpTransportInternal { } bool ReadyToSendData() override { return true; } void set_debug_name_for_testing(const char* debug_name) override {} + int max_message_size() const override { return 0; } // Methods exposed for testing void SendSignalReadyToSendData() { SignalReadyToSendData(); } diff --git a/pc/session_description.h b/pc/session_description.h index 27b781f385..fe08e3a909 100644 --- a/pc/session_description.h +++ b/pc/session_description.h @@ -498,7 +498,8 @@ class SctpDataContentDescription : public MediaContentDescription { bool use_sctpmap_ = true; // Note: "true" is no longer conformant. // Defaults should be constants imported from SCTP. Quick hack. int port_ = 5000; - int max_message_size_ = 256 * 1024; + // draft-ietf-mmusic-sdp-sctp-23: Max message size default is 64K + int max_message_size_ = 64 * 1024; std::unique_ptr shim_; }; diff --git a/pc/test/fake_sctp_transport.h b/pc/test/fake_sctp_transport.h index b2c5c1ae21..b3eeee07db 100644 --- a/pc/test/fake_sctp_transport.h +++ b/pc/test/fake_sctp_transport.h @@ -21,9 +21,10 @@ class FakeSctpTransport : public cricket::SctpTransportInternal { public: void SetDtlsTransport(rtc::PacketTransportInternal* transport) override {} - bool Start(int local_port, int remote_port) override { + bool Start(int local_port, int remote_port, int max_message_size) override { local_port_.emplace(local_port); remote_port_.emplace(remote_port); + max_message_size_ = max_message_size; return true; } bool OpenStream(int sid) override { return true; } @@ -36,12 +37,14 @@ class FakeSctpTransport : public cricket::SctpTransportInternal { bool ReadyToSendData() override { return true; } void set_debug_name_for_testing(const char* debug_name) override {} + int max_message_size() const { return max_message_size_; } int local_port() const { return *local_port_; } int remote_port() const { return *remote_port_; } private: absl::optional local_port_; absl::optional remote_port_; + int max_message_size_; }; class FakeSctpTransportFactory : public cricket::SctpTransportInternalFactory { diff --git a/pc/webrtc_sdp.cc b/pc/webrtc_sdp.cc index c6968b19e1..0c09fa2ae8 100644 --- a/pc/webrtc_sdp.cc +++ b/pc/webrtc_sdp.cc @@ -172,6 +172,7 @@ static const char kAttributeInactive[] = "inactive"; // a=sctp-port, a=max-message-size static const char kAttributeSctpPort[] = "sctp-port"; static const char kAttributeMaxMessageSize[] = "max-message-size"; +static const int kDefaultSctpMaxMessageSize = 65536; // draft-ietf-mmusic-sdp-simulcast-13 // a=simulcast static const char kAttributeSimulcast[] = "simulcast"; @@ -272,9 +273,6 @@ static void BuildMediaDescription(const ContentInfo* content_info, const std::vector& candidates, int msid_signaling, std::string* message); -static void BuildSctpContentAttributes(std::string* message, - int sctp_port, - bool use_sctpmap); static void BuildRtpContentAttributes(const MediaContentDescription* media_desc, const cricket::MediaType media_type, int msid_signaling, @@ -1323,6 +1321,33 @@ bool ParseExtmap(const std::string& line, return true; } +static void BuildSctpContentAttributes( + std::string* message, + const cricket::SctpDataContentDescription* data_desc) { + rtc::StringBuilder os; + if (data_desc->use_sctpmap()) { + // draft-ietf-mmusic-sctp-sdp-04 + // a=sctpmap:sctpmap-number protocol [streams] + rtc::StringBuilder os; + InitAttrLine(kAttributeSctpmap, &os); + os << kSdpDelimiterColon << data_desc->port() << kSdpDelimiterSpace + << kDefaultSctpmapProtocol << kSdpDelimiterSpace + << cricket::kMaxSctpStreams; + AddLine(os.str(), message); + } else { + // draft-ietf-mmusic-sctp-sdp-23 + // a=sctp-port: + InitAttrLine(kAttributeSctpPort, &os); + os << kSdpDelimiterColon << data_desc->port(); + AddLine(os.str(), message); + if (data_desc->max_message_size() != kDefaultSctpMaxMessageSize) { + InitAttrLine(kAttributeMaxMessageSize, &os); + os << kSdpDelimiterColon << data_desc->max_message_size(); + AddLine(os.str(), message); + } + } +} + void BuildMediaDescription(const ContentInfo* content_info, const TransportInfo* transport_info, const cricket::MediaType media_type, @@ -1518,34 +1543,12 @@ void BuildMediaDescription(const ContentInfo* content_info, if (cricket::IsDtlsSctp(media_desc->protocol())) { const cricket::SctpDataContentDescription* data_desc = media_desc->as_sctp(); - bool use_sctpmap = data_desc->use_sctpmap(); - BuildSctpContentAttributes(message, data_desc->port(), use_sctpmap); + BuildSctpContentAttributes(message, data_desc); } else if (cricket::IsRtpProtocol(media_desc->protocol())) { BuildRtpContentAttributes(media_desc, media_type, msid_signaling, message); } } -void BuildSctpContentAttributes(std::string* message, - int sctp_port, - bool use_sctpmap) { - rtc::StringBuilder os; - if (use_sctpmap) { - // draft-ietf-mmusic-sctp-sdp-04 - // a=sctpmap:sctpmap-number protocol [streams] - InitAttrLine(kAttributeSctpmap, &os); - os << kSdpDelimiterColon << sctp_port << kSdpDelimiterSpace - << kDefaultSctpmapProtocol << kSdpDelimiterSpace - << cricket::kMaxSctpStreams; - } else { - // draft-ietf-mmusic-sctp-sdp-23 - // a=sctp-port: - InitAttrLine(kAttributeSctpPort, &os); - os << kSdpDelimiterColon << sctp_port; - // TODO(zstein): emit max-message-size here - } - AddLine(os.str(), message); -} - void BuildRtpContentAttributes(const MediaContentDescription* media_desc, const cricket::MediaType media_type, int msid_signaling, @@ -2706,6 +2709,9 @@ bool ParseMediaDescription( // m=application UDP/DTLS/SCTP webrtc-datachannel // use_sctpmap should be false. auto data_desc = absl::make_unique(); + // Default max message size is 64K + // according to draft-ietf-mmusic-sctp-sdp-26 + data_desc->set_max_message_size(kDefaultSctpMaxMessageSize); int p; if (rtc::FromString(fields[3], &p)) { data_desc->set_port(p); diff --git a/pc/webrtc_sdp_unittest.cc b/pc/webrtc_sdp_unittest.cc index 1d3e6a3ce2..a3a87492a5 100644 --- a/pc/webrtc_sdp_unittest.cc +++ b/pc/webrtc_sdp_unittest.cc @@ -2891,6 +2891,34 @@ TEST_F(WebRtcSdpTest, DeserializeSdpWithSctpDataChannelsWithMaxMessageSize) { EXPECT_TRUE(CompareSessionDescription(jdesc, jdesc_output)); } +TEST_F(WebRtcSdpTest, SerializeSdpWithSctpDataChannelWithMaxMessageSize) { + bool use_sctpmap = false; + AddSctpDataChannel(use_sctpmap); + JsepSessionDescription jdesc(kDummyType); + MutateJsepSctpMaxMessageSize(desc_, 12345, &jdesc); + std::string message = webrtc::SdpSerialize(jdesc); + EXPECT_NE(std::string::npos, + message.find("\r\na=max-message-size:12345\r\n")); + JsepSessionDescription jdesc_output(kDummyType); + EXPECT_TRUE(SdpDeserialize(message, &jdesc_output)); + EXPECT_TRUE(CompareSessionDescription(jdesc, jdesc_output)); +} + +TEST_F(WebRtcSdpTest, + SerializeSdpWithSctpDataChannelWithDefaultMaxMessageSize) { + // https://tools.ietf.org/html/draft-ietf-mmusic-sctp-sdp-26#section-6 + // The default max message size is 64K. + bool use_sctpmap = false; + AddSctpDataChannel(use_sctpmap); + JsepSessionDescription jdesc(kDummyType); + MutateJsepSctpMaxMessageSize(desc_, 65536, &jdesc); + std::string message = webrtc::SdpSerialize(jdesc); + EXPECT_EQ(std::string::npos, message.find("\r\na=max-message-size:")); + JsepSessionDescription jdesc_output(kDummyType); + EXPECT_TRUE(SdpDeserialize(message, &jdesc_output)); + EXPECT_TRUE(CompareSessionDescription(jdesc, jdesc_output)); +} + // Test to check the behaviour if sctp-port is specified // on the m= line and in a=sctp-port. TEST_F(WebRtcSdpTest, DeserializeSdpWithMultiSctpPort) {