Bug 1165520: Negotiate rtcp-fb r=jesup
authorByron Campen [:bwc] <docfaraday@gmail.com>
Fri, 15 May 2015 12:19:19 -0700
changeset 245779 ec28ae5f5f93002cb6be6b0d1637c533a2d9e8e4
parent 245778 13fda43e7e957947e3ad7db6d56f4aab209ec330
child 245780 a5f31bacc839708a0d0c8f9408b00f9b4767c601
push id13198
push userkwierso@gmail.com
push dateThu, 28 May 2015 00:24:10 +0000
treeherderfx-team@242916838c5c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjesup
bugs1165520
milestone41.0a1
Bug 1165520: Negotiate rtcp-fb r=jesup
media/webrtc/signaling/src/jsep/JsepCodecDescription.h
media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
media/webrtc/signaling/test/signaling_unittests.cpp
--- a/media/webrtc/signaling/src/jsep/JsepCodecDescription.h
+++ b/media/webrtc/signaling/src/jsep/JsepCodecDescription.h
@@ -89,28 +89,16 @@ struct JsepCodecDescription {
 
   virtual bool
   ParametersMatch(const std::string& fmt,
                   const SdpMediaSection& remoteMsection) const
   {
     return true;
   }
 
-  UniquePtr<JsepCodecDescription>
-  MakeNegotiatedCodec(const SdpMediaSection& remoteMsection) const
-  {
-    UniquePtr<JsepCodecDescription> negotiated(Clone());
-
-    if (!negotiated->Negotiate(remoteMsection)) {
-      negotiated.reset();
-    }
-
-    return negotiated;
-  }
-
   virtual bool
   Negotiate(const SdpMediaSection& remoteMsection)
   {
     return true;
   }
 
   // TODO(bug 1142105): This probably should be a helper function in
   // /sdp
@@ -126,40 +114,16 @@ struct JsepCodecDescription {
         if (i->format == pt && i->parameters) {
           return i->parameters.get();
         }
       }
     }
     return nullptr;
   }
 
-  UniquePtr<JsepCodecDescription>
-  MakeSendCodec(const mozilla::SdpMediaSection& remoteMsection) const
-  {
-    UniquePtr<JsepCodecDescription> sendCodec(Clone());
-
-    if (!sendCodec->LoadSendParameters(remoteMsection)) {
-      sendCodec.reset();
-    }
-
-    return sendCodec;
-  }
-
-  UniquePtr<JsepCodecDescription>
-  MakeRecvCodec(const mozilla::SdpMediaSection& remoteMsection) const
-  {
-    UniquePtr<JsepCodecDescription> recvCodec(Clone());
-
-    if (!recvCodec->LoadRecvParameters(remoteMsection)) {
-      recvCodec.reset();
-    }
-
-    return recvCodec;
-  }
-
   virtual bool LoadSendParameters(
       const mozilla::SdpMediaSection& remoteMsection)
   {
     return true;
   }
 
   virtual bool LoadRecvParameters(
       const mozilla::SdpMediaSection& remoteMsection)
@@ -272,18 +236,26 @@ struct JsepVideoCodecDescription : publi
       : JsepCodecDescription(mozilla::SdpMediaSection::kVideo, defaultPt, name,
                              clock, 0, enabled),
         mMaxFs(0),
         mMaxFr(0),
         mPacketizationMode(0),
         mMaxMbps(0),
         mMaxCpb(0),
         mMaxDpb(0),
