Bug 1328429 - don't output empty fmtp line when no redundant encodings are indicated for RED. r=drno draft
authorMichael Froman <mfroman@mozilla.com>
Thu, 05 Jan 2017 12:09:28 -0600
changeset 456994 d4f8b8f6e69490467609b694136be0be8bb59142
parent 456417 b548da4e16f067e5b69349376e37b2db97983cf7
child 541370 70a0719b06d19e39dbae601586310e0f3269cc1c
push id40656
push userbmo:mfroman@nostrum.com
push dateFri, 06 Jan 2017 15:47:55 +0000
reviewersdrno
bugs1328429
milestone53.0a1
Bug 1328429 - don't output empty fmtp line when no redundant encodings are indicated for RED. r=drno MozReview-Commit-ID: GYK8UMegRjL
media/webrtc/signaling/gtest/jsep_session_unittest.cpp
media/webrtc/signaling/gtest/sdp_unittests.cpp
media/webrtc/signaling/src/jsep/JsepCodecDescription.h
--- a/media/webrtc/signaling/gtest/jsep_session_unittest.cpp
+++ b/media/webrtc/signaling/gtest/jsep_session_unittest.cpp
@@ -3119,16 +3119,117 @@ TEST_F(JsepSessionTest, ValidateOfferedA
 
   auto& parsed_dtmf_params =
       *static_cast<const SdpFmtpAttributeList::TelephoneEventParameters*>
           (dtmf_params);
 
   ASSERT_EQ("0-15", parsed_dtmf_params.dtmfTones);
 }
 
