SCTP: Treat message size zero as "responder selects"
This also refactors some of the code in peerconnection for
handling SCTP transports to be internal to the webrtc::SctpTransport
class, rather than being in peerconnection.
Bug: webrtc:10358, webrtc:10629
Change-Id: I15ecf95c199f56b08909e5a9311d446a412ed162
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/137041
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27960}
diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index 1013f7c..dd9ca6b 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -295,6 +295,7 @@
"../call:rtp_interfaces",
"../call:rtp_receiver",
"../logging:rtc_event_log_api",
+ "../media:rtc_data",
"../media:rtc_media_base",
"../media:rtc_media_tests_utils",
"../modules/rtp_rtcp:rtp_rtcp_format",
diff --git a/pc/media_session.cc b/pc/media_session.cc
index 0924e6d..1b3d015 100644
--- a/pc/media_session.cc
+++ b/pc/media_session.cc
@@ -2584,8 +2584,15 @@
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));
+ // 0 is treated specially - it means "I can accept any size". Since
+ // we do not implement infinite size messages, reply with
+ // kSctpSendBufferSize.
+ if (offer_data_description->max_message_size() == 0) {
+ data_answer->as_sctp()->set_max_message_size(kSctpSendBufferSize);
+ } else {
+ 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/media_session_unittest.cc b/pc/media_session_unittest.cc
index cb90bbc..b14e14c 100644
--- a/pc/media_session_unittest.cc
+++ b/pc/media_session_unittest.cc
@@ -17,6 +17,7 @@
#include "absl/memory/memory.h"
#include "media/base/codec.h"
#include "media/base/test_utils.h"
+#include "media/sctp/sctp_transport_internal.h"
#include "p2p/base/p2p_constants.h"
#include "p2p/base/transport_description.h"
#include "p2p/base/transport_info.h"
@@ -1427,6 +1428,64 @@
}
}
+TEST_F(MediaSessionDescriptionFactoryTest,
+ TestCreateDataAnswerToOfferWithDefinedMessageSize) {
+ // Need to enable DTLS offer/answer generation (disabled by default in this
+ // test).
+ f1_.set_secure(SEC_ENABLED);
+ f2_.set_secure(SEC_ENABLED);
+ tdf1_.set_secure(SEC_ENABLED);
+ tdf2_.set_secure(SEC_ENABLED);
+
+ MediaSessionOptions opts;
+ AddDataSection(cricket::DCT_SCTP, RtpTransceiverDirection::kSendRecv, &opts);
+ std::unique_ptr<SessionDescription> offer = f1_.CreateOffer(opts, nullptr);
+ ASSERT_TRUE(offer.get() != nullptr);
+ ContentInfo* dc_offer = offer->GetContentByName("data");
+ ASSERT_TRUE(dc_offer != nullptr);
+ SctpDataContentDescription* dcd_offer =
+ dc_offer->media_description()->as_sctp();
+ ASSERT_TRUE(dcd_offer);
+ dcd_offer->set_max_message_size(1234);
+ std::unique_ptr<SessionDescription> answer =
+ f2_.CreateAnswer(offer.get(), opts, nullptr);
+ const ContentInfo* dc_answer = answer->GetContentByName("data");
+ ASSERT_TRUE(dc_answer != nullptr);
+ const SctpDataContentDescription* dcd_answer =
+ dc_answer->media_description()->as_sctp();
+ EXPECT_FALSE(dc_answer->rejected);
+ EXPECT_EQ(1234, dcd_answer->max_message_size());
+}
+
+TEST_F(MediaSessionDescriptionFactoryTest,
+ TestCreateDataAnswerToOfferWithZeroMessageSize) {
+ // Need to enable DTLS offer/answer generation (disabled by default in this
+ // test).
+ f1_.set_secure(SEC_ENABLED);
+ f2_.set_secure(SEC_ENABLED);
+ tdf1_.set_secure(SEC_ENABLED);
+ tdf2_.set_secure(SEC_ENABLED);
+
+ MediaSessionOptions opts;
+ AddDataSection(cricket::DCT_SCTP, RtpTransceiverDirection::kSendRecv, &opts);
+ std::unique_ptr<SessionDescription> offer = f1_.CreateOffer(opts, nullptr);
+ ASSERT_TRUE(offer.get() != nullptr);
+ ContentInfo* dc_offer = offer->GetContentByName("data");
+ ASSERT_TRUE(dc_offer != nullptr);
+ SctpDataContentDescription* dcd_offer =
+ dc_offer->media_description()->as_sctp();
+ ASSERT_TRUE(dcd_offer);
+ dcd_offer->set_max_message_size(0);
+ std::unique_ptr<SessionDescription> answer =
+ f2_.CreateAnswer(offer.get(), opts, nullptr);
+ const ContentInfo* dc_answer = answer->GetContentByName("data");
+ ASSERT_TRUE(dc_answer != nullptr);
+ const SctpDataContentDescription* dcd_answer =
+ dc_answer->media_description()->as_sctp();
+ EXPECT_FALSE(dc_answer->rejected);
+ EXPECT_EQ(cricket::kSctpSendBufferSize, dcd_answer->max_message_size());
+}
+
// Verifies that the order of the media contents in the offer is preserved in
// the answer.
TEST_F(MediaSessionDescriptionFactoryTest, TestCreateAnswerContentOrder) {
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index e1cc6ef..065247d 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -5657,35 +5657,24 @@
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<bool>(
- 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.");
+ int max_message_size;
+ // A remote max message size of zero means "any size supported".
+ // We configure the connection with our own max message size.
+ if (remote_sctp_description->max_message_size() == 0) {
+ max_message_size = local_sctp_description->max_message_size();
+ } else {
+ max_message_size =
+ std::min(local_sctp_description->max_message_size(),
+ remote_sctp_description->max_message_size());
}
+ sctp_transport_->Start(local_sctp_description->port(),
+ remote_sctp_description->port(), max_message_size);
}
}
return RTCError::OK();
}
-bool PeerConnection::PushdownSctpParameters_n(cricket::ContentSource source,
- int local_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,
- max_message_size);
-}
-
RTCError PeerConnection::PushdownTransportDescription(
cricket::ContentSource source,
SdpType type) {
diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index e83ef1e..3009185 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -877,10 +877,6 @@
// down to all of the channels.
RTCError PushdownMediaDescription(SdpType type, cricket::ContentSource source)
RTC_RUN_ON(signaling_thread());
- bool PushdownSctpParameters_n(cricket::ContentSource source,
- int local_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 d23d619..1c0289a9 100644
--- a/pc/sctp_transport.cc
+++ b/pc/sctp_transport.cc
@@ -96,6 +96,27 @@
UpdateInformation(next_state);
}
+void SctpTransport::Start(int local_port,
+ int remote_port,
+ int max_message_size) {
+ {
+ rtc::CritScope scope(&lock_);
+ // Record max message size on calling thread.
+ info_ = SctpTransportInformation(info_.state(), info_.dtls_transport(),
+ max_message_size, info_.MaxChannels());
+ }
+ if (owner_thread_->IsCurrent()) {
+ if (!internal()->Start(local_port, remote_port, max_message_size)) {
+ RTC_LOG(LS_ERROR) << "Failed to push down SCTP parameters, closing.";
+ UpdateInformation(SctpTransportState::kClosed);
+ }
+ } else {
+ owner_thread_->Invoke<void>(
+ RTC_FROM_HERE, rtc::Bind(&SctpTransport::Start, this, local_port,
+ remote_port, max_message_size));
+ }
+}
+
void SctpTransport::UpdateInformation(SctpTransportState state) {
RTC_DCHECK_RUN_ON(owner_thread_);
bool must_send_update;
@@ -103,12 +124,11 @@
{
rtc::CritScope scope(&lock_);
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.
+ // TODO(https://bugs.webrtc.org/10358): Update max channels from internal
+ // SCTP transport when available.
if (internal_sctp_transport_) {
info_ = SctpTransportInformation(
- state, dtls_transport_, internal_sctp_transport_->max_message_size(),
- info_.MaxChannels());
+ state, dtls_transport_, info_.MaxMessageSize(), info_.MaxChannels());
} else {
info_ = SctpTransportInformation(
state, dtls_transport_, info_.MaxMessageSize(), info_.MaxChannels());
diff --git a/pc/sctp_transport.h b/pc/sctp_transport.h
index f1e5092..4a1c6f5 100644
--- a/pc/sctp_transport.h
+++ b/pc/sctp_transport.h
@@ -36,9 +36,16 @@
void RegisterObserver(SctpTransportObserverInterface* observer) override;
void UnregisterObserver() override;
+ // Internal functions
void Clear();
void SetDtlsTransport(rtc::scoped_refptr<DtlsTransport>);
+ // Initialize the cricket::SctpTransport. This can be called from
+ // the signaling thread.
+ void Start(int local_port, int remote_port, int max_message_size);
+ // TODO(https://bugs.webrtc.org/10629): Move functions that need
+ // internal() to be functions on the webrtc::SctpTransport interface,
+ // and make the internal() function private.
cricket::SctpTransportInternal* internal() {
rtc::CritScope scope(&lock_);
return internal_sctp_transport_.get();
@@ -58,7 +65,9 @@
void OnInternalClosingProcedureStartedRemotely(int sid);
void OnInternalClosingProcedureComplete(int sid);
- const rtc::Thread* owner_thread_;
+ // Note - owner_thread never changes, but can't be const if we do
+ // Invoke() on it.
+ rtc::Thread* owner_thread_;
rtc::CriticalSection lock_;
// Variables accessible off-thread, guarded by lock_
SctpTransportInformation info_ RTC_GUARDED_BY(lock_);
