Bug 1362581 - Don't offer to receive extensions that are send only on reoffer; r?bwc draft
authorDan Minor <dminor@mozilla.com>
Tue, 19 Sep 2017 14:17:55 -0400
changeset 670083 a7aa612010296c3d4cd7d625e177b35b3150be73
parent 669742 5f3f19824efa14cc6db546baf59c54a0fc15ddc9
child 733120 c720398fc21d1ae1a121c594307092456f47ec83
push id81504
push userbmo:dminor@mozilla.com
push dateMon, 25 Sep 2017 19:37:32 +0000
reviewersbwc
bugs1362581
milestone58.0a1
Bug 1362581 - Don't offer to receive extensions that are send only on reoffer; r?bwc Currently, when we switch roles after a ICE restart, we offer to receive any extensions that the other side offered. This can lead to us offering to receive an extension which is send only for Firefox. This change filters these extensions out so that they do not appear in the offer. MozReview-Commit-ID: LeaAuieoYih
media/webrtc/signaling/gtest/jsep_session_unittest.cpp
media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
--- a/media/webrtc/signaling/gtest/jsep_session_unittest.cpp
+++ b/media/webrtc/signaling/gtest/jsep_session_unittest.cpp
@@ -1425,16 +1425,52 @@ TEST_P(JsepSessionTest, DISABLED_Renegot
   }
 
   ASSERT_EQ(answererPairs.size(), newOffererPairs.size());
   for (size_t i = 0; i < answererPairs.size(); ++i) {
     ASSERT_TRUE(Equals(answererPairs[i], newOffererPairs[i]));
   }
 }
 
+// We don't want to offer to receive extensions that are send only.
+// This tests that we filter them out properly when we reoffer.
+// See Bug 1362581.
+TEST_F(JsepSessionTest, RenegotiationSwappedRolesCheckAudioLevelExt)
+{
+  AddTracks(*mSessionOff, "audio,video");
+  mSessionOff->AddAudioRtpExtension("urn:ietf:params:rtp-hdrext:ssrc-audio-level",
+                                    SdpDirectionAttribute::Direction::kSendonly);
+  std::string offer = CreateOffer();
+
+  SetLocalOffer(offer, CHECK_SUCCESS);
+  SetRemoteOffer(offer, CHECK_SUCCESS);
+
+  UniquePtr<Sdp> parsedOffer(Parse(offer));
+  auto& offerMediaAttrs = parsedOffer->GetMediaSection(0).GetAttributeList();
+  ASSERT_TRUE(offerMediaAttrs.HasAttribute(SdpAttribute::kExtmapAttribute));
+  auto& offerExtmap = offerMediaAttrs.GetExtmap().mExtmaps;
+  ASSERT_EQ(1U, offerExtmap.size());
+  ASSERT_EQ("urn:ietf:params:rtp-hdrext:ssrc-audio-level",
+      offerExtmap[0].extensionname);
+
+  std::string answer = CreateAnswer();
+  SetLocalAnswer(answer, CHECK_SUCCESS);
+  SetRemoteAnswer(answer, CHECK_SUCCESS);
+
+  UniquePtr<Sdp> parsedAnswer(Parse(answer));
+  auto& answerMediaAttrs = parsedAnswer->GetMediaSection(0).GetAttributeList();
+  ASSERT_FALSE(answerMediaAttrs.HasAttribute(SdpAttribute::kExtmapAttribute));
+
+  SwapOfferAnswerRoles();
+
+  std::string reoffer = CreateOffer();
+  UniquePtr<Sdp> parsedReoffer(Parse(reoffer));
+  auto& reofferMediaAttrs = parsedReoffer->GetMediaSection(0).GetAttributeList();
+  ASSERT_FALSE(reofferMediaAttrs.HasAttribute(SdpAttribute::kExtmapAttribute));
+}
 
 TEST_P(JsepSessionTest, RenegotiationOffererAddsTrack)
 {
   AddTracks(*mSessionOff);
   AddTracks(*mSessionAns);
 
   OfferAnswer();
 
--- a/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
+++ b/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
@@ -526,16 +526,30 @@ JsepSessionImpl::BindRemoteTracks(SdpMed
 
     if (!track.mAssignedMLine.isSome()) {
       MOZ_ASSERT(false);
       continue;
     }
 
     auto& msection = sdp->GetMediaSection(*track.mAssignedMLine);
 
+    // Filter out extensions that are send only so we don't offer to
+    // receive them.
+    auto& attrs = msection.GetAttributeList();
+    if (attrs.HasAttribute(SdpAttribute::kExtmapAttribute)) {
+      UniquePtr<SdpExtmapAttributeList> new_list(new SdpExtmapAttributeList);
+      for (auto& ext : attrs.GetExtmap().mExtmaps) {
+        if (ext.direction != SdpDirectionAttribute::kSendonly) {
+          new_list->mExtmaps.push_back(ext);
+        }
+      }
+      attrs.RemoveAttribute(SdpAttribute::kExtmapAttribute);
+      attrs.SetAttribute(new_list.release());
+    }
+
     if (mSdpHelper.MsectionIsDisabled(msection)) {
       // TODO(bug 1095226) Content probably disabled this? Should we allow
       // content to do this?
       continue;
     }
 
     track.mTrack->AddToOffer(&msection);