Bug 1306873: extmap direction support and cleanup r=bwc
authorRandell Jesup <rjesup@jesup.org>
Sat, 01 Oct 2016 20:35:50 -0400
changeset 361249 e71268c4cc73bfd5f44518124bd820956a146b5f
parent 361248 0eed0d06ea6d4053f1f9f85ba6bc0e0db6b4eac6
child 361250 1378de3d013b2f69f51ac916ad779638802afc4b
push id1369
push userjlorenzo@mozilla.com
push dateMon, 27 Feb 2017 14:59:41 +0000
treeherdermozilla-release@d75a1dba431f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbwc
bugs1306873
milestone52.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 1306873: extmap direction support and cleanup r=bwc Also removes RID support from audio MozReview-Commit-ID: 28pmbvm4kw4
media/webrtc/signaling/src/jsep/JsepSession.h
media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
media/webrtc/signaling/src/jsep/JsepSessionImpl.h
media/webrtc/signaling/src/sdp/SdpHelper.cpp
media/webrtc/signaling/test/jsep_session_unittest.cpp
--- a/media/webrtc/signaling/src/jsep/JsepSession.h
+++ b/media/webrtc/signaling/src/jsep/JsepSession.h
@@ -90,18 +90,20 @@ public:
   virtual nsresult SetBundlePolicy(JsepBundlePolicy policy) = 0;
   virtual bool RemoteIsIceLite() const = 0;
   virtual bool RemoteIceIsRestarting() const = 0;
   virtual std::vector<std::string> GetIceOptions() const = 0;
 
   virtual nsresult AddDtlsFingerprint(const std::string& algorithm,
                                       const std::vector<uint8_t>& value) = 0;
 
-  virtual nsresult AddAudioRtpExtension(const std::string& extensionName) = 0;
-  virtual nsresult AddVideoRtpExtension(const std::string& extensionName) = 0;
+  virtual nsresult AddAudioRtpExtension(const std::string& extensionName,
+                                        SdpDirectionAttribute::Direction direction) = 0;
+  virtual nsresult AddVideoRtpExtension(const std::string& extensionName,
+                                        SdpDirectionAttribute::Direction direction) = 0;
 
   // Kinda gross to be locking down the data structure type like this, but
   // returning by value is problematic due to the lack of stl move semantics in
   // our build config, since we can't use UniquePtr in the container. The
   // alternative is writing a raft of accessor functions that allow arbitrary
   // manipulation (which will be unwieldy), or allowing functors to be injected
   // that manipulate the data structure (still pretty unwieldy).
   virtual std::vector<JsepCodecDescription*>& Codecs() = 0;
--- a/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
+++ b/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
@@ -193,54 +193,50 @@ JsepSessionImpl::AddDtlsFingerprint(cons
   fp.mValue = value;
 
   mDtlsFingerprints.push_back(fp);
 
   return NS_OK;
 }
 
 nsresult
