Bug 1432955: Implemented sdp parsing comparer, r=bwc,francois draft
authorJohannes Willbold <j.willbold@mozilla.com>
Thu, 05 Jul 2018 16:06:41 -0700
changeset 823192 ab9106915620f4bc0b8340cdec94d2e5c80966ae
parent 821749 fe48e26ca88c7919f0c075dc01d5f9fdccdb1260
push id117606
push userbmo:johannes.willbold@rub.de
push dateThu, 26 Jul 2018 20:02:50 +0000
reviewersbwc, francois
bugs1432955
milestone63.0a1
Bug 1432955: Implemented sdp parsing comparer, r=bwc,francois Added signaling/src/sdp/ParsingResultComparer.h/.cpp Added SDP comparer with telemetry and debug logging MozReview-Commit-ID: ErdnLGPxHHF
media/webrtc/signaling/gtest/sdp_unittests.cpp
media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp
media/webrtc/signaling/src/sdp/ParsingResultComparer.h
media/webrtc/signaling/src/sdp/moz.build
toolkit/components/telemetry/Scalars.yaml
--- a/media/webrtc/signaling/gtest/sdp_unittests.cpp
+++ b/media/webrtc/signaling/gtest/sdp_unittests.cpp
@@ -17,16 +17,17 @@
 #include "ssl.h"
 
 #include "nsThreadUtils.h"
 
 #include "signaling/src/sdp/RsdparsaSdpParser.h"
 #include "signaling/src/sdp/SipccSdpParser.h"
 #include "signaling/src/sdp/SdpMediaSection.h"
 #include "signaling/src/sdp/SdpAttribute.h"
+#include "signaling/src/sdp/ParsingResultComparer.h"
 
 extern "C" {
 #include "signaling/src/sdp/sipcc/sdp.h"
 #include "signaling/src/sdp/sipcc/sdp_private.h"
 }
 
 #ifdef CRLF
 #undef CRLF
@@ -4127,16 +4128,32 @@ TEST_P(NewSdpTest, CheckAddDataChannel) 
   ASSERT_TRUE(mediaSection.GetAttributeList().
               HasAttribute(SdpAttribute::kMaxMessageSizeAttribute));
   ASSERT_EQ(1800U, mediaSection.GetAttributeList().GetMaxMessageSize());
   ASSERT_TRUE(mediaSection.GetAttributeList().
               HasAttribute(SdpAttribute::kSctpPortAttribute));
   ASSERT_EQ(15000U, mediaSection.GetAttributeList().GetSctpPort());
 }
 
