diff --git a/api/uma_metrics.h b/api/uma_metrics.h index a975b82aeb..c4fecf0269 100644 --- a/api/uma_metrics.h +++ b/api/uma_metrics.h @@ -112,36 +112,6 @@ enum SdpSemanticRequested { kSdpSemanticRequestMax }; -// These values are persisted to logs. Entries should not be renumbered and -// numeric values should never be reused. -enum SdpSemanticNegotiated { - kSdpSemanticNegotiatedNone = 0, - kSdpSemanticNegotiatedPlanB = 1, - kSdpSemanticNegotiatedUnifiedPlan = 2, - kSdpSemanticNegotiatedMixed = 3, - kSdpSemanticNegotiatedMax -}; - -// Metric which records the format of the received SDP for tracking how much the -// difference between Plan B and Unified Plan affect users. -// These values are persisted to logs. Entries should not be renumbered and -// numeric values should never be reused. -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 = 0, - // No more than one audio and one video track. Should be compatible with both - // Plan B and Unified Plan endpoints. - kSdpFormatReceivedSimple = 1, - // 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 = 2, - // More than one audio track or more than one video track in the Unified Plan - // format (e.g., two audio media sections). - kSdpFormatReceivedComplexUnifiedPlan = 3, - kSdpFormatReceivedMax -}; - // Metric for counting the outcome of adding an ICE candidate // These values are persisted to logs. Entries should not be renumbered and // numeric values should never be reused. diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 517669d226..e05a2b730e 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -2559,50 +2559,6 @@ bool PeerConnection::ValidateBundleSettings( return true; } -void PeerConnection::ReportSdpFormatReceived( - const SessionDescriptionInterface& remote_description) { - 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_description.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; - } - switch (remote_description.GetType()) { - case SdpType::kOffer: - // Historically only offers were counted. - RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.SdpFormatReceived", - format, kSdpFormatReceivedMax); - break; - case SdpType::kAnswer: - RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.SdpFormatReceivedAnswer", - format, kSdpFormatReceivedMax); - break; - default: - RTC_LOG(LS_ERROR) << "Can not report SdpFormatReceived for " - << SdpTypeToString(remote_description.GetType()); - break; - } -} - void PeerConnection::ReportSdpBundleUsage( const SessionDescriptionInterface& remote_description) { RTC_DCHECK_RUN_ON(signaling_thread()); diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 4398058628..36a9d2ac74 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -371,10 +371,6 @@ class PeerConnection : public PeerConnectionInternal, void AddRemoteCandidate(const std::string& mid, const cricket::Candidate& candidate) override; - // Report the UMA metric SdpFormatReceived for the given remote description. - void ReportSdpFormatReceived( - const SessionDescriptionInterface& remote_description) override; - // Report the UMA metric BundleUsage for the given remote description. void ReportSdpBundleUsage( const SessionDescriptionInterface& remote_description) override; diff --git a/pc/peer_connection_internal.h b/pc/peer_connection_internal.h index 1bca156316..ecf8fbfc83 100644 --- a/pc/peer_connection_internal.h +++ b/pc/peer_connection_internal.h @@ -53,10 +53,6 @@ class PeerConnectionSdpMethods { virtual const PeerConnectionInterface::RTCConfiguration* configuration() const = 0; - // Report the UMA metric SdpFormatReceived for the given remote description. - virtual void ReportSdpFormatReceived( - const SessionDescriptionInterface& remote_description) = 0; - // Report the UMA metric BundleUsage for the given remote description. virtual void ReportSdpBundleUsage( const SessionDescriptionInterface& remote_description) = 0; diff --git a/pc/peer_connection_rtp_unittest.cc b/pc/peer_connection_rtp_unittest.cc index 268101ad05..e17c52b5ab 100644 --- a/pc/peer_connection_rtp_unittest.cc +++ b/pc/peer_connection_rtp_unittest.cc @@ -1811,10 +1811,6 @@ TEST_F(PeerConnectionMsidSignalingTest, UnifiedPlanTalkingToOurself) { auto* answer = caller->pc()->remote_description(); EXPECT_EQ(cricket::kMsidSignalingMediaSection, answer->description()->msid_signaling()); - // Check that this is counted correctly - EXPECT_METRIC_THAT( - metrics::Samples("WebRTC.PeerConnection.SdpSemanticNegotiated"), - ElementsAre(Pair(kSdpSemanticNegotiatedUnifiedPlan, 2))); } TEST_F(PeerConnectionMsidSignalingTest, PlanBOfferToUnifiedPlanAnswer) { @@ -1890,92 +1886,6 @@ TEST_F(PeerConnectionMsidSignalingTest, PureUnifiedPlanToUs) { answer->description()->msid_signaling()); } -// Test that the correct UMA metrics are reported for simple/complex SDP. - -class SdpFormatReceivedTest : public PeerConnectionRtpTestUnifiedPlan {}; - -#ifdef WEBRTC_HAVE_SCTP -TEST_F(SdpFormatReceivedTest, DataChannelOnlyIsReportedAsNoTracks) { - auto caller = CreatePeerConnectionWithUnifiedPlan(); - caller->CreateDataChannel("dc"); - auto callee = CreatePeerConnectionWithUnifiedPlan(); - - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); - // Note that only the callee does ReportSdpFormatReceived. - EXPECT_METRIC_THAT( - metrics::Samples("WebRTC.PeerConnection.SdpFormatReceived"), - ElementsAre(Pair(kSdpFormatReceivedNoTracks, 1))); -} -#endif // WEBRTC_HAVE_SCTP - -TEST_F(SdpFormatReceivedTest, SimpleUnifiedPlanIsReportedAsSimple) { - auto caller = CreatePeerConnectionWithUnifiedPlan(); - caller->AddAudioTrack("audio"); - caller->AddVideoTrack("video"); - auto callee = CreatePeerConnectionWithPlanB(); - - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); - // Note that only the callee does ReportSdpFormatReceived. - EXPECT_METRIC_THAT( - metrics::Samples("WebRTC.PeerConnection.SdpFormatReceived"), - ElementsAre(Pair(kSdpFormatReceivedSimple, 1))); -} - -TEST_F(SdpFormatReceivedTest, SimplePlanBIsReportedAsSimple) { - auto caller = CreatePeerConnectionWithPlanB(); - caller->AddVideoTrack("video"); // Video only. - auto callee = CreatePeerConnectionWithUnifiedPlan(); - - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); - // Note that only the callee does ReportSdpFormatReceived. - EXPECT_METRIC_THAT( - metrics::Samples("WebRTC.PeerConnection.SdpFormatReceived"), - ElementsAre(Pair(kSdpFormatReceivedSimple, 1))); -} - -TEST_F(SdpFormatReceivedTest, ComplexUnifiedIsReportedAsComplexUnifiedPlan) { - auto caller = CreatePeerConnectionWithUnifiedPlan(); - caller->AddAudioTrack("audio1"); - caller->AddAudioTrack("audio2"); - caller->AddVideoTrack("video"); - auto callee = CreatePeerConnectionWithPlanB(); - - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); - // Note that only the callee does ReportSdpFormatReceived. - EXPECT_METRIC_THAT( - metrics::Samples("WebRTC.PeerConnection.SdpFormatReceived"), - ElementsAre(Pair(kSdpFormatReceivedComplexUnifiedPlan, 1))); -} - -TEST_F(SdpFormatReceivedTest, ComplexPlanBIsReportedAsComplexPlanB) { - auto caller = CreatePeerConnectionWithPlanB(); - caller->AddVideoTrack("video1"); - caller->AddVideoTrack("video2"); - auto callee = CreatePeerConnectionWithUnifiedPlan(); - - // This fails since Unified Plan cannot set a session description with - // multiple "Plan B tracks" in the same media section. But we still expect the - // SDP Format to be recorded. - ASSERT_FALSE(callee->SetRemoteDescription(caller->CreateOffer())); - // Note that only the callee does ReportSdpFormatReceived. - EXPECT_METRIC_THAT( - metrics::Samples("WebRTC.PeerConnection.SdpFormatReceived"), - ElementsAre(Pair(kSdpFormatReceivedComplexPlanB, 1))); -} - -TEST_F(SdpFormatReceivedTest, AnswerIsReported) { - auto caller = CreatePeerConnectionWithPlanB(); - caller->AddAudioTrack("audio"); - caller->AddVideoTrack("video"); - auto callee = CreatePeerConnectionWithUnifiedPlan(); - - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - ASSERT_TRUE(caller->SetRemoteDescription(callee->CreateAnswer())); - EXPECT_METRIC_THAT( - metrics::Samples("WebRTC.PeerConnection.SdpFormatReceivedAnswer"), - ElementsAre(Pair(kSdpFormatReceivedSimple, 1))); -} - // Sender setups in a call. TEST_P(PeerConnectionRtpTest, CreateTwoSendersWithSameTrack) { diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 35e65e0105..7b41fceb09 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -799,7 +799,6 @@ class SdpOfferAnswerHandler::RemoteDescriptionOperation { void ReportOfferAnswerUma() { RTC_DCHECK(ok()); if (type_ == SdpType::kOffer || type_ == SdpType::kAnswer) { - handler_->pc_->ReportSdpFormatReceived(*desc_.get()); handler_->pc_->ReportSdpBundleUsage(*desc_.get()); } } @@ -2200,8 +2199,6 @@ void SdpOfferAnswerHandler::DoSetLocalDescription( // MaybeStartGathering... context_->network_thread()->BlockingCall( [this] { port_allocator()->DiscardCandidatePool(); }); - // Make UMA notes about what was agreed to. - ReportNegotiatedSdpSemantics(*local_description()); } observer->OnSetLocalDescriptionComplete(RTCError::OK()); @@ -2404,8 +2401,6 @@ void SdpOfferAnswerHandler::SetRemoteDescriptionPostProcess(bool was_answer) { // MaybeStartGathering... context_->network_thread()->BlockingCall( [this] { port_allocator()->DiscardCandidatePool(); }); - // Make UMA notes about what was agreed to. - ReportNegotiatedSdpSemantics(*remote_description()); } pc_->NoteUsageEvent(UsageEvent::SET_REMOTE_DESCRIPTION_SUCCEEDED); @@ -4698,30 +4693,6 @@ void SdpOfferAnswerHandler::RemoveUnusedChannels( } } -void SdpOfferAnswerHandler::ReportNegotiatedSdpSemantics( - const SessionDescriptionInterface& answer) { - SdpSemanticNegotiated semantics_negotiated; - switch (answer.description()->msid_signaling()) { - case 0: - semantics_negotiated = kSdpSemanticNegotiatedNone; - break; - case cricket::kMsidSignalingMediaSection: - semantics_negotiated = kSdpSemanticNegotiatedUnifiedPlan; - break; - case cricket::kMsidSignalingSsrcAttribute: - semantics_negotiated = kSdpSemanticNegotiatedPlanB; - break; - case cricket::kMsidSignalingMediaSection | - cricket::kMsidSignalingSsrcAttribute: - semantics_negotiated = kSdpSemanticNegotiatedMixed; - break; - default: - RTC_DCHECK_NOTREACHED(); - } - RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.SdpSemanticNegotiated", - semantics_negotiated, kSdpSemanticNegotiatedMax); -} - void SdpOfferAnswerHandler::UpdateEndedRemoteMediaStreams() { RTC_DCHECK_RUN_ON(signaling_thread()); std::vector> streams_to_remove; diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h index bc5087c2b3..c493dc0229 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h @@ -491,10 +491,6 @@ class SdpOfferAnswerHandler : public SdpStateProvider { // `desc` can be null. This means that all channels are deleted. void RemoveUnusedChannels(const cricket::SessionDescription* desc); - // Report inferred negotiated SDP semantics from a local/remote answer to the - // UMA observer. - void ReportNegotiatedSdpSemantics(const SessionDescriptionInterface& answer); - // Finds remote MediaStreams without any tracks and removes them from // `remote_streams_` and notifies the observer that the MediaStreams no longer // exist. diff --git a/pc/test/fake_peer_connection_base.h b/pc/test/fake_peer_connection_base.h index 9ee69be993..e27705813d 100644 --- a/pc/test/fake_peer_connection_base.h +++ b/pc/test/fake_peer_connection_base.h @@ -294,9 +294,6 @@ class FakePeerConnectionBase : public PeerConnectionInternal { return nullptr; } - void ReportSdpFormatReceived( - const SessionDescriptionInterface& remote_description) override {} - void ReportSdpBundleUsage( const SessionDescriptionInterface& remote_description) override {} diff --git a/pc/test/mock_peer_connection_internal.h b/pc/test/mock_peer_connection_internal.h index 09b3d43247..733ec41961 100644 --- a/pc/test/mock_peer_connection_internal.h +++ b/pc/test/mock_peer_connection_internal.h @@ -202,10 +202,6 @@ class MockPeerConnectionInternal : public PeerConnectionInternal { configuration, (), (const, override)); - MOCK_METHOD(void, - ReportSdpFormatReceived, - (const SessionDescriptionInterface&), - (override)); MOCK_METHOD(void, ReportSdpBundleUsage, (const SessionDescriptionInterface&),