From a41959e550925f09e6b2cc29c044f088d81e6932 Mon Sep 17 00:00:00 2001 From: Steve Anton Date: Wed, 28 Nov 2018 11:15:33 -0800 Subject: [PATCH] [Unified Plan] Fix old GetStats() not associating track id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The method for looking up track ID by SSRC was never updated for Unified Plan so it only looked at the first audio section and the first video section. This CL changes the method to look through all audio and video media sections rather than just the first. Bug: chromium:906988 Change-Id: Ie79e6162b2bd24b8ac9e983b5fa7360c96f030da Reviewed-on: https://webrtc-review.googlesource.com/c/112223 Commit-Queue: Steve Anton Reviewed-by: Henrik Boström Reviewed-by: Seth Hampson Cr-Commit-Position: refs/heads/master@{#25833} --- pc/peerconnection.cc | 65 +++++++++++---------------- pc/peerconnection.h | 4 +- pc/peerconnection_integrationtest.cc | 29 ++++++++++++ pc/peerconnectioninternal.h | 4 +- pc/statscollector.cc | 43 +++++++++--------- pc/statscollector.h | 5 +-- pc/test/fakepeerconnectionbase.h | 8 ++-- pc/test/fakepeerconnectionforstats.h | 16 +++---- pc/test/mockpeerconnectionobservers.h | 10 +++++ 9 files changed, 101 insertions(+), 83 deletions(-) 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_; };