Move Destroy/Create steps for DataChannelTransport to PeerConnection.
This moves steps from the sdp code for pc state over to the PC class
and slightly simplifies the contract between the two classes.
Moving forward it's easier to consolidate those steps in the PC
class with other grouped operations e.g. during teardown.
Also removing GetDataMid() method in favor of the sctp_mid() property.
Bug: none
Change-Id: I938f953099d327377abd94e6b2c9ece803d88e40
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/324300
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40981}
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index e349da4..c31f671 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -2115,20 +2115,31 @@
Observer()->OnIceSelectedCandidatePairChanged(event);
}
-absl::optional<std::string> PeerConnection::GetDataMid() const {
+bool PeerConnection::CreateDataChannelTransport(absl::string_view mid) {
RTC_DCHECK_RUN_ON(signaling_thread());
- return sctp_mid_s_;
-}
+ RTC_DCHECK(!sctp_mid().has_value() || mid == sctp_mid().value());
+ RTC_LOG(LS_INFO) << "Creating data channel, mid=" << mid;
-void PeerConnection::SetSctpDataInfo(absl::string_view mid,
- absl::string_view transport_name) {
- RTC_DCHECK_RUN_ON(signaling_thread());
+ absl::optional<std::string> transport_name =
+ network_thread()->BlockingCall([&] {
+ RTC_DCHECK_RUN_ON(network_thread());
+ return SetupDataChannelTransport_n(mid);
+ });
+ if (!transport_name)
+ return false;
+
sctp_mid_s_ = std::string(mid);
- SetSctpTransportName(std::string(transport_name));
+ SetSctpTransportName(transport_name.value());
+
+ return true;
}
-void PeerConnection::ResetSctpDataInfo() {
+void PeerConnection::DestroyDataChannelTransport(RTCError error) {
RTC_DCHECK_RUN_ON(signaling_thread());
+ network_thread()->BlockingCall([&] {
+ RTC_DCHECK_RUN_ON(network_thread());
+ TeardownDataChannelTransport_n(error);
+ });
sctp_mid_s_.reset();
SetSctpTransportName("");
}
diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index aac1635..ea1a9d9 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -394,15 +394,8 @@
const std::map<std::string, const cricket::ContentGroup*>&
bundle_groups_by_mid) override;
- // Returns the MID for the data section associated with the
- // SCTP data channel, if it has been set. If no data
- // channels are configured this will return nullopt.
- absl::optional<std::string> GetDataMid() const override;
-
- void SetSctpDataInfo(absl::string_view mid,
- absl::string_view transport_name) override;
-
- void ResetSctpDataInfo() override;
+ bool CreateDataChannelTransport(absl::string_view mid) override;
+ void DestroyDataChannelTransport(RTCError error) override;
// Asynchronously calls SctpTransport::Start() on the network thread for
// `sctp_mid()` if set. Called as part of setting the local description.
@@ -430,9 +423,9 @@
// this session.
bool SrtpRequired() const override;
- absl::optional<std::string> SetupDataChannelTransport_n(
- absl::string_view mid) override RTC_RUN_ON(network_thread());
- void TeardownDataChannelTransport_n(RTCError error) override
+ absl::optional<std::string> SetupDataChannelTransport_n(absl::string_view mid)
+ RTC_RUN_ON(network_thread());
+ void TeardownDataChannelTransport_n(RTCError error)
RTC_RUN_ON(network_thread());
const FieldTrialsView& trials() const override { return *trials_; }
diff --git a/pc/peer_connection_internal.h b/pc/peer_connection_internal.h
index c91a44a..6fc1222 100644
--- a/pc/peer_connection_internal.h
+++ b/pc/peer_connection_internal.h
@@ -95,7 +95,6 @@
const std::map<std::string, const cricket::ContentGroup*>&
bundle_groups_by_mid) = 0;
- virtual absl::optional<std::string> GetDataMid() const = 0;
// Internal implementation for AddTransceiver family of methods. If
// `fire_callback` is set, fires OnRenegotiationNeeded callback if successful.
virtual RTCErrorOr<rtc::scoped_refptr<RtpTransceiverInterface>>
@@ -117,17 +116,14 @@
// Returns true if SRTP (either using DTLS-SRTP or SDES) is required by
// this session.
virtual bool SrtpRequired() const = 0;
- // Configures the data channel transport on the network thread.
- // The return value will be unset if an error occurs. If the setup succeeded
- // the return value will be set and contain the name of the transport
- // (empty string if a name isn't available).
- virtual absl::optional<std::string> SetupDataChannelTransport_n(
- absl::string_view mid) = 0;
- virtual void TeardownDataChannelTransport_n(RTCError error) = 0;
- virtual void SetSctpDataInfo(absl::string_view mid,
- absl::string_view transport_name) = 0;
- virtual void ResetSctpDataInfo() = 0;
-
+ // Initializes the data channel transport for the peerconnection instance.
+ // This will have the effect that `sctp_mid()` and `sctp_transport_name()`
+ // will return a set value (even though it might be an empty string) and the
+ // dc transport will be initialized on the network thread.
+ virtual bool CreateDataChannelTransport(absl::string_view mid) = 0;
+ // Tears down the data channel transport state and clears the `sctp_mid()` and
+ // `sctp_transport_name()` properties.
+ virtual void DestroyDataChannelTransport(RTCError error) = 0;
virtual const FieldTrialsView& trials() const = 0;
virtual void ClearStatsCache() = 0;
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc
index 4a77b75..c795ccf 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -3791,13 +3791,15 @@
return error;
}
} else if (media_type == cricket::MEDIA_TYPE_DATA) {
- if (pc_->GetDataMid() && new_content.name != *(pc_->GetDataMid())) {
+ const auto data_mid = pc_->sctp_mid();
+ if (data_mid && new_content.name != data_mid.value()) {
// Ignore all but the first data section.
RTC_LOG(LS_INFO) << "Ignoring data media section with MID="
<< new_content.name;
continue;
}
- RTCError error = UpdateDataChannel(source, new_content, bundle_group);
+ RTCError error =
+ UpdateDataChannelTransport(source, new_content, bundle_group);
if (!error.ok()) {
return error;
}
@@ -3979,7 +3981,7 @@
return RTCError::OK();
}
-RTCError SdpOfferAnswerHandler::UpdateDataChannel(
+RTCError SdpOfferAnswerHandler::UpdateDataChannelTransport(
cricket::ContentSource source,
const cricket::ContentInfo& content,
const cricket::ContentGroup* bundle_group) {
@@ -3991,8 +3993,8 @@
sb << "Rejected data channel transport with mid=" << content.mid();
RTCError error(RTCErrorType::OPERATION_ERROR_WITH_DATA, sb.Release());
error.set_error_detail(RTCErrorDetailType::DATA_CHANNEL_FAILURE);
- DestroyDataChannelTransport(error);
- } else if (!CreateDataChannel(content.name)) {
+ pc_->DestroyDataChannelTransport(error);
+ } else if (!pc_->CreateDataChannelTransport(content.name)) {
LOG_AND_RETURN_ERROR(RTCErrorType::INTERNAL_ERROR,
"Failed to create data channel.");
}
@@ -4323,8 +4325,9 @@
session_options->media_description_options.push_back(
GetMediaDescriptionOptionsForRejectedData(mid));
} else {
- RTC_CHECK(pc_->GetDataMid());
- if (mid == *(pc_->GetDataMid())) {
+ const auto data_mid = pc_->sctp_mid();
+ RTC_CHECK(data_mid);
+ if (mid == data_mid.value()) {
session_options->media_description_options.push_back(
GetMediaDescriptionOptionsForActiveData(mid));
} else {
@@ -4365,9 +4368,9 @@
}
// Lastly, add a m-section if we have requested local data channels and an
// m section does not already exist.
- if (!pc_->GetDataMid() && data_channel_controller()->HasDataChannels()) {
+ if (!pc_->sctp_mid() && data_channel_controller()->HasDataChannels()) {
// Attempt to recycle a stopped m-line.
- // TODO(crbug.com/1442604): GetDataMid() should return the mid if one was
+ // TODO(crbug.com/1442604): sctp_mid() should return the mid if one was
// ever created but rejected.
bool recycled = false;
for (size_t i = 0; i < session_options->media_description_options.size();
@@ -4510,7 +4513,7 @@
// Reject all data sections if data channels are disabled.
// Reject a data section if it has already been rejected.
// Reject all data sections except for the first one.
- if (content.rejected || content.name != *(pc_->GetDataMid())) {
+ if (content.rejected || content.name != *(pc_->sctp_mid())) {
session_options->media_description_options.push_back(
GetMediaDescriptionOptionsForRejectedData(content.name));
} else {
@@ -5003,14 +5006,14 @@
RTCError error(RTCErrorType::OPERATION_ERROR_WITH_DATA,
"No data channel section in the description.");
error.set_error_detail(RTCErrorDetailType::DATA_CHANNEL_FAILURE);
- DestroyDataChannelTransport(error);
+ pc_->DestroyDataChannelTransport(error);
} else if (data_info->rejected) {
rtc::StringBuilder sb;
sb << "Rejected data channel with mid=" << data_info->name << ".";
RTCError error(RTCErrorType::OPERATION_ERROR_WITH_DATA, sb.Release());
error.set_error_detail(RTCErrorDetailType::DATA_CHANNEL_FAILURE);
- DestroyDataChannelTransport(error);
+ pc_->DestroyDataChannelTransport(error);
}
}
@@ -5193,7 +5196,7 @@
}
const cricket::ContentInfo* data = cricket::GetFirstDataContent(&desc);
- if (data && !data->rejected && !CreateDataChannel(data->name)) {
+ if (data && !data->rejected && !pc_->CreateDataChannelTransport(data->name)) {
LOG_AND_RETURN_ERROR(RTCErrorType::INTERNAL_ERROR,
"Failed to create data channel.");
}
@@ -5201,33 +5204,6 @@
return RTCError::OK();
}
-bool SdpOfferAnswerHandler::CreateDataChannel(const std::string& mid) {
- RTC_DCHECK_RUN_ON(signaling_thread());
- RTC_DCHECK(!pc_->sctp_mid().has_value() || mid == pc_->sctp_mid().value());
- RTC_LOG(LS_INFO) << "Creating data channel, mid=" << mid;
-
- absl::optional<std::string> transport_name =
- context_->network_thread()->BlockingCall([&] {
- RTC_DCHECK_RUN_ON(context_->network_thread());
- return pc_->SetupDataChannelTransport_n(mid);
- });
- if (!transport_name)
- return false;
-
- pc_->SetSctpDataInfo(mid, *transport_name);
- return true;
-}
-
-void SdpOfferAnswerHandler::DestroyDataChannelTransport(RTCError error) {
- RTC_DCHECK_RUN_ON(signaling_thread());
- context_->network_thread()->BlockingCall(
- [&, data_channel_controller = data_channel_controller()] {
- RTC_DCHECK_RUN_ON(context_->network_thread());
- pc_->TeardownDataChannelTransport_n(error);
- });
- pc_->ResetSctpDataInfo();
-}
-
void SdpOfferAnswerHandler::DestroyAllChannels() {
RTC_DCHECK_RUN_ON(signaling_thread());
if (!transceivers()) {
@@ -5252,7 +5228,7 @@
}
}
- DestroyDataChannelTransport({});
+ pc_->DestroyDataChannelTransport({});
}
void SdpOfferAnswerHandler::GenerateMediaDescriptionOptions(
diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h
index 80a2139..eb7c705 100644
--- a/pc/sdp_offer_answer.h
+++ b/pc/sdp_offer_answer.h
@@ -359,9 +359,9 @@
// Either creates or destroys the local data channel according to the given
// media section.
- RTCError UpdateDataChannel(cricket::ContentSource source,
- const cricket::ContentInfo& content,
- const cricket::ContentGroup* bundle_group)
+ RTCError UpdateDataChannelTransport(cricket::ContentSource source,
+ const cricket::ContentInfo& content,
+ const cricket::ContentGroup* bundle_group)
RTC_RUN_ON(signaling_thread());
// Check if a call to SetLocalDescription is acceptable with a session
// description of the given type.
@@ -526,12 +526,6 @@
// This method will also delete any existing media channels before creating.
RTCError CreateChannels(const cricket::SessionDescription& desc);
- bool CreateDataChannel(const std::string& mid);
-
- // Destroys the RTP data channel transport and/or the SCTP data channel
- // transport and clears it.
- void DestroyDataChannelTransport(RTCError error);
-
// Generates MediaDescriptionOptions for the `session_opts` based on existing
// local description or remote description.
void GenerateMediaDescriptionOptions(
diff --git a/pc/test/fake_peer_connection_base.h b/pc/test/fake_peer_connection_base.h
index 743c181..a1c8dca 100644
--- a/pc/test/fake_peer_connection_base.h
+++ b/pc/test/fake_peer_connection_base.h
@@ -339,9 +339,6 @@
return false;
}
- absl::optional<std::string> GetDataMid() const override {
- return absl::nullopt;
- }
RTCErrorOr<rtc::scoped_refptr<RtpTransceiverInterface>> AddTransceiver(
cricket::MediaType media_type,
rtc::scoped_refptr<MediaStreamTrackInterface> track,
@@ -358,14 +355,10 @@
Call* call_ptr() override { return nullptr; }
bool SrtpRequired() const override { return false; }
- absl::optional<std::string> SetupDataChannelTransport_n(
- absl::string_view mid) override {
- return absl::nullopt;
+ bool CreateDataChannelTransport(absl::string_view mid) override {
+ return false;
}
- void TeardownDataChannelTransport_n(RTCError error) override {}
- void SetSctpDataInfo(absl::string_view mid,
- absl::string_view transport_name) override {}
- void ResetSctpDataInfo() override {}
+ void DestroyDataChannelTransport(RTCError error) override {}
const FieldTrialsView& trials() const override { return field_trials_; }
diff --git a/pc/test/mock_peer_connection_internal.h b/pc/test/mock_peer_connection_internal.h
index 58d13ed..5fd7a50 100644
--- a/pc/test/mock_peer_connection_internal.h
+++ b/pc/test/mock_peer_connection_internal.h
@@ -248,7 +248,6 @@
(const cricket::SessionDescription*,
(const std::map<std::string, const cricket::ContentGroup*>&)),
(override));
- MOCK_METHOD(absl::optional<std::string>, GetDataMid, (), (const, override));
MOCK_METHOD(RTCErrorOr<rtc::scoped_refptr<RtpTransceiverInterface>>,
AddTransceiver,
(cricket::MediaType,
@@ -263,16 +262,11 @@
(override));
MOCK_METHOD(Call*, call_ptr, (), (override));
MOCK_METHOD(bool, SrtpRequired, (), (const, override));
- MOCK_METHOD(absl::optional<std::string>,
- SetupDataChannelTransport_n,
- (absl::string_view mid),
+ MOCK_METHOD(bool,
+ CreateDataChannelTransport,
+ (absl::string_view),
(override));
- MOCK_METHOD(void, TeardownDataChannelTransport_n, (RTCError), (override));
- MOCK_METHOD(void,
- SetSctpDataInfo,
- (absl::string_view, absl::string_view),
- (override));
- MOCK_METHOD(void, ResetSctpDataInfo, (), (override));
+ MOCK_METHOD(void, DestroyDataChannelTransport, (RTCError error), (override));
MOCK_METHOD(const FieldTrialsView&, trials, (), (const, override));
// PeerConnectionInternal