-JsepSessionImpl::AddAudioRtpExtension(const std::string& extensionName)
+JsepSessionImpl::AddRtpExtension(std::vector<SdpExtmapAttributeList::Extmap>& extensions,
+                                 const std::string& extensionName,
+                                 SdpDirectionAttribute::Direction direction)
 {
   mLastError.clear();
 
-  if (mAudioRtpExtensions.size() + 1 > UINT16_MAX) {
-    JSEP_SET_ERROR("Too many audio rtp extensions have been added");
+  if (extensions.size() + 1 > UINT16_MAX) {
+    JSEP_SET_ERROR("Too many rtp extensions have been added");
     return NS_ERROR_FAILURE;
   }
 
   SdpExtmapAttributeList::Extmap extmap =
-      { static_cast<uint16_t>(mAudioRtpExtensions.size() + 1),
-        SdpDirectionAttribute::kSendrecv,
-        false, // don't actually specify direction
+      { static_cast<uint16_t>(extensions.size() + 1),
+        direction,
+        direction != SdpDirectionAttribute::kSendrecv, // do we want to specify direction?
         extensionName,
         "" };
 
-  mAudioRtpExtensions.push_back(extmap);
+  extensions.push_back(extmap);
   return NS_OK;
 }
 
 nsresult
-JsepSessionImpl::AddVideoRtpExtension(const std::string& extensionName)
+JsepSessionImpl::AddAudioRtpExtension(const std::string& extensionName,
+                                      SdpDirectionAttribute::Direction direction)
 {
-  mLastError.clear();
-
-  if (mVideoRtpExtensions.size() + 1 > UINT16_MAX) {
-    JSEP_SET_ERROR("Too many video rtp extensions have been added");
-    return NS_ERROR_FAILURE;
-  }
+  return AddRtpExtension(mAudioRtpExtensions, extensionName, direction);
+}
 
-  SdpExtmapAttributeList::Extmap extmap =
-      { static_cast<uint16_t>(mVideoRtpExtensions.size() + 1),
-        SdpDirectionAttribute::kSendrecv,
-        false, // don't actually specify direction
-        extensionName, "" };
-
-  mVideoRtpExtensions.push_back(extmap);
-  return NS_OK;
+nsresult
+JsepSessionImpl::AddVideoRtpExtension(const std::string& extensionName,
+                                      SdpDirectionAttribute::Direction direction)
+{
+  return AddRtpExtension(mVideoRtpExtensions, extensionName, direction);
 }
 
 template<class T>
 std::vector<RefPtr<JsepTrack>>
 GetTracks(const std::vector<T>& wrappedTracks)
 {
   std::vector<RefPtr<JsepTrack>> result;
   for (auto i = wrappedTracks.begin(); i != wrappedTracks.end(); ++i) {
@@ -2203,19 +2199,20 @@ JsepSessionImpl::SetupDefaultCodecs()
   // Update the redundant encodings for the RED codec with the supported
   // codecs.  Note: only uses the video codecs.
   red->UpdateRedundantEncodings(mSupportedCodecs.values);
 }
 
 void
 JsepSessionImpl::SetupDefaultRtpExtensions()
 {
-  AddAudioRtpExtension("urn:ietf:params:rtp-hdrext:ssrc-audio-level");
-  AddAudioRtpExtension("urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id");
-  AddVideoRtpExtension("urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id");
+  AddAudioRtpExtension("urn:ietf:params:rtp-hdrext:ssrc-audio-level",
+                       SdpDirectionAttribute::Direction::kSendonly);
+  AddVideoRtpExtension(webrtc::RtpExtension::kRtpStreamId,
+                       SdpDirectionAttribute::Direction::kSendonly);
 }
 
 void
 JsepSessionImpl::SetState(JsepSignalingState state)
 {
   if (state == mState)
     return;
 
--- a/media/webrtc/signaling/src/jsep/JsepSessionImpl.h
+++ b/media/webrtc/signaling/src/jsep/JsepSessionImpl.h
@@ -74,21 +74,28 @@ public:
   GetIceOptions() const override
   {
     return mIceOptions;
   }
 
   virtual nsresult AddDtlsFingerprint(const std::string& algorithm,
                                       const std::vector<uint8_t>& value) override;
 
+  nsresult AddRtpExtension(std::vector<SdpExtmapAttributeList::Extmap>& extensions,
+                           const std::string& extensionName,
+                           SdpDirectionAttribute::Direction direction);
   virtual nsresult AddAudioRtpExtension(
-      const std::string& extensionName) override;
+      const std::string& extensionName,
+      SdpDirectionAttribute::Direction direction =
+      SdpDirectionAttribute::Direction::kSendrecv) override;
 
   virtual nsresult AddVideoRtpExtension(
-      const std::string& extensionName) override;
+      const std::string& extensionName,
+      SdpDirectionAttribute::Direction direction =
+      SdpDirectionAttribute::Direction::kSendrecv) override;
 
   virtual std::vector<JsepCodecDescription*>&
   Codecs() override
   {
     return mSupportedCodecs.values;
   }
 
   virtual nsresult ReplaceTrack(const std::string& oldStreamId,
--- a/media/webrtc/signaling/src/sdp/SdpHelper.cpp
+++ b/media/webrtc/signaling/src/sdp/SdpHelper.cpp
@@ -741,18 +741,35 @@ SdpHelper::AddCommonExtmaps(
         SdpAttribute::kExtmapAttribute)) {
     return;
   }
 
   UniquePtr<SdpExtmapAttributeList> localExtmap(new SdpExtmapAttributeList);
   auto& theirExtmap = remoteMsection.GetAttributeList().GetExtmap().mExtmaps;
   for (auto i = theirExtmap.begin(); i != theirExtmap.end(); ++i) {
     for (auto j = localExtensions.begin(); j != localExtensions.end(); ++j) {
-      if (i->extensionname == j->extensionname) {
-        localExtmap->mExtmaps.push_back(*i);
+      // verify we have a valid combination of directions.  For kInactive
+      // we'll just not add the response
+      if (i->extensionname == j->extensionname &&
+          (((i->direction == SdpDirectionAttribute::Direction::kSendrecv ||
+             i->direction == SdpDirectionAttribute::Direction::kSendonly) &&
+            (j->direction == SdpDirectionAttribute::Direction::kSendrecv ||
+             j->direction == SdpDirectionAttribute::Direction::kRecvonly)) ||
+
+           ((i->direction == SdpDirectionAttribute::Direction::kSendrecv ||
+             i->direction == SdpDirectionAttribute::Direction::kRecvonly) &&
+            (j->direction == SdpDirectionAttribute::Direction::kSendrecv ||
+             j->direction == SdpDirectionAttribute::Direction::kSendonly)))) {
+        auto k = *i; // we need to modify it
+        if (j->direction == SdpDirectionAttribute::Direction::kSendonly) {
+          k.direction = SdpDirectionAttribute::Direction::kRecvonly;
+        } else if (j->direction == SdpDirectionAttribute::Direction::kRecvonly) {
+          k.direction = SdpDirectionAttribute::Direction::kSendonly;
+        }
+        localExtmap->mExtmaps.push_back(k);
 
         // RFC 5285 says that ids >= 4096 can be used by the offerer to
         // force the answerer to pick, otherwise the value in the offer is
         // used.
         if (localExtmap->mExtmaps.back().entry >= 4096) {
           localExtmap->mExtmaps.back().entry = j->entry;
         }
       }
--- a/media/webrtc/signaling/test/jsep_session_unittest.cpp
+++ b/media/webrtc/signaling/test/jsep_session_unittest.cpp
@@ -3604,26 +3604,20 @@ TEST_F(JsepSessionTest, TestExtmap)
   ASSERT_EQ(4U, offerExtmap[3].entry);
 
   UniquePtr<Sdp> parsedAnswer(Parse(answer));
   ASSERT_EQ(1U, parsedAnswer->GetMediaSectionCount());
 
   auto& answerMediaAttrs = parsedAnswer->GetMediaSection(0).GetAttributeList();
   ASSERT_TRUE(answerMediaAttrs.HasAttribute(SdpAttribute::kExtmapAttribute));
   auto& answerExtmap = answerMediaAttrs.GetExtmap().mExtmaps;
-  ASSERT_EQ(3U, answerExtmap.size());
-  ASSERT_EQ("urn:ietf:params:rtp-hdrext:ssrc-audio-level",
-      answerExtmap[0].extensionname);
-  ASSERT_EQ(1U, answerExtmap[0].entry);
-  ASSERT_EQ("urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id",
-      answerExtmap[1].extensionname);
-  ASSERT_EQ(2U, answerExtmap[1].entry);
+  ASSERT_EQ(1U, answerExtmap.size());
   // We ensure that the entry for "bar" matches what was in the offer
-  ASSERT_EQ("bar", answerExtmap[2].extensionname);
-  ASSERT_EQ(4U, answerExtmap[2].entry);
+  ASSERT_EQ("bar", answerExtmap[0].extensionname);
+  ASSERT_EQ(3U, answerExtmap[0].entry);
 }
 
 TEST_F(JsepSessionTest, TestRtcpFbStar)
 {
   AddTracks(mSessionOff, "video");
   AddTracks(mSessionAns, "video");
 
   std::string offer = CreateOffer();