Avoid `channel()` in LegacyStatsCollector transport lookup
Refactors ExtractSessionInfo_n to use JsepTransportController for
transport name lookup, avoiding unsafe access to
RtpTransceiver::channel() on the network thread.
Key changes:
- ExtractSessionInfo_n now takes a vector of mids instead of a map.
- Added virtual GetTransportName() to handle transport lookup, allowing
overrides in unit tests where JsepTransportController is absent.
Bug: None
Change-Id: I33afbb7c0f56245e1b332b820fb09cd8e20cebf5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/436840
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#46544}
diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index bbf173a..6938af7 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -1414,6 +1414,8 @@
":channel",
":channel_interface",
":data_channel_utils",
+ ":jsep_transport",
+ ":jsep_transport_controller",
":legacy_stats_collector_interface",
":peer_connection_internal",
":rtp_receiver",
@@ -1437,6 +1439,7 @@
"../call:call_interfaces",
"../media:media_channel",
"../p2p:connection_info",
+ "../p2p:dtls_transport_internal",
"../p2p:ice_transport_internal",
"../p2p:p2p_constants",
"../p2p:port",
diff --git a/pc/legacy_stats_collector.cc b/pc/legacy_stats_collector.cc
index 06abf6b..6049200 100644
--- a/pc/legacy_stats_collector.cc
+++ b/pc/legacy_stats_collector.cc
@@ -41,9 +41,11 @@
#include "p2p/base/ice_transport_internal.h"
#include "p2p/base/p2p_constants.h"
#include "p2p/base/port.h"
+#include "p2p/dtls/dtls_transport_internal.h"
#include "pc/channel.h"
#include "pc/channel_interface.h"
#include "pc/data_channel_utils.h"
+#include "pc/jsep_transport_controller.h"
#include "pc/peer_connection_internal.h"
#include "pc/rtp_receiver.h"
#include "pc/rtp_receiver_proxy.h"
@@ -886,10 +888,10 @@
StatsCollection::Container data_report_collection;
auto transceivers = pc_->GetTransceiversInternal();
- std::map<RtpTransceiver*, std::string> mids;
+ std::vector<std::string> mids;
for (const auto& transceiver : transceivers) {
if (transceiver->mid()) {
- mids[transceiver->internal()] = *transceiver->mid();
+ mids.push_back(*transceiver->mid());
}
}
@@ -899,22 +901,36 @@
StatsCollection data_reports;
ExtractDataInfo_n(&data_reports);
data_report_collection = data_reports.DetachCollection();
- return ExtractSessionInfo_n(transceivers, mids,
- std::move(sctp_transport_name),
+ return ExtractSessionInfo_n(mids, std::move(sctp_transport_name),
std::move(sctp_mid));
});
reports_.MergeCollection(std::move(data_report_collection));
ExtractSessionInfo_s(stats);
-
return std::move(stats.transport_names_by_mid);
}
+std::optional<std::string> LegacyStatsCollector::GetTransportName(
+ absl::string_view mid) {
+ RTC_DCHECK_RUN_ON(pc_->network_thread());
+ JsepTransportController* controller = pc_->transport_controller_n();
+ if (!controller) {
+ return std::nullopt;
+ }
+ DtlsTransportInternal* dtls_transport = controller->GetDtlsTransport(mid);
+ if (!dtls_transport) {
+ return std::nullopt;
+ }
+ IceTransportInternal* ice_transport = dtls_transport->ice_transport();
+ if (!ice_transport) {
+ return std::nullopt;
+ }
+ return ice_transport->transport_name();
+}
+
LegacyStatsCollector::SessionStats LegacyStatsCollector::ExtractSessionInfo_n(
- const std::vector<scoped_refptr<
- RtpTransceiverProxyWithInternal<RtpTransceiver>>>& transceivers,
- const std::map<RtpTransceiver*, std::string>& mids,
+ const std::vector<std::string>& mids,
std::optional<std::string> sctp_transport_name,
std::optional<std::string> sctp_mid) {
TRACE_EVENT0("webrtc", "LegacyStatsCollector::ExtractSessionInfo_n");
@@ -922,14 +938,9 @@
Thread::ScopedDisallowBlockingCalls no_blocking_calls;
SessionStats stats;
stats.candidate_stats = pc_->GetPooledCandidateStats();
- for (auto& transceiver : transceivers) {
- ChannelInterface* channel = transceiver->internal()->channel();
- if (channel) {
- auto it = mids.find(transceiver->internal());
- if (it != mids.end()) {
- stats.transport_names_by_mid[it->second] =
- std::string(channel->transport_name());
- }
+ for (const std::string& mid : mids) {
+ if (std::optional<std::string> transport_name = GetTransportName(mid)) {
+ stats.transport_names_by_mid[mid] = *transport_name;
}
}
@@ -1240,8 +1251,10 @@
if (transceiver->mid()) {
gatherer->mid = *transceiver->mid();
}
- gatherer->transport_name = transport_names_by_mid.at(gatherer->mid);
-
+ auto it = transport_names_by_mid.find(gatherer->mid);
+ if (it != transport_names_by_mid.end()) {
+ gatherer->transport_name = it->second;
+ }
for (const auto& sender : transceiver->internal()->senders()) {
auto track = sender->track();
std::string track_id = (track ? track->id() : "");
diff --git a/pc/legacy_stats_collector.h b/pc/legacy_stats_collector.h
index 517ba7f..941c657 100644
--- a/pc/legacy_stats_collector.h
+++ b/pc/legacy_stats_collector.h
@@ -24,16 +24,15 @@
#include <utility>
#include <vector>
+#include "absl/strings/string_view.h"
#include "api/candidate.h"
#include "api/legacy_stats_types.h"
#include "api/media_stream_interface.h"
#include "api/peer_connection_interface.h"
-#include "api/scoped_refptr.h"
#include "p2p/base/connection_info.h"
#include "p2p/base/port.h"
#include "pc/legacy_stats_collector_interface.h"
#include "pc/peer_connection_internal.h"
-#include "pc/rtp_transceiver.h"
#include "pc/transport_stats.h"
#include "rtc_base/network_constants.h"
#include "rtc_base/ssl_certificate.h"
@@ -142,6 +141,9 @@
std::map<std::string, std::string> transport_names_by_mid;
};
+ // Overridden in unit tests to fake transport lookup.
+ virtual std::optional<std::string> GetTransportName(absl::string_view mid);
+
// Overridden in unit tests to fake timing.
virtual double GetTimeNow();
@@ -189,9 +191,7 @@
void UpdateTrackReports();
SessionStats ExtractSessionInfo_n(
- const std::vector<scoped_refptr<
- RtpTransceiverProxyWithInternal<RtpTransceiver>>>& transceivers,
- const std::map<RtpTransceiver*, std::string>& mids,
+ const std::vector<std::string>& mids,
std::optional<std::string> sctp_transport_name,
std::optional<std::string> sctp_mid);
void ExtractSessionInfo_s(SessionStats& session_stats);
diff --git a/pc/legacy_stats_collector_unittest.cc b/pc/legacy_stats_collector_unittest.cc
index 4d41052..b21c944 100644
--- a/pc/legacy_stats_collector_unittest.cc
+++ b/pc/legacy_stats_collector_unittest.cc
@@ -20,6 +20,7 @@
#include "absl/algorithm/container.h"
#include "absl/strings/str_cat.h"
+#include "absl/strings/string_view.h"
#include "api/audio/audio_processing_statistics.h"
#include "api/audio_codecs/audio_encoder.h"
#include "api/candidate.h"
@@ -585,12 +586,26 @@
class LegacyStatsCollectorForTest : public LegacyStatsCollector {
public:
explicit LegacyStatsCollectorForTest(PeerConnectionInternal* pc, Clock& clock)
- : LegacyStatsCollector(pc, clock), time_now_(19477) {}
+ : LegacyStatsCollector(pc, clock),
+ time_now_(19477),
+ pc_(static_cast<FakePeerConnectionForStats*>(pc)) {}
double GetTimeNow() override { return time_now_; }
+ std::optional<std::string> GetTransportName(absl::string_view mid) override {
+ for (const auto& transceiver : pc_->GetTransceiversInternal()) {
+ if (transceiver->mid() == mid) {
+ if (auto* channel = transceiver->internal()->channel()) {
+ return std::string(channel->transport_name());
+ }
+ }
+ }
+ return std::nullopt;
+ }
+
private:
double time_now_;
+ FakePeerConnectionForStats* pc_;
};
class LegacyStatsCollectorTest : public ::testing::Test {