+TEST(NewSdpTestNoFixture, CheckParsingResultComparer) {
+  auto check_comparison = [] (const std::string sdp_string) {
+    SipccSdpParser sipccParser;
+    RsdparsaSdpParser rustParser;
+
+    auto sipccSdp = sipccParser.Parse(sdp_string);
+    auto rustSdp = rustParser.Parse(sdp_string);
+
+    ParsingResultComparer comparer;
+    return comparer.Compare(*rustSdp, *sipccSdp, sdp_string);
+  };
+
+  ASSERT_TRUE(check_comparison(kBasicAudioVideoOffer));
+  ASSERT_TRUE(check_comparison(kH264AudioVideoOffer));
+}
+
 TEST(NewSdpTestNoFixture, CheckAttributeTypeSerialize) {
   for (auto a = static_cast<size_t>(SdpAttribute::kFirstAttribute);
        a <= static_cast<size_t>(SdpAttribute::kLastAttribute);
        ++a) {
 
     SdpAttribute::AttributeType type =
       static_cast<SdpAttribute::AttributeType>(a);
 
--- a/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
+++ b/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
@@ -25,16 +25,17 @@
 
 #include "signaling/src/jsep/JsepTrack.h"
 #include "signaling/src/jsep/JsepTransport.h"
 #include "signaling/src/sdp/RsdparsaSdpParser.h"
 #include "signaling/src/sdp/Sdp.h"
 #include "signaling/src/sdp/SipccSdp.h"
 #include "signaling/src/sdp/SipccSdpParser.h"
 #include "mozilla/net/DataChannelProtocol.h"
+#include "signaling/src/sdp/ParsingResultComparer.h"
 
 namespace mozilla {
 
 MOZ_MTLOG_MODULE("jsep")
 
 #define JSEP_SET_ERROR(error)                                                  \
   do {                                                                         \
     std::ostringstream os;                                                     \
@@ -1283,16 +1284,18 @@ JsepSessionImpl::CopyPreviousMsid(const 
 }
 
 nsresult
 JsepSessionImpl::ParseSdp(const std::string& sdp, UniquePtr<Sdp>* parsedp)
 {
   UniquePtr<Sdp> parsed = mSipccParser.Parse(sdp);
   if (mRunRustParser) {
     UniquePtr<Sdp> rustParsed = mRsdparsaParser.Parse(sdp);
+    ParsingResultComparer comparer;
+    comparer.Compare(*rustParsed, *parsed, sdp);
   }
   if (!parsed) {
     std::string error = "Failed to parse SDP: ";
     mSdpHelper.appendSdpParseErrors(mSipccParser.GetParseErrors(), &error);
     JSEP_SET_ERROR(error);
     return NS_ERROR_INVALID_ARG;
   }
 
new file mode 100644
--- /dev/null
+++ b/media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp
@@ -0,0 +1,299 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include "signaling/src/sdp/Sdp.h"
+#include "signaling/src/sdp/ParsingResultComparer.h"
+
+#include <string>
+#include <ostream>
+#include <regex>
+
+#include "mozilla/Assertions.h"
+#include "mozilla/Telemetry.h"
+#include "mozilla/Logging.h"
+
+using mozilla::LogLevel;
+static mozilla::LazyLogModule sSdpDiffLogger("sdpdiff_logger");
+
+#define LOGD(msg) MOZ_LOG(sSdpDiffLogger, LogLevel::Debug, msg)
+
+
+namespace mozilla
+{
+
+using AttributeType = SdpAttribute::AttributeType;
+
+template<typename T>
+std::string ToString(const T& serializable)
+{
+  std::ostringstream os;
+
+  os << serializable;
+  return os.str();
+}
+
+bool
+ParsingResultComparer::Compare(const Sdp& rsdparsaSdp, const Sdp& sipccSdp,
+                               const std::string& originalSdp)
+{
+  bool result = true;
+  mOriginalSdp = originalSdp;
+
+  LOGD(("The original sdp: \n%s", mOriginalSdp.c_str()));
+
+  const std::string sipccSdpStr = sipccSdp.ToString();
+  const std::string rsdparsaSdpStr = rsdparsaSdp.ToString();
+
+  if (rsdparsaSdpStr == sipccSdpStr) {
+    Telemetry::ScalarAdd(Telemetry::ScalarID::WEBRTC_SDP_PARSER_DIFF,
+                         NS_LITERAL_STRING("serialization_is_equal"), 1);
+    LOGD(("Serialization is equal"));
+    return true;
+  }
+
+  Telemetry::ScalarAdd(Telemetry::ScalarID::WEBRTC_SDP_PARSER_DIFF,
+                       NS_LITERAL_STRING("serialization_is_not_equal"), 1);
+  LOGD(("Serialization is not equal\n"
+        " --- Sipcc SDP ---\n"
+        "%s\n"
+        "--- Rsdparsa SDP ---\n"
+        "%s\n",
+        sipccSdpStr.c_str(), rsdparsaSdpStr.c_str()));
+
+  const std::string rsdparsaOriginStr = ToString(rsdparsaSdp.GetOrigin());
+  const std::string sipccOriginStr = ToString(sipccSdp.GetOrigin());
+
+  // Compare the session level
+  if (rsdparsaOriginStr != sipccOriginStr) {
+    Telemetry::ScalarAdd(Telemetry::ScalarID::WEBRTC_SDP_PARSER_DIFF,
+                         NS_LITERAL_STRING("o="), 1);
+    LOGD(("origin is not equal\nrust origin: %s\nsipcc origin: %s",
+          rsdparsaOriginStr.c_str(),
+          sipccOriginStr.c_str()));
+    result = false;
+  }
+
+  if (MOZ_LOG_TEST(sSdpDiffLogger, LogLevel::Debug)) {
+    const auto rust_sess_attr_count = rsdparsaSdp.GetAttributeList().Count();
+    const auto sipcc_sess_attr_count = sipccSdp.GetAttributeList().Count();
+
+    if (rust_sess_attr_count != sipcc_sess_attr_count) {
+      LOGD(("Session level attribute count is NOT equal, rsdparsa: %u, "
+            "sipcc: %u\n", rust_sess_attr_count, sipcc_sess_attr_count));
+    }
+  }
+
+  result &= CompareAttrLists(rsdparsaSdp.GetAttributeList(),
+                             sipccSdp.GetAttributeList(),
+                             -1);
+
+  const uint32_t sipccMediaSecCount = static_cast<uint32_t>(
+                                            sipccSdp.GetMediaSectionCount());
+  const uint32_t rsdparsaMediaSecCount = static_cast<uint32_t>(
+                                            rsdparsaSdp.GetMediaSectionCount());
+
+  if (sipccMediaSecCount != rsdparsaMediaSecCount) {
+    Telemetry::ScalarAdd(Telemetry::ScalarID::WEBRTC_SDP_PARSER_DIFF,
+                         NS_LITERAL_STRING("inequal_msec_count"), 1);
+    LOGD(("Media section count is NOT equal, rsdparsa: %d, sipcc: %d \n",
+          rsdparsaMediaSecCount, sipccMediaSecCount));
+    result = false;
+  }
+
+  for (size_t i = 0; i < std::min(sipccMediaSecCount,
+                                  rsdparsaMediaSecCount); i++) {
+    result &= CompareMediaSections(rsdparsaSdp.GetMediaSection(i),
+                                   sipccSdp.GetMediaSection(i));
+  }
+
+  return result;
+}
+
+bool
+ParsingResultComparer::CompareMediaSections(const SdpMediaSection&
+                                              rustMediaSection,
+                                            const SdpMediaSection&
+                                              sipccMediaSection) const
+{
+  bool result = true;
+  auto trackMediaLineMismatch = [&result] (auto rustValue, auto sipccValue,
+                                         const nsString& valueDescription) {
+    nsString typeStr = NS_LITERAL_STRING("m=");
+    typeStr += valueDescription;
+    Telemetry::ScalarAdd(Telemetry::ScalarID::WEBRTC_SDP_PARSER_DIFF,
+                         typeStr, 1);
+    LOGD(("The media line values %s are not equal\n"
+          "rsdparsa value: %s\n"
+          "sipcc value: %s\n",
+          NS_LossyConvertUTF16toASCII(valueDescription).get(),
+          ToString(rustValue).c_str(),
+          ToString(sipccValue).c_str()));
+    result = false;
+  };
+
+  auto compareMediaLineValue = [trackMediaLineMismatch]
+                                (auto rustValue, auto sipccValue,
+                                const nsString& valueDescription) {
+    if (rustValue != sipccValue) {
+      trackMediaLineMismatch(rustValue, sipccValue, valueDescription);
+    }
+  };
+
+  auto compareSimpleMediaLineValue = [&rustMediaSection, &sipccMediaSection,
+                                      compareMediaLineValue]
+                                     (auto valGetFuncPtr,
+                                      const nsString& valueDescription) {
+    compareMediaLineValue((rustMediaSection.*valGetFuncPtr)(),
+                          (sipccMediaSection.*valGetFuncPtr)(),
+                           valueDescription);
+  };
+
+  compareSimpleMediaLineValue(&SdpMediaSection::GetMediaType,
+                              NS_LITERAL_STRING("media_type"));
+  compareSimpleMediaLineValue(&SdpMediaSection::GetPort,
+                              NS_LITERAL_STRING("port"));
+  compareSimpleMediaLineValue(&SdpMediaSection::GetPortCount,
+                              NS_LITERAL_STRING("port_count"));
+  compareSimpleMediaLineValue(&SdpMediaSection::GetProtocol,
+                              NS_LITERAL_STRING("protocol"));
+  compareSimpleMediaLineValue(&SdpMediaSection::IsReceiving,
+                              NS_LITERAL_STRING("is_receiving"));
+  compareSimpleMediaLineValue(&SdpMediaSection::IsSending,
+                              NS_LITERAL_STRING("is_sending"));
+  compareSimpleMediaLineValue(&SdpMediaSection::GetDirection,
+                              NS_LITERAL_STRING("direction"));
+  compareSimpleMediaLineValue(&SdpMediaSection::GetLevel,
+                              NS_LITERAL_STRING("level"));
+
+  compareMediaLineValue(ToString(rustMediaSection.GetConnection()),
+                        ToString(sipccMediaSection.GetConnection()),
+                        NS_LITERAL_STRING("connection"));
+
+  result &= CompareAttrLists(rustMediaSection.GetAttributeList(),
+                             sipccMediaSection.GetAttributeList(),
+                             static_cast<int>(rustMediaSection.GetLevel()));
+  return result;
+}
+
+bool
+ParsingResultComparer::CompareAttrLists(const SdpAttributeList& rustAttrlist,
+                                        const SdpAttributeList& sipccAttrlist,
+                                        int level) const
+{
+  bool result = true;
+
+  for (size_t i = AttributeType::kFirstAttribute; i <=
+       static_cast<size_t>(AttributeType::kLastAttribute); i++) {
+    const AttributeType type = static_cast<AttributeType>(i);
+    std::string attrStr;
+    if (type != AttributeType::kDirectionAttribute) {
+      attrStr = "a=" + SdpAttribute::GetAttributeTypeString(type);
+    } else {
+      attrStr = "a=_direction_attribute_";
+    }
+
+    if (sipccAttrlist.HasAttribute(type, false)) {
+      auto sipccAttrStr = ToString(*sipccAttrlist.GetAttribute(type, false));
+
+      if (!rustAttrlist.HasAttribute(type, false)) {
+        nsString typeStr;
+        typeStr.AssignASCII(attrStr.c_str());
+        typeStr += NS_LITERAL_STRING("_missing");
+        Telemetry::ScalarAdd(Telemetry::ScalarID::WEBRTC_SDP_PARSER_DIFF,
+                             typeStr, 1);
+        LOGD(("Rust is missing the attribute: %s\n", attrStr.c_str()));
+        LOGD(("Rust is missing: %s\n",sipccAttrStr.c_str()));
+
+        result = false;
+        continue;
+      }
+
+      auto rustAttrStr = ToString(*rustAttrlist.GetAttribute(type, false));
+
+      if (rustAttrStr != sipccAttrStr) {
+        std::string originalAttrStr = GetAttributeLines(attrStr, level);
+        if (rustAttrStr != originalAttrStr) {
+          nsString typeStr;
+          typeStr.AssignASCII(attrStr.c_str());
+          typeStr += NS_LITERAL_STRING("_inequal");
+          Telemetry::ScalarAdd(Telemetry::ScalarID::WEBRTC_SDP_PARSER_DIFF,
+                               typeStr, 1);
+          LOGD(("%s is neither equal to sipcc nor to the orginal sdp\n"
+                "--------------rsdparsa attribute---------------\n"
+                "%s"
+                "--------------sipcc attribute---------------\n"
+                "%s"
+                "--------------original attribute---------------\n"
+                "%s\n",
+                attrStr.c_str(), rustAttrStr.c_str(), sipccAttrStr.c_str(),
+                originalAttrStr.c_str()));
+          result = false;
+        } else {
+          LOGD(("But the rust serialization is equal to the orignal sdp\n"));
+        }
+      }
+    } else {
+      if (rustAttrlist.HasAttribute(type, false)) {
+        nsString typeStr;
+        typeStr.AssignASCII(attrStr.c_str());
+        typeStr += NS_LITERAL_STRING("_unexpected");
+        Telemetry::ScalarAdd(Telemetry::ScalarID::WEBRTC_SDP_PARSER_DIFF,
+                             typeStr, 1);
+      }
+    }
+  }
+
+  return result;
+}
+
+std::vector<std::string>
+SplitLines(const std::string& sdp)
+{
+    std::stringstream ss(sdp);
+    std::string to;
+    std::vector<std::string> lines;
+
+    while(std::getline(ss, to, '\n')) {
+      lines.push_back(to);
+    }
+
+    return lines;
+}
+
+std::string
+ParsingResultComparer::GetAttributeLines(const std::string& attrType,
+                                         int level) const
+{
+  std::vector<std::string> lines = SplitLines(mOriginalSdp);
+  std::string attrToFind = attrType + ":";
+  std::string attrLines;
+  int currentLevel = -1;
+  // Filters rtcp-fb lines that contain "x-..." types
+  // This is because every SDP from Edge contains these rtcp-fb x- types
+  // for example: a=rtcp-fb:121 x-foo
+  std::regex customRtcpFbLines("a\\=rtcp\\-fb\\:(\\d+|\\*).* x\\-.*");
+
+  for (auto line : lines) {
+
+    if (line.find("m=") == 0) {
+      if (level > currentLevel) {
+        attrLines.clear();
+        currentLevel++;
+      } else {
+        break;
+      }
+    } else if (line.find(attrToFind) == 0) {
+
+      if (std::regex_match(line, customRtcpFbLines)) {
+        continue;
+      }
+
+      attrLines += (line + '\n');
+    }
+  }
+
+  return attrLines;
+}
+
+}
new file mode 100644
--- /dev/null
+++ b/media/webrtc/signaling/src/sdp/ParsingResultComparer.h
@@ -0,0 +1,44 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=2 et sw=2 tw=80: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef _PARSINGRESULTCOMPARER_H_
+#define _PARSINGRESULTCOMPARER_H_
+
+#include <string>
+
+
+namespace mozilla
+{
+
+class Sdp;
+class SdpMediaSection;
+class SdpAttributeList;
+
+class ParsingResultComparer
+{
+public:
+
+  ParsingResultComparer() = default;
+
+  bool Compare(const Sdp& rsdparsaSdp, const Sdp& sipccSdp,
+               const std::string& originalSdp);
+  bool CompareMediaSections(const SdpMediaSection& rustMediaSection,
+                            const SdpMediaSection& sipccMediaSection) const;
+  bool CompareAttrLists(const SdpAttributeList& rustAttrlist,
+                        const SdpAttributeList& sipccAttrlist,
+                        int level) const;
+
+private:
+
+  std::string mOriginalSdp;
+
+  std::string GetAttributeLines(const std::string& attrType, int level) const;
+
+};
+
+} // namespace mozilla
+
+#endif // _PARSINGRESULTCOMPARER_H_
--- a/media/webrtc/signaling/src/sdp/moz.build
+++ b/media/webrtc/signaling/src/sdp/moz.build
@@ -25,16 +25,17 @@ include('/tools/fuzzing/libfuzzer-config
 LOCAL_INCLUDES += [
     '/media/mtransport',
     '/media/webrtc',
     '/media/webrtc/signaling/src/common/browser_logging',
     '/media/webrtc/trunk',
 ]
 
 UNIFIED_SOURCES += [
+    'ParsingResultComparer.cpp',
     'SdpAttribute.cpp',
     'SdpHelper.cpp',
     'SdpMediaSection.cpp',
     'SipccSdp.cpp',
     'SipccSdpAttributeList.cpp',
     'SipccSdpMediaSection.cpp',
     'SipccSdpParser.cpp',
 ]
--- a/toolkit/components/telemetry/Scalars.yaml
+++ b/toolkit/components/telemetry/Scalars.yaml
@@ -723,16 +723,37 @@ webrtc.nicer:
     notification_emails:
       - webrtc-ice-telemetry-alerts@mozilla.com
       - nohlmeier@mozilla.com
     release_channel_collection: opt-in
     record_in_processes:
       - 'main'
       - 'content'
 
+webrtc.sdp:
+  parser_diff:
+    bug_numbers:
+      - 1432955
+    description: >
+      The number of differences between the C based sipcc SDP parser and
+      the new rust based rsdparsa SDP parser keyed by predefined
+      names of attributes and values that do not match between
+      the sipcc parsing result and the rsdparsa parsing result or
+      between the rsdparsa parsing result and the original sdp.
+      This should help to improve the new rsdparsa to replace
+      the sipcc parser.
+    expires: "64"
+    kind: uint
+    keyed: true
+    notification_emails:
+      - nohlmeier@mozilla.com
+    release_channel_collection: opt-in
+    record_in_processes:
+      - 'content'
+
 mathml:
   doc_count:
     bug_numbers:
       - 1362187
     description: >
       The number of documents that contained enabled MathML elements.
     expires: "63"
     kind: uint