diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 9c04c31834..ad8bd1a26d 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -518,33 +518,6 @@ bool VerifyIceUfragPwdPresent(const SessionDescription* desc) { return true; } -bool GetTrackIdBySsrc(const SessionDescription* session_description, - uint32_t ssrc, - std::string* track_id) { - RTC_DCHECK(track_id != NULL); - - const cricket::AudioContentDescription* audio_desc = - cricket::GetFirstAudioContentDescription(session_description); - if (audio_desc) { - const auto* found = cricket::GetStreamBySsrc(audio_desc->streams(), ssrc); - if (found) { - *track_id = found->id; - return true; - } - } - - const cricket::VideoContentDescription* video_desc = - cricket::GetFirstVideoContentDescription(session_description); - if (video_desc) { - const auto* found = cricket::GetStreamBySsrc(video_desc->streams(), ssrc); - if (found) { - *track_id = found->id; - return true; - } - } - return false; -} - // Get the SCTP port out of a SessionDescription. // Return -1 if not found. int GetSctpPort(const SessionDescription* session_description) { @@ -5246,22 +5219,34 @@ cricket::IceConfig PeerConnection::ParseIceConfig( return ice_config; } -bool PeerConnection::GetLocalTrackIdBySsrc(uint32_t ssrc, - std::string* track_id) { - if (!local_description()) { - return false; +static absl::string_view GetTrackIdBySsrc( + const SessionDescriptionInterface* session_description, + uint32_t ssrc) { + if (!session_description) { + return {}; } - return webrtc::GetTrackIdBySsrc(local_description()->description(), ssrc, - track_id); + 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 {}; } -bool PeerConnection::GetRemoteTrackIdBySsrc(uint32_t ssrc, - std::string* track_id) { - if (!remote_description()) { - return false; - } - return webrtc::GetTrackIdBySsrc(remote_description()->description(), ssrc, - track_id); +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, diff --git a/pc/peerconnection.h b/pc/peerconnection.h index 3bdd37e171..f63f4a2a03 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -228,8 +228,8 @@ class PeerConnection : public PeerConnectionInternal, return transceivers_; } - bool GetLocalTrackIdBySsrc(uint32_t ssrc, std::string* track_id) override; - bool GetRemoteTrackIdBySsrc(uint32_t ssrc, std::string* track_id) override; + 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/peerconnection_integrationtest.cc b/pc/peerconnection_integrationtest.cc index a7f7aad0f5..295006f203 100644 --- a/pc/peerconnection_integrationtest.cc +++ b/pc/peerconnection_integrationtest.cc @@ -78,6 +78,7 @@ using cricket::StreamParams; using rtc::SocketAddress; using ::testing::Combine; using ::testing::ElementsAre; +using ::testing::UnorderedElementsAreArray; using ::testing::Return; using ::testing::SetArgPointee; using ::testing::Values; @@ -2616,6 +2617,34 @@ TEST_P(PeerConnectionIntegrationTest, GetCaptureStartNtpTimeWithOldStatsApi) { 2 * kMaxWaitForFramesMs); } +// Test that the track ID is associated with all local and remote SSRC stats +// using the old GetStats() and more than 1 audio and more than 1 video track. +// This is a regression test for crbug.com/906988 +TEST_F(PeerConnectionIntegrationTestUnifiedPlan, + OldGetStatsAssociatesTrackIdForManyMediaSections) { + ASSERT_TRUE(CreatePeerConnectionWrappers()); + ConnectFakeSignaling(); + auto audio_sender_1 = caller()->AddAudioTrack(); + auto video_sender_1 = caller()->AddVideoTrack(); + auto audio_sender_2 = caller()->AddAudioTrack(); + auto video_sender_2 = caller()->AddVideoTrack(); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + + MediaExpectations media_expectations; + media_expectations.CalleeExpectsSomeAudioAndVideo(); + ASSERT_TRUE_WAIT(ExpectNewFrames(media_expectations), kDefaultTimeout); + + std::vector track_ids = { + audio_sender_1->track()->id(), video_sender_1->track()->id(), + audio_sender_2->track()->id(), video_sender_2->track()->id()}; + + auto caller_stats = caller()->OldGetStats(); + EXPECT_THAT(caller_stats->TrackIds(), UnorderedElementsAreArray(track_ids)); + auto callee_stats = callee()->OldGetStats(); + EXPECT_THAT(callee_stats->TrackIds(), UnorderedElementsAreArray(track_ids)); +} + // Test that we can get stats (using the new stats implemnetation) for // unsignaled streams. Meaning when SSRCs/MSIDs aren't signaled explicitly in // SDP. diff --git a/pc/peerconnectioninternal.h b/pc/peerconnectioninternal.h index 8a169547cc..cb3627bfbc 100644 --- a/pc/peerconnectioninternal.h +++ b/pc/peerconnectioninternal.h @@ -42,8 +42,8 @@ class PeerConnectionInternal : public PeerConnectionInterface { GetTransceiversInternal() const = 0; // Get the id used as a media stream track's "id" field from ssrc. - virtual bool GetLocalTrackIdBySsrc(uint32_t ssrc, std::string* track_id) = 0; - virtual bool GetRemoteTrackIdBySsrc(uint32_t ssrc, std::string* track_id) = 0; + virtual absl::string_view GetLocalTrackIdBySsrc(uint32_t ssrc) = 0; + virtual absl::string_view GetRemoteTrackIdBySsrc(uint32_t ssrc) = 0; virtual sigslot::signal1& SignalDataChannelCreated() = 0; diff --git a/pc/statscollector.cc b/pc/statscollector.cc index a1d52db04e..3cd85e69b2 100644 --- a/pc/statscollector.cc +++ b/pc/statscollector.cc @@ -586,28 +586,31 @@ StatsReport* StatsCollector::PrepareReport(bool local, StatsReport* report = reports_.Find(id); // Use the ID of the track that is currently mapped to the SSRC, if any. - std::string track_id; - if (!GetTrackIdBySsrc(ssrc, &track_id, direction)) { + 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) + if (v) { track_id = v->string_val(); + } } } - if (!report) + if (!report) { report = reports_.InsertNew(id); + } // FYI - for remote reports, the timestamp will be overwritten later. report->set_timestamp(stats_gathering_started_); report->AddInt64(StatsReport::kStatsValueNameSsrc, ssrc); if (!track_id.empty()) { - report->AddString(StatsReport::kStatsValueNameTrackId, track_id); + report->AddString(StatsReport::kStatsValueNameTrackId, + std::string(track_id)); } // Add the mapping of SSRC to transport. report->AddId(StatsReport::kStatsValueNameTransportId, transport_id); @@ -1095,26 +1098,24 @@ void StatsCollector::UpdateReportFromAudioTrack(AudioTrackInterface* track, } } -bool StatsCollector::GetTrackIdBySsrc(uint32_t ssrc, - std::string* track_id, - StatsReport::Direction direction) { +absl::string_view StatsCollector::GetTrackIdBySsrc( + uint32_t ssrc, + StatsReport::Direction direction) { RTC_DCHECK(pc_->signaling_thread()->IsCurrent()); + absl::string_view track_id; if (direction == StatsReport::kSend) { - if (!pc_->GetLocalTrackIdBySsrc(ssrc, track_id)) { - RTC_LOG(LS_WARNING) << "The SSRC " << ssrc - << " is not associated with a sending track"; - return false; - } + track_id = pc_->GetLocalTrackIdBySsrc(ssrc); } else { - RTC_DCHECK(direction == StatsReport::kReceive); - if (!pc_->GetRemoteTrackIdBySsrc(ssrc, track_id)) { - RTC_LOG(LS_WARNING) << "The SSRC " << ssrc - << " is not associated with a receiving track"; - return false; - } + RTC_DCHECK_EQ(direction, StatsReport::kReceive); + track_id = pc_->GetRemoteTrackIdBySsrc(ssrc); } - - return true; + if (track_id.empty()) { + RTC_LOG(LS_WARNING) << "The SSRC " << ssrc << " is not associated with a " + << (direction == StatsReport::kSend ? "sending" + : "receiving") + << " track"; + } + return track_id; } void StatsCollector::UpdateTrackReports() { diff --git a/pc/statscollector.h b/pc/statscollector.h index eb810e5dac..786d5df0f2 100644 --- a/pc/statscollector.h +++ b/pc/statscollector.h @@ -132,9 +132,8 @@ class StatsCollector { // Helper method to get the id for the track identified by ssrc. // |direction| tells if the track is for sending or receiving. - bool GetTrackIdBySsrc(uint32_t ssrc, - std::string* track_id, - StatsReport::Direction direction); + 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/test/fakepeerconnectionbase.h b/pc/test/fakepeerconnectionbase.h index 93380c0953..c190628beb 100644 --- a/pc/test/fakepeerconnectionbase.h +++ b/pc/test/fakepeerconnectionbase.h @@ -229,11 +229,9 @@ class FakePeerConnectionBase : public PeerConnectionInternal { return {}; } - bool GetLocalTrackIdBySsrc(uint32_t ssrc, std::string* track_id) override { - return false; - } - bool GetRemoteTrackIdBySsrc(uint32_t ssrc, std::string* track_id) override { - return false; + absl::string_view GetLocalTrackIdBySsrc(uint32_t ssrc) override { return {}; } + absl::string_view GetRemoteTrackIdBySsrc(uint32_t ssrc) override { + return {}; } sigslot::signal1& SignalDataChannelCreated() override { diff --git a/pc/test/fakepeerconnectionforstats.h b/pc/test/fakepeerconnectionforstats.h index af86639eca..a9f9d17d16 100644 --- a/pc/test/fakepeerconnectionforstats.h +++ b/pc/test/fakepeerconnectionforstats.h @@ -249,24 +249,20 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase { return transceivers_; } - bool GetLocalTrackIdBySsrc(uint32_t ssrc, std::string* track_id) override { + 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()) { - *track_id = it->second; - return true; - } else { - return false; + return it->second; } + return {}; } - bool GetRemoteTrackIdBySsrc(uint32_t ssrc, std::string* track_id) override { + 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()) { - *track_id = it->second; - return true; - } else { - return false; + return it->second; } + return {}; } std::vector> sctp_data_channels() diff --git a/pc/test/mockpeerconnectionobservers.h b/pc/test/mockpeerconnectionobservers.h index 62c0c9415f..0657853c29 100644 --- a/pc/test/mockpeerconnectionobservers.h +++ b/pc/test/mockpeerconnectionobservers.h @@ -366,6 +366,9 @@ class MockStatsObserver : public webrtc::StatsObserver { &stats_.bytes_sent); GetInt64Value(r, StatsReport::kStatsValueNameCaptureStartNtpTimeMs, &stats_.capture_start_ntp_time); + stats_.track_ids.emplace_back(); + GetStringValue(r, StatsReport::kStatsValueNameTrackId, + &stats_.track_ids.back()); } else if (r->type() == StatsReport::kStatsReportTypeBwe) { stats_.timestamp = r->timestamp(); GetIntValue(r, StatsReport::kStatsValueNameAvailableReceiveBandwidth, @@ -424,6 +427,11 @@ class MockStatsObserver : public webrtc::StatsObserver { return stats_.srtp_cipher; } + std::vector TrackIds() const { + RTC_CHECK(called_); + return stats_.track_ids; + } + private: bool GetIntValue(const StatsReport* report, StatsReport::StatsValueName name, @@ -469,6 +477,7 @@ class MockStatsObserver : public webrtc::StatsObserver { available_receive_bandwidth = 0; dtls_cipher.clear(); srtp_cipher.clear(); + track_ids.clear(); } size_t number_of_reports; @@ -481,6 +490,7 @@ class MockStatsObserver : public webrtc::StatsObserver { int available_receive_bandwidth; std::string dtls_cipher; std::string srtp_cipher; + std::vector track_ids; } stats_; };