Bug 1288105 - Part 1: Do not set recv tracks' payload types based on the remote SDP. Some related simplifications/fixes. r=mjf
authorByron Campen [:bwc] <docfaraday@gmail.com>
Fri, 05 Apr 2019 17:23:45 +0000
changeset 468370 f06d1670906984bff44e9b1c74ed0d89d519cde8
parent 468369 e7003b05592a69c0fe6e2ee1aee41111abba504a
child 468371 9fcab041c5d73b9525b22afde2ad658574965992
push id35835
push useraciure@mozilla.com
push dateMon, 08 Apr 2019 19:00:29 +0000
treeherdermozilla-central@40456af7da1c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmjf
bugs1288105
milestone68.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 1288105 - Part 1: Do not set recv tracks' payload types based on the remote SDP. Some related simplifications/fixes. r=mjf Differential Revision: https://phabricator.services.mozilla.com/D26238
media/webrtc/signaling/src/jsep/JsepCodecDescription.h
media/webrtc/signaling/src/jsep/JsepTrack.cpp
media/webrtc/signaling/src/jsep/JsepTrack.h
--- a/media/webrtc/signaling/src/jsep/JsepCodecDescription.h
+++ b/media/webrtc/signaling/src/jsep/JsepCodecDescription.h
@@ -63,25 +63,26 @@ class JsepCodecDescription {
   }
 
   virtual bool ParametersMatch(const std::string& fmt,
                                const SdpMediaSection& remoteMsection) const {
     return true;
   }
 
   virtual bool Negotiate(const std::string& pt,
-                         const SdpMediaSection& remoteMsection) {
-    mDefaultPt = pt;
+                         const SdpMediaSection& remoteMsection, bool isOffer) {
+    if (mDirection == sdp::kSend || isOffer) {
+      mDefaultPt = pt;
+    }
     return true;
   }
 
   virtual void AddToMediaSection(SdpMediaSection& msection) const {
     if (mEnabled && msection.GetMediaType() == mType) {
-      // Both send and recv codec will have the same pt, so don't add twice
-      if (!msection.HasFormat(mDefaultPt)) {
+      if (mDirection == sdp::kRecv) {
         msection.AddCodec(mDefaultPt, mName, mClock, mChannels);
       }
 
       AddParametersToMSection(msection);
     }
   }
 
   virtual void AddParametersToMSection(SdpMediaSection& msection) const {}
@@ -162,19 +163,19 @@ class JsepAudioCodecDescription : public
     } else if (mName == "telephone-event") {
       // add the default dtmf tones
       SdpFmtpAttributeList::TelephoneEventParameters teParams(
           GetTelephoneEventParameters(mDefaultPt, msection));
       msection.SetFmtp(SdpFmtpAttributeList::Fmtp(mDefaultPt, teParams));
     }
   }
 
