Bug 1328429 - don't output empty fmtp line when no redundant encodings are indicated for RED. r=drno
MozReview-Commit-ID: GYK8UMegRjL
--- 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(