Add UKM metrics for usage of complex SDP in RTCPeerConnection

This metric find cases of Web sites that use complex SDP.
Complex SDP can lead to errors if the application assumes the wrong
default SDP format. Tracking this is important during the transition
of the default SDP format from Plan B to Unified Plan.

Review: https://docs.google.com/document/d/1kiUze0n2yJGK2_TVhyKoGbER-xFlwZZaLLrjy0w4dek/edit


Bug: 900951
Change-Id: Ic0eb065e73f2e7578f883b75477193e0019fcc62
Reviewed-on: https://chromium-review.googlesource.com/c/1347364
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Henrik Boström <hbos@chromium.org>
Reviewed-by: Harald Alvestrand <hta@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612092}

third_party/blink/renderer/modules/peerconnection/BUILD.gn

@@ -60,6 +60,8 @@ blink_modules_sources("peerconnection") {
     "rtc_legacy_stats_report.h",
     "rtc_peer_connection.cc",
     "rtc_peer_connection.h",
+    "rtc_peer_connection_controller.cc",
+    "rtc_peer_connection_controller.h",
     "rtc_peer_connection_ice_event.cc",
     "rtc_peer_connection_ice_event.h",
     "rtc_quic_stream.cc",

third_party/blink/renderer/modules/peerconnection/rtc_peer_connection.cc

@@ -996,18 +996,49 @@ DOMException* RTCPeerConnection::checkSdpForStateErrors(
   return nullptr;
 }
 
-bool RTCPeerConnection::ShouldShowComplexPlanBSdpWarning(
+base::Optional<ComplexSdpCategory> RTCPeerConnection::CheckForComplexSdp(
     const RTCSessionDescriptionInit* session_description_init) const {
-  if (sdp_semantics_specified_)
-    return false;
   if (!session_description_init->hasType() ||
       !session_description_init->hasSdp())
-    return false;
-  auto sdp_format = DeduceSdpFormat(session_description_init->type(),
-                                    session_description_init->sdp());
-  if (!sdp_format)
-    return false;
-  return *sdp_format == SdpFormat::kComplexPlanB;
+    return base::nullopt;
+
+  base::Optional<SdpFormat> sdp_format = DeduceSdpFormat(
+      session_description_init->type(), session_description_init->sdp());
+  if (!sdp_format) {
+    return sdp_semantics_specified_
+               ? ComplexSdpCategory::kErrorExplicitSemantics
+               : ComplexSdpCategory::kErrorImplicitSemantics;
+  }
+
+  if (*sdp_format == SdpFormat::kComplexPlanB) {
+    return sdp_semantics_specified_
+               ? ComplexSdpCategory::kPlanBExplicitSemantics
+               : ComplexSdpCategory::kPlanBImplicitSemantics;
+  } else if (*sdp_format == SdpFormat::kComplexUnifiedPlan) {
+    return sdp_semantics_specified_
+               ? ComplexSdpCategory::kUnifiedPlanExplicitSemantics
+               : ComplexSdpCategory::kUnifiedPlanImplicitSemantics;
+  }
+
+  return base::nullopt;
+}
+
+void RTCPeerConnection::MaybeWarnAboutUnsafeSdp(
+    const RTCSessionDescriptionInit* session_description_init) const {
+  base::Optional<ComplexSdpCategory> complex_sdp_category =
+      CheckForComplexSdp(session_description_init);
+  if (!complex_sdp_category)
+    return;
+
+  Document* document = To<Document>(GetExecutionContext());
+  RTCPeerConnectionController::From(*document).MaybeReportComplexSdp(
+      *complex_sdp_category);
+
+  if (*complex_sdp_category == ComplexSdpCategory::kPlanBImplicitSemantics) {
+    Deprecation::CountDeprecation(
+        GetExecutionContext(),
+        WebFeature::kRTCPeerConnectionComplexPlanBSdpUsingDefaultSdpSemantics);
+  }
 }
 
 const CallSetupStateTracker& RTCPeerConnection::call_setup_state_tracker()
@@ -1137,11 +1168,7 @@ void RTCPeerConnection::ReportSetSdpUsage(
 ScriptPromise RTCPeerConnection::setLocalDescription(
     ScriptState* script_state,
     const RTCSessionDescriptionInit* session_description_init) {
-  if (ShouldShowComplexPlanBSdpWarning(session_description_init)) {
-    Deprecation::CountDeprecation(
-        GetExecutionContext(),
-        WebFeature::kRTCPeerConnectionComplexPlanBSdpUsingDefaultSdpSemantics);
-  }
+  MaybeWarnAboutUnsafeSdp(session_description_init);
   ReportSetSdpUsage(SetSdpOperationType::kSetLocalDescription,
                     session_description_init);
   String sdp;
@@ -1168,11 +1195,7 @@ ScriptPromise RTCPeerConnection::setLocalDescription(
     const RTCSessionDescriptionInit* session_description_init,
     V8VoidFunction* success_callback,
     V8RTCPeerConnectionErrorCallback* error_callback) {
-  if (ShouldShowComplexPlanBSdpWarning(session_description_init)) {
-    Deprecation::CountDeprecation(
-        GetExecutionContext(),
-        WebFeature::kRTCPeerConnectionComplexPlanBSdpUsingDefaultSdpSemantics);
-  }
+  MaybeWarnAboutUnsafeSdp(session_description_init);
   ReportSetSdpUsage(SetSdpOperationType::kSetLocalDescription,
                     session_description_init);
   ExecutionContext* context = ExecutionContext::From(script_state);
@@ -1245,11 +1268,7 @@ RTCSessionDescription* RTCPeerConnection::pendingLocalDescription() {
 ScriptPromise RTCPeerConnection::setRemoteDescription(
     ScriptState* script_state,
     const RTCSessionDescriptionInit* session_description_init) {
-  if (ShouldShowComplexPlanBSdpWarning(session_description_init)) {
-    Deprecation::CountDeprecation(
-        GetExecutionContext(),
-        WebFeature::kRTCPeerConnectionComplexPlanBSdpUsingDefaultSdpSemantics);
-  }
+  MaybeWarnAboutUnsafeSdp(session_description_init);
   ReportSetSdpUsage(SetSdpOperationType::kSetRemoteDescription,
                     session_description_init);
   if (signaling_state_ ==
@@ -1278,11 +1297,7 @@ ScriptPromise RTCPeerConnection::setRemoteDescription(
     const RTCSessionDescriptionInit* session_description_init,
     V8VoidFunction* success_callback,
     V8RTCPeerConnectionErrorCallback* error_callback) {
-  if (ShouldShowComplexPlanBSdpWarning(session_description_init)) {
-    Deprecation::CountDeprecation(
-        GetExecutionContext(),
-        WebFeature::kRTCPeerConnectionComplexPlanBSdpUsingDefaultSdpSemantics);
-  }
+  MaybeWarnAboutUnsafeSdp(session_description_init);
   ReportSetSdpUsage(SetSdpOperationType::kSetRemoteDescription,
                     session_description_init);
   ExecutionContext* context = ExecutionContext::From(script_state);

third_party/blink/renderer/modules/peerconnection/rtc_peer_connection.h

@@ -44,6 +44,7 @@
 #include "third_party/blink/renderer/modules/mediastream/media_stream.h"
 #include "third_party/blink/renderer/modules/peerconnection/call_setup_state_tracker.h"
 #include "third_party/blink/renderer/modules/peerconnection/rtc_ice_candidate.h"
+#include "third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_controller.h"
 #include "third_party/blink/renderer/modules/peerconnection/rtc_rtp_transceiver.h"
 #include "third_party/blink/renderer/modules/peerconnection/rtc_session_description_enums.h"
 #include "third_party/blink/renderer/platform/async_method_runner.h"
@@ -92,7 +93,7 @@ enum class SdpUsageCategory {
   kMaxValue = kUnknown,
 };
 
-SdpUsageCategory MODULES_EXPORT
+MODULES_EXPORT SdpUsageCategory
 DeduceSdpUsageCategory(const String& sdp_type,
                        const String& sdp,
                        bool sdp_semantics_specified,
@@ -302,17 +303,15 @@ class MODULES_EXPORT RTCPeerConnection final
   static int PeerConnectionCount();
   static int PeerConnectionCountLimit();
 
-  // SLD/SRD helper method, public for testing.
-  // "Complex" Plan B SDP is SDP that is not compatible with Unified Plan, i.e.
-  // SDP that has multiple tracks listed under the same m= sections. We should
-  // show a deprecation warning when setLocalDescription() or
-  // setRemoteDescription() is called and:
-  // - The SDP is complex Plan B SDP.
-  // - sdpSemantics was not specified at RTCPeerConnection construction.
-  // Such calls would normally succeed, but as soon as the default switches to
-  // Unified Plan they would fail. This decides whether to show deprecation for
-  // WebFeature::kRTCPeerConnectionComplexPlanBSdpUsingDefaultSdpSemantics.
-  bool ShouldShowComplexPlanBSdpWarning(const RTCSessionDescriptionInit*) const;
+  // SLD/SRD Helper method, public for testing.
+  // This function returns a value that indicates if complex SDP is being used
+  // and whether a format is explicitly specified. If the SDP is not complex or
+  // it could not be parsed, base::nullopt is returned.
+  // When "Complex" SDP (i.e., SDP that has multiple tracks) is used without
+  // explicitly specifying the SDP format, there may be errors if the
+  // application assumes a format that differs from the actual default format.
+  base::Optional<ComplexSdpCategory> CheckForComplexSdp(
+      const RTCSessionDescriptionInit* session_description_init) const;
 
   const CallSetupStateTracker& call_setup_state_tracker() const;
   void NoteCallSetupStateEventPending(
@@ -470,6 +469,8 @@ class MODULES_EXPORT RTCPeerConnection final
   DOMException* checkSdpForStateErrors(ExecutionContext*,
                                        const RTCSessionDescriptionInit*,
                                        String* sdp);
+  void MaybeWarnAboutUnsafeSdp(
+      const RTCSessionDescriptionInit* session_description_init) const;
 
   webrtc::PeerConnectionInterface::SignalingState signaling_state_;
   webrtc::PeerConnectionInterface::IceGatheringState ice_gathering_state_;

third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_controller.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_controller.h"
+
+#include "services/metrics/public/cpp/ukm_builders.h"
+
+namespace blink {
+
+// static
+const char RTCPeerConnectionController::kSupplementName[] =
+    "RTCPeerConnectionController";
+
+// static
+RTCPeerConnectionController& RTCPeerConnectionController::From(
+    Document& document) {
+  RTCPeerConnectionController* supplement =
+      Supplement<Document>::From<RTCPeerConnectionController>(document);
+  if (!supplement) {
+    supplement = new RTCPeerConnectionController(document);
+    Supplement<Document>::ProvideTo(document, supplement);
+  }
+  return *supplement;
+}
+
+RTCPeerConnectionController::RTCPeerConnectionController(Document& document)
+    : Supplement<Document>(document) {}
+
+void RTCPeerConnectionController::MaybeReportComplexSdp(
+    ComplexSdpCategory complex_sdp_category) {
+  if (has_reported_ukm_)
+    return;
+
+  // Report only the first observation for the document and ignore all others.
+  // This provides a good balance between privacy and meaningful metrics.
+  has_reported_ukm_ = true;
+  ukm::SourceId source_id = GetSupplementable()->UkmSourceID();
+  ukm::builders::WebRTC_ComplexSdp(source_id)
+      .SetCategory(static_cast<int64_t>(complex_sdp_category))
+      .Record(GetSupplementable()->UkmRecorder());
+}
+
+void RTCPeerConnectionController::Trace(Visitor* visitor) {
+  Supplement<Document>::Trace(visitor);
+}
+
+}  // namespace blink

third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_controller.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef THIRD_PARTY_BLINK_RENDERER_MODULES_PEERCONNECTION_RTC_PEER_CONNECTION_CONTROLLER_H_
+#define THIRD_PARTY_BLINK_RENDERER_MODULES_PEERCONNECTION_RTC_PEER_CONNECTION_CONTROLLER_H_
+
+#include "third_party/blink/renderer/core/dom/document.h"
+#include "third_party/blink/renderer/platform/heap/handle.h"
+#include "third_party/blink/renderer/platform/supplementable.h"
+
+namespace blink {
+
+// This enum is used to track usage of Complex SDP with UKM.
+// The prefix indicates the SDP format used (PlanB or UnifiedPlan) or error if
+// the SDP could not be parsed.
+// The suffix indicates whether the peer connection has been configured with
+// explicit SDP semantics (ExplicitSemantics) or not (ImplicitSemantics). The
+// SDP format used does not necessarily match the semantics with which the peer
+// connection has been configured.
+enum class ComplexSdpCategory {
+  kPlanBImplicitSemantics = 0,
+  kPlanBExplicitSemantics = 1,
+  kUnifiedPlanImplicitSemantics = 2,
+  kUnifiedPlanExplicitSemantics = 3,
+  kErrorImplicitSemantics = 4,
+  kErrorExplicitSemantics = 5,
+};
+
+class RTCPeerConnectionController
+    : public GarbageCollected<RTCPeerConnectionController>,
+      public Supplement<Document> {
+  USING_GARBAGE_COLLECTED_MIXIN(RTCPeerConnectionController);
+
+ public:
+  static const char kSupplementName[];
+  static RTCPeerConnectionController& From(Document&);
+
+  void MaybeReportComplexSdp(ComplexSdpCategory);
+
+  void Trace(Visitor*) override;
+
+ private:
+  explicit RTCPeerConnectionController(Document&);
+
+  bool has_reported_ukm_ = false;
+};
+
+}  // namespace blink
+
+#endif  // THIRD_PARTY_BLINK_RENDERER_MODULES_PEERCONNECTION_RTC_PEER_CONNECTION_CONTROLLER_H_

third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_test.cc

@@ -591,78 +591,76 @@ TEST_F(RTCPeerConnectionTest, GetTrackRemoveStreamAndGCWithPersistentStream) {
   EXPECT_FALSE(pc->GetTrack(track_component));
 }
 
-TEST_F(RTCPeerConnectionTest, PlanBSdpWarningNotShownWhenPlanBSpecified) {
+TEST_F(RTCPeerConnectionTest, CheckForComplexSdpWithSdpSemanticsPlanB) {
   V8TestingScope scope;
   Persistent<RTCPeerConnection> pc = CreatePC(scope, "plan-b");
   RTCSessionDescriptionInit* sdp = RTCSessionDescriptionInit::Create();
   sdp->setType("offer");
-  // It doesn't matter the SDP, never show a warning if sdpSemantics was
-  // specified at construction.
-  sdp->setSdp(kOfferSdpUnifiedPlanSingleAudioSingleVideo);
-  ASSERT_FALSE(pc->ShouldShowComplexPlanBSdpWarning(sdp));
   sdp->setSdp(kOfferSdpUnifiedPlanMultipleAudioTracks);
-  ASSERT_FALSE(pc->ShouldShowComplexPlanBSdpWarning(sdp));
-  sdp->setSdp(kOfferSdpPlanBSingleAudioSingleVideo);
-  ASSERT_FALSE(pc->ShouldShowComplexPlanBSdpWarning(sdp));
+  ASSERT_TRUE(pc->CheckForComplexSdp(sdp).has_value());
+  ASSERT_EQ(pc->CheckForComplexSdp(sdp),
+            ComplexSdpCategory::kUnifiedPlanExplicitSemantics);
   sdp->setSdp(kOfferSdpPlanBMultipleAudioTracks);
-  ASSERT_FALSE(pc->ShouldShowComplexPlanBSdpWarning(sdp));
+  ASSERT_TRUE(pc->CheckForComplexSdp(sdp).has_value());
+  ASSERT_EQ(pc->CheckForComplexSdp(sdp),
+            ComplexSdpCategory::kPlanBExplicitSemantics);
+  sdp->setSdp("invalid sdp");
+  ASSERT_TRUE(pc->CheckForComplexSdp(sdp).has_value());
+  ASSERT_EQ(pc->CheckForComplexSdp(sdp),
+            ComplexSdpCategory::kErrorExplicitSemantics);
+  // No Complex SDP is detected if only a single track per m= section is used.
+  sdp->setSdp(kOfferSdpUnifiedPlanSingleAudioSingleVideo);
+  ASSERT_FALSE(pc->CheckForComplexSdp(sdp).has_value());
+  sdp->setSdp(kOfferSdpPlanBSingleAudioSingleVideo);
+  ASSERT_FALSE(pc->CheckForComplexSdp(sdp).has_value());
 }
 
-TEST_F(RTCPeerConnectionTest, PlanBSdpWarningNotShownWhenUnifiedPlanSpecified) {
+TEST_F(RTCPeerConnectionTest, CheckForComplexSdpWithSdpSemanticsUnifiedPlan) {
   V8TestingScope scope;
   Persistent<RTCPeerConnection> pc = CreatePC(scope, "unified-plan");
   RTCSessionDescriptionInit* sdp = RTCSessionDescriptionInit::Create();
   sdp->setType("offer");
-  // It doesn't matter the SDP, never show a warning if sdpSemantics was
-  // specified at construction.
-  sdp->setSdp(kOfferSdpUnifiedPlanSingleAudioSingleVideo);
-  ASSERT_FALSE(pc->ShouldShowComplexPlanBSdpWarning(sdp));
   sdp->setSdp(kOfferSdpUnifiedPlanMultipleAudioTracks);
-  ASSERT_FALSE(pc->ShouldShowComplexPlanBSdpWarning(sdp));
-  sdp->setSdp(kOfferSdpPlanBSingleAudioSingleVideo);
-  ASSERT_FALSE(pc->ShouldShowComplexPlanBSdpWarning(sdp));
+  ASSERT_TRUE(pc->CheckForComplexSdp(sdp).has_value());
+  ASSERT_EQ(pc->CheckForComplexSdp(sdp),
+            ComplexSdpCategory::kUnifiedPlanExplicitSemantics);
   sdp->setSdp(kOfferSdpPlanBMultipleAudioTracks);
-  ASSERT_FALSE(pc->ShouldShowComplexPlanBSdpWarning(sdp));
-}
-
-TEST_F(RTCPeerConnectionTest, PlanBSdpWarningNotShownWhenInvalidSdp) {
-  V8TestingScope scope;
-  Persistent<RTCPeerConnection> pc = CreatePC(scope);
-  RTCSessionDescriptionInit* sdp = RTCSessionDescriptionInit::Create();
-  sdp->setType("offer");
+  ASSERT_TRUE(pc->CheckForComplexSdp(sdp).has_value());
+  ASSERT_EQ(pc->CheckForComplexSdp(sdp),
+            ComplexSdpCategory::kPlanBExplicitSemantics);
   sdp->setSdp("invalid sdp");
-  ASSERT_FALSE(pc->ShouldShowComplexPlanBSdpWarning(sdp));
-}
-
-TEST_F(RTCPeerConnectionTest, PlanBSdpWarningNotShownForSingleTracks) {
-  V8TestingScope scope;
-  Persistent<RTCPeerConnection> pc = CreatePC(scope);
-  RTCSessionDescriptionInit* sdp = RTCSessionDescriptionInit::Create();
-  sdp->setType("offer");
-  // Neither Unified Plan or Plan B SDP should result in a warning if only a
-  // single track per m= section is used.
+  ASSERT_TRUE(pc->CheckForComplexSdp(sdp).has_value());
+  ASSERT_EQ(pc->CheckForComplexSdp(sdp),
+            ComplexSdpCategory::kErrorExplicitSemantics);
+  // No Complex SDP is detected if only a single track per m= section is used.
   sdp->setSdp(kOfferSdpUnifiedPlanSingleAudioSingleVideo);
-  ASSERT_FALSE(pc->ShouldShowComplexPlanBSdpWarning(sdp));
+  ASSERT_FALSE(pc->CheckForComplexSdp(sdp).has_value());
   sdp->setSdp(kOfferSdpPlanBSingleAudioSingleVideo);
-  ASSERT_FALSE(pc->ShouldShowComplexPlanBSdpWarning(sdp));
+  ASSERT_FALSE(pc->CheckForComplexSdp(sdp).has_value());
 }
 
-TEST_F(RTCPeerConnectionTest, PlanBSdpWarningShownForComplexPlanB) {
+TEST_F(RTCPeerConnectionTest, CheckForComplexSdpWithSdpSemanticsUnspecified) {
   V8TestingScope scope;
   Persistent<RTCPeerConnection> pc = CreatePC(scope);
   RTCSessionDescriptionInit* sdp = RTCSessionDescriptionInit::Create();
   sdp->setType("offer");
   sdp->setSdp(kOfferSdpPlanBMultipleAudioTracks);
-  ASSERT_TRUE(pc->ShouldShowComplexPlanBSdpWarning(sdp));
-}
-
-TEST_F(RTCPeerConnectionTest, PlanBSdpWarningNotShownForComplexUnifiedPlan) {
-  V8TestingScope scope;
-  Persistent<RTCPeerConnection> pc = CreatePC(scope);
-  RTCSessionDescriptionInit* sdp = RTCSessionDescriptionInit::Create();
-  sdp->setType("offer");
+  ASSERT_TRUE(pc->CheckForComplexSdp(sdp).has_value());
+  ASSERT_EQ(pc->CheckForComplexSdp(sdp),
+            ComplexSdpCategory::kPlanBImplicitSemantics);
   sdp->setSdp(kOfferSdpUnifiedPlanMultipleAudioTracks);
-  ASSERT_FALSE(pc->ShouldShowComplexPlanBSdpWarning(sdp));
+  ASSERT_TRUE(pc->CheckForComplexSdp(sdp).has_value());
+  ASSERT_EQ(pc->CheckForComplexSdp(sdp),
+            ComplexSdpCategory::kUnifiedPlanImplicitSemantics);
+  sdp->setSdp("invalid sdp");
+  ASSERT_TRUE(pc->CheckForComplexSdp(sdp).has_value());
+  ASSERT_EQ(pc->CheckForComplexSdp(sdp),
+            ComplexSdpCategory::kErrorImplicitSemantics);
+  // No Complex SDP is detected if only a single track per m= section is used.
+  sdp->setSdp(kOfferSdpUnifiedPlanSingleAudioSingleVideo);
+  ASSERT_FALSE(pc->CheckForComplexSdp(sdp).has_value());
+  sdp->setSdp(kOfferSdpPlanBSingleAudioSingleVideo);
+  ASSERT_FALSE(pc->CheckForComplexSdp(sdp).has_value());
 }
 
 enum class AsyncOperationAction {

tools/metrics/ukm/ukm.xml

@@ -5634,6 +5634,24 @@ be describing additional metrics about the same event.
   </metric>
 </event>
 
+<event name="WebRTC.ComplexSdp">
+  <owner>hbos@chromium.org</owner>
+  <owner>guidou@chromium.org</owner>
+  <summary>
+    Logged when an application uses complex SDP in a WebRTC PeerConnection or
+    experiences an error attempting to parse SDP. Such cases will result in
+    errors if the application assumes the wrong default SDP format. This is an
+    important concern during the transition of the default SDP format from Plan
+    B to Unified Plan.
+  </summary>
+  <metric name="Category">
+    <summary>
+      A value that represents how complex/unparsable SDP is used. The categories
+      are defined in blink::ComplexSdpCategory.
+    </summary>
+  </metric>
+</event>
+
 <event name="IOS.FindInPageSearchMatches">
   <owner>thegreenfrog@chromium.org</owner>
   <owner>michaeldo@chromium.org</owner>