+TEST_F(JsepSessionTest, ValidateNoFmtpLineForRedInOfferAndAnswer)
+{
+  types.push_back(SdpMediaSection::kAudio);
+  types.push_back(SdpMediaSection::kVideo);
+
+  RefPtr<JsepTrack> msta(
+      new JsepTrack(SdpMediaSection::kAudio, "offerer_stream", "a1"));
+  mSessionOff.AddTrack(msta);
+  RefPtr<JsepTrack> mstv1(
+      new JsepTrack(SdpMediaSection::kVideo, "offerer_stream", "v1"));
+  mSessionOff.AddTrack(mstv1);
+
+  std::string offer = CreateOffer();
+
+  // look for line with fmtp:122 and remove it
+  size_t start = offer.find("a=fmtp:122");
+  size_t end = offer.find("\r\n", start);
+  offer.replace(start, end+2-start, "");
+
+  SetLocalOffer(offer);
+  SetRemoteOffer(offer);
+
+  RefPtr<JsepTrack> msta_ans(
+      new JsepTrack(SdpMediaSection::kAudio, "answerer_stream", "a1"));
+  mSessionAns.AddTrack(msta);
+  RefPtr<JsepTrack> mstv1_ans(
+      new JsepTrack(SdpMediaSection::kVideo, "answerer_stream", "v1"));
+  mSessionAns.AddTrack(mstv1);
+
+  std::string answer = CreateAnswer();
+  // because parsing will throw out the malformed fmtp, make sure it is not
+  // in the answer sdp string
+  ASSERT_EQ(std::string::npos, answer.find("a=fmtp:122"));
+
+  UniquePtr<Sdp> outputSdp(Parse(answer));
+  ASSERT_TRUE(!!outputSdp);
+
+  ASSERT_EQ(2U, outputSdp->GetMediaSectionCount());
+  auto& video_section = outputSdp->GetMediaSection(1);
+  ASSERT_EQ(SdpMediaSection::kVideo, video_section.GetMediaType());
+  auto& video_attrs = video_section.GetAttributeList();
+  ASSERT_EQ(SdpDirectionAttribute::kSendrecv, video_attrs.GetDirection());
+
+  ASSERT_EQ(6U, video_section.GetFormats().size());
+  ASSERT_EQ("120", video_section.GetFormats()[0]);
+  ASSERT_EQ("121", video_section.GetFormats()[1]);
+  ASSERT_EQ("126", video_section.GetFormats()[2]);
+  ASSERT_EQ("97", video_section.GetFormats()[3]);
+  ASSERT_EQ("122", video_section.GetFormats()[4]);
+  ASSERT_EQ("123", video_section.GetFormats()[5]);
+
+  // Validate rtpmap
+  ASSERT_TRUE(video_attrs.HasAttribute(SdpAttribute::kRtpmapAttribute));
+  auto& rtpmaps = video_attrs.GetRtpmap();
+  ASSERT_TRUE(rtpmaps.HasEntry("120"));
+  ASSERT_TRUE(rtpmaps.HasEntry("121"));
+  ASSERT_TRUE(rtpmaps.HasEntry("126"));
+  ASSERT_TRUE(rtpmaps.HasEntry("97"));
+  ASSERT_TRUE(rtpmaps.HasEntry("122"));
+  ASSERT_TRUE(rtpmaps.HasEntry("123"));
+
+  // Validate fmtps
+  ASSERT_TRUE(video_attrs.HasAttribute(SdpAttribute::kFmtpAttribute));
+  auto& fmtps = video_attrs.GetFmtp().mFmtps;
+
+  ASSERT_EQ(4U, fmtps.size());
+  ASSERT_EQ("126", fmtps[0].format);
+  ASSERT_EQ("97", fmtps[1].format);
+  ASSERT_EQ("120", fmtps[2].format);
+  ASSERT_EQ("121", fmtps[3].format);
+
+  SetLocalAnswer(answer);
+  SetRemoteAnswer(answer);
+
+  auto offerPairs = mSessionOff.GetNegotiatedTrackPairs();
+  ASSERT_EQ(2U, offerPairs.size());
+  ASSERT_TRUE(offerPairs[1].mSending);
+  ASSERT_TRUE(offerPairs[1].mReceiving);
+  ASSERT_TRUE(offerPairs[1].mSending->GetNegotiatedDetails());
+  ASSERT_TRUE(offerPairs[1].mReceiving->GetNegotiatedDetails());
+  ASSERT_EQ(6U,
+      offerPairs[1].mSending->GetNegotiatedDetails()->GetEncoding(0)
+      .GetCodecs().size());
+  ASSERT_EQ(6U,
+      offerPairs[1].mReceiving->GetNegotiatedDetails()->GetEncoding(0)
+      .GetCodecs().size());
+
+  auto answerPairs = mSessionAns.GetNegotiatedTrackPairs();
+  ASSERT_EQ(2U, answerPairs.size());
+  ASSERT_TRUE(answerPairs[1].mSending);
+  ASSERT_TRUE(answerPairs[1].mReceiving);
+  ASSERT_TRUE(answerPairs[1].mSending->GetNegotiatedDetails());
+  ASSERT_TRUE(answerPairs[1].mReceiving->GetNegotiatedDetails());
+  ASSERT_EQ(6U,
+      answerPairs[1].mSending->GetNegotiatedDetails()->GetEncoding(0)
+      .GetCodecs().size());
+  ASSERT_EQ(6U,
+      answerPairs[1].mReceiving->GetNegotiatedDetails()->GetEncoding(0)
+      .GetCodecs().size());
+}
+
 TEST_F(JsepSessionTest, ValidateAnsweredCodecParams)
 {
   // TODO(bug 1099351): Once fixed, we can allow red in this offer,
   // which will also cause multiple codecs in answer.  For now,
   // red/ulpfec for video are behind a pref to mitigate potential for
   // errors.
   SetCodecEnabled(mSessionOff, "red", false);
   for (auto i = mSessionAns.Codecs().begin(); i != mSessionAns.Codecs().end();
--- a/media/webrtc/signaling/gtest/sdp_unittests.cpp
+++ b/media/webrtc/signaling/gtest/sdp_unittests.cpp
@@ -2483,16 +2483,41 @@ TEST_P(NewSdpTest, CheckRedNoFmtp) {
   ASSERT_EQ(3U, video_format_params.size());
 
   // make sure we don't get a fmtp for codec 122
   for (size_t i = 0; i < video_format_params.size(); ++i) {
     ASSERT_NE("122", video_format_params[i].format);
   }
 }
 
+TEST_P(NewSdpTest, CheckRedEmptyFmtp) {
+  // if serializing and re-parsing, we expect errors
+  if (GetParam()) {
+    ParseSdp(kVideoWithRedAndUlpfecSdp + "a=fmtp:122" CRLF);
+  } else {
+    ParseSdp(kVideoWithRedAndUlpfecSdp + "a=fmtp:122" CRLF, false);
+    ASSERT_NE(0U, GetParseErrors().size());
+  }
+
+  ASSERT_TRUE(!!mSdp) << "Parse failed: " << GetParseErrors();
+  ASSERT_EQ(1U, mSdp->GetMediaSectionCount())
+    << "Wrong number of media sections";
+
+  ASSERT_TRUE(mSdp->GetMediaSection(0).GetAttributeList().HasAttribute(
+              SdpAttribute::kFmtpAttribute));
+  auto video_format_params =
+      mSdp->GetMediaSection(0).GetAttributeList().GetFmtp().mFmtps;
+  ASSERT_EQ(3U, video_format_params.size());
+
+  // make sure we don't get a fmtp for codec 122
+  for (size_t i = 0; i < video_format_params.size(); ++i) {
+    ASSERT_NE("122", video_format_params[i].format);
+  }
+}
+
 TEST_P(NewSdpTest, CheckRedFmtpWith2Codecs) {
   ParseSdp(kVideoWithRedAndUlpfecSdp + "a=fmtp:122 120/121" CRLF);
   ASSERT_TRUE(!!mSdp) << "Parse failed: " << GetParseErrors();
   ASSERT_EQ(1U, mSdp->GetMediaSectionCount())
     << "Wrong number of media sections";
 
   ASSERT_TRUE(mSdp->GetMediaSection(0).GetAttributeList().HasAttribute(
               SdpAttribute::kFmtpAttribute));
--- a/media/webrtc/signaling/src/jsep/JsepCodecDescription.h
+++ b/media/webrtc/signaling/src/jsep/JsepCodecDescription.h
@@ -313,17 +313,17 @@ class JsepVideoCodecDescription : public
       }
 
       // Parameters that apply to both the send and recv directions
       h264Params.packetization_mode = mPacketizationMode;
       // Hard-coded, may need to change someday?
       h264Params.level_asymmetry_allowed = true;
 
       msection.SetFmtp(SdpFmtpAttributeList::Fmtp(mDefaultPt, h264Params));
-    } else if (mName == "red") {
+    } else if (mName == "red" && mRedundantEncodings.size()) {
       SdpFmtpAttributeList::RedParameters redParams(
           GetRedParameters(mDefaultPt, msection));
       redParams.encodings = mRedundantEncodings;
       msection.SetFmtp(SdpFmtpAttributeList::Fmtp(mDefaultPt, redParams));
     } else if (mName == "VP8" || mName == "VP9") {
       if (mDirection == sdp::kRecv) {
         // VP8 and VP9 share the same SDP parameters thus far
         SdpFmtpAttributeList::VP8Parameters vp8Params(