diff --git a/webrtc/api/rtcstatscollector.cc b/webrtc/api/rtcstatscollector.cc index a6d691c333..3628544c15 100644 --- a/webrtc/api/rtcstatscollector.cc +++ b/webrtc/api/rtcstatscollector.cc @@ -416,9 +416,32 @@ void RTCStatsCollector::GetStatsReport( invoker_.AsyncInvoke(RTC_FROM_HERE, signaling_thread_, rtc::Bind(&RTCStatsCollector::ProducePartialResultsOnSignalingThread, rtc::scoped_refptr(this), timestamp_us)); + + // TODO(hbos): No stats are gathered by + // |ProducePartialResultsOnWorkerThread|, remove it. invoker_.AsyncInvoke(RTC_FROM_HERE, worker_thread_, rtc::Bind(&RTCStatsCollector::ProducePartialResultsOnWorkerThread, rtc::scoped_refptr(this), timestamp_us)); + + // Prepare |channel_names_| and |media_info_| for use in + // |ProducePartialResultsOnNetworkThread|. + channel_name_pairs_.reset(new ChannelNamePairs()); + if (pc_->session()->voice_channel()) { + channel_name_pairs_->voice = rtc::Optional( + ChannelNamePair(pc_->session()->voice_channel()->content_name(), + pc_->session()->voice_channel()->transport_name())); + } + if (pc_->session()->video_channel()) { + channel_name_pairs_->video = rtc::Optional( + ChannelNamePair(pc_->session()->video_channel()->content_name(), + pc_->session()->video_channel()->transport_name())); + } + if (pc_->session()->data_channel()) { + channel_name_pairs_->data = rtc::Optional( + ChannelNamePair(pc_->session()->data_channel()->content_name(), + pc_->session()->data_channel()->transport_name())); + } + media_info_.reset(PrepareMediaInfo_s().release()); invoker_.AsyncInvoke(RTC_FROM_HERE, network_thread_, rtc::Bind(&RTCStatsCollector::ProducePartialResultsOnNetworkThread, rtc::scoped_refptr(this), timestamp_us)); @@ -436,23 +459,6 @@ void RTCStatsCollector::ProducePartialResultsOnSignalingThread( rtc::scoped_refptr report = RTCStatsReport::Create( timestamp_us); - SessionStats session_stats; - if (pc_->session()->GetTransportStats(&session_stats)) { - std::map transport_cert_stats = - PrepareTransportCertificateStats(session_stats); - MediaInfo media_info = PrepareMediaInfo(session_stats); - - ProduceCertificateStats_s( - timestamp_us, transport_cert_stats, report.get()); - ProduceCodecStats_s( - timestamp_us, media_info, report.get()); - ProduceIceCandidateAndPairStats_s( - timestamp_us, session_stats, report.get()); - ProduceRTPStreamStats_s( - timestamp_us, session_stats, media_info, report.get()); - ProduceTransportStats_s( - timestamp_us, session_stats, transport_cert_stats, report.get()); - } ProduceDataChannelStats_s(timestamp_us, report.get()); ProduceMediaStreamAndTrackStats_s(timestamp_us, report.get()); ProducePeerConnectionStats_s(timestamp_us, report.get()); @@ -466,13 +472,8 @@ void RTCStatsCollector::ProducePartialResultsOnWorkerThread( rtc::scoped_refptr report = RTCStatsReport::Create( timestamp_us); - // TODO(hbos): Gather stats on worker thread. - // pc_->session()'s channels are owned by the signaling thread but there are - // some stats that are gathered on the worker thread. Instead of a synchronous - // invoke on "s->w" we could to the "w" work here asynchronously if it wasn't - // for the ownership issue. Synchronous invokes in other places makes it - // difficult to introduce locks without introducing deadlocks and the channels - // are not reference counted. + // TODO(hbos): There are no stats to be gathered on this thread, remove this + // method. AddPartialResults(report); } @@ -483,13 +484,23 @@ void RTCStatsCollector::ProducePartialResultsOnNetworkThread( rtc::scoped_refptr report = RTCStatsReport::Create( timestamp_us); - // TODO(hbos): Gather stats on network thread. - // pc_->session()'s channels are owned by the signaling thread but there are - // some stats that are gathered on the network thread. Instead of a - // synchronous invoke on "s->n" we could to the "n" work here asynchronously - // if it wasn't for the ownership issue. Synchronous invokes in other places - // makes it difficult to introduce locks without introducing deadlocks and the - // channels are not reference counted. + std::unique_ptr session_stats = + pc_->session()->GetStats(*channel_name_pairs_); + if (session_stats) { + std::map transport_cert_stats = + PrepareTransportCertificateStats_n(*session_stats); + + ProduceCertificateStats_n( + timestamp_us, transport_cert_stats, report.get()); + ProduceCodecStats_n( + timestamp_us, *media_info_, report.get()); + ProduceIceCandidateAndPairStats_n( + timestamp_us, *session_stats, report.get()); + ProduceRTPStreamStats_n( + timestamp_us, *session_stats, *media_info_, report.get()); + ProduceTransportStats_n( + timestamp_us, *session_stats, transport_cert_stats, report.get()); + } AddPartialResults(report); } @@ -534,11 +545,11 @@ void RTCStatsCollector::DeliverCachedReport() { callbacks_.clear(); } -void RTCStatsCollector::ProduceCertificateStats_s( +void RTCStatsCollector::ProduceCertificateStats_n( int64_t timestamp_us, const std::map& transport_cert_stats, RTCStatsReport* report) const { - RTC_DCHECK(signaling_thread_->IsCurrent()); + RTC_DCHECK(network_thread_->IsCurrent()); for (const auto& transport_cert_stats_pair : transport_cert_stats) { if (transport_cert_stats_pair.second.local) { ProduceCertificateStatsFromSSLCertificateStats( @@ -551,10 +562,10 @@ void RTCStatsCollector::ProduceCertificateStats_s( } } -void RTCStatsCollector::ProduceCodecStats_s( +void RTCStatsCollector::ProduceCodecStats_n( int64_t timestamp_us, const MediaInfo& media_info, RTCStatsReport* report) const { - RTC_DCHECK(signaling_thread_->IsCurrent()); + RTC_DCHECK(network_thread_->IsCurrent()); // Audio if (media_info.voice) { // Inbound @@ -605,10 +616,10 @@ void RTCStatsCollector::ProduceDataChannelStats_s( } } -void RTCStatsCollector::ProduceIceCandidateAndPairStats_s( +void RTCStatsCollector::ProduceIceCandidateAndPairStats_n( int64_t timestamp_us, const SessionStats& session_stats, RTCStatsReport* report) const { - RTC_DCHECK(signaling_thread_->IsCurrent()); + RTC_DCHECK(network_thread_->IsCurrent()); for (const auto& transport_stats : session_stats.transport_stats) { for (const auto& channel_stats : transport_stats.second.channel_stats) { std::string transport_id = RTCTransportStatsIDFromTransportChannel( @@ -681,10 +692,10 @@ void RTCStatsCollector::ProducePeerConnectionStats_s( report->AddStats(std::move(stats)); } -void RTCStatsCollector::ProduceRTPStreamStats_s( +void RTCStatsCollector::ProduceRTPStreamStats_n( int64_t timestamp_us, const SessionStats& session_stats, const MediaInfo& media_info, RTCStatsReport* report) const { - RTC_DCHECK(signaling_thread_->IsCurrent()); + RTC_DCHECK(network_thread_->IsCurrent()); // Audio if (media_info.voice) { @@ -788,11 +799,11 @@ void RTCStatsCollector::ProduceRTPStreamStats_s( } } -void RTCStatsCollector::ProduceTransportStats_s( +void RTCStatsCollector::ProduceTransportStats_n( int64_t timestamp_us, const SessionStats& session_stats, const std::map& transport_cert_stats, RTCStatsReport* report) const { - RTC_DCHECK(signaling_thread_->IsCurrent()); + RTC_DCHECK(network_thread_->IsCurrent()); for (const auto& transport : session_stats.transport_stats) { // Get reference to RTCP channel, if it exists. std::string rtcp_transport_stats_id; @@ -855,9 +866,9 @@ void RTCStatsCollector::ProduceTransportStats_s( } std::map -RTCStatsCollector::PrepareTransportCertificateStats( +RTCStatsCollector::PrepareTransportCertificateStats_n( const SessionStats& session_stats) const { - RTC_DCHECK(signaling_thread_->IsCurrent()); + RTC_DCHECK(network_thread_->IsCurrent()); std::map transport_cert_stats; for (const auto& transport_stats : session_stats.transport_stats) { CertificateStatsPair certificate_stats_pair; @@ -880,20 +891,21 @@ RTCStatsCollector::PrepareTransportCertificateStats( return transport_cert_stats; } -RTCStatsCollector::MediaInfo RTCStatsCollector::PrepareMediaInfo( - const SessionStats& session_stats) const { - MediaInfo media_info; +std::unique_ptr +RTCStatsCollector::PrepareMediaInfo_s() const { + RTC_DCHECK(signaling_thread_->IsCurrent()); + std::unique_ptr media_info(new MediaInfo()); if (pc_->session()->voice_channel()) { cricket::VoiceMediaInfo voice_media_info; if (pc_->session()->voice_channel()->GetStats(&voice_media_info)) { - media_info.voice = rtc::Optional( + media_info->voice = rtc::Optional( std::move(voice_media_info)); } } if (pc_->session()->video_channel()) { cricket::VideoMediaInfo video_media_info; if (pc_->session()->video_channel()->GetStats(&video_media_info)) { - media_info.video = rtc::Optional( + media_info->video = rtc::Optional( std::move(video_media_info)); } } diff --git a/webrtc/api/rtcstatscollector.h b/webrtc/api/rtcstatscollector.h index c32d65f246..7b23d24726 100644 --- a/webrtc/api/rtcstatscollector.h +++ b/webrtc/api/rtcstatscollector.h @@ -41,6 +41,7 @@ namespace webrtc { class PeerConnection; struct SessionStats; +struct ChannelNamePairs; class RTCStatsCollectorCallback : public virtual rtc::RefCountInterface { public: @@ -97,19 +98,19 @@ class RTCStatsCollector : public virtual rtc::RefCountInterface, void DeliverCachedReport(); // Produces |RTCCertificateStats|. - void ProduceCertificateStats_s( + void ProduceCertificateStats_n( int64_t timestamp_us, const std::map& transport_cert_stats, RTCStatsReport* report) const; // Produces |RTCCodecStats|. - void ProduceCodecStats_s( + void ProduceCodecStats_n( int64_t timestamp_us, const MediaInfo& media_info, RTCStatsReport* report) const; // Produces |RTCDataChannelStats|. void ProduceDataChannelStats_s( int64_t timestamp_us, RTCStatsReport* report) const; // Produces |RTCIceCandidatePairStats| and |RTCIceCandidateStats|. - void ProduceIceCandidateAndPairStats_s( + void ProduceIceCandidateAndPairStats_n( int64_t timestamp_us, const SessionStats& session_stats, RTCStatsReport* report) const; // Produces |RTCMediaStreamStats| and |RTCMediaStreamTrackStats|. @@ -119,19 +120,19 @@ class RTCStatsCollector : public virtual rtc::RefCountInterface, void ProducePeerConnectionStats_s( int64_t timestamp_us, RTCStatsReport* report) const; // Produces |RTCInboundRTPStreamStats| and |RTCOutboundRTPStreamStats|. - void ProduceRTPStreamStats_s( + void ProduceRTPStreamStats_n( int64_t timestamp_us, const SessionStats& session_stats, const MediaInfo& media_info, RTCStatsReport* report) const; // Produces |RTCTransportStats|. - void ProduceTransportStats_s( + void ProduceTransportStats_n( int64_t timestamp_us, const SessionStats& session_stats, const std::map& transport_cert_stats, RTCStatsReport* report) const; // Helper function to stats-producing functions. std::map - PrepareTransportCertificateStats(const SessionStats& session_stats) const; - MediaInfo PrepareMediaInfo(const SessionStats& session_stats) const; + PrepareTransportCertificateStats_n(const SessionStats& session_stats) const; + std::unique_ptr PrepareMediaInfo_s() const; // Slots for signals (sigslot) that are wired up to |pc_|. void OnDataChannelCreated(DataChannel* channel); @@ -150,6 +151,13 @@ class RTCStatsCollector : public virtual rtc::RefCountInterface, rtc::scoped_refptr partial_report_; std::vector> callbacks_; + // Set in |GetStatsReport|, used in |ProducePartialResultsOnNetworkThread| + // (not passed as arguments to avoid copies). This is thread safe - it is set + // at the start of |GetStatsReport| after making sure there are no pending + // stats requests in progress. + std::unique_ptr channel_name_pairs_; + std::unique_ptr media_info_; + // A timestamp, in microseconds, that is based on a timer that is // monotonically increasing. That is, even if the system clock is modified the // difference between the timer and this timestamp is how fresh the cached diff --git a/webrtc/api/rtcstatscollector_unittest.cc b/webrtc/api/rtcstatscollector_unittest.cc index 697cc30079..4be7491724 100644 --- a/webrtc/api/rtcstatscollector_unittest.cc +++ b/webrtc/api/rtcstatscollector_unittest.cc @@ -313,7 +313,7 @@ class RTCStatsCollectorTestHelper : public SetSessionDescriptionObserver { ReturnRef(data_channels_)); EXPECT_CALL(session_, video_channel()).WillRepeatedly(ReturnNull()); EXPECT_CALL(session_, voice_channel()).WillRepeatedly(ReturnNull()); - EXPECT_CALL(session_, GetTransportStats(_)).WillRepeatedly(Return(false)); + EXPECT_CALL(session_, GetStats(_)).WillRepeatedly(ReturnNull()); EXPECT_CALL(session_, GetLocalCertificate(_, _)).WillRepeatedly( Return(false)); EXPECT_CALL(session_, GetRemoteSSLCertificate_ReturnsRawPointer(_)) @@ -628,10 +628,11 @@ TEST_F(RTCStatsCollectorTest, CollectRTCCertificateStatsSingle) { std::vector({ "(remote) single certificate" })); // Mock the session to return the local and remote certificates. - EXPECT_CALL(test_->session(), GetTransportStats(_)).WillRepeatedly(Invoke( - [this](SessionStats* stats) { + EXPECT_CALL(test_->session(), GetStats(_)).WillRepeatedly(Invoke( + [this](const ChannelNamePairs&) { + std::unique_ptr stats(new SessionStats()); stats->transport_stats["transport"].transport_name = "transport"; - return true; + return stats; })); EXPECT_CALL(test_->session(), GetLocalCertificate(_, _)).WillRepeatedly( Invoke([this, &local_certinfo](const std::string& transport_name, @@ -713,8 +714,10 @@ TEST_F(RTCStatsCollectorTest, CollectRTCCodecStats) { session_stats.transport_stats["TransportName"].transport_name = "TransportName"; - EXPECT_CALL(test_->session(), GetTransportStats(_)) - .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats), Return(true))); + EXPECT_CALL(test_->session(), GetStats(_)).WillRepeatedly(Invoke( + [&session_stats](const ChannelNamePairs&) { + return std::unique_ptr(new SessionStats(session_stats)); + })); EXPECT_CALL(test_->session(), voice_channel()) .WillRepeatedly(Return(&voice_channel)); EXPECT_CALL(test_->session(), video_channel()) @@ -791,11 +794,12 @@ TEST_F(RTCStatsCollectorTest, CollectRTCCertificateStatsMultiple) { video_remote_certinfo->ders); // Mock the session to return the local and remote certificates. - EXPECT_CALL(test_->session(), GetTransportStats(_)).WillRepeatedly(Invoke( - [this](SessionStats* stats) { + EXPECT_CALL(test_->session(), GetStats(_)).WillRepeatedly(Invoke( + [this](const ChannelNamePairs&) { + std::unique_ptr stats(new SessionStats()); stats->transport_stats["audio"].transport_name = "audio"; stats->transport_stats["video"].transport_name = "video"; - return true; + return stats; })); EXPECT_CALL(test_->session(), GetLocalCertificate(_, _)).WillRepeatedly( Invoke([this, &audio_local_certinfo, &video_local_certinfo]( @@ -850,10 +854,11 @@ TEST_F(RTCStatsCollectorTest, CollectRTCCertificateStatsChain) { CreateFakeCertificateAndInfoFromDers(remote_ders); // Mock the session to return the local and remote certificates. - EXPECT_CALL(test_->session(), GetTransportStats(_)).WillRepeatedly(Invoke( - [this](SessionStats* stats) { + EXPECT_CALL(test_->session(), GetStats(_)).WillRepeatedly(Invoke( + [this](const ChannelNamePairs&) { + std::unique_ptr stats(new SessionStats()); stats->transport_stats["transport"].transport_name = "transport"; - return true; + return stats; })); EXPECT_CALL(test_->session(), GetLocalCertificate(_, _)).WillRepeatedly( Invoke([this, &local_certinfo](const std::string& transport_name, @@ -978,10 +983,9 @@ TEST_F(RTCStatsCollectorTest, CollectRTCIceCandidateStats) { b_transport_channel_stats); // Mock the session to return the desired candidates. - EXPECT_CALL(test_->session(), GetTransportStats(_)).WillRepeatedly(Invoke( - [this, &session_stats](SessionStats* stats) { - *stats = session_stats; - return true; + EXPECT_CALL(test_->session(), GetStats(_)).WillRepeatedly(Invoke( + [&session_stats](const ChannelNamePairs&) { + return std::unique_ptr(new SessionStats(session_stats)); })); rtc::scoped_refptr report = GetStatsReport(); @@ -1022,10 +1026,9 @@ TEST_F(RTCStatsCollectorTest, CollectRTCIceCandidatePairStats) { transport_channel_stats); // Mock the session to return the desired candidates. - EXPECT_CALL(test_->session(), GetTransportStats(_)).WillRepeatedly(Invoke( - [this, &session_stats](SessionStats* stats) { - *stats = session_stats; - return true; + EXPECT_CALL(test_->session(), GetStats(_)).WillRepeatedly(Invoke( + [&session_stats](const ChannelNamePairs&) { + return std::unique_ptr(new SessionStats(session_stats)); })); rtc::scoped_refptr report = GetStatsReport(); @@ -1387,8 +1390,10 @@ TEST_F(RTCStatsCollectorTest, CollectRTCInboundRTPStreamStats_Audio) { session_stats.transport_stats["TransportName"].channel_stats.push_back( channel_stats); - EXPECT_CALL(test_->session(), GetTransportStats(_)) - .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats), Return(true))); + EXPECT_CALL(test_->session(), GetStats(_)).WillRepeatedly(Invoke( + [&session_stats](const ChannelNamePairs&) { + return std::unique_ptr(new SessionStats(session_stats)); + })); EXPECT_CALL(test_->session(), voice_channel()) .WillRepeatedly(Return(&voice_channel)); @@ -1459,8 +1464,10 @@ TEST_F(RTCStatsCollectorTest, CollectRTCInboundRTPStreamStats_Video) { session_stats.transport_stats["TransportName"].channel_stats.push_back( channel_stats); - EXPECT_CALL(test_->session(), GetTransportStats(_)) - .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats), Return(true))); + EXPECT_CALL(test_->session(), GetStats(_)).WillRepeatedly(Invoke( + [&session_stats](const ChannelNamePairs&) { + return std::unique_ptr(new SessionStats(session_stats)); + })); EXPECT_CALL(test_->session(), video_channel()) .WillRepeatedly(Return(&video_channel)); @@ -1529,8 +1536,10 @@ TEST_F(RTCStatsCollectorTest, CollectRTCOutboundRTPStreamStats_Audio) { session_stats.transport_stats["TransportName"].channel_stats.push_back( channel_stats); - EXPECT_CALL(test_->session(), GetTransportStats(_)) - .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats), Return(true))); + EXPECT_CALL(test_->session(), GetStats(_)).WillRepeatedly(Invoke( + [&session_stats](const ChannelNamePairs&) { + return std::unique_ptr(new SessionStats(session_stats)); + })); EXPECT_CALL(test_->session(), voice_channel()) .WillRepeatedly(Return(&voice_channel)); @@ -1597,8 +1606,10 @@ TEST_F(RTCStatsCollectorTest, CollectRTCOutboundRTPStreamStats_Video) { session_stats.transport_stats["TransportName"].channel_stats.push_back( channel_stats); - EXPECT_CALL(test_->session(), GetTransportStats(_)) - .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats), Return(true))); + EXPECT_CALL(test_->session(), GetStats(_)).WillRepeatedly(Invoke( + [&session_stats](const ChannelNamePairs&) { + return std::unique_ptr(new SessionStats(session_stats)); + })); EXPECT_CALL(test_->session(), video_channel()) .WillRepeatedly(Return(&video_channel)); @@ -1677,8 +1688,10 @@ TEST_F(RTCStatsCollectorTest, CollectRTCOutboundRTPStreamStats_Default) { session_stats.transport_stats["TransportName"].channel_stats.push_back( channel_stats); - EXPECT_CALL(test_->session(), GetTransportStats(_)) - .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats), Return(true))); + EXPECT_CALL(test_->session(), GetStats(_)).WillRepeatedly(Invoke( + [&session_stats](const ChannelNamePairs&) { + return std::unique_ptr(new SessionStats(session_stats)); + })); EXPECT_CALL(test_->session(), voice_channel()) .WillRepeatedly(Return(&voice_channel)); EXPECT_CALL(test_->session(), video_channel()) @@ -1754,10 +1767,9 @@ TEST_F(RTCStatsCollectorTest, CollectRTCTransportStats) { // Mock the session to return the desired candidates. - EXPECT_CALL(test_->session(), GetTransportStats(_)).WillRepeatedly(Invoke( - [this, &session_stats](SessionStats* stats) { - *stats = session_stats; - return true; + EXPECT_CALL(test_->session(), GetStats(_)).WillRepeatedly(Invoke( + [&session_stats](const ChannelNamePairs&) { + return std::unique_ptr(new SessionStats(session_stats)); })); // Get stats without RTCP, an active connection or certificates. diff --git a/webrtc/api/statscollector.cc b/webrtc/api/statscollector.cc index 991ceb4fa4..fb6583ab91 100644 --- a/webrtc/api/statscollector.cc +++ b/webrtc/api/statscollector.cc @@ -675,8 +675,8 @@ void StatsCollector::ExtractSessionInfo() { report->AddBoolean(StatsReport::kStatsValueNameInitiator, pc_->session()->initial_offerer()); - SessionStats stats; - if (!pc_->session()->GetTransportStats(&stats)) { + std::unique_ptr stats = pc_->session()->GetStats_s(); + if (!stats) { return; } @@ -686,9 +686,9 @@ void StatsCollector::ExtractSessionInfo() { // the proxy map directly from the session stats. // As is, if GetStats() failed, we could be using old (incorrect?) proxy // data. - proxy_to_transport_ = stats.proxy_to_transport; + proxy_to_transport_ = stats->proxy_to_transport; - for (const auto& transport_iter : stats.transport_stats) { + for (const auto& transport_iter : stats->transport_stats) { // Attempt to get a copy of the certificates from the transport and // expose them in stats reports. All channels in a transport share the // same local and remote certificates. diff --git a/webrtc/api/statscollector_unittest.cc b/webrtc/api/statscollector_unittest.cc index bf60d0ed95..50ae51d6b0 100644 --- a/webrtc/api/statscollector_unittest.cc +++ b/webrtc/api/statscollector_unittest.cc @@ -41,6 +41,7 @@ using testing::_; using testing::DoAll; using testing::Field; +using testing::Invoke; using testing::Return; using testing::ReturnNull; using testing::ReturnRef; @@ -509,7 +510,7 @@ class StatsCollectorTest : public testing::Test { &event_log_)), session_(media_controller_.get()) { // By default, we ignore session GetStats calls. - EXPECT_CALL(session_, GetTransportStats(_)).WillRepeatedly(Return(false)); + EXPECT_CALL(session_, GetStats(_)).WillRepeatedly(ReturnNull()); // Add default returns for mock classes. EXPECT_CALL(session_, video_channel()).WillRepeatedly(ReturnNull()); EXPECT_CALL(session_, voice_channel()).WillRepeatedly(ReturnNull()); @@ -612,9 +613,11 @@ class StatsCollectorTest : public testing::Test { // Instruct the session to return stats containing the transport channel. InitSessionStats(vc_name); - EXPECT_CALL(session_, GetTransportStats(_)) - .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats_), - Return(true))); + EXPECT_CALL(session_, GetStats(_)).WillRepeatedly(Invoke( + [this](const ChannelNamePairs&) { + return std::unique_ptr( + new SessionStats(session_stats_)); + })); // Constructs an ssrc stats update. if (voice_sender_info) @@ -712,9 +715,11 @@ class StatsCollectorTest : public testing::Test { EXPECT_CALL(session_, GetRemoteSSLCertificate_ReturnsRawPointer( transport_stats.transport_name)) .WillOnce(Return(remote_cert.release())); - EXPECT_CALL(session_, GetTransportStats(_)) - .WillOnce(DoAll(SetArgPointee<0>(session_stats), - Return(true))); + EXPECT_CALL(session_, GetStats(_)).WillOnce(Invoke( + [&session_stats](const ChannelNamePairs&) { + return std::unique_ptr( + new SessionStats(session_stats)); + })); stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard); @@ -853,9 +858,11 @@ TEST_F(StatsCollectorTest, BytesCounterHandles64Bits) { const char kVideoChannelName[] = "video"; InitSessionStats(kVideoChannelName); - EXPECT_CALL(session_, GetTransportStats(_)) - .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats_), - Return(true))); + EXPECT_CALL(session_, GetStats(_)).WillRepeatedly(Invoke( + [this](const ChannelNamePairs&) { + return std::unique_ptr( + new SessionStats(session_stats_)); + })); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); cricket::VideoChannel video_channel( @@ -900,9 +907,11 @@ TEST_F(StatsCollectorTest, BandwidthEstimationInfoIsReported) { const char kVideoChannelName[] = "video"; InitSessionStats(kVideoChannelName); - EXPECT_CALL(session_, GetTransportStats(_)) - .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats_), - Return(true))); + EXPECT_CALL(session_, GetStats(_)).WillRepeatedly(Invoke( + [this](const ChannelNamePairs&) { + return std::unique_ptr( + new SessionStats(session_stats_)); + })); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); cricket::VideoChannel video_channel( @@ -1013,9 +1022,11 @@ TEST_F(StatsCollectorTest, TrackAndSsrcObjectExistAfterUpdateSsrcStats) { const char kVideoChannelName[] = "video"; InitSessionStats(kVideoChannelName); - EXPECT_CALL(session_, GetTransportStats(_)) - .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats_), - Return(true))); + EXPECT_CALL(session_, GetStats(_)).WillRepeatedly(Invoke( + [this](const ChannelNamePairs&) { + return std::unique_ptr( + new SessionStats(session_stats_)); + })); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); cricket::VideoChannel video_channel( @@ -1110,9 +1121,11 @@ TEST_F(StatsCollectorTest, TransportObjectLinkedFromSsrcObject) { Return(true))); InitSessionStats(kVcName); - EXPECT_CALL(session_, GetTransportStats(_)) - .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats_), - Return(true))); + EXPECT_CALL(session_, GetStats(_)).WillRepeatedly(Invoke( + [this](const ChannelNamePairs&) { + return std::unique_ptr( + new SessionStats(session_stats_)); + })); stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard); StatsReports reports; @@ -1181,9 +1194,11 @@ TEST_F(StatsCollectorTest, RemoteSsrcInfoIsPresent) { // Instruct the session to return stats containing the transport channel. InitSessionStats(kVcName); - EXPECT_CALL(session_, GetTransportStats(_)) - .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats_), - Return(true))); + EXPECT_CALL(session_, GetStats(_)).WillRepeatedly(Invoke( + [this](const ChannelNamePairs&) { + return std::unique_ptr( + new SessionStats(session_stats_)); + })); // Constructs an ssrc stats update. cricket::VideoMediaInfo stats_read; @@ -1224,9 +1239,11 @@ TEST_F(StatsCollectorTest, ReportsFromRemoteTrack) { const char kVideoChannelName[] = "video"; InitSessionStats(kVideoChannelName); - EXPECT_CALL(session_, GetTransportStats(_)) - .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats_), - Return(true))); + EXPECT_CALL(session_, GetStats(_)).WillRepeatedly(Invoke( + [this](const ChannelNamePairs&) { + return std::unique_ptr( + new SessionStats(session_stats_)); + })); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); cricket::VideoChannel video_channel( @@ -1430,9 +1447,11 @@ TEST_F(StatsCollectorTest, NoTransport) { transport_stats; // Configure MockWebRtcSession - EXPECT_CALL(session_, GetTransportStats(_)) - .WillOnce(DoAll(SetArgPointee<0>(session_stats), - Return(true))); + EXPECT_CALL(session_, GetStats(_)).WillRepeatedly(Invoke( + [&session_stats](const ChannelNamePairs&) { + return std::unique_ptr( + new SessionStats(session_stats)); + })); stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard); stats.GetStats(NULL, &reports); @@ -1487,9 +1506,11 @@ TEST_F(StatsCollectorTest, NoCertificates) { transport_stats; // Configure MockWebRtcSession - EXPECT_CALL(session_, GetTransportStats(_)) - .WillOnce(DoAll(SetArgPointee<0>(session_stats), - Return(true))); + EXPECT_CALL(session_, GetStats(_)).WillRepeatedly(Invoke( + [&session_stats](const ChannelNamePairs&) { + return std::unique_ptr( + new SessionStats(session_stats)); + })); stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard); stats.GetStats(NULL, &reports); @@ -1566,8 +1587,11 @@ TEST_F(StatsCollectorTest, FilterOutNegativeInitialValues) { // Instruct the session to return stats containing the transport channel. InitSessionStats(kVcName); - EXPECT_CALL(session_, GetTransportStats(_)) - .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats_), Return(true))); + EXPECT_CALL(session_, GetStats(_)).WillRepeatedly(Invoke( + [this](const ChannelNamePairs&) { + return std::unique_ptr( + new SessionStats(session_stats_)); + })); cricket::VoiceSenderInfo voice_sender_info; voice_sender_info.add_ssrc(kSsrcOfTrack); @@ -1720,9 +1744,11 @@ TEST_F(StatsCollectorTest, GetStatsAfterRemoveAudioStream) { // Instruct the session to return stats containing the transport channel. InitSessionStats(kVcName); - EXPECT_CALL(session_, GetTransportStats(_)) - .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats_), - Return(true))); + EXPECT_CALL(session_, GetStats(_)).WillRepeatedly(Invoke( + [this](const ChannelNamePairs&) { + return std::unique_ptr( + new SessionStats(session_stats_)); + })); stats.RemoveLocalAudioTrack(audio_track_.get(), kSsrcOfTrack); cricket::VoiceSenderInfo voice_sender_info; @@ -1794,9 +1820,11 @@ TEST_F(StatsCollectorTest, LocalAndRemoteTracksWithSameSsrc) { // Instruct the session to return stats containing the transport channel. InitSessionStats(kVcName); - EXPECT_CALL(session_, GetTransportStats(_)) - .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats_), - Return(true))); + EXPECT_CALL(session_, GetStats(_)).WillRepeatedly(Invoke( + [this](const ChannelNamePairs&) { + return std::unique_ptr( + new SessionStats(session_stats_)); + })); cricket::VoiceSenderInfo voice_sender_info; InitVoiceSenderInfo(&voice_sender_info); @@ -1914,8 +1942,11 @@ TEST_F(StatsCollectorTest, VerifyVideoSendSsrcStats) { const char kVideoChannelName[] = "video"; InitSessionStats(kVideoChannelName); - EXPECT_CALL(session_, GetTransportStats(_)) - .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats_), Return(true))); + EXPECT_CALL(session_, GetStats(_)).WillRepeatedly(Invoke( + [this](const ChannelNamePairs&) { + return std::unique_ptr( + new SessionStats(session_stats_)); + })); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); cricket::VideoChannel video_channel( @@ -1959,8 +1990,11 @@ TEST_F(StatsCollectorTest, VerifyVideoReceiveSsrcStats) { const char kVideoChannelName[] = "video"; InitSessionStats(kVideoChannelName); - EXPECT_CALL(session_, GetTransportStats(_)) - .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats_), Return(true))); + EXPECT_CALL(session_, GetStats(_)).WillRepeatedly(Invoke( + [this](const ChannelNamePairs&) { + return std::unique_ptr( + new SessionStats(session_stats_)); + })); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); cricket::VideoChannel video_channel( diff --git a/webrtc/api/test/mock_webrtcsession.h b/webrtc/api/test/mock_webrtcsession.h index 108bdcaac0..7fefad857d 100644 --- a/webrtc/api/test/mock_webrtcsession.h +++ b/webrtc/api/test/mock_webrtcsession.h @@ -42,7 +42,8 @@ class MockWebRtcSession : public webrtc::WebRtcSession { // track. MOCK_METHOD2(GetLocalTrackIdBySsrc, bool(uint32_t, std::string*)); MOCK_METHOD2(GetRemoteTrackIdBySsrc, bool(uint32_t, std::string*)); - MOCK_METHOD1(GetTransportStats, bool(SessionStats*)); + MOCK_METHOD1(GetStats, + std::unique_ptr(const ChannelNamePairs&)); MOCK_METHOD2(GetLocalCertificate, bool(const std::string& transport_name, rtc::scoped_refptr* certificate)); diff --git a/webrtc/api/webrtcsession.cc b/webrtc/api/webrtcsession.cc index b05862d37c..ea724a344c 100644 --- a/webrtc/api/webrtcsession.cc +++ b/webrtc/api/webrtcsession.cc @@ -970,50 +970,43 @@ bool WebRtcSession::GetTransportDescription( return true; } -bool WebRtcSession::GetTransportStats(SessionStats* stats) { +std::unique_ptr WebRtcSession::GetStats_s() { ASSERT(signaling_thread()->IsCurrent()); - return (GetChannelTransportStats(voice_channel(), stats) && - GetChannelTransportStats(video_channel(), stats) && - GetChannelTransportStats(data_channel(), stats)); + ChannelNamePairs channel_name_pairs; + if (voice_channel()) { + channel_name_pairs.voice = rtc::Optional(ChannelNamePair( + voice_channel()->content_name(), voice_channel()->transport_name())); + } + if (video_channel()) { + channel_name_pairs.video = rtc::Optional(ChannelNamePair( + video_channel()->content_name(), video_channel()->transport_name())); + } + if (data_channel()) { + channel_name_pairs.data = rtc::Optional(ChannelNamePair( + data_channel()->content_name(), data_channel()->transport_name())); + } + return GetStats(channel_name_pairs); } -bool WebRtcSession::GetChannelTransportStats(cricket::BaseChannel* ch, - SessionStats* stats) { - ASSERT(signaling_thread()->IsCurrent()); - if (!ch) { - // Not using this channel. - return true; +std::unique_ptr WebRtcSession::GetStats( + const ChannelNamePairs& channel_name_pairs) { + if (network_thread()->IsCurrent()) { + return GetStats_n(channel_name_pairs); } - - const std::string& content_name = ch->content_name(); - const std::string& transport_name = ch->transport_name(); - stats->proxy_to_transport[content_name] = transport_name; - if (stats->transport_stats.find(transport_name) != - stats->transport_stats.end()) { - // Transport stats already done for this transport. - return true; - } - - cricket::TransportStats tstats; - if (!transport_controller_->GetStats(transport_name, &tstats)) { - return false; - } - - stats->transport_stats[transport_name] = tstats; - return true; + return network_thread()->Invoke>( + RTC_FROM_HERE, + rtc::Bind(&WebRtcSession::GetStats_n, this, channel_name_pairs)); } bool WebRtcSession::GetLocalCertificate( const std::string& transport_name, rtc::scoped_refptr* certificate) { - ASSERT(signaling_thread()->IsCurrent()); return transport_controller_->GetLocalCertificate(transport_name, certificate); } std::unique_ptr WebRtcSession::GetRemoteSSLCertificate( const std::string& transport_name) { - ASSERT(signaling_thread()->IsCurrent()); return transport_controller_->GetRemoteSSLCertificate(transport_name); } @@ -1716,6 +1709,28 @@ bool WebRtcSession::CreateDataChannel(const cricket::ContentInfo* content, return true; } +std::unique_ptr WebRtcSession::GetStats_n( + const ChannelNamePairs& channel_name_pairs) { + ASSERT(network_thread()->IsCurrent()); + std::unique_ptr session_stats(new SessionStats()); + for (const auto channel_name_pair : { &channel_name_pairs.voice, + &channel_name_pairs.video, + &channel_name_pairs.data }) { + if (*channel_name_pair) { + cricket::TransportStats transport_stats; + if (!transport_controller_->GetStats((*channel_name_pair)->transport_name, + &transport_stats)) { + return nullptr; + } + session_stats->proxy_to_transport[(*channel_name_pair)->content_name] = + (*channel_name_pair)->transport_name; + session_stats->transport_stats[(*channel_name_pair)->transport_name] = + std::move(transport_stats); + } + } + return session_stats; +} + void WebRtcSession::OnDtlsSetupFailure(cricket::BaseChannel*, bool rtcp) { SetError(ERROR_TRANSPORT, rtcp ? kDtlsSetupFailureRtcp : kDtlsSetupFailureRtp); diff --git a/webrtc/api/webrtcsession.h b/webrtc/api/webrtcsession.h index 06d3375e15..241fb626d9 100644 --- a/webrtc/api/webrtcsession.h +++ b/webrtc/api/webrtcsession.h @@ -115,6 +115,20 @@ struct SessionStats { TransportStatsMap transport_stats; }; +struct ChannelNamePair { + ChannelNamePair( + const std::string& content_name, const std::string& transport_name) + : content_name(content_name), transport_name(transport_name) {} + std::string content_name; + std::string transport_name; +}; + +struct ChannelNamePairs { + rtc::Optional voice; + rtc::Optional video; + rtc::Optional data; +}; + // A WebRtcSession manages general session state. This includes negotiation // of both the application-level and network-level protocols: the former // defines what will be sent and the latter defines how it will be sent. Each @@ -194,6 +208,7 @@ class WebRtcSession : virtual cricket::DataChannel* data_channel() { return data_channel_.get(); } + std::unique_ptr GetChannelNamePairs(); cricket::BaseChannel* GetChannel(const std::string& content_name); @@ -261,10 +276,14 @@ class WebRtcSession : // Returns stats for all channels of all transports. // This avoids exposing the internal structures used to track them. - virtual bool GetTransportStats(SessionStats* stats); - - // Get stats for a specific channel - bool GetChannelTransportStats(cricket::BaseChannel* ch, SessionStats* stats); + // The parameterless version creates |ChannelNamePairs| from |voice_channel|, + // |video_channel| and |voice_channel| if available - this requires it to be + // called on the signaling thread - and invokes the other |GetStats|. The + // other |GetStats| can be invoked on any thread; if not invoked on the + // network thread a thread hop will happen. + std::unique_ptr GetStats_s(); + virtual std::unique_ptr GetStats( + const ChannelNamePairs& channel_name_pairs); // virtual so it can be mocked in unit tests virtual bool GetLocalCertificate( @@ -415,6 +434,9 @@ class WebRtcSession : bool CreateDataChannel(const cricket::ContentInfo* content, const std::string* bundle_transport); + std::unique_ptr GetStats_n( + const ChannelNamePairs& channel_name_pairs); + // Listens to SCTP CONTROL messages on unused SIDs and process them as OPEN // messages. void OnDataChannelMessageReceived(cricket::DataChannel* channel, @@ -489,7 +511,7 @@ class WebRtcSession : const std::string sid_; bool initial_offerer_ = false; - std::unique_ptr transport_controller_; + const std::unique_ptr transport_controller_; MediaControllerInterface* media_controller_; std::unique_ptr voice_channel_; std::unique_ptr video_channel_; diff --git a/webrtc/api/webrtcsession_unittest.cc b/webrtc/api/webrtcsession_unittest.cc index fe3468820e..116c8d5739 100644 --- a/webrtc/api/webrtcsession_unittest.cc +++ b/webrtc/api/webrtcsession_unittest.cc @@ -3039,9 +3039,8 @@ TEST_F(WebRtcSessionTest, TestIgnoreCandidatesForUnusedTransportWhenBundling) { // Checks if one of the transport channels contains a connection using a given // port. auto connection_with_remote_port = [this, voice_channel](int port) { - SessionStats stats; - session_->GetChannelTransportStats(voice_channel, &stats); - for (auto& kv : stats.transport_stats) { + std::unique_ptr stats = session_->GetStats_s(); + for (auto& kv : stats->transport_stats) { for (auto& chan_stat : kv.second.channel_stats) { for (auto& conn_info : chan_stat.connection_infos) { if (conn_info.remote_candidate.address().port() == port) { diff --git a/webrtc/p2p/base/transportcontroller.cc b/webrtc/p2p/base/transportcontroller.cc index 01394a97aa..4a3a00e10f 100644 --- a/webrtc/p2p/base/transportcontroller.cc +++ b/webrtc/p2p/base/transportcontroller.cc @@ -141,6 +141,9 @@ bool TransportController::SetLocalCertificate( bool TransportController::GetLocalCertificate( const std::string& transport_name, rtc::scoped_refptr* certificate) const { + if (network_thread_->IsCurrent()) { + return GetLocalCertificate_n(transport_name, certificate); + } return network_thread_->Invoke( RTC_FROM_HERE, rtc::Bind(&TransportController::GetLocalCertificate_n, this, transport_name, certificate)); @@ -149,6 +152,9 @@ bool TransportController::GetLocalCertificate( std::unique_ptr TransportController::GetRemoteSSLCertificate( const std::string& transport_name) const { + if (network_thread_->IsCurrent()) { + return GetRemoteSSLCertificate_n(transport_name); + } return network_thread_->Invoke>( RTC_FROM_HERE, rtc::Bind(&TransportController::GetRemoteSSLCertificate_n, this, transport_name)); @@ -206,6 +212,9 @@ bool TransportController::ReadyForRemoteCandidates( bool TransportController::GetStats(const std::string& transport_name, TransportStats* stats) { + if (network_thread_->IsCurrent()) { + return GetStats_n(transport_name, stats); + } return network_thread_->Invoke( RTC_FROM_HERE, rtc::Bind(&TransportController::GetStats_n, this, transport_name, stats));