Bug 1341995 - Use negotiated values for RED and ULPFEC payload types; r=bwc
authorDan Minor <dminor@mozilla.com>
Mon, 27 Feb 2017 09:37:30 -0500
changeset 374416 47c6c4caad99d65ff37e5494e253fd7d408d617b
parent 374415 706398e37dbba13604d924820340e9ecb37b20b1
child 374417 99aabb33581ad1f67728f3f20a04888d6efafb03
push id10863
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 23:02:23 +0000
treeherdermozilla-aurora@0931190cd725 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbwc
bugs1341995
milestone54.0a1
Bug 1341995 - Use negotiated values for RED and ULPFEC payload types; r=bwc MozReview-Commit-ID: 33jkKWThcL2
media/webrtc/signaling/src/jsep/JsepCodecDescription.h
media/webrtc/signaling/src/jsep/JsepTrack.cpp
media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp
--- a/media/webrtc/signaling/src/jsep/JsepCodecDescription.h
+++ b/media/webrtc/signaling/src/jsep/JsepCodecDescription.h
@@ -259,23 +259,34 @@ class JsepVideoCodecDescription : public
     // PeerConnectionImpl::ConfigureJsepSessionCodecs
     if (!mRembEnabled) {
       mRembEnabled = true;
       mOtherFbTypes.push_back({ "", SdpRtcpFbAttributeList::kRemb, "", ""});
     }
   }
 
   virtual void
-  EnableFec() {
+  EnableFec(std::string redPayloadType, std::string ulpfecPayloadType) {
     // Enabling FEC for video works a little differently than enabling
     // REMB or TMMBR.  Support for FEC is indicated by the presence of
     // particular codes (red and ulpfec) instead of using rtcpfb
     // attributes on a given codec.  There is no rtcpfb to push for FEC
     // as can be seen above when REMB or TMMBR are enabled.
+
+    // Ensure we have valid payload types. This returns zero on failure, which
+    // is a valid payload type.
+    uint16_t redPt, ulpfecPt;
+    if (!SdpHelper::GetPtAsInt(redPayloadType, &redPt) ||
+        !SdpHelper::GetPtAsInt(ulpfecPayloadType, &ulpfecPt)) {
+      return;
+    }
+
     mFECEnabled = true;
+    mREDPayloadType = redPt;
+    mULPFECPayloadType = ulpfecPt;
   }
 
   void
   AddParametersToMSection(SdpMediaSection& msection) const override
   {
     AddFmtpsToMSection(msection);
     AddRtcpFbsToMSection(msection);
   }
@@ -705,36 +716,36 @@ class JsepVideoCodecDescription : public
 
   virtual void
   UpdateRedundantEncodings(std::vector<JsepCodecDescription*> codecs)
   {
     for (const auto codec : codecs) {
       if (codec->mType == SdpMediaSection::kVideo &&
           codec->mEnabled &&
           codec->mName != "red") {
-        uint8_t pt = (uint8_t)strtoul(codec->mDefaultPt.c_str(), nullptr, 10);
-        // returns 0 if failed to convert, and since zero could
-        // be valid, check the defaultPt for 0
-        if (pt == 0 && codec->mDefaultPt != "0") {
+        uint16_t pt;
+        if (!SdpHelper::GetPtAsInt(codec->mDefaultPt, &pt)) {
           continue;
         }
         mRedundantEncodings.push_back(pt);
       }
     }
   }
 
   JSEP_CODEC_CLONE(JsepVideoCodecDescription)
 
   std::vector<std::string> mAckFbTypes;
   std::vector<std::string> mNackFbTypes;
   std::vector<std::string> mCcmFbTypes;
   std::vector<SdpRtcpFbAttributeList::Feedback> mOtherFbTypes;
   bool mTmmbrEnabled;
   bool mRembEnabled;
   bool mFECEnabled;
+  uint8_t mREDPayloadType;
+  uint8_t mULPFECPayloadType;
   std::vector<uint8_t> mRedundantEncodings;
 
   // H264-specific stuff
   uint32_t mProfileLevelId;
   uint32_t mPacketizationMode;
   std::string mSpropParameterSets;
 };
 
--- a/media/webrtc/signaling/src/jsep/JsepTrack.cpp
+++ b/media/webrtc/signaling/src/jsep/JsepTrack.cpp
@@ -385,17 +385,17 @@ JsepTrack::NegotiateCodecs(
   // 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) {
       if (codec->mName != "red" && codec->mName != "ulpfec") {
         JsepVideoCodecDescription* videoCodec =
             static_cast<JsepVideoCodecDescription*>(codec);
-        videoCodec->EnableFec();
+        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.
--- a/media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp
+++ b/media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp
@@ -130,16 +130,20 @@ JsepCodecDescToCodecConfig(const JsepCod
   configRaw = new VideoCodecConfig(
       pt, desc.mName, desc.mConstraints, h264Config.get());
 
   configRaw->mAckFbTypes = desc.mAckFbTypes;
   configRaw->mNackFbTypes = desc.mNackFbTypes;
   configRaw->mCcmFbTypes = desc.mCcmFbTypes;
   configRaw->mRembFbSet = desc.RtcpFbRembIsSet();
   configRaw->mFECFbSet = desc.mFECEnabled;
+  if (desc.mFECEnabled) {
+    configRaw->mREDPayloadType = desc.mREDPayloadType;
+    configRaw->mULPFECPayloadType = desc.mULPFECPayloadType;
+  }
 
   *aConfig = configRaw;
   return NS_OK;
 }
 
 static nsresult
 NegotiatedDetailsToVideoCodecConfigs(const JsepTrackNegotiatedDetails& aDetails,
                                      PtrVector<VideoCodecConfig>* aConfigs)