From 183e09d23c90a05d6d9afcdad4fb2d5be341ffbb Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Thu, 28 Jun 2018 12:04:41 +0200 Subject: [PATCH] Correct data histogram entry for incoming DC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both incoming and outgoing datachannels should cause the DATA_ADDED flag to be set. This CL also moves all tests into their own file, and improves scaffolding. Bug: chromium:718508 Change-Id: I5c4c257ccb6f26799f7593bce8b27ebf59015b1e Reviewed-on: https://webrtc-review.googlesource.com/85348 Commit-Queue: Harald Alvestrand Reviewed-by: Henrik Boström Cr-Commit-Position: refs/heads/master@{#23766} --- pc/peerconnection.cc | 9 +- pc/peerconnection_histogram_unittest.cc | 116 ++++++++++++++++++++++-- pc/peerconnection_integrationtest.cc | 40 -------- 3 files changed, 113 insertions(+), 52 deletions(-) diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index d9538335a2..15966bafe3 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -1040,10 +1040,10 @@ bool PeerConnection::Initialize( RtpTransceiverProxyWithInternal::Create( signaling_thread(), new RtpTransceiver(cricket::MEDIA_TYPE_VIDEO))); } - signaling_thread()->PostDelayed( - RTC_FROM_HERE, - return_histogram_very_quickly_ ? 0 : REPORT_USAGE_PATTERN_DELAY_MS, this, - MSG_REPORT_USAGE_PATTERN, nullptr); + int delay_ms = + return_histogram_very_quickly_ ? 0 : REPORT_USAGE_PATTERN_DELAY_MS; + signaling_thread()->PostDelayed(RTC_FROM_HERE, delay_ms, this, + MSG_REPORT_USAGE_PATTERN, nullptr); return true; } @@ -4547,6 +4547,7 @@ void PeerConnection::OnDataChannelOpenMessage( rtc::scoped_refptr proxy_channel = DataChannelProxy::Create(signaling_thread(), channel); observer_->OnDataChannel(std::move(proxy_channel)); + NoteUsageEvent(UsageEvent::DATA_ADDED); } rtc::scoped_refptr> diff --git a/pc/peerconnection_histogram_unittest.cc b/pc/peerconnection_histogram_unittest.cc index b554e86993..e6255d5ddf 100644 --- a/pc/peerconnection_histogram_unittest.cc +++ b/pc/peerconnection_histogram_unittest.cc @@ -18,9 +18,6 @@ #include "pc/peerconnectionfactory.h" #include "pc/peerconnectionwrapper.h" #include "pc/sdputils.h" -#ifdef WEBRTC_ANDROID -#include "pc/test/androidtestinitializer.h" -#endif #include "pc/test/fakesctptransport.h" #include "rtc_base/gunit.h" #include "rtc_base/ptr_util.h" @@ -64,7 +61,23 @@ class PeerConnectionFactoryForUsageHistogramTest void ReturnHistogramVeryQuickly() { return_histogram_very_quickly_ = true; } private: - bool return_histogram_very_quickly_; + bool return_histogram_very_quickly_ = false; +}; + +class PeerConnectionWrapperForUsageHistogramTest; +typedef PeerConnectionWrapperForUsageHistogramTest* RawWrapperPtr; + +class ObserverForUsageHistogramTest : public MockPeerConnectionObserver { + public: + void OnIceCandidate(const webrtc::IceCandidateInterface* candidate) override; + void PrepareToExchangeCandidates(RawWrapperPtr other) { + candidate_target_ = other; + } + + bool HaveDataChannel() { return last_datachannel_; } + + private: + RawWrapperPtr candidate_target_; // Note: Not thread-safe against deletions. }; class PeerConnectionWrapperForUsageHistogramTest @@ -78,8 +91,35 @@ class PeerConnectionWrapperForUsageHistogramTest pc()); return static_cast(pci->internal()); } + + void PrepareToExchangeCandidates( + PeerConnectionWrapperForUsageHistogramTest* other) { + static_cast(observer()) + ->PrepareToExchangeCandidates(other); + static_cast(other->observer()) + ->PrepareToExchangeCandidates(this); + } + + bool IsConnected() { + return pc()->ice_connection_state() == + PeerConnectionInterface::kIceConnectionConnected || + pc()->ice_connection_state() == + PeerConnectionInterface::kIceConnectionCompleted; + } + + bool HaveDataChannel() { + return static_cast(observer()) + ->HaveDataChannel(); + } }; +void ObserverForUsageHistogramTest::OnIceCandidate( + const webrtc::IceCandidateInterface* candidate) { + if (candidate_target_) { + candidate_target_->pc()->AddIceCandidate(candidate); + } +} + class PeerConnectionUsageHistogramTest : public ::testing::Test { protected: typedef std::unique_ptr @@ -87,9 +127,6 @@ class PeerConnectionUsageHistogramTest : public ::testing::Test { PeerConnectionUsageHistogramTest() : vss_(new rtc::VirtualSocketServer()), main_(vss_.get()) { -#ifdef WEBRTC_ANDROID - InitializeAndroidObjects(); -#endif } WrapperPtr CreatePeerConnection() { @@ -113,7 +150,7 @@ class PeerConnectionUsageHistogramTest : public ::testing::Test { if (immediate_report) { pc_factory->ReturnHistogramVeryQuickly(); } - auto observer = rtc::MakeUnique(); + auto observer = rtc::MakeUnique(); auto pc = pc_factory->CreatePeerConnection(config, nullptr, nullptr, observer.get()); if (!pc) { @@ -142,4 +179,67 @@ TEST_F(PeerConnectionUsageHistogramTest, UsageFingerprintHistogramFromTimeout) { kDefaultTimeout); } +#ifndef WEBRTC_ANDROID +// These tests do not work on Android. Why is unclear. +// https://bugs.webrtc.org/9461 + +// Test getting the usage fingerprint for an audio/video connection. +TEST_F(PeerConnectionUsageHistogramTest, FingerprintAudioVideo) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + // Register UMA observer before signaling begins. + auto caller_observer = caller->RegisterFakeMetricsObserver(); + auto callee_observer = callee->RegisterFakeMetricsObserver(); + caller->AddAudioTrack("audio"); + caller->AddVideoTrack("video"); + caller->PrepareToExchangeCandidates(callee.get()); + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + ASSERT_TRUE_WAIT(caller->IsConnected(), kDefaultTimeout); + ASSERT_TRUE_WAIT(callee->IsConnected(), kDefaultTimeout); + caller->pc()->Close(); + callee->pc()->Close(); + int expected_fingerprint = MakeUsageFingerprint( + {PeerConnection::UsageEvent::AUDIO_ADDED, + PeerConnection::UsageEvent::VIDEO_ADDED, + PeerConnection::UsageEvent::SET_LOCAL_DESCRIPTION_CALLED, + PeerConnection::UsageEvent::SET_REMOTE_DESCRIPTION_CALLED, + PeerConnection::UsageEvent::CANDIDATE_COLLECTED, + PeerConnection::UsageEvent::REMOTE_CANDIDATE_ADDED, + PeerConnection::UsageEvent::ICE_STATE_CONNECTED, + PeerConnection::UsageEvent::CLOSE_CALLED}); + EXPECT_TRUE(caller_observer->ExpectOnlySingleEnumCount( + webrtc::kEnumCounterUsagePattern, expected_fingerprint)); + EXPECT_TRUE(callee_observer->ExpectOnlySingleEnumCount( + webrtc::kEnumCounterUsagePattern, expected_fingerprint)); +} + +#ifdef HAVE_SCTP +TEST_F(PeerConnectionUsageHistogramTest, FingerprintDataOnly) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + // Register UMA observer before signaling begins. + auto caller_observer = caller->RegisterFakeMetricsObserver(); + auto callee_observer = callee->RegisterFakeMetricsObserver(); + caller->CreateDataChannel("foodata"); + caller->PrepareToExchangeCandidates(callee.get()); + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + ASSERT_TRUE_WAIT(callee->HaveDataChannel(), kDefaultTimeout); + caller->pc()->Close(); + callee->pc()->Close(); + int expected_fingerprint = MakeUsageFingerprint( + {PeerConnection::UsageEvent::DATA_ADDED, + PeerConnection::UsageEvent::SET_LOCAL_DESCRIPTION_CALLED, + PeerConnection::UsageEvent::SET_REMOTE_DESCRIPTION_CALLED, + PeerConnection::UsageEvent::CANDIDATE_COLLECTED, + PeerConnection::UsageEvent::REMOTE_CANDIDATE_ADDED, + PeerConnection::UsageEvent::ICE_STATE_CONNECTED, + PeerConnection::UsageEvent::CLOSE_CALLED}); + EXPECT_TRUE(caller_observer->ExpectOnlySingleEnumCount( + webrtc::kEnumCounterUsagePattern, expected_fingerprint)); + EXPECT_TRUE(callee_observer->ExpectOnlySingleEnumCount( + webrtc::kEnumCounterUsagePattern, expected_fingerprint)); +} +#endif // HAVE_SCTP +#endif // WEBRTC_ANDROID + } // namespace webrtc diff --git a/pc/peerconnection_integrationtest.cc b/pc/peerconnection_integrationtest.cc index c30ecef7d4..dc0f06f1c9 100644 --- a/pc/peerconnection_integrationtest.cc +++ b/pc/peerconnection_integrationtest.cc @@ -178,14 +178,6 @@ int FindFirstMediaStatsIndexByKind( return -1; } -int MakeUsageFingerprint(std::set events) { - int signature = 0; - for (const auto it : events) { - signature |= static_cast(it); - } - return signature; -} - class SignalingMessageReceiver { public: virtual void ReceiveSdpMessage(SdpType type, const std::string& msg) = 0; @@ -4633,38 +4625,6 @@ TEST_P(PeerConnectionIntegrationInteropTest, ASSERT_TRUE(ExpectNewFrames(media_expectations)); } -// Test getting the usage fingerprint for a simple test case. -TEST_P(PeerConnectionIntegrationTest, UsageFingerprintHistogram) { - ASSERT_TRUE(CreatePeerConnectionWrappers()); - ConnectFakeSignaling(); - // Register UMA observer before signaling begins. - rtc::scoped_refptr caller_observer = - new rtc::RefCountedObject(); - caller()->pc()->RegisterUMAObserver(caller_observer); - rtc::scoped_refptr callee_observer = - new rtc::RefCountedObject(); - callee()->pc()->RegisterUMAObserver(callee_observer); - caller()->AddAudioTrack(); - caller()->AddVideoTrack(); - caller()->CreateAndSetAndSignalOffer(); - ASSERT_TRUE_WAIT(DtlsConnected(), kDefaultTimeout); - caller()->pc()->Close(); - callee()->pc()->Close(); - int expected_fingerprint = MakeUsageFingerprint( - {PeerConnection::UsageEvent::AUDIO_ADDED, - PeerConnection::UsageEvent::VIDEO_ADDED, - PeerConnection::UsageEvent::SET_LOCAL_DESCRIPTION_CALLED, - PeerConnection::UsageEvent::SET_REMOTE_DESCRIPTION_CALLED, - PeerConnection::UsageEvent::CANDIDATE_COLLECTED, - PeerConnection::UsageEvent::REMOTE_CANDIDATE_ADDED, - PeerConnection::UsageEvent::ICE_STATE_CONNECTED, - PeerConnection::UsageEvent::CLOSE_CALLED}); - EXPECT_TRUE(caller_observer->ExpectOnlySingleEnumCount( - webrtc::kEnumCounterUsagePattern, expected_fingerprint)); - EXPECT_TRUE(callee_observer->ExpectOnlySingleEnumCount( - webrtc::kEnumCounterUsagePattern, expected_fingerprint)); -} - INSTANTIATE_TEST_CASE_P( PeerConnectionIntegrationTest, PeerConnectionIntegrationInteropTest,