From 8546666cb9db79a39ddded7048bb5f82e9f4af87 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Mon, 19 Apr 2021 21:21:36 +0000 Subject: [PATCH] Add threading assertions to TransceiverList MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also add a function for accessing the list as internal transceivers rather than accessing the proxy objects; this exposes where the internal objects are accessed and where we need external references. Used the new list function in sdp_offer_answer wherever possible. Adds an UnsafeList function that is not thread guarded, so that the job of rooting out those instances can be done in a later CL. Bug: webrtc:12692 Change-Id: Ia591f22a1c8f82ec452a1a66a94fbf9ab9debd14 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/215581 Commit-Queue: Harald Alvestrand Reviewed-by: Tommi Reviewed-by: Henrik Boström Cr-Commit-Position: refs/heads/master@{#33781} --- pc/BUILD.gn | 3 + pc/peer_connection.cc | 7 +- pc/peer_connection.h | 4 +- pc/rtp_transceiver.cc | 6 +- pc/sdp_offer_answer.cc | 157 +++++++++++++++++++---------------------- pc/sdp_offer_answer.h | 20 +++--- pc/transceiver_list.cc | 12 ++++ pc/transceiver_list.h | 40 ++++++++++- 8 files changed, 143 insertions(+), 106 deletions(-) diff --git a/pc/BUILD.gn b/pc/BUILD.gn index f179eab987..18a81b9e1e 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -454,7 +454,10 @@ rtc_library("transceiver_list") { "../api:rtc_error", "../api:rtp_parameters", "../api:scoped_refptr", + "../api:sequence_checker", "../rtc_base:checks", + "../rtc_base:macromagic", + "../rtc_base/system:no_unique_address", ] absl_deps = [ "//third_party/abseil-cpp/absl/types:optional" ] } diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index b5116b57fd..95abb108b9 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -41,13 +41,13 @@ #include "p2p/base/p2p_constants.h" #include "p2p/base/p2p_transport_channel.h" #include "p2p/base/transport_info.h" +#include "pc/channel.h" #include "pc/ice_server_parsing.h" #include "pc/rtp_receiver.h" #include "pc/rtp_sender.h" #include "pc/sctp_transport.h" #include "pc/simulcast_description.h" #include "pc/webrtc_session_description_factory.h" -#include "rtc_base/callback_list.h" #include "rtc_base/helpers.h" #include "rtc_base/ip_address.h" #include "rtc_base/location.h" @@ -2084,8 +2084,7 @@ void PeerConnection::StopRtcEventLog_w() { cricket::ChannelInterface* PeerConnection::GetChannel( const std::string& content_name) { - RTC_DCHECK_RUN_ON(network_thread()); - for (const auto& transceiver : rtp_manager()->transceivers()->List()) { + for (const auto& transceiver : rtp_manager()->transceivers()->UnsafeList()) { cricket::ChannelInterface* channel = transceiver->internal()->channel(); if (channel && channel->content_name() == content_name) { return channel; @@ -2636,7 +2635,7 @@ void PeerConnection::ReportTransportStats() { rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; std::map> media_types_by_transport_name; - for (const auto& transceiver : rtp_manager()->transceivers()->List()) { + for (const auto& transceiver : rtp_manager()->transceivers()->UnsafeList()) { if (transceiver->internal()->channel()) { const std::string& transport_name = transceiver->internal()->channel()->transport_name(); diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 4225b7e499..d321fd5667 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -97,6 +97,7 @@ #include "rtc_base/thread.h" #include "rtc_base/thread_annotations.h" #include "rtc_base/unique_id_generator.h" +#include "rtc_base/weak_ptr.h" namespace webrtc { @@ -428,7 +429,8 @@ class PeerConnection : public PeerConnectionInternal, bool SetupDataChannelTransport_n(const std::string& mid) RTC_RUN_ON(network_thread()); void TeardownDataChannelTransport_n() RTC_RUN_ON(network_thread()); - cricket::ChannelInterface* GetChannel(const std::string& content_name); + cricket::ChannelInterface* GetChannel(const std::string& content_name) + RTC_RUN_ON(network_thread()); // Functions made public for testing. void ReturnHistogramVeryQuicklyForTesting() { diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc index 53a3702c40..d2d05bcedd 100644 --- a/pc/rtp_transceiver.cc +++ b/pc/rtp_transceiver.cc @@ -141,7 +141,10 @@ RtpTransceiver::RtpTransceiver( } RtpTransceiver::~RtpTransceiver() { - StopInternal(); + if (!stopped_) { + RTC_DCHECK_RUN_ON(thread_); + StopInternal(); + } } void RtpTransceiver::SetChannel(cricket::ChannelInterface* channel) { @@ -401,6 +404,7 @@ RTCError RtpTransceiver::StopStandard() { } void RtpTransceiver::StopInternal() { + RTC_DCHECK_RUN_ON(thread_); StopTransceiverProcedure(); } diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 7bb84525a6..c9ee82495b 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -22,7 +22,6 @@ #include "absl/strings/string_view.h" #include "api/array_view.h" #include "api/crypto/crypto_options.h" -#include "api/data_channel_interface.h" #include "api/dtls_transport_interface.h" #include "api/media_stream_proxy.h" #include "api/rtp_parameters.h" @@ -32,6 +31,7 @@ #include "media/base/codec.h" #include "media/base/media_engine.h" #include "media/base/rid_description.h" +#include "p2p/base/ice_transport_internal.h" #include "p2p/base/p2p_constants.h" #include "p2p/base/p2p_transport_channel.h" #include "p2p/base/port.h" @@ -39,14 +39,13 @@ #include "p2p/base/transport_description_factory.h" #include "p2p/base/transport_info.h" #include "pc/data_channel_utils.h" -#include "pc/media_protocol_names.h" +#include "pc/dtls_transport.h" #include "pc/media_stream.h" #include "pc/peer_connection.h" #include "pc/peer_connection_message_handler.h" #include "pc/rtp_media_utils.h" #include "pc/rtp_sender.h" #include "pc/rtp_transport_internal.h" -#include "pc/sctp_transport.h" #include "pc/simulcast_description.h" #include "pc/stats_collector.h" #include "pc/usage_pattern.h" @@ -56,7 +55,6 @@ #include "rtc_base/logging.h" #include "rtc_base/ref_counted_object.h" #include "rtc_base/rtc_certificate.h" -#include "rtc_base/socket_address.h" #include "rtc_base/ssl_stream_adapter.h" #include "rtc_base/string_encode.h" #include "rtc_base/strings/string_builder.h" @@ -249,7 +247,7 @@ void ReportSimulcastApiVersion(const char* name, } const ContentInfo* FindTransceiverMSection( - RtpTransceiverProxyWithInternal* transceiver, + RtpTransceiver* transceiver, const SessionDescriptionInterface* session_description) { return transceiver->mid() ? session_description->description()->GetContentByName( @@ -419,7 +417,7 @@ bool VerifyIceUfragPwdPresent(const SessionDescription* desc) { return true; } -static RTCError ValidateMids(const cricket::SessionDescription& description) { +RTCError ValidateMids(const cricket::SessionDescription& description) { std::set mids; for (const cricket::ContentInfo& content : description.contents()) { if (content.name.empty()) { @@ -471,7 +469,7 @@ std::string GetSignalingStateString( // This method will extract any send encodings that were sent by the remote // connection. This is currently only relevant for Simulcast scenario (where // the number of layers may be communicated by the server). -static std::vector GetSendEncodingsFromRemoteDescription( +std::vector GetSendEncodingsFromRemoteDescription( const MediaContentDescription& desc) { if (!desc.HasSimulcast()) { return {}; @@ -495,7 +493,7 @@ static std::vector GetSendEncodingsFromRemoteDescription( return result; } -static RTCError UpdateSimulcastLayerStatusInSender( +RTCError UpdateSimulcastLayerStatusInSender( const std::vector& layers, rtc::scoped_refptr sender) { RTC_DCHECK(sender); @@ -526,9 +524,8 @@ static RTCError UpdateSimulcastLayerStatusInSender( return result; } -static bool SimulcastIsRejected( - const ContentInfo* local_content, - const MediaContentDescription& answer_media_desc) { +bool SimulcastIsRejected(const ContentInfo* local_content, + const MediaContentDescription& answer_media_desc) { bool simulcast_offered = local_content && local_content->media_description() && local_content->media_description()->HasSimulcast(); @@ -538,7 +535,7 @@ static bool SimulcastIsRejected( return simulcast_offered && (!simulcast_answered || !rids_supported); } -static RTCError DisableSimulcastInSender( +RTCError DisableSimulcastInSender( rtc::scoped_refptr sender) { RTC_DCHECK(sender); RtpParameters parameters = sender->GetParametersInternal(); @@ -556,7 +553,7 @@ static RTCError DisableSimulcastInSender( // The SDP parser used to populate these values by default for the 'content // name' if an a=mid line was absent. -static absl::string_view GetDefaultMidForPlanB(cricket::MediaType media_type) { +absl::string_view GetDefaultMidForPlanB(cricket::MediaType media_type) { switch (media_type) { case cricket::MEDIA_TYPE_AUDIO: return cricket::CN_AUDIO; @@ -595,10 +592,8 @@ void AddPlanBRtpSenderOptions( } } -static cricket::MediaDescriptionOptions -GetMediaDescriptionOptionsForTransceiver( - rtc::scoped_refptr> - transceiver, +cricket::MediaDescriptionOptions GetMediaDescriptionOptionsForTransceiver( + RtpTransceiver* transceiver, const std::string& mid, bool is_create_offer) { // NOTE: a stopping transceiver should be treated as a stopped one in @@ -618,7 +613,7 @@ GetMediaDescriptionOptionsForTransceiver( // 2. If the MSID is included, then it must be included in any subsequent // offer/answer exactly the same until the RtpTransceiver is stopped. if (stopped || (!RtpTransceiverDirectionHasSend(transceiver->direction()) && - !transceiver->internal()->has_ever_been_used_to_send())) { + !transceiver->has_ever_been_used_to_send())) { return media_description_options; } @@ -629,7 +624,7 @@ GetMediaDescriptionOptionsForTransceiver( // The following sets up RIDs and Simulcast. // RIDs are included if Simulcast is requested or if any RID was specified. RtpParameters send_parameters = - transceiver->internal()->sender_internal()->GetParametersInternal(); + transceiver->sender_internal()->GetParametersInternal(); bool has_rids = std::any_of(send_parameters.encodings.begin(), send_parameters.encodings.end(), [](const RtpEncodingParameters& encoding) { @@ -661,9 +656,8 @@ GetMediaDescriptionOptionsForTransceiver( } // Returns the ContentInfo at mline index |i|, or null if none exists. -static const ContentInfo* GetContentByIndex( - const SessionDescriptionInterface* sdesc, - size_t i) { +const ContentInfo* GetContentByIndex(const SessionDescriptionInterface* sdesc, + size_t i) { if (!sdesc) { return nullptr; } @@ -1291,7 +1285,8 @@ RTCError SdpOfferAnswerHandler::ApplyLocalDescription( } std::vector> remove_list; std::vector> removed_streams; - for (const auto& transceiver : transceivers()->List()) { + for (const auto& transceiver_ext : transceivers()->List()) { + auto transceiver = transceiver_ext->internal(); if (transceiver->stopped()) { continue; } @@ -1302,10 +1297,8 @@ RTCError SdpOfferAnswerHandler::ApplyLocalDescription( if (transceiver->mid()) { auto dtls_transport = LookupDtlsTransportByMid( pc_->network_thread(), transport_controller(), *transceiver->mid()); - transceiver->internal()->sender_internal()->set_transport( - dtls_transport); - transceiver->internal()->receiver_internal()->set_transport( - dtls_transport); + transceiver->sender_internal()->set_transport(dtls_transport); + transceiver->receiver_internal()->set_transport(dtls_transport); } const ContentInfo* content = @@ -1322,16 +1315,15 @@ RTCError SdpOfferAnswerHandler::ApplyLocalDescription( // "recvonly", process the removal of a remote track for the media // description, given transceiver, removeList, and muteTracks. if (!RtpTransceiverDirectionHasRecv(media_desc->direction()) && - (transceiver->internal()->fired_direction() && - RtpTransceiverDirectionHasRecv( - *transceiver->internal()->fired_direction()))) { - ProcessRemovalOfRemoteTrack(transceiver, &remove_list, + (transceiver->fired_direction() && + RtpTransceiverDirectionHasRecv(*transceiver->fired_direction()))) { + ProcessRemovalOfRemoteTrack(transceiver_ext, &remove_list, &removed_streams); } // 2.2.7.1.6.2: Set transceiver's [[CurrentDirection]] and // [[FiredDirection]] slots to direction. - transceiver->internal()->set_current_direction(media_desc->direction()); - transceiver->internal()->set_fired_direction(media_desc->direction()); + transceiver->set_current_direction(media_desc->direction()); + transceiver->set_fired_direction(media_desc->direction()); } } auto observer = pc_->Observer(); @@ -1380,7 +1372,10 @@ RTCError SdpOfferAnswerHandler::ApplyLocalDescription( } if (IsUnifiedPlan()) { - for (const auto& transceiver : transceivers()->List()) { + // We must use List and not ListInternal here because + // transceivers()->StableState() is indexed by the non-internal refptr. + for (const auto& transceiver_ext : transceivers()->List()) { + auto transceiver = transceiver_ext->internal(); if (transceiver->stopped()) { continue; } @@ -1389,25 +1384,22 @@ RTCError SdpOfferAnswerHandler::ApplyLocalDescription( if (!content) { continue; } - cricket::ChannelInterface* channel = transceiver->internal()->channel(); + cricket::ChannelInterface* channel = transceiver->channel(); if (content->rejected || !channel || channel->local_streams().empty()) { // 0 is a special value meaning "this sender has no associated send // stream". Need to call this so the sender won't attempt to configure // a no longer existing stream and run into DCHECKs in the lower // layers. - transceiver->internal()->sender_internal()->SetSsrc(0); + transceiver->sender_internal()->SetSsrc(0); } else { // Get the StreamParams from the channel which could generate SSRCs. const std::vector& streams = channel->local_streams(); - transceiver->internal()->sender_internal()->set_stream_ids( - streams[0].stream_ids()); - auto encodings = - transceiver->internal()->sender_internal()->init_send_encodings(); - transceiver->internal()->sender_internal()->SetSsrc( - streams[0].first_ssrc()); + transceiver->sender_internal()->set_stream_ids(streams[0].stream_ids()); + auto encodings = transceiver->sender_internal()->init_send_encodings(); + transceiver->sender_internal()->SetSsrc(streams[0].first_ssrc()); if (!encodings.empty()) { transceivers() - ->StableState(transceiver) + ->StableState(transceiver_ext) ->SetInitSendEncodings(encodings); } } @@ -1654,7 +1646,8 @@ RTCError SdpOfferAnswerHandler::ApplyRemoteDescription( std::vector> remove_list; std::vector> added_streams; std::vector> removed_streams; - for (const auto& transceiver : transceivers()->List()) { + for (const auto& transceiver_ext : transceivers()->List()) { + const auto transceiver = transceiver_ext->internal(); const ContentInfo* content = FindMediaSectionForTransceiver(transceiver, remote_description()); if (!content) { @@ -1674,14 +1667,13 @@ RTCError SdpOfferAnswerHandler::ApplyRemoteDescription( stream_ids = media_desc->streams()[0].stream_ids(); } transceivers() - ->StableState(transceiver) + ->StableState(transceiver_ext) ->SetRemoteStreamIdsIfUnset(transceiver->receiver()->stream_ids()); RTC_LOG(LS_INFO) << "Processing the MSIDs for MID=" << content->name << " (" << GetStreamIdsString(stream_ids) << ")."; - SetAssociatedRemoteStreams(transceiver->internal()->receiver_internal(), - stream_ids, &added_streams, - &removed_streams); + SetAssociatedRemoteStreams(transceiver->receiver_internal(), stream_ids, + &added_streams, &removed_streams); // From the WebRTC specification, steps 2.2.8.5/6 of section 4.4.1.6 // "Set the RTCSessionDescription: If direction is sendrecv or recvonly, // and transceiver's current direction is neither sendrecv nor recvonly, @@ -1701,26 +1693,24 @@ RTCError SdpOfferAnswerHandler::ApplyRemoteDescription( if (!RtpTransceiverDirectionHasRecv(local_direction) && (transceiver->fired_direction() && RtpTransceiverDirectionHasRecv(*transceiver->fired_direction()))) { - ProcessRemovalOfRemoteTrack(transceiver, &remove_list, + ProcessRemovalOfRemoteTrack(transceiver_ext, &remove_list, &removed_streams); } // 2.2.8.1.10: Set transceiver's [[FiredDirection]] slot to direction. - transceiver->internal()->set_fired_direction(local_direction); + transceiver->set_fired_direction(local_direction); // 2.2.8.1.11: If description is of type "answer" or "pranswer", then run // the following steps: if (type == SdpType::kPrAnswer || type == SdpType::kAnswer) { // 2.2.8.1.11.1: Set transceiver's [[CurrentDirection]] slot to // direction. - transceiver->internal()->set_current_direction(local_direction); + transceiver->set_current_direction(local_direction); // 2.2.8.1.11.[3-6]: Set the transport internal slots. if (transceiver->mid()) { auto dtls_transport = LookupDtlsTransportByMid(pc_->network_thread(), transport_controller(), *transceiver->mid()); - transceiver->internal()->sender_internal()->set_transport( - dtls_transport); - transceiver->internal()->receiver_internal()->set_transport( - dtls_transport); + transceiver->sender_internal()->set_transport(dtls_transport); + transceiver->receiver_internal()->set_transport(dtls_transport); } } // 2.2.8.1.12: If the media description is rejected, and transceiver is @@ -1728,18 +1718,16 @@ RTCError SdpOfferAnswerHandler::ApplyRemoteDescription( if (content->rejected && !transceiver->stopped()) { RTC_LOG(LS_INFO) << "Stopping transceiver for MID=" << content->name << " since the media section was rejected."; - transceiver->internal()->StopTransceiverProcedure(); + transceiver->StopTransceiverProcedure(); } if (!content->rejected && RtpTransceiverDirectionHasRecv(local_direction)) { if (!media_desc->streams().empty() && media_desc->streams()[0].has_ssrcs()) { uint32_t ssrc = media_desc->streams()[0].first_ssrc(); - transceiver->internal()->receiver_internal()->SetupMediaChannel(ssrc); + transceiver->receiver_internal()->SetupMediaChannel(ssrc); } else { - transceiver->internal() - ->receiver_internal() - ->SetupUnsignaledMediaChannel(); + transceiver->receiver_internal()->SetupUnsignaledMediaChannel(); } } } @@ -2859,12 +2847,12 @@ bool SdpOfferAnswerHandler::CheckIfNegotiationIsNeeded() { // 5. For each transceiver in connection's set of transceivers, perform the // following checks: - for (const auto& transceiver : transceivers()->List()) { + for (const auto& transceiver : transceivers()->ListInternal()) { const ContentInfo* current_local_msection = - FindTransceiverMSection(transceiver.get(), description); + FindTransceiverMSection(transceiver, description); - const ContentInfo* current_remote_msection = FindTransceiverMSection( - transceiver.get(), current_remote_description()); + const ContentInfo* current_remote_msection = + FindTransceiverMSection(transceiver, current_remote_description()); // 5.4 If transceiver is stopped and is associated with an m= section, // but the associated m= section is not yet rejected in @@ -2952,7 +2940,7 @@ bool SdpOfferAnswerHandler::CheckIfNegotiationIsNeeded() { return true; const ContentInfo* offered_remote_msection = - FindTransceiverMSection(transceiver.get(), remote_description()); + FindTransceiverMSection(transceiver, remote_description()); RtpTransceiverDirection offered_direction = offered_remote_msection @@ -3456,19 +3444,17 @@ SdpOfferAnswerHandler::FindAvailableTransceiverToReceive( const cricket::ContentInfo* SdpOfferAnswerHandler::FindMediaSectionForTransceiver( - rtc::scoped_refptr> - transceiver, + const RtpTransceiver* transceiver, const SessionDescriptionInterface* sdesc) const { RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK(transceiver); RTC_DCHECK(sdesc); if (IsUnifiedPlan()) { - if (!transceiver->internal()->mid()) { + if (!transceiver->mid()) { // This transceiver is not associated with a media section yet. return nullptr; } - return sdesc->description()->GetContentByName( - *transceiver->internal()->mid()); + return sdesc->description()->GetContentByName(*transceiver->mid()); } else { // Plan B only allows at most one audio and one video section, so use the // first media section of that type. @@ -3665,7 +3651,7 @@ void SdpOfferAnswerHandler::GetOptionsForUnifiedPlanOffer( } else { session_options->media_description_options.push_back( GetMediaDescriptionOptionsForTransceiver( - transceiver, mid, + transceiver->internal(), mid, /*is_create_offer=*/true)); // CreateOffer shouldn't really cause any state changes in // PeerConnection, but we need a way to match new transceivers to new @@ -3703,7 +3689,7 @@ void SdpOfferAnswerHandler::GetOptionsForUnifiedPlanOffer( // and not associated). Reuse media sections marked as recyclable first, // otherwise append to the end of the offer. New media sections should be // added in the order they were added to the PeerConnection. - for (const auto& transceiver : transceivers()->List()) { + for (const auto& transceiver : transceivers()->ListInternal()) { if (transceiver->mid() || transceiver->stopping()) { continue; } @@ -3723,7 +3709,7 @@ void SdpOfferAnswerHandler::GetOptionsForUnifiedPlanOffer( /*is_create_offer=*/true)); } // See comment above for why CreateOffer changes the transceiver's state. - transceiver->internal()->set_mline_index(mline_index); + transceiver->set_mline_index(mline_index); } // Lastly, add a m-section if we have local data channels and an m section // does not already exist. @@ -3826,7 +3812,7 @@ void SdpOfferAnswerHandler::GetOptionsForUnifiedPlanAnswer( if (transceiver) { session_options->media_description_options.push_back( GetMediaDescriptionOptionsForTransceiver( - transceiver, content.name, + transceiver->internal(), content.name, /*is_create_offer=*/false)); } else { // This should only happen with rejected transceivers. @@ -4149,8 +4135,8 @@ void SdpOfferAnswerHandler::UpdateRemoteSendersList( void SdpOfferAnswerHandler::EnableSending() { RTC_DCHECK_RUN_ON(signaling_thread()); - for (const auto& transceiver : transceivers()->List()) { - cricket::ChannelInterface* channel = transceiver->internal()->channel(); + for (const auto& transceiver : transceivers()->ListInternal()) { + cricket::ChannelInterface* channel = transceiver->channel(); if (channel && !channel->enabled()) { channel->Enable(true); } @@ -4175,10 +4161,10 @@ RTCError SdpOfferAnswerHandler::PushdownMediaDescription( } // Push down the new SDP media section for each audio/video transceiver. - for (const auto& transceiver : transceivers()->List()) { + for (const auto& transceiver : transceivers()->ListInternal()) { const ContentInfo* content_info = FindMediaSectionForTransceiver(transceiver, sdesc); - cricket::ChannelInterface* channel = transceiver->internal()->channel(); + cricket::ChannelInterface* channel = transceiver->channel(); if (!channel || !content_info || content_info->rejected) { continue; } @@ -4258,10 +4244,10 @@ void SdpOfferAnswerHandler::RemoveStoppedTransceivers() { if (!transceiver->stopped()) { continue; } - const ContentInfo* local_content = - FindMediaSectionForTransceiver(transceiver, local_description()); - const ContentInfo* remote_content = - FindMediaSectionForTransceiver(transceiver, remote_description()); + const ContentInfo* local_content = FindMediaSectionForTransceiver( + transceiver->internal(), local_description()); + const ContentInfo* remote_content = FindMediaSectionForTransceiver( + transceiver->internal(), remote_description()); if ((local_content && local_content->rejected) || (remote_content && remote_content->rejected)) { RTC_LOG(LS_INFO) << "Dissociating transceiver" @@ -4824,8 +4810,8 @@ bool SdpOfferAnswerHandler::UpdatePayloadTypeDemuxingState( // single Invoke; necessary due to thread guards. std::vector> channels_to_update; - for (const auto& transceiver : transceivers()->List()) { - cricket::ChannelInterface* channel = transceiver->internal()->channel(); + for (const auto& transceiver : transceivers()->ListInternal()) { + cricket::ChannelInterface* channel = transceiver->channel(); const ContentInfo* content = FindMediaSectionForTransceiver(transceiver, sdesc); if (!channel || !content) { @@ -4836,8 +4822,7 @@ bool SdpOfferAnswerHandler::UpdatePayloadTypeDemuxingState( if (source == cricket::CS_REMOTE) { local_direction = RtpTransceiverDirectionReversed(local_direction); } - channels_to_update.emplace_back(local_direction, - transceiver->internal()->channel()); + channels_to_update.emplace_back(local_direction, transceiver->channel()); } if (channels_to_update.empty()) { diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h index 2074821ebd..0608c38ce5 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h @@ -13,7 +13,6 @@ #include #include - #include #include #include @@ -175,15 +174,6 @@ class SdpOfferAnswerHandler : public SdpStateProvider, bool HasNewIceCredentials(); void UpdateNegotiationNeeded(); - // Returns the media section in the given session description that is - // associated with the RtpTransceiver. Returns null if none found or this - // RtpTransceiver is not associated. Logic varies depending on the - // SdpSemantics specified in the configuration. - const cricket::ContentInfo* FindMediaSectionForTransceiver( - rtc::scoped_refptr> - transceiver, - const SessionDescriptionInterface* sdesc) const; - // Destroys all BaseChannels and destroys the SCTP data channel, if present. void DestroyAllChannels(); @@ -319,6 +309,14 @@ class SdpOfferAnswerHandler : public SdpStateProvider, const cricket::ContentInfo* old_remote_content) RTC_RUN_ON(signaling_thread()); + // Returns the media section in the given session description that is + // associated with the RtpTransceiver. Returns null if none found or this + // RtpTransceiver is not associated. Logic varies depending on the + // SdpSemantics specified in the configuration. + const cricket::ContentInfo* FindMediaSectionForTransceiver( + const RtpTransceiver* transceiver, + const SessionDescriptionInterface* sdesc) const; + // If the BUNDLE policy is max-bundle, then we know for sure that all // transports will be bundled from the start. This method returns the BUNDLE // group if that's the case, or null if BUNDLE will be negotiated later. An @@ -420,7 +418,7 @@ class SdpOfferAnswerHandler : public SdpStateProvider, // |removed_streams| is the list of streams which no longer have a receiving // track so should be removed. void ProcessRemovalOfRemoteTrack( - rtc::scoped_refptr> + const rtc::scoped_refptr> transceiver, std::vector>* remove_list, std::vector>* removed_streams); diff --git a/pc/transceiver_list.cc b/pc/transceiver_list.cc index 63d3e67ad8..235c9af036 100644 --- a/pc/transceiver_list.cc +++ b/pc/transceiver_list.cc @@ -41,8 +41,18 @@ void TransceiverStableState::SetInitSendEncodings( init_send_encodings_ = encodings; } +std::vector TransceiverList::ListInternal() const { + RTC_DCHECK_RUN_ON(&sequence_checker_); + std::vector internals; + for (auto transceiver : transceivers_) { + internals.push_back(transceiver->internal()); + } + return internals; +} + RtpTransceiverProxyRefPtr TransceiverList::FindBySender( rtc::scoped_refptr sender) const { + RTC_DCHECK_RUN_ON(&sequence_checker_); for (auto transceiver : transceivers_) { if (transceiver->sender() == sender) { return transceiver; @@ -53,6 +63,7 @@ RtpTransceiverProxyRefPtr TransceiverList::FindBySender( RtpTransceiverProxyRefPtr TransceiverList::FindByMid( const std::string& mid) const { + RTC_DCHECK_RUN_ON(&sequence_checker_); for (auto transceiver : transceivers_) { if (transceiver->mid() == mid) { return transceiver; @@ -63,6 +74,7 @@ RtpTransceiverProxyRefPtr TransceiverList::FindByMid( RtpTransceiverProxyRefPtr TransceiverList::FindByMLineIndex( size_t mline_index) const { + RTC_DCHECK_RUN_ON(&sequence_checker_); for (auto transceiver : transceivers_) { if (transceiver->internal()->mline_index() == mline_index) { return transceiver; diff --git a/pc/transceiver_list.h b/pc/transceiver_list.h index 2eb4313915..568c9c7e7a 100644 --- a/pc/transceiver_list.h +++ b/pc/transceiver_list.h @@ -21,9 +21,14 @@ #include "absl/types/optional.h" #include "api/media_types.h" #include "api/rtc_error.h" +#include "api/rtp_parameters.h" #include "api/rtp_sender_interface.h" #include "api/scoped_refptr.h" +#include "api/sequence_checker.h" #include "pc/rtp_transceiver.h" +#include "rtc_base/checks.h" +#include "rtc_base/system/no_unique_address.h" +#include "rtc_base/thread_annotations.h" namespace webrtc { @@ -68,14 +73,36 @@ class TransceiverStableState { bool newly_created_ = false; }; +// This class encapsulates the active list of transceivers on a +// PeerConnection, and offers convenient functions on that list. +// It is a single-thread class; all operations must be performed +// on the same thread. class TransceiverList { public: - std::vector List() const { return transceivers_; } + // Returns a copy of the currently active list of transceivers. The + // list consists of rtc::scoped_refptrs, which will keep the transceivers + // from being deallocated, even if they are removed from the TransceiverList. + std::vector List() const { + RTC_DCHECK_RUN_ON(&sequence_checker_); + return transceivers_; + } + // As above, but does not check thread ownership. Unsafe. + // TODO(bugs.webrtc.org/12692): Refactor and remove + std::vector UnsafeList() const { + return transceivers_; + } + + // Returns a list of the internal() pointers of the currently active list + // of transceivers. These raw pointers are not thread-safe, so need to + // be consumed on the same thread. + std::vector ListInternal() const; void Add(RtpTransceiverProxyRefPtr transceiver) { + RTC_DCHECK_RUN_ON(&sequence_checker_); transceivers_.push_back(transceiver); } void Remove(RtpTransceiverProxyRefPtr transceiver) { + RTC_DCHECK_RUN_ON(&sequence_checker_); transceivers_.erase( std::remove(transceivers_.begin(), transceivers_.end(), transceiver), transceivers_.end()); @@ -87,26 +114,33 @@ class TransceiverList { // Find or create the stable state for a transceiver. TransceiverStableState* StableState(RtpTransceiverProxyRefPtr transceiver) { + RTC_DCHECK_RUN_ON(&sequence_checker_); return &(transceiver_stable_states_by_transceivers_[transceiver]); } void DiscardStableStates() { + RTC_DCHECK_RUN_ON(&sequence_checker_); transceiver_stable_states_by_transceivers_.clear(); } std::map& StableStates() { + RTC_DCHECK_RUN_ON(&sequence_checker_); return transceiver_stable_states_by_transceivers_; } private: + RTC_NO_UNIQUE_ADDRESS SequenceChecker sequence_checker_; std::vector transceivers_; + // TODO(bugs.webrtc.org/12692): Add RTC_GUARDED_BY(sequence_checker_); + // Holds changes made to transceivers during applying descriptors for // potential rollback. Gets cleared once signaling state goes to stable. std::map - transceiver_stable_states_by_transceivers_; + transceiver_stable_states_by_transceivers_ + RTC_GUARDED_BY(sequence_checker_); // Holds remote stream ids for transceivers from stable state. std::map> - remote_stream_ids_by_transceivers_; + remote_stream_ids_by_transceivers_ RTC_GUARDED_BY(sequence_checker_); }; } // namespace webrtc