-  bool Negotiate(const std::string& pt,
-                 const SdpMediaSection& remoteMsection) override {
-    JsepCodecDescription::Negotiate(pt, remoteMsection);
+  bool Negotiate(const std::string& pt, const SdpMediaSection& remoteMsection,
+                 bool isOffer) override {
+    JsepCodecDescription::Negotiate(pt, remoteMsection, isOffer);
     if (mName == "opus" && mDirection == sdp::kSend) {
       SdpFmtpAttributeList::OpusParameters opusParams(
           GetOpusParameters(mDefaultPt, remoteMsection));
 
       mMaxPlaybackRate = opusParams.maxplaybackrate;
       mForceMono = !opusParams.stereo;
       // draft-ietf-rtcweb-fec-03.txt section 4.2 says support for FEC
       // at the received side is declarative and can be negotiated
@@ -399,18 +400,19 @@ class JsepVideoCodecDescription : public
     // Removes rtcp-fb types that the other side doesn't support
     NegotiateRtcpFb(remote, SdpRtcpFbAttributeList::kAck, &mAckFbTypes);
     NegotiateRtcpFb(remote, SdpRtcpFbAttributeList::kNack, &mNackFbTypes);
     NegotiateRtcpFb(remote, SdpRtcpFbAttributeList::kCcm, &mCcmFbTypes);
     NegotiateRtcpFb(remote, &mOtherFbTypes);
   }
 
   virtual bool Negotiate(const std::string& pt,
-                         const SdpMediaSection& remoteMsection) override {
-    JsepCodecDescription::Negotiate(pt, remoteMsection);
+                         const SdpMediaSection& remoteMsection,
+                         bool isOffer) override {
+    JsepCodecDescription::Negotiate(pt, remoteMsection, isOffer);
     if (mName == "H264") {
       SdpFmtpAttributeList::H264Parameters h264Params(
           GetH264Parameters(mDefaultPt, remoteMsection));
 
       // Level is negotiated symmetrically if level asymmetry is disallowed
       if (!h264Params.level_asymmetry_allowed) {
         SetSaneH264Level(std::min(GetSaneH264Level(h264Params.profile_level_id),
                                   GetSaneH264Level(mProfileLevelId)),
@@ -722,28 +724,28 @@ class JsepApplicationCodecDescription : 
       return nsCRT::strcasecmp(mName.c_str(), sctp_map->name.c_str()) == 0;
     }
 
     return false;
   }
 
   virtual void AddToMediaSection(SdpMediaSection& msection) const override {
     if (mEnabled && msection.GetMediaType() == mType) {
-      if (msection.GetFormats().empty()) {
+      if (mDirection == sdp::kRecv) {
         msection.AddDataChannel(mName, mLocalPort, mChannels,
                                 mLocalMaxMessageSize);
       }
 
       AddParametersToMSection(msection);
     }
   }
 
-  bool Negotiate(const std::string& pt,
-                 const SdpMediaSection& remoteMsection) override {
-    JsepCodecDescription::Negotiate(pt, remoteMsection);
+  bool Negotiate(const std::string& pt, const SdpMediaSection& remoteMsection,
+                 bool isOffer) override {
+    JsepCodecDescription::Negotiate(pt, remoteMsection, isOffer);
 
     uint32_t message_size;
     mRemoteMMSSet = remoteMsection.GetMaxMessageSize(&message_size);
     if (mRemoteMMSSet) {
       mRemoteMaxMessageSize = message_size;
     } else {
       mRemoteMaxMessageSize =
           WEBRTC_DATACHANNEL_MAX_MESSAGE_SIZE_REMOTE_DEFAULT;
--- a/media/webrtc/signaling/src/jsep/JsepTrack.cpp
+++ b/media/webrtc/signaling/src/jsep/JsepTrack.cpp
@@ -90,16 +90,17 @@ void JsepTrack::EnsureSsrcs(SsrcGenerato
       return;
     }
     mSsrcs.push_back(ssrc);
   }
 }
 
 void JsepTrack::PopulateCodecs(
     const std::vector<UniquePtr<JsepCodecDescription>>& prototype) {
+  mPrototypeCodecs.clear();
   for (const auto& prototypeCodec : prototype) {
     if (prototypeCodec->mType == mType) {
       mPrototypeCodecs.emplace_back(prototypeCodec->Clone());
       mPrototypeCodecs.back()->mDirection = mDirection;
     }
   }
 
   EnsureNoDuplicatePayloadTypes(&mPrototypeCodecs);
@@ -118,19 +119,18 @@ void JsepTrack::AddToOffer(SsrcGenerator
   }
 }
 
 void JsepTrack::AddToAnswer(const SdpMediaSection& offer,
                             SsrcGenerator& ssrcGenerator, bool encodeTrackId,
                             SdpMediaSection* answer) {
   // We do not modify mPrototypeCodecs here, since we're only creating an
   // answer. Once offer/answer concludes, we will update mPrototypeCodecs.
-  std::vector<UniquePtr<JsepCodecDescription>> codecs;
-  codecs = GetCodecClones();
-  NegotiateCodecs(offer, &codecs);
+  std::vector<UniquePtr<JsepCodecDescription>> codecs =
+      NegotiateCodecs(offer, true);
   if (codecs.empty()) {
     return;
   }
 
   AddToMsection(codecs, encodeTrackId, answer);
 
   if (mDirection == sdp::kSend) {
     std::vector<JsConstraints> constraints;
@@ -370,49 +370,62 @@ std::vector<UniquePtr<JsepCodecDescripti
   return clones;
 }
 
 static bool CompareCodec(const UniquePtr<JsepCodecDescription>& lhs,
                          const UniquePtr<JsepCodecDescription>& rhs) {
   return lhs->mStronglyPreferred && !rhs->mStronglyPreferred;
 }
 
-void JsepTrack::NegotiateCodecs(
-    const SdpMediaSection& remote,
-    std::vector<UniquePtr<JsepCodecDescription>>* codecs,
-    std::map<std::string, std::string>* formatChanges) const {
-  MOZ_ASSERT(codecs->size());
-  std::vector<UniquePtr<JsepCodecDescription>> unnegotiatedCodecs;
-  std::swap(unnegotiatedCodecs, *codecs);
+std::vector<UniquePtr<JsepCodecDescription>> JsepTrack::NegotiateCodecs(
+    const SdpMediaSection& remote, bool isOffer) {
+  std::vector<UniquePtr<JsepCodecDescription>> negotiatedCodecs;
+  std::vector<UniquePtr<JsepCodecDescription>> newPrototypeCodecs;
 
   // Outer loop establishes the remote side's preference
   for (const std::string& fmt : remote.GetFormats()) {
-    for (auto& codec : unnegotiatedCodecs) {
+    for (auto& codec : mPrototypeCodecs) {
       if (!codec || !codec->mEnabled || !codec->Matches(fmt, remote)) {
         continue;
       }
 
-      std::string originalFormat = codec->mDefaultPt;
-      if (codec->Negotiate(fmt, remote)) {
-        if (formatChanges) {
-          (*formatChanges)[originalFormat] = codec->mDefaultPt;
-        }
-        codecs->push_back(std::move(codec));
+      // First codec of ours that matches. See if we can negotiate it.
+      UniquePtr<JsepCodecDescription> clone(codec->Clone());
+      if (clone->Negotiate(fmt, remote, isOffer)) {
+        // If negotiation changed the payload type, remember that for later
+        codec->mDefaultPt = clone->mDefaultPt;
+        // Moves the codec out of mPrototypeCodecs, leaving an empty
+        // UniquePtr, so we don't use it again. Also causes successfully
+        // negotiated codecs to be placed up front in the future.
+        newPrototypeCodecs.emplace_back(std::move(codec));
+        negotiatedCodecs.emplace_back(std::move(clone));
         break;
       }
     }
   }
 
+  // newPrototypeCodecs contains just the negotiated stuff so far. Add the rest.
+  for (auto& codec : mPrototypeCodecs) {
+    if (codec) {
+      newPrototypeCodecs.emplace_back(std::move(codec));
+    }
+  }
+
+  // Negotiated stuff is up front, so it will take precedence when ensuring
+  // there are no duplicate payload types.
+  EnsureNoDuplicatePayloadTypes(&newPrototypeCodecs);
+  std::swap(newPrototypeCodecs, mPrototypeCodecs);
+
   // Find the (potential) red codec and ulpfec codec or telephone-event
   JsepVideoCodecDescription* red = nullptr;
   JsepVideoCodecDescription* ulpfec = nullptr;
   JsepAudioCodecDescription* dtmf = nullptr;
   // We can safely cast here since JsepTrack has a MediaType and only codecs
   // that match that MediaType (kAudio or kVideo) are added.
-  for (auto& codec : *codecs) {
+  for (auto& codec : negotiatedCodecs) {
     if (codec->mName == "red") {
       red = static_cast<JsepVideoCodecDescription*>(codec.get());
     } else if (codec->mName == "ulpfec") {
       ulpfec = static_cast<JsepVideoCodecDescription*>(codec.get());
     } else if (codec->mName == "telephone-event") {
       dtmf = static_cast<JsepAudioCodecDescription*>(codec.get());
     }
   }
@@ -420,98 +433,84 @@ void JsepTrack::NegotiateCodecs(
   if (red) {
     // Since we could have an externally specified redundant endcodings
     // list, we shouldn't simply rebuild the redundant encodings list
     // based on the current list of codecs.
     std::vector<uint8_t> unnegotiatedEncodings;
     std::swap(unnegotiatedEncodings, red->mRedundantEncodings);
     for (auto redundantPt : unnegotiatedEncodings) {
       std::string pt = std::to_string(redundantPt);
-      for (const auto& codec : *codecs) {
+      for (const auto& codec : negotiatedCodecs) {
         if (pt == codec->mDefaultPt) {
           red->mRedundantEncodings.push_back(redundantPt);
           break;
         }
       }
     }
   }
   // Video FEC is indicated by the existence of the red and ulpfec
   // codecs and not an attribute on the particular video codec (like in
   // a rtcpfb attr). If we see both red and ulpfec codecs, we enable FEC
   // on all the other codecs.
   if (red && ulpfec) {
-    for (auto& codec : *codecs) {
+    for (auto& codec : negotiatedCodecs) {
       if (codec->mName != "red" && codec->mName != "ulpfec") {
         JsepVideoCodecDescription* videoCodec =
             static_cast<JsepVideoCodecDescription*>(codec.get());
         videoCodec->EnableFec(red->mDefaultPt, ulpfec->mDefaultPt);
       }
     }
   }
 
   // Dtmf support is indicated by the existence of the telephone-event
   // codec, and not an attribute on the particular audio codec (like in a
   // rtcpfb attr). If we see the telephone-event codec, we enabled dtmf
   // support on all the other audio codecs.
   if (dtmf) {
-    for (auto& codec : *codecs) {
+    for (auto& codec : negotiatedCodecs) {
       JsepAudioCodecDescription* audioCodec =
           static_cast<JsepAudioCodecDescription*>(codec.get());
       audioCodec->mDtmfEnabled = true;
     }
   }
 
   // Make sure strongly preferred codecs are up front, overriding the remote
   // side's preference.
-  std::stable_sort(codecs->begin(), codecs->end(), CompareCodec);
+  std::stable_sort(negotiatedCodecs.begin(), negotiatedCodecs.end(),
+                   CompareCodec);
 
   // TODO(bug 814227): Remove this once we're ready to put multiple codecs in an
   // answer.  For now, remove all but the first codec unless the red codec
   // exists, in which case we include the others per RFC 5109, section 14.2.
-  if (!codecs->empty() && !red) {
+  if (!negotiatedCodecs.empty() && !red) {
     std::vector<UniquePtr<JsepCodecDescription>> codecsToKeep;
 
     bool foundPreferredCodec = false;
-    for (auto& codec : *codecs) {
+    for (auto& codec : negotiatedCodecs) {
       if (codec.get() == dtmf) {
         codecsToKeep.push_back(std::move(codec));
         // TODO: keep ulpfec when we enable it in Bug 875922
         // } else if (codec.get() == ulpfec) {
         //   codecsToKeep.push_back(std::move(codec));
       } else if (!foundPreferredCodec) {
         codecsToKeep.insert(codecsToKeep.begin(), std::move(codec));
         foundPreferredCodec = true;
       }
     }
 
-    *codecs = std::move(codecsToKeep);
+    negotiatedCodecs = std::move(codecsToKeep);
   }
+
+  return negotiatedCodecs;
 }
 
 void JsepTrack::Negotiate(const SdpMediaSection& answer,
                           const SdpMediaSection& remote) {
-  std::vector<UniquePtr<JsepCodecDescription>> negotiatedCodecs;
-  negotiatedCodecs = GetCodecClones();
-
-  std::map<std::string, std::string> formatChanges;
-  NegotiateCodecs(remote, &negotiatedCodecs, &formatChanges);
-
-  // Use |formatChanges| to update mPrototypeCodecs
-  size_t insertPos = 0;
-  for (auto& codec : mPrototypeCodecs) {
-    if (formatChanges.count(codec->mDefaultPt)) {
-      // Update the payload type to what was negotiated
-      codec->mDefaultPt = formatChanges[codec->mDefaultPt];
-      // Move this negotiated codec up front
-      std::swap(mPrototypeCodecs[insertPos], codec);
-      ++insertPos;
-    }
-  }
-
-  EnsureNoDuplicatePayloadTypes(&mPrototypeCodecs);
+  std::vector<UniquePtr<JsepCodecDescription>> negotiatedCodecs =
+      NegotiateCodecs(remote, &answer != &remote);
 
   UniquePtr<JsepTrackNegotiatedDetails> negotiatedDetails =
       MakeUnique<JsepTrackNegotiatedDetails>();
 
   CreateEncodings(remote, negotiatedCodecs, negotiatedDetails.get());
 
   if (answer.GetAttributeList().HasAttribute(SdpAttribute::kExtmapAttribute)) {
     for (auto& extmapAttr : answer.GetAttributeList().GetExtmap().mExtmaps) {
--- a/media/webrtc/signaling/src/jsep/JsepTrack.h
+++ b/media/webrtc/signaling/src/jsep/JsepTrack.h
@@ -134,16 +134,17 @@ class JsepTrack {
       mTrackId = rhs.mTrackId;
       mCNAME = rhs.mCNAME;
       mDirection = rhs.mDirection;
       mJsEncodeConstraints = rhs.mJsEncodeConstraints;
       mSsrcs = rhs.mSsrcs;
       mActive = rhs.mActive;
       mRemoteSetSendBit = rhs.mRemoteSetSendBit;
 
+      mPrototypeCodecs.clear();
       for (const auto& codec : rhs.mPrototypeCodecs) {
         mPrototypeCodecs.emplace_back(codec->Clone());
       }
       if (rhs.mNegotiatedDetails) {
         mNegotiatedDetails.reset(
             new JsepTrackNegotiatedDetails(*rhs.mNegotiatedDetails));
       }
     }
@@ -241,34 +242,27 @@ class JsepTrack {
 
  private:
   std::vector<UniquePtr<JsepCodecDescription>> GetCodecClones() const;
   static void EnsureNoDuplicatePayloadTypes(
       std::vector<UniquePtr<JsepCodecDescription>>* codecs);
   static void GetPayloadTypes(
       const std::vector<UniquePtr<JsepCodecDescription>>& codecs,
       std::vector<uint16_t>* pts);
-  static void EnsurePayloadTypeIsUnique(std::set<uint16_t>* uniquePayloadTypes,
-                                        JsepCodecDescription* codec);
   void AddToMsection(const std::vector<UniquePtr<JsepCodecDescription>>& codecs,
                      bool encodeTrackId, SdpMediaSection* msection);
   void GetRids(const SdpMediaSection& msection, sdp::Direction direction,
                std::vector<SdpRidAttributeList::Rid>* rids) const;
   void CreateEncodings(
       const SdpMediaSection& remote,
       const std::vector<UniquePtr<JsepCodecDescription>>& negotiatedCodecs,
       JsepTrackNegotiatedDetails* details);
 
-  // |formatChanges| is set on completion of offer/answer, and records how the
-  // formats in |codecs| were changed, which is used by |Negotiate| to update
-  // |mPrototypeCodecs|.
-  virtual void NegotiateCodecs(
-      const SdpMediaSection& remote,
-      std::vector<UniquePtr<JsepCodecDescription>>* codecs,
-      std::map<std::string, std::string>* formatChanges = nullptr) const;
+  virtual std::vector<UniquePtr<JsepCodecDescription>> NegotiateCodecs(
+      const SdpMediaSection& remote, bool isOffer);
 
   JsConstraints* FindConstraints(
       const std::string& rid,
       std::vector<JsConstraints>& constraintsList) const;
   void NegotiateRids(const std::vector<SdpRidAttributeList::Rid>& rids,
                      std::vector<JsConstraints>* constraints) const;
   void UpdateSsrcs(SsrcGenerator& ssrcGenerator, size_t encodings);