Bug 1481548: Added additonal comparison for fmtp r=bwc
authorNils Ohlmeier [:drno] <drno@ohlmeier.org>
Tue, 14 Aug 2018 00:36:23 +0000
changeset 486455 5db1348b915fa90b9b8b33f33a4867d1b7e0148f
parent 486454 aec3b4405809dd67ac64ca2dfde8c2349e8db97d
child 486456 302ce3cdb98fd24ebcf184fdba5eefe109b32e5b
push id9719
push userffxbld-merge
push dateFri, 24 Aug 2018 17:49:46 +0000
treeherdermozilla-beta@719ec98fba77 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbwc
bugs1481548
milestone63.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1481548: Added additonal comparison for fmtp r=bwc Added additional fmtp comparison for the parsing resutl comparer by implementing the C++ == operator for SdpFmtpAttributeList. Differential Revision: https://phabricator.services.mozilla.com/D3228
media/webrtc/signaling/gtest/sdp_unittests.cpp
media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp
media/webrtc/signaling/src/sdp/SdpAttribute.cpp
media/webrtc/signaling/src/sdp/SdpAttribute.h
--- a/media/webrtc/signaling/gtest/sdp_unittests.cpp
+++ b/media/webrtc/signaling/gtest/sdp_unittests.cpp
@@ -4142,16 +4142,83 @@ TEST(NewSdpTestNoFixture, CheckParsingRe
     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));
+
+  // Check the Fmtp comprison
+  const std::string kBasicOpusFmtp1 =
+  "v=0" CRLF
+  "o=Mozilla-SIPUA-35.0a1 5184 0 IN IP4 0.0.0.0" CRLF
+  "s=SIP Call" CRLF
+  "c=IN IP4 224.0.0.1/100/12" CRLF
+  "t=0 0" CRLF
+  "m=video 9 RTP/SAVPF 120" CRLF
+  "a=rtpmap:120 opus/48000" CRLF
+  "a=fmtp:120 stereo=1;useinbandfec=1" CRLF;
+  ASSERT_TRUE(check_comparison(kBasicOpusFmtp1));
+
+  const std::string kBasicOpusFmtp2 =
+  "v=0" CRLF
+  "o=Mozilla-SIPUA-35.0a1 5184 0 IN IP4 0.0.0.0" CRLF
+  "s=SIP Call" CRLF
+  "c=IN IP4 224.0.0.1/100/12" CRLF
+  "t=0 0" CRLF
+  "m=video 9 RTP/SAVPF 120" CRLF
+  "a=rtpmap:120 opus/48000" CRLF
+  "a=fmtp:120 useinbandfec=1;stereo=1;maxplaybackrate=32000" CRLF;
+  ASSERT_TRUE(check_comparison(kBasicOpusFmtp2));
+
+  const std::string kBasicVP8Fmtp1 =
+  "v=0" CRLF
+  "o=Mozilla-SIPUA-35.0a1 5184 0 IN IP4 0.0.0.0" CRLF
+  "s=SIP Call" CRLF
+  "c=IN IP4 224.0.0.1/100/12" CRLF
+  "t=0 0" CRLF
+  "m=video 9 RTP/SAVPF 120" CRLF
+  "a=rtpmap:120 VP8/90000" CRLF
+  "a=fmtp:120 max-fs=3600;max-fr=60" CRLF;
+  ASSERT_TRUE(check_comparison(kBasicVP8Fmtp1));
+  //
+  const std::string kBasicVP8Fmtp2 =
+  "v=0" CRLF
+  "o=Mozilla-SIPUA-35.0a1 5184 0 IN IP4 0.0.0.0" CRLF
+  "s=SIP Call" CRLF
+  "c=IN IP4 224.0.0.1/100/12" CRLF
+  "t=0 0" CRLF
+  "m=video 9 RTP/SAVPF 120" CRLF
+  "a=rtpmap:120 VP8/90000" CRLF
+  "a=fmtp:120 max-fr=60;max-fs=3600" CRLF;
+  ASSERT_TRUE(check_comparison(kBasicVP8Fmtp2));
+
+  const std::string kBasicH264Fmtp1 =
+  "v=0" CRLF
+  "o=Mozilla-SIPUA-35.0a1 5184 0 IN IP4 0.0.0.0" CRLF
+  "s=SIP Call" CRLF
+  "c=IN IP4 224.0.0.1/100/12" CRLF
+  "t=0 0" CRLF
+  "m=video 9 RTP/SAVPF 120" CRLF
+  "a=rtpmap:120 H264/90000" CRLF
+  "a=fmtp:120 profile-level-id=42a01e;level_asymmetry_allowed=1" CRLF;
+  ASSERT_TRUE(check_comparison(kBasicH264Fmtp1));
+
+  const std::string kBasicH264Fmtp2 =
+  "v=0" CRLF
+  "o=Mozilla-SIPUA-35.0a1 5184 0 IN IP4 0.0.0.0" CRLF
+  "s=SIP Call" CRLF
+  "c=IN IP4 224.0.0.1/100/12" CRLF
+  "t=0 0" CRLF
+  "m=video 9 RTP/SAVPF 120" CRLF
+  "a=rtpmap:120 H264/90000" CRLF
+  "a=fmtp:120 level_asymmetry_allowed=1;profile-level-id=42a01e;max_fs=3600" CRLF;
+  ASSERT_TRUE(check_comparison(kBasicH264Fmtp2));
 }
 
 TEST(NewSdpTestNoFixture, CheckAttributeTypeSerialize) {
   for (auto a = static_cast<size_t>(SdpAttribute::kFirstAttribute);
        a <= static_cast<size_t>(SdpAttribute::kLastAttribute);
        ++a) {
 
     SdpAttribute::AttributeType type =
--- a/media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp
+++ b/media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp
@@ -207,16 +207,23 @@ ParsingResultComparer::CompareAttrLists(
 
         result = false;
         continue;
       }
 
       auto rustAttrStr = ToString(*rustAttrlist.GetAttribute(type, false));
 
       if (rustAttrStr != sipccAttrStr) {
+
+        if (type == AttributeType::kFmtpAttribute) {
+          if (rustAttrlist.GetFmtp() == sipccAttrlist.GetFmtp()) {
+            continue;
+          }
+        }
+
         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"
--- a/media/webrtc/signaling/src/sdp/SdpAttribute.cpp
+++ b/media/webrtc/signaling/src/sdp/SdpAttribute.cpp
@@ -172,16 +172,22 @@ SdpFingerprintAttributeList::ParseFinger
       fp.clear(); // error
       return fp;
     }
     fp[fpIndex++] = high << 4 | low;
   }
   return fp;
 }
 