-        mMaxBr(0)
+        mMaxBr(0),
+        mUseTmmbr(false)
   {
+    // Add supported rtcp-fb types
+    mNackFbTypes.push_back("");
+    mNackFbTypes.push_back(SdpRtcpFbAttributeList::pli);
+    mCcmFbTypes.push_back(SdpRtcpFbAttributeList::fir);
+    if (mUseTmmbr) {
+      mCcmFbTypes.push_back(SdpRtcpFbAttributeList::tmmbr);
+    }
   }
 
   virtual void
   AddFmtps(SdpFmtpAttributeList& fmtp) const override
   {
     if (mName == "H264") {
       UniquePtr<SdpFmtpAttributeList::H264Parameters> params =
           MakeUnique<SdpFmtpAttributeList::H264Parameters>();
@@ -313,25 +285,24 @@ struct JsepVideoCodecDescription : publi
       params->max_fr = mMaxFr;
       fmtp.PushEntry(mDefaultPt, "", mozilla::Move(params));
     }
   }
 
   virtual void
   AddRtcpFbs(SdpRtcpFbAttributeList& rtcpfb) const override
   {
-    // Just hard code for now
-    rtcpfb.PushEntry(mDefaultPt, SdpRtcpFbAttributeList::kNack);
-    rtcpfb.PushEntry(
-        mDefaultPt, SdpRtcpFbAttributeList::kNack, SdpRtcpFbAttributeList::pli);
-    rtcpfb.PushEntry(
-        mDefaultPt, SdpRtcpFbAttributeList::kCcm, SdpRtcpFbAttributeList::fir);
-    if (mUseTmmbr) {
-      rtcpfb.PushEntry(
-          mDefaultPt, SdpRtcpFbAttributeList::kCcm, SdpRtcpFbAttributeList::tmmbr);
+    for (const std::string& type : mAckFbTypes) {
+      rtcpfb.PushEntry(mDefaultPt, SdpRtcpFbAttributeList::kAck, type);
+    }
+    for (const std::string& type : mNackFbTypes) {
+      rtcpfb.PushEntry(mDefaultPt, SdpRtcpFbAttributeList::kNack, type);
+    }
+    for (const std::string& type : mCcmFbTypes) {
+      rtcpfb.PushEntry(mDefaultPt, SdpRtcpFbAttributeList::kCcm, type);
     }
   }
 
   SdpFmtpAttributeList::H264Parameters
   GetH264Parameters(const std::string& pt,
                     const SdpMediaSection& msection) const
   {
     // Will contain defaults if nothing else
@@ -362,31 +333,80 @@ struct JsepVideoCodecDescription : publi
     if (params && params->codec_type == expectedType) {
       result =
         static_cast<const SdpFmtpAttributeList::VP8Parameters&>(*params);
     }
 
     return result;
   }
 
+  static bool
+  HasRtcpFb(const SdpMediaSection& msection,
+            const std::string& pt,
+            SdpRtcpFbAttributeList::Type type,
+            const std::string& subType)
+  {
+    const SdpAttributeList& attrs(msection.GetAttributeList());
+
+    if (!attrs.HasAttribute(SdpAttribute::kRtcpFbAttribute)) {
+      return false;
+    }
+
+    for (auto& rtcpfb : attrs.GetRtcpFb().mFeedbacks) {
+      if (rtcpfb.type == type) {
+        if (rtcpfb.pt == "*" || rtcpfb.pt == pt) {
+          if (rtcpfb.parameter == subType) {
+            return true;
+          }
+        }
+      }
+    }
+
+    return false;
+  }
+
+  void
+  NegotiateRtcpFb(const SdpMediaSection& remoteMsection,
+                  SdpRtcpFbAttributeList::Type type,
+                  std::vector<std::string>* supportedTypes)
+  {
+    std::vector<std::string> temp;
+    for (auto& subType : *supportedTypes) {
+      if (HasRtcpFb(remoteMsection, mDefaultPt, type, subType)) {
+        temp.push_back(subType);
+      }
+    }
+    *supportedTypes = temp;
+  }
+
+  void
+  NegotiateRtcpFb(const SdpMediaSection& remote)
+  {
+    // 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);
+  }
+
   virtual bool
   Negotiate(const SdpMediaSection& remoteMsection) override
   {
     if (mName == "H264") {
       SdpFmtpAttributeList::H264Parameters h264Params(
           GetH264Parameters(mDefaultPt, remoteMsection));
       if (!h264Params.level_asymmetry_allowed) {
         SetSaneH264Level(std::min(GetSaneH264Level(h264Params.profile_level_id),
                                   GetSaneH264Level(mProfileLevelId)),
                          &mProfileLevelId);
       }
 
       // TODO(bug 1143709): max-recv-level support
     }
 
+    NegotiateRtcpFb(remoteMsection);
     return JsepCodecDescription::Negotiate(remoteMsection);
   }
 
   // Maps the not-so-sane encoding of H264 level into something that is
   // ordered in the way one would expect
   // 1b is 0xAB, everything else is the level left-shifted one half-byte
   // (eg; 1.0 is 0xA0, 1.1 is 0xB0, 3.1 is 0x1F0)
   static uint32_t
@@ -462,57 +482,25 @@ struct JsepVideoCodecDescription : publi
       mSpropParameterSets = h264Params.sprop_parameter_sets;
     } else if (mName == "VP8" || mName == "VP9") {
       SdpFmtpAttributeList::VP8Parameters vp8Params(
           GetVP8Parameters(mDefaultPt, remoteMsection));
 
       mMaxFs = vp8Params.max_fs;
       mMaxFr = vp8Params.max_fr;
     }
