From 2d25b44f470afdd56513b75d641166f6e7cdcd04 Mon Sep 17 00:00:00 2001 From: "changbin.shao@webrtc.org" Date: Mon, 16 Mar 2015 04:14:34 +0000 Subject: [PATCH] Check associated payload type when negotiate RTX codecs. At the moment, only payload name is checked when match two RTX codecs. This will cause wrong behavior of codec negotiation if multiple RTX codecs are added. BUG= R=pthatcher@webrtc.org Review URL: https://webrtc-codereview.appspot.com/34189004 Cr-Commit-Position: refs/heads/master@{#8727} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8727 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/app/webrtc/webrtcsdp.cc | 26 ++-- talk/app/webrtc/webrtcsession_unittest.cc | 42 ++++++ talk/media/base/codec.h | 15 +++ talk/media/base/rtpdataengine.cc | 14 -- talk/session/media/mediasession.cc | 39 +++++- talk/session/media/mediasession_unittest.cc | 137 +++++++++++++------- 6 files changed, 196 insertions(+), 77 deletions(-) diff --git a/talk/app/webrtc/webrtcsdp.cc b/talk/app/webrtc/webrtcsdp.cc index b2b33fa27d..be1fe47ca3 100644 --- a/talk/app/webrtc/webrtcsdp.cc +++ b/talk/app/webrtc/webrtcsdp.cc @@ -2380,18 +2380,14 @@ void AddFeedbackParameters(const cricket::FeedbackParams& feedback_params, } // Gets the current codec setting associated with |payload_type|. If there -// is no AudioCodec associated with that payload type it returns an empty codec +// is no Codec associated with that payload type it returns an empty codec // with that payload type. template -T GetCodec(const std::vector& codecs, int payload_type) { - for (typename std::vector::const_iterator codec = codecs.begin(); - codec != codecs.end(); ++codec) { - if (codec->id == payload_type) { - return *codec; - } +T GetCodecWithPayloadType(const std::vector& codecs, int payload_type) { + T ret_val; + if (!FindCodecById(codecs, payload_type, &ret_val)) { + ret_val.id = payload_type; } - T ret_val = T(); - ret_val.id = payload_type; return ret_val; } @@ -2423,7 +2419,8 @@ template void UpdateCodec(MediaContentDescription* content_desc, int payload_type, const cricket::CodecParameterMap& parameters) { // Codec might already have been populated (from rtpmap). - U new_codec = GetCodec(static_cast(content_desc)->codecs(), payload_type); + U new_codec = GetCodecWithPayloadType(static_cast(content_desc)->codecs(), + payload_type); AddParameters(parameters, &new_codec); AddOrReplaceCodec(content_desc, new_codec); } @@ -2434,7 +2431,8 @@ template void UpdateCodec(MediaContentDescription* content_desc, int payload_type, const cricket::FeedbackParam& feedback_param) { // Codec might already have been populated (from rtpmap). - U new_codec = GetCodec(static_cast(content_desc)->codecs(), payload_type); + U new_codec = GetCodecWithPayloadType(static_cast(content_desc)->codecs(), + payload_type); AddFeedbackParameter(feedback_param, &new_codec); AddOrReplaceCodec(content_desc, new_codec); } @@ -2889,7 +2887,8 @@ void UpdateCodec(int payload_type, const std::string& name, int clockrate, AudioContentDescription* audio_desc) { // Codec may already be populated with (only) optional parameters // (from an fmtp). - cricket::AudioCodec codec = GetCodec(audio_desc->codecs(), payload_type); + cricket::AudioCodec codec = + GetCodecWithPayloadType(audio_desc->codecs(), payload_type); codec.name = name; codec.clockrate = clockrate; codec.bitrate = bitrate; @@ -2906,7 +2905,8 @@ void UpdateCodec(int payload_type, const std::string& name, int width, VideoContentDescription* video_desc) { // Codec may already be populated with (only) optional parameters // (from an fmtp). - cricket::VideoCodec codec = GetCodec(video_desc->codecs(), payload_type); + cricket::VideoCodec codec = + GetCodecWithPayloadType(video_desc->codecs(), payload_type); codec.name = name; codec.width = width; codec.height = height; diff --git a/talk/app/webrtc/webrtcsession_unittest.cc b/talk/app/webrtc/webrtcsession_unittest.cc index ab711edfb8..9267c4c6a9 100644 --- a/talk/app/webrtc/webrtcsession_unittest.cc +++ b/talk/app/webrtc/webrtcsession_unittest.cc @@ -136,6 +136,26 @@ static const char kTooLongIceUfragPwd[] = "IceUfragIceUfragIceUfragIceUfragIceUfragIceUfragIceUfragIceUfragIceUfrag" "IceUfragIceUfragIceUfragIceUfragIceUfragIceUfragIceUfragIceUfragIceUfrag"; +static const char kSdpWithRtx[] = + "v=0\r\n" + "o=- 4104004319237231850 2 IN IP4 127.0.0.1\r\n" + "s=-\r\n" + "t=0 0\r\n" + "a=msid-semantic: WMS stream1\r\n" + "m=video 9 RTP/SAVPF 0 96\r\n" + "c=IN IP4 0.0.0.0\r\n" + "a=rtcp:9 IN IP4 0.0.0.0\r\n" + "a=ice-ufrag:CerjGp19G7wpXwl7\r\n" + "a=ice-pwd:cMvOlFvQ6ochez1ZOoC2uBEC\r\n" + "a=mid:video\r\n" + "a=sendrecv\r\n" + "a=rtcp-mux\r\n" + "a=crypto:1 AES_CM_128_HMAC_SHA1_80 " + "inline:5/4N5CDvMiyDArHtBByUM71VIkguH17ZNoX60GrA\r\n" + "a=rtpmap:0 fake_video_codec/90000\r\n" + "a=rtpmap:96 rtx/90000\r\n" + "a=fmtp:96 apt=0\r\n"; + // Add some extra |newlines| to the |message| after |line|. static void InjectAfter(const std::string& line, const std::string& newlines, @@ -3546,6 +3566,28 @@ TEST_F(WebRtcSessionTest, TestRenegotiateNewMediaWithCandidatesSeparated) { answer = CreateAnswer(NULL); SetLocalDescriptionWithoutError(answer); } +// Tests that RTX codec is removed from the answer when it isn't supported +// by local side. +TEST_F(WebRtcSessionTest, TestRtxRemovedByCreateAnswer) { + Init(); + mediastream_signaling_.SendAudioVideoStream1(); + std::string offer_sdp(kSdpWithRtx); + + SessionDescriptionInterface* offer = + CreateSessionDescription(JsepSessionDescription::kOffer, offer_sdp, NULL); + EXPECT_TRUE(offer->ToString(&offer_sdp)); + + // Offer SDP contains the RTX codec. + EXPECT_TRUE(offer_sdp.find("rtx") != std::string::npos); + SetRemoteDescriptionWithoutError(offer); + + SessionDescriptionInterface* answer = CreateAnswer(NULL); + std::string answer_sdp; + answer->ToString(&answer_sdp); + // Answer SDP removes the unsupported RTX codec. + EXPECT_TRUE(answer_sdp.find("rtx") == std::string::npos); + SetLocalDescriptionWithoutError(answer); +} // This verifies that the voice channel after bundle has both options from video // and voice channels. diff --git a/talk/media/base/codec.h b/talk/media/base/codec.h index f30af15ce0..a04f6b994a 100644 --- a/talk/media/base/codec.h +++ b/talk/media/base/codec.h @@ -252,6 +252,21 @@ struct VideoEncoderConfig { int cpu_profile; }; +// Get the codec setting associated with |payload_type|. If there +// is no codec associated with that payload type it returns false. +template +bool FindCodecById(const std::vector& codecs, + int payload_type, + Codec* codec_out) { + for (const auto& codec : codecs) { + if (codec.id == payload_type) { + *codec_out = codec; + return true; + } + } + return false; +} + } // namespace cricket #endif // TALK_MEDIA_BASE_CODEC_H_ diff --git a/talk/media/base/rtpdataengine.cc b/talk/media/base/rtpdataengine.cc index 24b6e84afb..d60da6fbd7 100644 --- a/talk/media/base/rtpdataengine.cc +++ b/talk/media/base/rtpdataengine.cc @@ -66,20 +66,6 @@ DataMediaChannel* RtpDataEngine::CreateChannel( return new RtpDataMediaChannel(timing_.get()); } -// TODO(pthatcher): Should we move these find/get functions somewhere -// common? -bool FindCodecById(const std::vector& codecs, - int id, DataCodec* codec_out) { - std::vector::const_iterator iter; - for (iter = codecs.begin(); iter != codecs.end(); ++iter) { - if (iter->id == id) { - *codec_out = *iter; - return true; - } - } - return false; -} - bool FindCodecByName(const std::vector& codecs, const std::string& name, DataCodec* codec_out) { std::vector::const_iterator iter; diff --git a/talk/session/media/mediasession.cc b/talk/session/media/mediasession.cc index 831527907a..b728517961 100644 --- a/talk/session/media/mediasession.cc +++ b/talk/session/media/mediasession.cc @@ -751,6 +751,24 @@ static bool CreateMediaContentOffer( return true; } +template +static bool ReferencedCodecsMatch(const std::vector& codecs1, + const std::string& codec1_id_str, + const std::vector& codecs2, + const std::string& codec2_id_str) { + int codec1_id; + int codec2_id; + C codec1; + C codec2; + if (!rtc::FromString(codec1_id_str, &codec1_id) || + !rtc::FromString(codec2_id_str, &codec2_id) || + !FindCodecById(codecs1, codec1_id, &codec1) || + !FindCodecById(codecs2, codec2_id, &codec2)) { + return false; + } + return codec1.Matches(codec2); +} + template static void NegotiateCodecs(const std::vector& local_codecs, const std::vector& offered_codecs, @@ -765,15 +783,26 @@ static void NegotiateCodecs(const std::vector& local_codecs, C negotiated = *ours; negotiated.IntersectFeedbackParams(*theirs); if (IsRtxCodec(negotiated)) { - // Only negotiate RTX if kCodecParamAssociatedPayloadType has been - // set. - std::string apt_value; - if (!theirs->GetParam(kCodecParamAssociatedPayloadType, &apt_value)) { + std::string offered_apt_value; + std::string local_apt_value; + if (!ours->GetParam(kCodecParamAssociatedPayloadType, + &local_apt_value) || + !theirs->GetParam(kCodecParamAssociatedPayloadType, + &offered_apt_value)) { LOG(LS_WARNING) << "RTX missing associated payload type."; continue; } - negotiated.SetParam(kCodecParamAssociatedPayloadType, apt_value); + // Only negotiate RTX if kCodecParamAssociatedPayloadType has been + // set in local and remote codecs, and they match. + if (!ReferencedCodecsMatch(local_codecs, local_apt_value, + offered_codecs, offered_apt_value)) { + LOG(LS_WARNING) << "RTX associated codecs don't match."; + continue; + } + negotiated.SetParam(kCodecParamAssociatedPayloadType, + offered_apt_value); } + negotiated.id = theirs->id; // RFC3264: Although the answerer MAY list the formats in their desired // order of preference, it is RECOMMENDED that unless there is a diff --git a/talk/session/media/mediasession_unittest.cc b/talk/session/media/mediasession_unittest.cc index 9ee60652d3..f487baa9c5 100644 --- a/talk/session/media/mediasession_unittest.cc +++ b/talk/session/media/mediasession_unittest.cc @@ -197,6 +197,22 @@ GetMediaDirection(const ContentInfo* content) { return desc->direction(); } +static void AddRtxCodec(const VideoCodec& rtx_codec, + std::vector* codecs) { + VideoCodec rtx; + ASSERT_FALSE(cricket::FindCodecById(*codecs, rtx_codec.id, &rtx)); + codecs->push_back(rtx_codec); +} + +template +static std::vector GetCodecNames(const std::vector& codecs) { + std::vector codec_names; + for (const auto& codec : codecs) { + codec_names.push_back(codec.name); + } + return codec_names; +} + class MediaSessionDescriptionFactoryTest : public testing::Test { public: MediaSessionDescriptionFactoryTest() @@ -1545,25 +1561,13 @@ TEST_F(MediaSessionDescriptionFactoryTest, opts.recv_video = true; opts.recv_audio = false; std::vector f1_codecs = MAKE_VECTOR(kVideoCodecs1); - VideoCodec rtx_f1; - rtx_f1.id = 126; - rtx_f1.name = cricket::kRtxCodecName; - // This creates rtx for H264 with the payload type |f1_| uses. - rtx_f1.params[cricket::kCodecParamAssociatedPayloadType] = - rtc::ToString(kVideoCodecs1[1].id); - f1_codecs.push_back(rtx_f1); + AddRtxCodec(VideoCodec::CreateRtxCodec(126, kVideoCodecs1[1].id), &f1_codecs); f1_.set_video_codecs(f1_codecs); std::vector f2_codecs = MAKE_VECTOR(kVideoCodecs2); - VideoCodec rtx_f2; - rtx_f2.id = 127; - rtx_f2.name = cricket::kRtxCodecName; - // This creates rtx for H264 with the payload type |f2_| uses. - rtx_f2.params[cricket::kCodecParamAssociatedPayloadType] = - rtc::ToString(kVideoCodecs2[0].id); - f2_codecs.push_back(rtx_f2); + AddRtxCodec(VideoCodec::CreateRtxCodec(125, kVideoCodecs2[0].id), &f2_codecs); f2_.set_video_codecs(f2_codecs); rtc::scoped_ptr offer(f1_.CreateOffer(opts, NULL)); @@ -1575,7 +1579,8 @@ TEST_F(MediaSessionDescriptionFactoryTest, GetFirstVideoContentDescription(answer.get()); std::vector expected_codecs = MAKE_VECTOR(kVideoCodecsAnswer); - expected_codecs.push_back(rtx_f1); + AddRtxCodec(VideoCodec::CreateRtxCodec(126, kVideoCodecs1[1].id), + &expected_codecs); EXPECT_EQ(expected_codecs, vcd->codecs()); @@ -1603,14 +1608,8 @@ TEST_F(MediaSessionDescriptionFactoryTest, TEST_F(MediaSessionDescriptionFactoryTest, RespondentCreatesOfferWithVideoAndRtxAfterCreatingAudioAnswer) { std::vector f1_codecs = MAKE_VECTOR(kVideoCodecs1); - VideoCodec rtx_f1; - rtx_f1.id = 126; - rtx_f1.name = cricket::kRtxCodecName; - // This creates rtx for H264 with the payload type |f1_| uses. - rtx_f1.params[cricket::kCodecParamAssociatedPayloadType] = - rtc::ToString(kVideoCodecs1[1].id); - f1_codecs.push_back(rtx_f1); + AddRtxCodec(VideoCodec::CreateRtxCodec(126, kVideoCodecs1[1].id), &f1_codecs); f1_.set_video_codecs(f1_codecs); MediaSessionOptions opts; @@ -1634,12 +1633,7 @@ TEST_F(MediaSessionDescriptionFactoryTest, std::vector f2_codecs = MAKE_VECTOR(kVideoCodecs2); int used_pl_type = acd->codecs()[0].id; f2_codecs[0].id = used_pl_type; // Set the payload type for H264. - VideoCodec rtx_f2; - rtx_f2.id = 127; - rtx_f2.name = cricket::kRtxCodecName; - rtx_f2.params[cricket::kCodecParamAssociatedPayloadType] = - rtc::ToString(used_pl_type); - f2_codecs.push_back(rtx_f2); + AddRtxCodec(VideoCodec::CreateRtxCodec(125, used_pl_type), &f2_codecs); f2_.set_video_codecs(f2_codecs); rtc::scoped_ptr updated_offer( @@ -1671,22 +1665,13 @@ TEST_F(MediaSessionDescriptionFactoryTest, RtxWithoutApt) { opts.recv_video = true; opts.recv_audio = false; std::vector f1_codecs = MAKE_VECTOR(kVideoCodecs1); - VideoCodec rtx_f1; - rtx_f1.id = 126; - rtx_f1.name = cricket::kRtxCodecName; - - f1_codecs.push_back(rtx_f1); + // This creates RTX without associated payload type parameter. + AddRtxCodec(VideoCodec(126, cricket::kRtxCodecName, 0, 0, 0, 0), &f1_codecs); f1_.set_video_codecs(f1_codecs); std::vector f2_codecs = MAKE_VECTOR(kVideoCodecs2); - VideoCodec rtx_f2; - rtx_f2.id = 127; - rtx_f2.name = cricket::kRtxCodecName; - - // This creates rtx for H264 with the payload type |f2_| uses. - rtx_f2.SetParam(cricket::kCodecParamAssociatedPayloadType, - rtc::ToString(kVideoCodecs2[0].id)); - f2_codecs.push_back(rtx_f2); + // This creates RTX for H264 with the payload type |f2_| uses. + AddRtxCodec(VideoCodec::CreateRtxCodec(125, kVideoCodecs2[0].id), &f2_codecs); f2_.set_video_codecs(f2_codecs); rtc::scoped_ptr offer(f1_.CreateOffer(opts, NULL)); @@ -1711,13 +1696,75 @@ TEST_F(MediaSessionDescriptionFactoryTest, RtxWithoutApt) { rtc::scoped_ptr answer( f2_.CreateAnswer(offer.get(), opts, NULL)); + std::vector codec_names = + GetCodecNames(GetFirstVideoContentDescription(answer.get())->codecs()); + EXPECT_EQ(codec_names.end(), std::find(codec_names.begin(), codec_names.end(), + cricket::kRtxCodecName)); +} + +// Test that RTX will be filtered out in the answer if its associated payload +// type doesn't match the local value. +TEST_F(MediaSessionDescriptionFactoryTest, FilterOutRtxIfAptDoesntMatch) { + MediaSessionOptions opts; + opts.recv_video = true; + opts.recv_audio = false; + std::vector f1_codecs = MAKE_VECTOR(kVideoCodecs1); + // This creates RTX for H264 in sender. + AddRtxCodec(VideoCodec::CreateRtxCodec(126, kVideoCodecs1[1].id), &f1_codecs); + f1_.set_video_codecs(f1_codecs); + + std::vector f2_codecs = MAKE_VECTOR(kVideoCodecs2); + // This creates RTX for H263 in receiver. + AddRtxCodec(VideoCodec::CreateRtxCodec(125, kVideoCodecs2[1].id), &f2_codecs); + f2_.set_video_codecs(f2_codecs); + + rtc::scoped_ptr offer(f1_.CreateOffer(opts, NULL)); + ASSERT_TRUE(offer.get() != NULL); + // Associated payload type doesn't match, therefore, RTX codec is removed in + // the answer. + rtc::scoped_ptr answer( + f2_.CreateAnswer(offer.get(), opts, NULL)); + + std::vector codec_names = + GetCodecNames(GetFirstVideoContentDescription(answer.get())->codecs()); + EXPECT_EQ(codec_names.end(), std::find(codec_names.begin(), codec_names.end(), + cricket::kRtxCodecName)); +} + +// Test that when multiple RTX codecs are offered, only the matched RTX codec +// is added in the answer, and the unsupported RTX codec is filtered out. +TEST_F(MediaSessionDescriptionFactoryTest, + FilterOutUnsupportedRtxWhenCreatingAnswer) { + MediaSessionOptions opts; + opts.recv_video = true; + opts.recv_audio = false; + std::vector f1_codecs = MAKE_VECTOR(kVideoCodecs1); + // This creates RTX for H264-SVC in sender. + AddRtxCodec(VideoCodec::CreateRtxCodec(125, kVideoCodecs1[0].id), &f1_codecs); + f1_.set_video_codecs(f1_codecs); + + // This creates RTX for H264 in sender. + AddRtxCodec(VideoCodec::CreateRtxCodec(126, kVideoCodecs1[1].id), &f1_codecs); + f1_.set_video_codecs(f1_codecs); + + std::vector f2_codecs = MAKE_VECTOR(kVideoCodecs2); + // This creates RTX for H264 in receiver. + AddRtxCodec(VideoCodec::CreateRtxCodec(124, kVideoCodecs2[0].id), &f2_codecs); + f2_.set_video_codecs(f2_codecs); + + // H264-SVC codec is removed in the answer, therefore, associated RTX codec + // for H264-SVC should also be removed. + rtc::scoped_ptr offer(f1_.CreateOffer(opts, NULL)); + ASSERT_TRUE(offer.get() != NULL); + rtc::scoped_ptr answer( + f2_.CreateAnswer(offer.get(), opts, NULL)); const VideoContentDescription* vcd = GetFirstVideoContentDescription(answer.get()); + std::vector expected_codecs = MAKE_VECTOR(kVideoCodecsAnswer); + AddRtxCodec(VideoCodec::CreateRtxCodec(126, kVideoCodecs1[1].id), + &expected_codecs); - for (std::vector::const_iterator iter = vcd->codecs().begin(); - iter != vcd->codecs().end(); ++iter) { - ASSERT_STRNE(iter->name.c_str(), cricket::kRtxCodecName); - } + EXPECT_EQ(expected_codecs, vcd->codecs()); } // Create an updated offer after creating an answer to the original offer and