+bool
+SdpFmtpAttributeList::operator==(const SdpFmtpAttributeList& other) const
+{
+  return mFmtps == other.mFmtps;
+}
+
 void
 SdpFmtpAttributeList::Serialize(std::ostream& os) const
 {
   for (auto i = mFmtps.begin(); i != mFmtps.end(); ++i) {
     if (i->parameters) {
       os << "a=" << mType << ":" << i->format << " ";
       i->parameters->Serialize(os);
       os << CRLF;
--- a/media/webrtc/signaling/src/sdp/SdpAttribute.h
+++ b/media/webrtc/signaling/src/sdp/SdpAttribute.h
@@ -1278,16 +1278,23 @@ public:
     explicit Parameters(SdpRtpmapAttributeList::CodecType aCodec)
         : codec_type(aCodec)
     {
     }
 
     virtual ~Parameters() {}
     virtual Parameters* Clone() const = 0;
     virtual void Serialize(std::ostream& os) const = 0;
+    virtual bool CompareEq(const Parameters& other) const = 0;
+
+    bool operator==(const Parameters& other) const
+    {
+        return codec_type == other.codec_type &&
+               CompareEq(other);
+    }
 
     SdpRtpmapAttributeList::CodecType codec_type;
   };
 
   class RedParameters : public Parameters
   {
   public:
     RedParameters()
@@ -1305,16 +1312,22 @@ public:
     Serialize(std::ostream& os) const override
     {
       for(size_t i = 0; i < encodings.size(); ++i) {
         os << (i != 0 ? "/" : "")
            << std::to_string(encodings[i]);
       }
     }
 
+    virtual bool
+    CompareEq(const Parameters& other) const override
+    {
+      return encodings == static_cast<const RedParameters&>(other).encodings;
+    }
+
     std::vector<uint8_t> encodings;
   };
 
   class H264Parameters : public Parameters
   {
   public:
     static const uint32_t kDefaultProfileLevelId = 0x420010;
 
@@ -1372,16 +1385,32 @@ public:
         os << ";max-dpb=" << max_dpb;
       }
 
       if (max_br != 0) {
         os << ";max-br=" << max_br;
       }
     }
 
+    virtual bool
+    CompareEq(const Parameters& other) const override
+    {
+      const auto& otherH264 = static_cast<const H264Parameters&>(other);
+
+      // sprop is not comapred here as it does not get parsed in the rsdparsa
+      return packetization_mode == otherH264.packetization_mode &&
+             level_asymmetry_allowed == otherH264.level_asymmetry_allowed &&
+             profile_level_id == otherH264.profile_level_id &&
+             max_mbps == otherH264.max_mbps &&
+             max_fs == otherH264.max_fs &&
+             max_cpb == otherH264.max_cpb &&
+             max_dpb == otherH264.max_dpb &&
+             max_br == otherH264.max_br;
+    }
+
     static const size_t max_sprop_len = 128;
     char sprop_parameter_sets[max_sprop_len];
     unsigned int packetization_mode;
     bool level_asymmetry_allowed;
     unsigned int profile_level_id;
     unsigned int max_mbps;
     unsigned int max_fs;
     unsigned int max_cpb;
@@ -1408,16 +1437,25 @@ public:
     Serialize(std::ostream& os) const override
     {
       // draft-ietf-payload-vp8-11 says these are mandatory, upper layer
       // needs to ensure they're set properly.
       os << "max-fs=" << max_fs;
       os << ";max-fr=" << max_fr;
     }
 
+    virtual bool
+    CompareEq(const Parameters& other) const override
+    {
+      const auto& otherVP8 = static_cast<const VP8Parameters&>(other);
+
+      return max_fs == otherVP8.max_fs &&
+             max_fr == otherVP8.max_fr;
+    }
+
     unsigned int max_fs;
     unsigned int max_fr;
   };
 
   class OpusParameters : public Parameters
   {
   public:
     enum { kDefaultMaxPlaybackRate = 48000,
@@ -1439,16 +1477,35 @@ public:
     void
     Serialize(std::ostream& os) const override
     {
       os << "maxplaybackrate=" << maxplaybackrate
          << ";stereo=" << stereo
          << ";useinbandfec=" << useInBandFec;
     }
 
+    virtual bool
+    CompareEq(const Parameters& other) const override
+    {
+      const auto& otherOpus = static_cast<const OpusParameters&>(other);
+
+      bool maxplaybackrateIsEq = (maxplaybackrate == otherOpus.maxplaybackrate);
+
+      // This is due to a bug in sipcc that causes maxplaybackrate to
+      // always be 0 if it appears in the fmtp
+      if (((maxplaybackrate == 0) && (otherOpus.maxplaybackrate != 0)) ||
+          ((maxplaybackrate != 0) && (otherOpus.maxplaybackrate == 0))) {
+           maxplaybackrateIsEq = true;
+       }
+
+      return maxplaybackrateIsEq &&
+             stereo == otherOpus.stereo &&
+             useInBandFec == otherOpus.useInBandFec;
+    }
+
     unsigned int maxplaybackrate;
     unsigned int stereo;
     unsigned int useInBandFec;
   };
 
   class TelephoneEventParameters : public Parameters
   {
   public:
@@ -1464,16 +1521,23 @@ public:
     }
 
     void
     Serialize(std::ostream& os) const override
     {
       os << dtmfTones;
     }
 
+    virtual bool
+    CompareEq(const Parameters& other) const override
+    {
+      return dtmfTones == static_cast<const TelephoneEventParameters&>(other)
+                          .dtmfTones;
+    }
+
     std::string dtmfTones;
   };
 
   class Fmtp
   {
   public:
     Fmtp(const std::string& aFormat, UniquePtr<Parameters> aParameters)
         : format(aFormat),
@@ -1494,27 +1558,35 @@ public:
     {
       if (this != &rhs) {
         format = rhs.format;
         parameters.reset(rhs.parameters ? rhs.parameters->Clone() : nullptr);
       }
       return *this;
     }
 
+    bool operator==(const Fmtp& other) const
+    {
+      return format == other.format &&
+             *parameters == *other.parameters;
+    }
+
     // The contract around these is as follows:
     // * |parameters| is only set if we recognized the media type and had
     //   a subclass of Parameters to represent that type of parameters
     // * |parameters| is a best-effort representation; it might be missing
     //   stuff
     // * Parameters::codec_type tells you the concrete class, eg
     //   kH264 -> H264Parameters
     std::string format;
     UniquePtr<Parameters> parameters;
   };
 
+  bool operator==(const SdpFmtpAttributeList& other) const;
+
   virtual void Serialize(std::ostream& os) const override;
 
   void
   PushEntry(const std::string& format, UniquePtr<Parameters> parameters)
   {
     mFmtps.push_back(Fmtp(format, std::move(parameters)));
   }