diff --git a/talk/app/webrtc/webrtcsdp.cc b/talk/app/webrtc/webrtcsdp.cc index b2b33fa27..be1fe47ca 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 ab711edfb..9267c4c6a 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 f30af15ce..a04f6b994 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 24b6e84af..d60da6fbd 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 831527907..b72851796 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 9ee60652d..f487baa9c 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