+
+    NegotiateRtcpFb(remoteMsection);
     return true;
   }
 
   virtual bool
   LoadRecvParameters(const mozilla::SdpMediaSection& remoteMsection) override
   {
-    const SdpAttributeList& attrs(remoteMsection.GetAttributeList());
-
-    if (attrs.HasAttribute(SdpAttribute::kRtcpFbAttribute)) {
-      auto& rtcpfbs = attrs.GetRtcpFb().mFeedbacks;
-      for (auto i = rtcpfbs.begin(); i != rtcpfbs.end(); ++i) {
-        if (i->pt == mDefaultPt || i->pt == "*") {
-          switch (i->type) {
-            case SdpRtcpFbAttributeList::kAck:
-              mAckFbTypes.push_back(i->parameter);
-              break;
-            case SdpRtcpFbAttributeList::kCcm:
-              mCcmFbTypes.push_back(i->parameter);
-              break;
-            case SdpRtcpFbAttributeList::kNack:
-              mNackFbTypes.push_back(i->parameter);
-              break;
-            case SdpRtcpFbAttributeList::kApp:
-            case SdpRtcpFbAttributeList::kTrrInt:
-              // We don't support these, ignore.
-              {}
-          }
-        }
-      }
-    }
-
-    if (mName == "H264") {
-      SdpFmtpAttributeList::H264Parameters h264Params(
-          GetH264Parameters(mDefaultPt, remoteMsection));
-      if (!h264Params.level_asymmetry_allowed) {
-        SetSaneH264Level(std::min(GetSaneH264Level(h264Params.profile_level_id),
-                                  GetSaneH264Level(mProfileLevelId)),
-                         &mProfileLevelId);
-      }
-    }
-    return true;
+    return Negotiate(remoteMsection);
   }
 
   enum Subprofile {
     kH264ConstrainedBaseline,
     kH264Baseline,
     kH264Main,
     kH264Extended,
     kH264High,
--- a/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
+++ b/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
@@ -1171,21 +1171,19 @@ JsepSessionImpl::CreateAnswerMSection(co
   if (remoteMsection.IsSending()) {
     msection.SetReceiving(true);
   }
 
   // Now add the codecs.
   PtrVector<JsepCodecDescription> matchingCodecs(
       GetCommonCodecs(remoteMsection));
 
-  for (const JsepCodecDescription* codec : matchingCodecs.values) {
-    UniquePtr<JsepCodecDescription> negotiated(
-        codec->MakeNegotiatedCodec(remoteMsection));
-    if (negotiated) {
-      negotiated->AddToMediaSection(msection);
+  for (JsepCodecDescription* codec : matchingCodecs.values) {
+    if (codec->Negotiate(remoteMsection)) {
+      codec->AddToMediaSection(msection);
       // TODO(bug 1099351): Once bug 1073475 is fixed on all supported
       // versions, we can remove this limitation.
       break;
     }
   }
 
   // Add extmap attributes.
   AddCommonExtmaps(remoteMsection, &msection);
@@ -1716,52 +1714,49 @@ JsepSessionImpl::NegotiateTrack(const Sd
   UniquePtr<JsepTrackNegotiatedDetailsImpl> negotiatedDetails =
       MakeUnique<JsepTrackNegotiatedDetailsImpl>();
   negotiatedDetails->mProtocol = remoteMsection.GetProtocol();
 
   // Insert all the codecs we jointly support.
   PtrVector<JsepCodecDescription> commonCodecs(
       GetCommonCodecs(remoteMsection));
 
-  for (const JsepCodecDescription* codec : commonCodecs.values) {
+  for (JsepCodecDescription* codec : commonCodecs.values) {
     bool sending = (direction == JsepTrack::kJsepTrackSending);
 
     // Everywhere else in JsepSessionImpl, a JsepCodecDescription describes
     // what one side puts in its SDP. However, we don't want that here; we want
     // a JsepCodecDescription that instead encapsulates all the parameters
     // that deal with sending (or receiving). For sending, some of these
-    // parameters will come from the local codec config (eg; rtcp-fb), others
-    // will come from the remote SDP (eg; max-fps), and still others can only be
-    // determined by inspecting both local config and remote SDP (eg;
+    // parameters will come from the remote SDP (eg; max-fps), and others can
+    // only be determined by inspecting both local config and remote SDP (eg;
     // profile-level-id when level-asymmetry-allowed is 0).
-    UniquePtr<JsepCodecDescription> sendOrReceiveCodec;
-
     if (sending) {
-      sendOrReceiveCodec = Move(codec->MakeSendCodec(remoteMsection));
+      if (!codec->LoadSendParameters(remoteMsection)) {
+        continue;
+      }
     } else {
-      sendOrReceiveCodec = Move(codec->MakeRecvCodec(remoteMsection));
-    }
-
-    if (!sendOrReceiveCodec) {
-      continue;
+      if (!codec->LoadRecvParameters(remoteMsection)) {
+        continue;
+      }
     }
 
     if (remoteMsection.GetMediaType() == SdpMediaSection::kAudio ||
         remoteMsection.GetMediaType() == SdpMediaSection::kVideo) {
       // Sanity-check that payload type can work with RTP
       uint16_t payloadType;
-      if (!sendOrReceiveCodec->GetPtAsInt(&payloadType) ||
+      if (!codec->GetPtAsInt(&payloadType) ||
           payloadType > UINT8_MAX) {
         JSEP_SET_ERROR("audio/video payload type is not an 8 bit unsigned int: "
                        << codec->mDefaultPt);
         return NS_ERROR_INVALID_ARG;
       }
     }
 
-    negotiatedDetails->mCodecs.values.push_back(sendOrReceiveCodec.release());
+    negotiatedDetails->mCodecs.values.push_back(codec->Clone());
     break;
   }
 
   if (negotiatedDetails->mCodecs.values.empty()) {
     JSEP_SET_ERROR("Failed to negotiate codec details for all codecs");
     return NS_ERROR_INVALID_ARG;
   }
 
--- a/media/webrtc/signaling/test/signaling_unittests.cpp
+++ b/media/webrtc/signaling/test/signaling_unittests.cpp
@@ -2003,16 +2003,18 @@ public:
     a2_->SetLocal(TestObserver::ANSWER, a2_->answer());
 
     std::string modifiedAnswer(HardcodeRtcpFb(a2_->answer(), feedback));
 
     a1_->SetRemote(TestObserver::ANSWER, modifiedAnswer);
 
     a1_->SetExpectedFrameRequestType(frameRequestType);
     a1_->mExpectNack = expectNack;
+    // Since we don't support rewriting rtcp-fb in answers, a2 still thinks it
+    // will be doing all of the normal rtcp-fb
 
     WaitForCompleted();
     CheckPipelines();
 
     CloseStreams();
   }
 
   void TestRtcpFbOffer(
@@ -2023,16 +2025,18 @@ public:
     OfferOptions options;
 
     a1_->CreateOffer(options, OFFER_AV);
     a1_->SetLocal(TestObserver::OFFER, a1_->offer());
 
     std::string modifiedOffer = HardcodeRtcpFb(a1_->offer(), feedback);
 
     a2_->SetRemote(TestObserver::OFFER, modifiedOffer);
+    a1_->SetExpectedFrameRequestType(frameRequestType);
+    a1_->mExpectNack = expectNack;
     a2_->SetExpectedFrameRequestType(frameRequestType);
     a2_->mExpectNack = expectNack;
 
     a2_->CreateAnswer(OFFER_AV | ANSWER_AV);
 
     a2_->SetLocal(TestObserver::ANSWER, a2_->answer());
     a1_->SetRemote(TestObserver::ANSWER, a2_->answer());