From efe4c92d54b8b1b1fc9faed6e5ca303546e0ca9b Mon Sep 17 00:00:00 2001 From: Steve Anton Date: Wed, 27 Mar 2019 10:26:06 -0700 Subject: [PATCH] Use RtpSender/RtpReceiver track ID for legacy GetStats Previously, legacy GetStats would look up the track ID by querying the local/remote SDP by SSRC. This doesn't work with Unified Plan since the RtpSender/RtpReceiver track IDs may not correspond to the track ID stored in the SDP. This CL changes legacy GetStats to pull the track ID directly from the RtpSenders and RtpReceivers as it generates the stats. This has a few additional benefits: 1) Unsignaled receive SSRC stats should now get correctly matched to the unsigneled RtpReceiver track ID for both Plan B and Unified Plan. 2) Removes a couple methods on PeerConnection that were only used by the legacy StatsCollector. 3) Keeps the SSRC -> track ID mapping more localized which should make the code easier to understand. Bug: chromium:943493 Change-Id: I43ecde8c3a3d1c5f9c749ba6c8dfb11e8c4950fd Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/129782 Commit-Queue: Steve Anton Reviewed-by: Harald Alvestrand Reviewed-by: Amit Hilbuch Cr-Commit-Position: refs/heads/master@{#27324} --- pc/peer_connection.cc | 30 --- pc/peer_connection.h | 3 - pc/peer_connection_internal.h | 4 - pc/stats_collector.cc | 241 ++++++++++++++--------- pc/stats_collector.h | 6 +- pc/stats_collector_unittest.cc | 161 +++++++++++++-- pc/test/fake_peer_connection_base.h | 5 - pc/test/fake_peer_connection_for_stats.h | 46 ++--- 8 files changed, 318 insertions(+), 178 deletions(-) diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index bf501d534e..211c5a7073 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -5684,36 +5684,6 @@ cricket::IceConfig PeerConnection::ParseIceConfig( return ice_config; } -static absl::string_view GetTrackIdBySsrc( - const SessionDescriptionInterface* session_description, - uint32_t ssrc) { - if (!session_description) { - return {}; - } - for (const cricket::ContentInfo& content : - session_description->description()->contents()) { - const cricket::MediaContentDescription& media = - *content.media_description(); - if (media.type() == cricket::MEDIA_TYPE_AUDIO || - media.type() == cricket::MEDIA_TYPE_VIDEO) { - const cricket::StreamParams* stream_params = - cricket::GetStreamBySsrc(media.streams(), ssrc); - if (stream_params) { - return stream_params->id; - } - } - } - return {}; -} - -absl::string_view PeerConnection::GetLocalTrackIdBySsrc(uint32_t ssrc) { - return GetTrackIdBySsrc(local_description(), ssrc); -} - -absl::string_view PeerConnection::GetRemoteTrackIdBySsrc(uint32_t ssrc) { - return GetTrackIdBySsrc(remote_description(), ssrc); -} - bool PeerConnection::SendData(const cricket::SendDataParams& params, const rtc::CopyOnWriteBuffer& payload, cricket::SendDataResult* result) { diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 66e3f00b07..30155eaf07 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -237,9 +237,6 @@ class PeerConnection : public PeerConnectionInternal, return transceivers_; } - absl::string_view GetLocalTrackIdBySsrc(uint32_t ssrc) override; - absl::string_view GetRemoteTrackIdBySsrc(uint32_t ssrc) override; - sigslot::signal1& SignalDataChannelCreated() override { return SignalDataChannelCreated_; } diff --git a/pc/peer_connection_internal.h b/pc/peer_connection_internal.h index 2cfee76167..a51ba4b8f6 100644 --- a/pc/peer_connection_internal.h +++ b/pc/peer_connection_internal.h @@ -41,10 +41,6 @@ class PeerConnectionInternal : public PeerConnectionInterface { rtc::scoped_refptr>> GetTransceiversInternal() const = 0; - // Get the id used as a media stream track's "id" field from ssrc. - virtual absl::string_view GetLocalTrackIdBySsrc(uint32_t ssrc) = 0; - virtual absl::string_view GetRemoteTrackIdBySsrc(uint32_t ssrc) = 0; - virtual sigslot::signal1& SignalDataChannelCreated() = 0; // Only valid when using deprecated RTP data channels. diff --git a/pc/stats_collector.cc b/pc/stats_collector.cc index 6bfce676ca..e1930a1fc3 100644 --- a/pc/stats_collector.cc +++ b/pc/stats_collector.cc @@ -15,6 +15,7 @@ #include #include +#include "absl/memory/memory.h" #include "pc/channel.h" #include "pc/peer_connection.h" #include "rtc_base/checks.h" @@ -372,26 +373,56 @@ void ExtractRemoteStats(const cricket::MediaReceiverInfo& info, // TODO(hta): Extract some stats here. } +std::string GetTrackIdBySsrc( + uint32_t ssrc, + StatsReport::Direction direction, + const std::map& track_id_by_ssrc) { + auto it = track_id_by_ssrc.find(ssrc); + if (it != track_id_by_ssrc.end()) { + return it->second; + } + if (direction == StatsReport::kReceive) { + // If the track ID was not found, this might be an unsignaled receive + // SSRC, so try looking up by the special SSRC 0. + it = track_id_by_ssrc.find(0); + if (it != track_id_by_ssrc.end()) { + RTC_LOG(LS_INFO) << "Assuming SSRC=" << ssrc + << " is an unsignalled receive stream corresponding " + "to the RtpReceiver with track ID \"" + << it->second << "\"."; + return it->second; + } + } + RTC_LOG(LS_INFO) << "Missing track ID for " + << (direction == StatsReport::kSend ? "send" : "recv") + << " SSRC=" << ssrc << "."; + return ""; +} + // Template to extract stats from a data vector. // In order to use the template, the functions that are called from it, // ExtractStats and ExtractRemoteStats, must be defined and overloaded // for each type. template -void ExtractStatsFromList(const std::vector& data, - const StatsReport::Id& transport_id, - StatsCollector* collector, - StatsReport::Direction direction) { +void ExtractStatsFromList( + const std::vector& data, + const StatsReport::Id& transport_id, + StatsCollector* collector, + StatsReport::Direction direction, + const std::map& track_id_by_ssrc) { for (const auto& d : data) { uint32_t ssrc = d.ssrc(); + std::string track_id = GetTrackIdBySsrc(ssrc, direction, track_id_by_ssrc); // Each track can have stats for both local and remote objects. // TODO(hta): Handle the case of multiple SSRCs per object. StatsReport* report = - collector->PrepareReport(true, ssrc, transport_id, direction); + collector->PrepareReport(true, ssrc, track_id, transport_id, direction); if (report) ExtractStats(d, report); if (!d.remote_stats.empty()) { - report = collector->PrepareReport(false, ssrc, transport_id, direction); + report = collector->PrepareReport(false, ssrc, track_id, transport_id, + direction); if (report) ExtractRemoteStats(d, report); } @@ -580,6 +611,7 @@ void StatsCollector::UpdateStats( StatsReport* StatsCollector::PrepareReport(bool local, uint32_t ssrc, + const std::string& track_id, const StatsReport::Id& transport_id, StatsReport::Direction direction) { RTC_DCHECK(pc_->signaling_thread()->IsCurrent()); @@ -588,22 +620,6 @@ StatsReport* StatsCollector::PrepareReport(bool local, : StatsReport::kStatsReportTypeRemoteSsrc, rtc::ToString(ssrc), direction)); StatsReport* report = reports_.Find(id); - - // Use the ID of the track that is currently mapped to the SSRC, if any. - absl::string_view track_id = GetTrackIdBySsrc(ssrc, direction); - if (track_id.empty()) { - // The SSRC is not used by any existing track (or lookup failed since the - // SSRC wasn't signaled in SDP). Try copying the track ID from a previous - // report: if one exists. - if (report) { - const StatsReport::Value* v = - report->FindValue(StatsReport::kStatsValueNameTrackId); - if (v) { - track_id = v->string_val(); - } - } - } - if (!report) { report = reports_.InsertNew(id); } @@ -613,8 +629,7 @@ StatsReport* StatsCollector::PrepareReport(bool local, report->AddInt64(StatsReport::kStatsValueNameSsrc, ssrc); if (!track_id.empty()) { - report->AddString(StatsReport::kStatsValueNameTrackId, - std::string(track_id)); + report->AddString(StatsReport::kStatsValueNameTrackId, track_id); } // Add the mapping of SSRC to transport. report->AddId(StatsReport::kStatsValueNameTransportId, transport_id); @@ -895,68 +910,140 @@ void StatsCollector::ExtractBweInfo() { namespace { -struct VoiceChannelStatsInfo { +class MediaChannelStatsGatherer { + public: + virtual ~MediaChannelStatsGatherer() = default; + + virtual bool GetStatsOnWorkerThread() = 0; + + virtual void ExtractStats(StatsCollector* collector) const = 0; + + virtual bool HasRemoteAudio() const = 0; + + std::string mid; std::string transport_name; - cricket::VoiceMediaChannel* voice_media_channel; + std::map sender_track_id_by_ssrc; + std::map receiver_track_id_by_ssrc; + + protected: + template + void ExtractSenderReceiverStats( + StatsCollector* collector, + const std::vector& receiver_data, + const std::vector& sender_data) const { + RTC_DCHECK(collector); + StatsReport::Id transport_id = StatsReport::NewComponentId( + transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP); + ExtractStatsFromList(receiver_data, transport_id, collector, + StatsReport::kReceive, receiver_track_id_by_ssrc); + ExtractStatsFromList(sender_data, transport_id, collector, + StatsReport::kSend, sender_track_id_by_ssrc); + } +}; + +class VoiceMediaChannelStatsGatherer final : public MediaChannelStatsGatherer { + public: + VoiceMediaChannelStatsGatherer( + cricket::VoiceMediaChannel* voice_media_channel) + : voice_media_channel_(voice_media_channel) { + RTC_DCHECK(voice_media_channel_); + } + + bool GetStatsOnWorkerThread() override { + return voice_media_channel_->GetStats(&voice_media_info); + } + + void ExtractStats(StatsCollector* collector) const override { + ExtractSenderReceiverStats(collector, voice_media_info.receivers, + voice_media_info.senders); + } + + bool HasRemoteAudio() const override { + return !voice_media_info.receivers.empty(); + } + + private: + cricket::VoiceMediaChannel* voice_media_channel_; cricket::VoiceMediaInfo voice_media_info; }; -struct VideoChannelStatsInfo { - std::string transport_name; - cricket::VideoMediaChannel* video_media_channel; +class VideoMediaChannelStatsGatherer final : public MediaChannelStatsGatherer { + public: + VideoMediaChannelStatsGatherer( + cricket::VideoMediaChannel* video_media_channel) + : video_media_channel_(video_media_channel) { + RTC_DCHECK(video_media_channel_); + } + + bool GetStatsOnWorkerThread() override { + return video_media_channel_->GetStats(&video_media_info); + } + + void ExtractStats(StatsCollector* collector) const override { + ExtractSenderReceiverStats(collector, video_media_info.receivers, + video_media_info.senders); + } + + bool HasRemoteAudio() const override { return false; } + + private: + cricket::VideoMediaChannel* video_media_channel_; cricket::VideoMediaInfo video_media_info; }; +std::unique_ptr CreateMediaChannelStatsGatherer( + cricket::MediaChannel* channel) { + RTC_DCHECK(channel); + if (channel->media_type() == cricket::MEDIA_TYPE_AUDIO) { + return absl::make_unique( + static_cast(channel)); + } else { + RTC_DCHECK_EQ(channel->media_type(), cricket::MEDIA_TYPE_VIDEO); + return absl::make_unique( + static_cast(channel)); + } +} + } // namespace void StatsCollector::ExtractMediaInfo() { RTC_DCHECK_RUN_ON(pc_->signaling_thread()); - std::vector voice_channel_infos; - std::vector video_channel_infos; + std::vector> gatherers; { rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; for (const auto& transceiver : pc_->GetTransceiversInternal()) { - if (!transceiver->internal()->channel()) { + cricket::ChannelInterface* channel = transceiver->internal()->channel(); + if (!channel) { continue; } - cricket::MediaType media_type = transceiver->internal()->media_type(); - if (media_type == cricket::MEDIA_TYPE_AUDIO) { - auto* voice_channel = static_cast( - transceiver->internal()->channel()); - voice_channel_infos.emplace_back(); - VoiceChannelStatsInfo& info = voice_channel_infos.back(); - info.transport_name = voice_channel->transport_name(); - info.voice_media_channel = voice_channel->media_channel(); - } else { - RTC_DCHECK_EQ(media_type, cricket::MEDIA_TYPE_VIDEO); - auto* video_channel = static_cast( - transceiver->internal()->channel()); - video_channel_infos.emplace_back(); - VideoChannelStatsInfo& info = video_channel_infos.back(); - info.transport_name = video_channel->transport_name(); - info.video_media_channel = video_channel->media_channel(); + std::unique_ptr gatherer = + CreateMediaChannelStatsGatherer(channel->media_channel()); + gatherer->mid = channel->content_name(); + gatherer->transport_name = channel->transport_name(); + for (const auto& sender : transceiver->internal()->senders()) { + std::string track_id = (sender->track() ? sender->track()->id() : ""); + gatherer->sender_track_id_by_ssrc.insert( + std::make_pair(sender->ssrc(), track_id)); } + for (const auto& receiver : transceiver->internal()->receivers()) { + gatherer->receiver_track_id_by_ssrc.insert(std::make_pair( + receiver->internal()->ssrc(), receiver->track()->id())); + } + gatherers.push_back(std::move(gatherer)); } } pc_->worker_thread()->Invoke(RTC_FROM_HERE, [&] { rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; - for (auto it = voice_channel_infos.begin(); it != voice_channel_infos.end(); + for (auto it = gatherers.begin(); it != gatherers.end(); /* incremented manually */) { - if (!it->voice_media_channel->GetStats(&it->voice_media_info)) { - RTC_LOG(LS_ERROR) << "Failed to get voice channel stats"; - it = voice_channel_infos.erase(it); - continue; - } - ++it; - } - for (auto it = video_channel_infos.begin(); it != video_channel_infos.end(); - /* incremented manually */) { - if (!it->video_media_channel->GetStats(&it->video_media_info)) { - RTC_LOG(LS_ERROR) << "Failed to get video channel stats"; - it = video_channel_infos.erase(it); + MediaChannelStatsGatherer* gatherer = it->get(); + if (!gatherer->GetStatsOnWorkerThread()) { + RTC_LOG(LS_ERROR) << "Failed to get media channel stats for mid=" + << gatherer->mid; + it = gatherers.erase(it); continue; } ++it; @@ -966,24 +1053,9 @@ void StatsCollector::ExtractMediaInfo() { rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; bool has_remote_audio = false; - for (const auto& info : voice_channel_infos) { - StatsReport::Id transport_id = StatsReport::NewComponentId( - info.transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP); - ExtractStatsFromList(info.voice_media_info.receivers, transport_id, this, - StatsReport::kReceive); - ExtractStatsFromList(info.voice_media_info.senders, transport_id, this, - StatsReport::kSend); - if (!info.voice_media_info.receivers.empty()) { - has_remote_audio = true; - } - } - for (const auto& info : video_channel_infos) { - StatsReport::Id transport_id = StatsReport::NewComponentId( - info.transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP); - ExtractStatsFromList(info.video_media_info.receivers, transport_id, this, - StatsReport::kReceive); - ExtractStatsFromList(info.video_media_info.senders, transport_id, this, - StatsReport::kSend); + for (const auto& gatherer : gatherers) { + gatherer->ExtractStats(this); + has_remote_audio |= gatherer->HasRemoteAudio(); } UpdateStatsFromExistingLocalAudioTracks(has_remote_audio); @@ -1103,17 +1175,6 @@ void StatsCollector::UpdateReportFromAudioTrack(AudioTrackInterface* track, } } -absl::string_view StatsCollector::GetTrackIdBySsrc( - uint32_t ssrc, - StatsReport::Direction direction) { - RTC_DCHECK(pc_->signaling_thread()->IsCurrent()); - if (direction == StatsReport::kSend) { - return pc_->GetLocalTrackIdBySsrc(ssrc); - } - RTC_DCHECK_EQ(direction, StatsReport::kReceive); - return pc_->GetRemoteTrackIdBySsrc(ssrc); -} - void StatsCollector::UpdateTrackReports() { RTC_DCHECK(pc_->signaling_thread()->IsCurrent()); diff --git a/pc/stats_collector.h b/pc/stats_collector.h index d0be338d52..b3d5cb956f 100644 --- a/pc/stats_collector.h +++ b/pc/stats_collector.h @@ -79,6 +79,7 @@ class StatsCollector { // in the ExtractStatsFromList template. StatsReport* PrepareReport(bool local, uint32_t ssrc, + const std::string& track_id, const StatsReport::Id& transport_id, StatsReport::Direction direction); @@ -130,11 +131,6 @@ class StatsCollector { StatsReport* report, bool has_remote_tracks); - // Helper method to get the id for the track identified by ssrc. - // |direction| tells if the track is for sending or receiving. - absl::string_view GetTrackIdBySsrc(uint32_t ssrc, - StatsReport::Direction direction); - // Helper method to update the timestamp of track records. void UpdateTrackReports(); diff --git a/pc/stats_collector_unittest.cc b/pc/stats_collector_unittest.cc index eed30ef1d1..7ebac7f598 100644 --- a/pc/stats_collector_unittest.cc +++ b/pc/stats_collector_unittest.cc @@ -27,6 +27,8 @@ #include "pc/stats_collector.h" #include "pc/test/fake_peer_connection_for_stats.h" #include "pc/test/fake_video_track_source.h" +#include "pc/test/mock_rtp_receiver_internal.h" +#include "pc/test/mock_rtp_sender_internal.h" #include "pc/transport_stats.h" #include "pc/video_track.h" #include "rtc_base/fake_ssl_identity.h" @@ -51,6 +53,8 @@ using cricket::VideoSenderInfo; using cricket::VoiceMediaInfo; using cricket::VoiceReceiverInfo; using cricket::VoiceSenderInfo; +using ::testing::Return; +using ::testing::UnorderedElementsAre; namespace webrtc { @@ -208,6 +212,38 @@ const StatsReport* FindNthReportByType(const StatsReports& reports, return nullptr; } +// Returns the value of the stat identified by |name| in the |n|-th report of +// type |type| in |reports|. +// |n| starts from 1 for finding the first report. +// If either the |n|-th report is not found, or the stat is not present in that +// report, then nullopt is returned. +absl::optional GetValueInNthReportByType( + const StatsReports& reports, + StatsReport::StatsType type, + StatsReport::StatsValueName name, + int n) { + const StatsReport* report = FindNthReportByType(reports, type, n); + if (!report) { + return absl::nullopt; + } + std::string value; + if (!GetValue(report, name, &value)) { + return absl::nullopt; + } + return value; +} + +std::vector GetReportsByType(const StatsReports& reports, + StatsReport::StatsType type) { + std::vector filtered_reports; + for (const StatsReport* report : reports) { + if (report->type() == type) { + filtered_reports.push_back(report); + } + } + return filtered_reports; +} + const StatsReport* FindReportById(const StatsReports& reports, const StatsReport::Id& id) { for (const auto* r : reports) { @@ -484,8 +520,9 @@ void VerifyVoiceSenderInfoReport(const StatsReport* report, } // Helper methods to avoid duplication of code. -void InitVoiceSenderInfo(cricket::VoiceSenderInfo* voice_sender_info) { - voice_sender_info->add_ssrc(kSsrcOfTrack); +void InitVoiceSenderInfo(cricket::VoiceSenderInfo* voice_sender_info, + uint32_t ssrc = kSsrcOfTrack) { + voice_sender_info->add_ssrc(ssrc); voice_sender_info->codec_name = "fake_codec"; voice_sender_info->bytes_sent = 100; voice_sender_info->packets_sent = 101; @@ -692,6 +729,36 @@ class StatsCollectorTest : public testing::Test { } }; +static rtc::scoped_refptr CreateMockSender( + rtc::scoped_refptr track, + uint32_t ssrc) { + rtc::scoped_refptr sender( + new rtc::RefCountedObject()); + EXPECT_CALL(*sender, track()).WillRepeatedly(Return(track)); + EXPECT_CALL(*sender, ssrc()).WillRepeatedly(Return(ssrc)); + EXPECT_CALL(*sender, media_type()) + .WillRepeatedly( + Return(track->kind() == MediaStreamTrackInterface::kAudioKind + ? cricket::MEDIA_TYPE_AUDIO + : cricket::MEDIA_TYPE_VIDEO)); + return sender; +} + +static rtc::scoped_refptr CreateMockReceiver( + rtc::scoped_refptr track, + uint32_t ssrc) { + rtc::scoped_refptr receiver( + new rtc::RefCountedObject()); + EXPECT_CALL(*receiver, track()).WillRepeatedly(Return(track)); + EXPECT_CALL(*receiver, ssrc()).WillRepeatedly(Return(ssrc)); + EXPECT_CALL(*receiver, media_type()) + .WillRepeatedly( + Return(track->kind() == MediaStreamTrackInterface::kAudioKind + ? cricket::MEDIA_TYPE_AUDIO + : cricket::MEDIA_TYPE_VIDEO)); + return receiver; +} + class StatsCollectorTrackTest : public StatsCollectorTest, public ::testing::WithParamInterface { public: @@ -710,7 +777,7 @@ class StatsCollectorTrackTest : public StatsCollectorTest, } else { stats->AddTrack(track_); } - pc->AddLocalTrack(kSsrcOfTrack, kLocalTrackId); + pc->AddSender(CreateMockSender(track_, kSsrcOfTrack)); } // Adds a incoming video track with a given SSRC into the stats. @@ -725,15 +792,16 @@ class StatsCollectorTrackTest : public StatsCollectorTest, } else { stats->AddTrack(track_); } - pc->AddRemoteTrack(kSsrcOfTrack, kRemoteTrackId); + pc->AddReceiver(CreateMockReceiver(track_, kSsrcOfTrack)); } // Adds a outgoing audio track with a given SSRC into the stats, // and register it into the stats object. // If GetParam() returns true, the track is also inserted into the local // stream, which is created if necessary. - void AddOutgoingAudioTrack(FakePeerConnectionForStats* pc, - StatsCollectorForTest* stats) { + rtc::scoped_refptr AddOutgoingAudioTrack( + FakePeerConnectionForStats* pc, + StatsCollectorForTest* stats) { audio_track_ = new rtc::RefCountedObject(kLocalTrackId); if (GetParam()) { if (!stream_) @@ -743,7 +811,7 @@ class StatsCollectorTrackTest : public StatsCollectorTest, } else { stats->AddTrack(audio_track_); } - pc->AddLocalTrack(kSsrcOfTrack, kLocalTrackId); + return pc->AddSender(CreateMockSender(audio_track_, kSsrcOfTrack)); } // Adds a incoming audio track with a given SSRC into the stats. @@ -758,7 +826,7 @@ class StatsCollectorTrackTest : public StatsCollectorTest, } else { stats->AddTrack(audio_track_); } - pc->AddRemoteTrack(kSsrcOfTrack, kRemoteTrackId); + pc->AddReceiver(CreateMockReceiver(audio_track_, kSsrcOfTrack)); } rtc::scoped_refptr stream_; @@ -1406,7 +1474,7 @@ TEST_P(StatsCollectorTrackTest, FilterOutNegativeInitialValues) { rtc::scoped_refptr local_track( new rtc::RefCountedObject(kLocalTrackId)); stream_->AddTrack(local_track); - pc->AddLocalTrack(kSsrcOfTrack, kLocalTrackId); + pc->AddSender(CreateMockSender(local_track, kSsrcOfTrack)); if (GetParam()) { stats->AddStream(stream_); } @@ -1418,7 +1486,7 @@ TEST_P(StatsCollectorTrackTest, FilterOutNegativeInitialValues) { rtc::scoped_refptr remote_track( new rtc::RefCountedObject(kRemoteTrackId)); remote_stream->AddTrack(remote_track); - pc->AddRemoteTrack(kSsrcOfTrack, kRemoteTrackId); + pc->AddReceiver(CreateMockReceiver(remote_track, kSsrcOfTrack)); if (GetParam()) { stats->AddStream(remote_stream); } @@ -1587,7 +1655,7 @@ TEST_P(StatsCollectorTrackTest, LocalAndRemoteTracksWithSameSsrc) { MediaStream::Create("remotestreamid")); rtc::scoped_refptr remote_track( new rtc::RefCountedObject(kRemoteTrackId)); - pc->AddRemoteTrack(kSsrcOfTrack, kRemoteTrackId); + pc->AddReceiver(CreateMockReceiver(remote_track, kSsrcOfTrack)); remote_stream->AddTrack(remote_track); stats->AddStream(remote_stream); @@ -1652,7 +1720,7 @@ TEST_P(StatsCollectorTrackTest, TwoLocalTracksWithSameSsrc) { auto stats = CreateStatsCollector(pc); // Create a local stream with a local audio track and adds it to the stats. - AddOutgoingAudioTrack(pc, stats.get()); + auto sender = AddOutgoingAudioTrack(pc, stats.get()); stats->AddLocalAudioTrack(audio_track_, kSsrcOfTrack); VoiceSenderInfo voice_sender_info; @@ -1671,12 +1739,13 @@ TEST_P(StatsCollectorTrackTest, TwoLocalTracksWithSameSsrc) { // Remove the previous audio track from the stream. stream_->RemoveTrack(audio_track_.get()); stats->RemoveLocalAudioTrack(audio_track_.get(), kSsrcOfTrack); + pc->RemoveSender(sender); // Create a new audio track and adds it to the stream and stats. static const std::string kNewTrackId = "new_track_id"; rtc::scoped_refptr new_audio_track( new rtc::RefCountedObject(kNewTrackId)); - pc->AddLocalTrack(kSsrcOfTrack, kNewTrackId); + pc->AddSender(CreateMockSender(new_audio_track, kSsrcOfTrack)); stream_->AddTrack(new_audio_track); stats->AddLocalAudioTrack(new_audio_track, kSsrcOfTrack); @@ -1694,6 +1763,72 @@ TEST_P(StatsCollectorTrackTest, TwoLocalTracksWithSameSsrc) { VerifyAudioTrackStats(new_audio_track, stats.get(), new_voice_info, &reports); } +// Test that if there are two local senders with the same track then two SSRC +// reports will be created, one for each sender, with the same track ID and one +// track report will be created for the shared track. +TEST_P(StatsCollectorTrackTest, TwoLocalSendersWithSameTrack) { + constexpr uint32_t kFirstSsrc = 22; + constexpr uint32_t kSecondSsrc = 33; + + auto pc = CreatePeerConnection(); + auto stats = CreateStatsCollector(pc); + + rtc::scoped_refptr local_track( + new rtc::RefCountedObject(kLocalTrackId)); + pc->AddSender(CreateMockSender(local_track, kFirstSsrc)); + stats->AddLocalAudioTrack(local_track.get(), kFirstSsrc); + pc->AddSender(CreateMockSender(local_track, kSecondSsrc)); + stats->AddLocalAudioTrack(local_track.get(), kSecondSsrc); + + VoiceSenderInfo first_sender_info; + InitVoiceSenderInfo(&first_sender_info, kFirstSsrc); + UpdateVoiceSenderInfoFromAudioTrack(local_track.get(), &first_sender_info, + false); + + VoiceSenderInfo second_sender_info; + InitVoiceSenderInfo(&second_sender_info, kSecondSsrc); + UpdateVoiceSenderInfoFromAudioTrack(local_track.get(), &second_sender_info, + false); + + VoiceMediaInfo voice_info; + voice_info.senders.push_back(first_sender_info); + voice_info.senders.push_back(second_sender_info); + + auto* voice_media_channel = pc->AddVoiceChannel("voice", "transport"); + voice_media_channel->SetStats(voice_info); + + stats->UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard); + + StatsReports reports; + stats->GetStats(local_track.get(), &reports); + RTC_LOG(LS_INFO) << reports.size(); + + // Both SSRC reports have the same track ID. + EXPECT_EQ(kLocalTrackId, GetValueInNthReportByType( + reports, StatsReport::kStatsReportTypeSsrc, + StatsReport::kStatsValueNameTrackId, 1)); + EXPECT_EQ(kLocalTrackId, GetValueInNthReportByType( + reports, StatsReport::kStatsReportTypeSsrc, + StatsReport::kStatsValueNameTrackId, 2)); + + // The SSRC in each SSRC report is different and correspond to the sender + // SSRC. + std::vector> ssrcs = { + GetValueInNthReportByType(reports, StatsReport::kStatsReportTypeSsrc, + StatsReport::kStatsValueNameSsrc, 1), + GetValueInNthReportByType(reports, StatsReport::kStatsReportTypeSsrc, + StatsReport::kStatsValueNameSsrc, 2)}; + EXPECT_THAT(ssrcs, UnorderedElementsAre(rtc::ToString(kFirstSsrc), + rtc::ToString(kSecondSsrc))); + + // There is one track report with the same track ID as the SSRC reports. + EXPECT_EQ( + 1u, GetReportsByType(reports, StatsReport::kStatsReportTypeTrack).size()); + EXPECT_EQ(kLocalTrackId, GetValueInNthReportByType( + reports, StatsReport::kStatsReportTypeTrack, + StatsReport::kStatsValueNameTrackId, 1)); +} + // This test verifies that stats are correctly set in video send ssrc stats. TEST_P(StatsCollectorTrackTest, VerifyVideoSendSsrcStats) { auto pc = CreatePeerConnection(); diff --git a/pc/test/fake_peer_connection_base.h b/pc/test/fake_peer_connection_base.h index d2fa1fd7d8..f67b305e89 100644 --- a/pc/test/fake_peer_connection_base.h +++ b/pc/test/fake_peer_connection_base.h @@ -239,11 +239,6 @@ class FakePeerConnectionBase : public PeerConnectionInternal { return {}; } - absl::string_view GetLocalTrackIdBySsrc(uint32_t ssrc) override { return {}; } - absl::string_view GetRemoteTrackIdBySsrc(uint32_t ssrc) override { - return {}; - } - sigslot::signal1& SignalDataChannelCreated() override { return SignalDataChannelCreated_; } diff --git a/pc/test/fake_peer_connection_for_stats.h b/pc/test/fake_peer_connection_for_stats.h index b954fc58dc..826439196b 100644 --- a/pc/test/fake_peer_connection_for_stats.h +++ b/pc/test/fake_peer_connection_for_stats.h @@ -98,16 +98,25 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase { return remote_streams_; } - void AddSender(rtc::scoped_refptr sender) { + rtc::scoped_refptr AddSender( + rtc::scoped_refptr sender) { // TODO(steveanton): Switch tests to use RtpTransceivers directly. auto sender_proxy = RtpSenderProxyWithInternal::Create( signaling_thread_, sender); GetOrCreateFirstTransceiverOfType(sender->media_type()) ->internal() ->AddSender(sender_proxy); + return sender_proxy; } - void AddReceiver(rtc::scoped_refptr receiver) { + void RemoveSender(rtc::scoped_refptr sender) { + GetOrCreateFirstTransceiverOfType(sender->media_type()) + ->internal() + ->RemoveSender(sender); + } + + rtc::scoped_refptr AddReceiver( + rtc::scoped_refptr receiver) { // TODO(steveanton): Switch tests to use RtpTransceivers directly. auto receiver_proxy = RtpReceiverProxyWithInternal::Create( @@ -115,6 +124,13 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase { GetOrCreateFirstTransceiverOfType(receiver->media_type()) ->internal() ->AddReceiver(receiver_proxy); + return receiver_proxy; + } + + void RemoveReceiver(rtc::scoped_refptr receiver) { + GetOrCreateFirstTransceiverOfType(receiver->media_type()) + ->internal() + ->RemoveReceiver(receiver); } FakeVoiceMediaChannelForStats* AddVoiceChannel( @@ -153,14 +169,6 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase { return video_media_channel_ptr; } - void AddLocalTrack(uint32_t ssrc, const std::string& track_id) { - local_track_id_by_ssrc_[ssrc] = track_id; - } - - void AddRemoteTrack(uint32_t ssrc, const std::string& track_id) { - remote_track_id_by_ssrc_[ssrc] = track_id; - } - void AddSctpDataChannel(const std::string& label) { AddSctpDataChannel(label, InternalDataChannelInit()); } @@ -250,22 +258,6 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase { return transceivers_; } - absl::string_view GetLocalTrackIdBySsrc(uint32_t ssrc) override { - auto it = local_track_id_by_ssrc_.find(ssrc); - if (it != local_track_id_by_ssrc_.end()) { - return it->second; - } - return {}; - } - - absl::string_view GetRemoteTrackIdBySsrc(uint32_t ssrc) override { - auto it = remote_track_id_by_ssrc_.find(ssrc); - if (it != remote_track_id_by_ssrc_.end()) { - return it->second; - } - return {}; - } - std::vector> sctp_data_channels() const override { return sctp_data_channels_; @@ -367,8 +359,6 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase { std::unique_ptr voice_channel_; std::unique_ptr video_channel_; - std::map local_track_id_by_ssrc_; - std::map remote_track_id_by_ssrc_; std::vector> sctp_data_channels_;