diff --git a/api/fakemetricsobserver.cc b/api/fakemetricsobserver.cc index d09bf17185..beb30c2c18 100644 --- a/api/fakemetricsobserver.cc +++ b/api/fakemetricsobserver.cc @@ -61,4 +61,27 @@ int FakeMetricsObserver::GetHistogramSample( return histogram_samples_[type]; } +bool FakeMetricsObserver::ExpectOnlySingleEnumCount( + PeerConnectionEnumCounterType type, + int counter) const { + RTC_DCHECK(thread_checker_.CalledOnValidThread()); + if (counters_.size() <= static_cast(type)) { + // If a counter has not been allocated then there has been no call to + // |IncrementEnumCounter| so all the values are 0. + return false; + } + bool pass = true; + if (GetEnumCounter(type, counter) != 1) { + RTC_LOG(LS_ERROR) << "Expected single count for counter: " << counter; + pass = false; + } + for (const auto& entry : counters_[type]) { + if (entry.first != counter && entry.second > 0) { + RTC_LOG(LS_ERROR) << "Expected no count for counter: " << entry.first; + pass = false; + } + } + return pass; +} + } // namespace webrtc diff --git a/api/fakemetricsobserver.h b/api/fakemetricsobserver.h index afd019391a..3adc5a6547 100644 --- a/api/fakemetricsobserver.h +++ b/api/fakemetricsobserver.h @@ -35,6 +35,11 @@ class FakeMetricsObserver : public MetricsObserverInterface { int GetEnumCounter(PeerConnectionEnumCounterType type, int counter) const; int GetHistogramSample(PeerConnectionMetricsName type) const; + // Returns true if and only if there is a count of 1 for the given counter and + // a count of 0 for all other counters of the given enum type. + bool ExpectOnlySingleEnumCount(PeerConnectionEnumCounterType type, + int counter) const; + protected: ~FakeMetricsObserver() {} diff --git a/api/umametrics.h b/api/umametrics.h index a2d055da7f..4de1ce492a 100644 --- a/api/umametrics.h +++ b/api/umametrics.h @@ -41,6 +41,7 @@ enum PeerConnectionEnumCounterType { kEnumCounterSdpSemanticRequested, kEnumCounterSdpSemanticNegotiated, kEnumCounterKeyProtocolMediaType, + kEnumCounterSdpFormatReceived, kPeerConnectionEnumCounterMax }; @@ -146,6 +147,24 @@ enum SdpSemanticNegotiated { kSdpSemanticNegotiatedMax }; +// Metric which records the format of the received SDP for tracking how much the +// difference between Plan B and Unified Plan affect users. +enum SdpFormatReceived { + // No audio or video tracks. This is worth special casing since it seems to be + // the most common scenario (data-channel only). + kSdpFormatReceivedNoTracks, + // No more than one audio and one video track. Should be compatible with both + // Plan B and Unified Plan endpoints. + kSdpFormatReceivedSimple, + // More than one audio track or more than one video track in the Plan B format + // (e.g., one audio media section with multiple streams). + kSdpFormatReceivedComplexPlanB, + // More than one audio track or more than one video track in the Unified Plan + // format (e.g., two audio media sections). + kSdpFormatReceivedComplexUnifiedPlan, + kSdpFormatReceivedMax +}; + class MetricsObserverInterface : public rtc::RefCountInterface { public: // |type| is the type of the enum counter to be incremented. |counter| diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 74fe030008..e343e0da73 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -2105,6 +2105,11 @@ void PeerConnection::SetRemoteDescription( } RTC_DCHECK(remote_description()); + if (type == SdpType::kOffer) { + // Report to UMA the format of the received offer. + ReportSdpFormatReceived(*remote_description()); + } + if (type == SdpType::kAnswer) { // TODO(deadbeef): We already had to hop to the network thread for // MaybeStartGathering... @@ -5761,6 +5766,39 @@ std::string PeerConnection::GetSessionErrorMsg() { return desc.str(); } +void PeerConnection::ReportSdpFormatReceived( + const SessionDescriptionInterface& remote_offer) { + if (!uma_observer_) { + return; + } + int num_audio_mlines = 0; + int num_video_mlines = 0; + int num_audio_tracks = 0; + int num_video_tracks = 0; + for (const ContentInfo& content : remote_offer.description()->contents()) { + cricket::MediaType media_type = content.media_description()->type(); + int num_tracks = std::max( + 1, static_cast(content.media_description()->streams().size())); + if (media_type == cricket::MEDIA_TYPE_AUDIO) { + num_audio_mlines += 1; + num_audio_tracks += num_tracks; + } else if (media_type == cricket::MEDIA_TYPE_VIDEO) { + num_video_mlines += 1; + num_video_tracks += num_tracks; + } + } + SdpFormatReceived format = kSdpFormatReceivedNoTracks; + if (num_audio_mlines > 1 || num_video_mlines > 1) { + format = kSdpFormatReceivedComplexUnifiedPlan; + } else if (num_audio_tracks > 1 || num_video_tracks > 1) { + format = kSdpFormatReceivedComplexPlanB; + } else if (num_audio_tracks > 0 || num_video_tracks > 0) { + format = kSdpFormatReceivedSimple; + } + uma_observer_->IncrementEnumCounter(kEnumCounterSdpFormatReceived, format, + kSdpFormatReceivedMax); +} + void PeerConnection::ReportNegotiatedSdpSemantics( const SessionDescriptionInterface& answer) { if (!uma_observer_) { diff --git a/pc/peerconnection.h b/pc/peerconnection.h index 90b087ac5a..f5b53ae382 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -851,6 +851,9 @@ class PeerConnection : public PeerConnectionInternal, const char* SessionErrorToString(SessionError error) const; std::string GetSessionErrorMsg(); + // Report the UMA metric SdpFormatReceived for the given remote offer. + void ReportSdpFormatReceived(const SessionDescriptionInterface& remote_offer); + // Report inferred negotiated SDP semantics from a local/remote answer to the // UMA observer. void ReportNegotiatedSdpSemantics(const SessionDescriptionInterface& answer); diff --git a/pc/peerconnection_rtp_unittest.cc b/pc/peerconnection_rtp_unittest.cc index 4d67b4ba55..83a185836b 100644 --- a/pc/peerconnection_rtp_unittest.cc +++ b/pc/peerconnection_rtp_unittest.cc @@ -13,10 +13,10 @@ #include "api/audio_codecs/builtin_audio_decoder_factory.h" #include "api/audio_codecs/builtin_audio_encoder_factory.h" -#include "api/fakemetricsobserver.h" #include "api/jsep.h" #include "api/mediastreaminterface.h" #include "api/peerconnectioninterface.h" +#include "api/umametrics.h" #include "pc/mediastream.h" #include "pc/mediastreamtrack.h" #include "pc/peerconnectionwrapper.h" @@ -1030,9 +1030,7 @@ TEST_F(PeerConnectionMsidSignalingTest, UnifiedPlanTalkingToOurself) { caller->AddAudioTrack("caller_audio"); auto callee = CreatePeerConnectionWithUnifiedPlan(); callee->AddAudioTrack("callee_audio"); - auto caller_observer = - new rtc::RefCountedObject(); - caller->pc()->RegisterUMAObserver(caller_observer); + auto caller_observer = caller->RegisterFakeMetricsObserver(); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); @@ -1047,18 +1045,8 @@ TEST_F(PeerConnectionMsidSignalingTest, UnifiedPlanTalkingToOurself) { EXPECT_EQ(cricket::kMsidSignalingMediaSection, answer->description()->msid_signaling()); // Check that this is counted correctly - EXPECT_EQ(1, caller_observer->GetEnumCounter( - webrtc::kEnumCounterSdpSemanticNegotiated, - webrtc::kSdpSemanticNegotiatedUnifiedPlan)); - EXPECT_EQ(0, caller_observer->GetEnumCounter( - webrtc::kEnumCounterSdpSemanticNegotiated, - webrtc::kSdpSemanticNegotiatedNone)); - EXPECT_EQ(0, caller_observer->GetEnumCounter( - webrtc::kEnumCounterSdpSemanticNegotiated, - webrtc::kSdpSemanticNegotiatedPlanB)); - EXPECT_EQ(0, caller_observer->GetEnumCounter( - webrtc::kEnumCounterSdpSemanticNegotiated, - webrtc::kSdpSemanticNegotiatedMixed)); + EXPECT_TRUE(caller_observer->ExpectOnlySingleEnumCount( + kEnumCounterSdpSemanticNegotiated, kSdpSemanticNegotiatedUnifiedPlan)); } TEST_F(PeerConnectionMsidSignalingTest, PlanBOfferToUnifiedPlanAnswer) { @@ -1101,6 +1089,76 @@ TEST_F(PeerConnectionMsidSignalingTest, PureUnifiedPlanToUs) { answer->description()->msid_signaling()); } +// Test that the correct UMA metrics are reported for simple/complex SDP. + +class SdpFormatReceivedTest : public PeerConnectionRtpTest {}; + +#ifdef HAVE_SCTP +TEST_F(SdpFormatReceivedTest, DataChannelOnlyIsReportedAsNoTracks) { + auto caller = CreatePeerConnectionWithUnifiedPlan(); + caller->CreateDataChannel("dc"); + auto callee = CreatePeerConnectionWithUnifiedPlan(); + auto callee_metrics = callee->RegisterFakeMetricsObserver(); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); + + EXPECT_TRUE(callee_metrics->ExpectOnlySingleEnumCount( + kEnumCounterSdpFormatReceived, kSdpFormatReceivedNoTracks)); +} +#endif // HAVE_SCTP + +TEST_F(SdpFormatReceivedTest, SimpleUnifiedPlanIsReportedAsSimple) { + auto caller = CreatePeerConnectionWithUnifiedPlan(); + caller->AddAudioTrack("audio"); + caller->AddVideoTrack("video"); + auto callee = CreatePeerConnectionWithPlanB(); + auto callee_metrics = callee->RegisterFakeMetricsObserver(); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); + + EXPECT_TRUE(callee_metrics->ExpectOnlySingleEnumCount( + kEnumCounterSdpFormatReceived, kSdpFormatReceivedSimple)); +} + +TEST_F(SdpFormatReceivedTest, SimplePlanBIsReportedAsSimple) { + auto caller = CreatePeerConnectionWithPlanB(); + caller->AddVideoTrack("video"); // Video only. + auto callee = CreatePeerConnectionWithUnifiedPlan(); + auto callee_metrics = callee->RegisterFakeMetricsObserver(); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); + + EXPECT_TRUE(callee_metrics->ExpectOnlySingleEnumCount( + kEnumCounterSdpFormatReceived, kSdpFormatReceivedSimple)); +} + +TEST_F(SdpFormatReceivedTest, ComplexUnifiedIsReportedAsComplexUnifiedPlan) { + auto caller = CreatePeerConnectionWithUnifiedPlan(); + caller->AddAudioTrack("audio1"); + caller->AddAudioTrack("audio2"); + caller->AddVideoTrack("video"); + auto callee = CreatePeerConnectionWithPlanB(); + auto callee_metrics = callee->RegisterFakeMetricsObserver(); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); + + EXPECT_TRUE(callee_metrics->ExpectOnlySingleEnumCount( + kEnumCounterSdpFormatReceived, kSdpFormatReceivedComplexUnifiedPlan)); +} + +TEST_F(SdpFormatReceivedTest, ComplexPlanBIsReportedAsComplexPlanB) { + auto caller = CreatePeerConnectionWithPlanB(); + caller->AddVideoTrack("video1"); + caller->AddVideoTrack("video2"); + auto callee = CreatePeerConnectionWithUnifiedPlan(); + auto callee_metrics = callee->RegisterFakeMetricsObserver(); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); + + EXPECT_TRUE(callee_metrics->ExpectOnlySingleEnumCount( + kEnumCounterSdpFormatReceived, kSdpFormatReceivedComplexPlanB)); +} + // Sender setups in a call. class PeerConnectionSenderTest : public PeerConnectionRtpTest {}; diff --git a/pc/peerconnectionwrapper.cc b/pc/peerconnectionwrapper.cc index 5d26539cc2..36d30723f1 100644 --- a/pc/peerconnectionwrapper.cc +++ b/pc/peerconnectionwrapper.cc @@ -322,4 +322,12 @@ PeerConnectionWrapper::GetStats() { return callback->report(); } +rtc::scoped_refptr +PeerConnectionWrapper::RegisterFakeMetricsObserver() { + RTC_DCHECK(!fake_metrics_observer_); + fake_metrics_observer_ = new rtc::RefCountedObject(); + pc_->RegisterUMAObserver(fake_metrics_observer_); + return fake_metrics_observer_; +} + } // namespace webrtc diff --git a/pc/peerconnectionwrapper.h b/pc/peerconnectionwrapper.h index 330809f748..b1abf62609 100644 --- a/pc/peerconnectionwrapper.h +++ b/pc/peerconnectionwrapper.h @@ -16,6 +16,7 @@ #include #include +#include "api/fakemetricsobserver.h" #include "api/peerconnectioninterface.h" #include "pc/test/mockpeerconnectionobservers.h" #include "rtc_base/function_view.h" @@ -170,6 +171,10 @@ class PeerConnectionWrapper { // report. If GetStats() fails, this method returns null and fails the test. rtc::scoped_refptr GetStats(); + // Creates a new FakeMetricsObserver and registers it with the PeerConnection + // as the UMA observer. + rtc::scoped_refptr RegisterFakeMetricsObserver(); + private: std::unique_ptr CreateSdp( rtc::FunctionView fn, @@ -180,6 +185,7 @@ class PeerConnectionWrapper { rtc::scoped_refptr pc_factory_; std::unique_ptr observer_; rtc::scoped_refptr pc_; + rtc::scoped_refptr fake_metrics_observer_; }; } // namespace webrtc