Bug 1481548: Added additonal comparison for fmtp r=bwc
authorNils Ohlmeier [:drno] <drno@ohlmeier.org>
Tue, 14 Aug 2018 00:36:23 +0000
changeset 828986 5db1348b915fa90b9b8b33f33a4867d1b7e0148f
parent 828985 aec3b4405809dd67ac64ca2dfde8c2349e8db97d
child 828987 302ce3cdb98fd24ebcf184fdba5eefe109b32e5b
push id118741
push userbmo:kshvmdn@gmail.com
push dateTue, 14 Aug 2018 18:31:47 +0000
reviewersbwc
bugs1481548
milestone63.0a1
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)));
   }