Require HasChannel calls to be called on the signaling thread Update stat collection to not call HasChannel or receivers() from the worker thread, grab state earlier in the gatherer object. Also, - Update RtpTransceiver::SetRtpTransport to return early if no channel exists (consistent with previous HasChannel() check). - Update FakePeerConnectionForStats to support DTLS transport lookups and property assignments (transport_name) to improve compatibility with production code gathering logic. Bug: webrtc:475126742 Change-Id: If31fbd25d5bcad911e9ed1866b40c41481b81cd3 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/468900 Reviewed-by: Harald Alvestrand <hta@webrtc.org> Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47607}
diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 1f0b54e..6380b7c 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn
@@ -4425,6 +4425,7 @@ "../p2p:fake_ice_transport", "../p2p:fake_port_allocator", "../p2p:p2p_constants", + "../p2p:p2p_test_utils", "../p2p:port", "../p2p:port_allocator", "../p2p:transport_description",
diff --git a/pc/legacy_stats_collector.cc b/pc/legacy_stats_collector.cc index 6978f1b..2fc17a0 100644 --- a/pc/legacy_stats_collector.cc +++ b/pc/legacy_stats_collector.cc
@@ -1163,8 +1163,19 @@ class ChannelStatsGatherer { public: + static std::vector< + scoped_refptr<RtpReceiverProxyWithInternal<RtpReceiverInternal>>> + GetReceiversSnapshot(RtpTransceiver* transceiver) { + RTC_ALLOW_PLAN_B_DEPRECATION_BEGIN() + return transceiver->receivers(); + RTC_ALLOW_PLAN_B_DEPRECATION_END() + } + explicit ChannelStatsGatherer(RtpTransceiver* absl_nonnull transceiver) - : transceiver_(transceiver) { + : mid(transceiver->mid().value_or("")), + transport_name(transceiver->transport_name().value_or("")), + receivers_(GetReceiversSnapshot(transceiver)), + transceiver_(transceiver) { RTC_DCHECK(transceiver_); } virtual ~ChannelStatsGatherer() = default; @@ -1175,8 +1186,14 @@ virtual bool HasRemoteAudio() const = 0; - std::string mid; - std::string transport_name; + const std::vector< + scoped_refptr<RtpReceiverProxyWithInternal<RtpReceiverInternal>>>& + receivers() const { + return receivers_; + } + + const std::string mid; + const std::string transport_name; std::map<uint32_t, std::string> sender_track_id_by_ssrc; std::map<uint32_t, std::string> receiver_track_id_by_ssrc; @@ -1197,6 +1214,9 @@ RtpTransceiver* transceiver() { return transceiver_; } private: + const std::vector< + scoped_refptr<RtpReceiverProxyWithInternal<RtpReceiverInternal>>> + receivers_; RtpTransceiver* const transceiver_; }; @@ -1302,13 +1322,6 @@ } std::unique_ptr<ChannelStatsGatherer> gatherer = CreateChannelStatsGatherer(transceiver->internal()); - if (transceiver->mid()) { - gatherer->mid = *transceiver->mid(); - } - auto it = transport_names_by_mid.find(gatherer->mid); - if (it != transport_names_by_mid.end()) { - gatherer->transport_name = it->second; - } RTC_ALLOW_PLAN_B_DEPRECATION_BEGIN() for (const auto& sender : transceiver->internal()->senders()) { auto track = sender->track(); @@ -1329,13 +1342,9 @@ pc_->worker_thread()->BlockingCall([&] { Thread::ScopedDisallowBlockingCalls no_blocking_calls; // Populate `receiver_track_id_by_ssrc` for the gatherers. - int i = 0; - for (const auto& transceiver : transceivers) { - if (!transceiver->internal()->HasChannel()) - continue; - ChannelStatsGatherer* gatherer = gatherers[i++].get(); + for (const std::unique_ptr<ChannelStatsGatherer>& gatherer : gatherers) { RTC_ALLOW_PLAN_B_DEPRECATION_BEGIN() - for (const auto& receiver : transceiver->internal()->receivers()) { + for (const auto& receiver : gatherer->receivers()) { gatherer->receiver_track_id_by_ssrc.insert(std::make_pair( receiver->internal()->ssrc().value_or(0), receiver->track()->id())); }
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 8483d22..a57b653 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc
@@ -3106,7 +3106,7 @@ for (const auto& transceiver : rtp_manager()->transceivers()->UnsafeList()) { auto internal = transceiver->internal(); - if (internal->HasChannel() && internal->mid() == mid) { + if (internal->mid() == mid) { ret = internal->SetRtpTransport(rtp_transport); } }
diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc index ca6809d..931f61a 100644 --- a/pc/rtp_transceiver.cc +++ b/pc/rtp_transceiver.cc
@@ -1369,7 +1369,10 @@ bool RtpTransceiver::SetRtpTransport(RtpTransportInternal* transport) { RTC_DCHECK_RUN_ON(context()->network_thread()); - RTC_DCHECK(channel_); + + if (!channel_) { + return true; + } if (transport == rtp_transport_) { return true;
diff --git a/pc/rtp_transceiver.h b/pc/rtp_transceiver.h index fe51e86..6708022 100644 --- a/pc/rtp_transceiver.h +++ b/pc/rtp_transceiver.h
@@ -399,8 +399,7 @@ // Wrappers for ChannelInterface bool HasChannel() const { - // Accessed from multiple threads. - // See https://issues.webrtc.org/475126742 + RTC_DCHECK_RUN_ON(thread_); return channel_ != nullptr; }
diff --git a/pc/test/fake_peer_connection_for_stats.h b/pc/test/fake_peer_connection_for_stats.h index 12c23e7..0de9786 100644 --- a/pc/test/fake_peer_connection_for_stats.h +++ b/pc/test/fake_peer_connection_for_stats.h
@@ -48,6 +48,7 @@ #include "p2p/base/port.h" #include "p2p/base/transport_description.h" #include "p2p/base/transport_info.h" +#include "p2p/dtls/fake_dtls_transport.h" #include "p2p/test/fake_ice_transport.h" #include "p2p/test/fake_port_allocator.h" #include "pc/channel.h" @@ -438,6 +439,13 @@ voice_media_send_channel_ptr, voice_media_receive_channel_ptr); auto dtls_transport = transport_controller_->LookupDtlsTransportByMid(mid); + if (!dtls_transport) { + auto fake_dtls = std::make_unique<FakeDtlsTransport>( + transport_name, ICE_CANDIDATE_COMPONENT_RTP); + auto wrapper = make_ref_counted<DtlsTransport>(fake_dtls.get()); + fake_dtls_transports_[mid] = std::move(fake_dtls); + dtls_transport = wrapper; + } transceiver->SetTransport(dtls_transport, transport_name); voice_media_send_channel_ptr->SetStats(initial_stats); voice_media_receive_channel_ptr->SetStats(initial_stats); @@ -477,6 +485,13 @@ video_media_send_channel_ptr, video_media_receive_channel_ptr); auto dtls_transport = transport_controller_->LookupDtlsTransportByMid(mid); + if (!dtls_transport) { + auto fake_dtls = std::make_unique<FakeDtlsTransport>( + transport_name, ICE_CANDIDATE_COMPONENT_RTP); + auto wrapper = make_ref_counted<DtlsTransport>(fake_dtls.get()); + fake_dtls_transports_[mid] = std::move(fake_dtls); + dtls_transport = wrapper; + } transceiver->SetTransport(dtls_transport, transport_name); video_media_send_channel_ptr->SetStats(initial_stats); video_media_receive_channel_ptr->SetStats(initial_stats); @@ -600,7 +615,15 @@ const std::set<std::string>& transport_names) override { RTC_DCHECK_RUN_ON(network_thread_); std::map<std::string, TransportStats> transport_stats_by_name; - for (const std::string& transport_name : transport_names) { + std::set<std::string> all_names = transport_names; + // In some legacy stats tests, the fake PeerConnection's + // JsepTransportController is not fully configured, causing + // `GetTransportName` to return `std::nullopt`. This fallback loop ensures + // stats are still retrieved for these transports. + for (const auto& entry : transport_names_by_mid_) { + all_names.insert(entry.second); + } + for (const std::string& transport_name : all_names) { transport_stats_by_name[transport_name] = GetTransportStatsByName(transport_name); } @@ -777,6 +800,8 @@ std::optional<FakePortAllocator> port_allocator_; std::unique_ptr<JsepTransportController> transport_controller_; std::map<std::string, std::string> transport_names_by_mid_; + std::map<std::string, std::unique_ptr<FakeDtlsTransport>> + fake_dtls_transports_; }; } // namespace webrtc