From 840cf786002f0dd7cc3e9c1ce6ff5f3d5048d836 Mon Sep 17 00:00:00 2001 From: Tommi Date: Sat, 21 Oct 2023 16:47:56 +0200 Subject: [PATCH] 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 Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#40981} --- pc/peer_connection.cc | 27 ++++++++---- pc/peer_connection.h | 17 +++----- pc/peer_connection_internal.h | 20 ++++----- pc/sdp_offer_answer.cc | 58 ++++++++----------------- pc/sdp_offer_answer.h | 12 ++--- pc/test/fake_peer_connection_base.h | 13 ++---- pc/test/mock_peer_connection_internal.h | 14 ++---- 7 files changed, 59 insertions(+), 102 deletions(-) diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index e349da42c9..c31f67175b 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -2115,20 +2115,31 @@ void PeerConnection::OnSelectedCandidatePairChanged( Observer()->OnIceSelectedCandidatePairChanged(event); } -absl::optional 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; + + absl::optional transport_name = + network_thread()->BlockingCall([&] { + RTC_DCHECK_RUN_ON(network_thread()); + return SetupDataChannelTransport_n(mid); + }); + if (!transport_name) + return false; -void PeerConnection::SetSctpDataInfo(absl::string_view mid, - absl::string_view transport_name) { - RTC_DCHECK_RUN_ON(signaling_thread()); 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 aac1635484..ea1a9d9d90 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -394,15 +394,8 @@ class PeerConnection : public PeerConnectionInternal, const std::map& 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 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 @@ class PeerConnection : public PeerConnectionInternal, // this session. bool SrtpRequired() const override; - absl::optional SetupDataChannelTransport_n( - absl::string_view mid) override RTC_RUN_ON(network_thread()); - void TeardownDataChannelTransport_n(RTCError error) override + absl::optional 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 c91a44a148..6fc1222804 100644 --- a/pc/peer_connection_internal.h +++ b/pc/peer_connection_internal.h @@ -95,7 +95,6 @@ class PeerConnectionSdpMethods { const std::map& bundle_groups_by_mid) = 0; - virtual absl::optional GetDataMid() const = 0; // Internal implementation for AddTransceiver family of methods. If // `fire_callback` is set, fires OnRenegotiationNeeded callback if successful. virtual RTCErrorOr> @@ -117,17 +116,14 @@ class PeerConnectionSdpMethods { // 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 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 4a77b7546c..c795ccff6c 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -3791,13 +3791,15 @@ RTCError SdpOfferAnswerHandler::UpdateTransceiversAndDataChannels( 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 @@ RTCError SdpOfferAnswerHandler::UpdateTransceiverChannel( 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 @@ RTCError SdpOfferAnswerHandler::UpdateDataChannel( 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 @@ void SdpOfferAnswerHandler::GetOptionsForUnifiedPlanOffer( 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 @@ void SdpOfferAnswerHandler::GetOptionsForUnifiedPlanOffer( } // 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 @@ void SdpOfferAnswerHandler::GetOptionsForUnifiedPlanAnswer( // 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 @@ void SdpOfferAnswerHandler::RemoveUnusedChannels( 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 @@ RTCError SdpOfferAnswerHandler::CreateChannels(const SessionDescription& desc) { } 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 @@ RTCError SdpOfferAnswerHandler::CreateChannels(const SessionDescription& desc) { 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 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 @@ void SdpOfferAnswerHandler::DestroyAllChannels() { } } - DestroyDataChannelTransport({}); + pc_->DestroyDataChannelTransport({}); } void SdpOfferAnswerHandler::GenerateMediaDescriptionOptions( diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h index 80a21391b9..eb7c705461 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h @@ -359,9 +359,9 @@ class SdpOfferAnswerHandler : public SdpStateProvider { // 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 @@ class SdpOfferAnswerHandler : public SdpStateProvider { // 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 743c18122a..a1c8dca12e 100644 --- a/pc/test/fake_peer_connection_base.h +++ b/pc/test/fake_peer_connection_base.h @@ -339,9 +339,6 @@ class FakePeerConnectionBase : public PeerConnectionInternal { return false; } - absl::optional GetDataMid() const override { - return absl::nullopt; - } RTCErrorOr> AddTransceiver( cricket::MediaType media_type, rtc::scoped_refptr track, @@ -358,14 +355,10 @@ class FakePeerConnectionBase : public PeerConnectionInternal { Call* call_ptr() override { return nullptr; } bool SrtpRequired() const override { return false; } - absl::optional 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 58d13ede2f..5fd7a50b4f 100644 --- a/pc/test/mock_peer_connection_internal.h +++ b/pc/test/mock_peer_connection_internal.h @@ -248,7 +248,6 @@ class MockPeerConnectionInternal : public PeerConnectionInternal { (const cricket::SessionDescription*, (const std::map&)), (override)); - MOCK_METHOD(absl::optional, GetDataMid, (), (const, override)); MOCK_METHOD(RTCErrorOr>, AddTransceiver, (cricket::MediaType, @@ -263,16 +262,11 @@ class MockPeerConnectionInternal : public PeerConnectionInternal { (override)); MOCK_METHOD(Call*, call_ptr, (), (override)); MOCK_METHOD(bool, SrtpRequired, (), (const, override)); - MOCK_METHOD(absl::optional, - 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