diff --git a/webrtc/pc/mediasession.cc b/webrtc/pc/mediasession.cc index f3c4a3c912..643c9f2457 100644 --- a/webrtc/pc/mediasession.cc +++ b/webrtc/pc/mediasession.cc @@ -10,6 +10,7 @@ #include "webrtc/pc/mediasession.h" +#include // For std::find_if. #include #include #include @@ -810,57 +811,57 @@ template static void NegotiateCodecs(const std::vector& local_codecs, const std::vector& offered_codecs, std::vector* negotiated_codecs) { - typename std::vector::const_iterator ours; - for (ours = local_codecs.begin(); - ours != local_codecs.end(); ++ours) { - typename std::vector::const_iterator theirs; - for (theirs = offered_codecs.begin(); - theirs != offered_codecs.end(); ++theirs) { - if (ours->Matches(*theirs)) { - C negotiated = *ours; - negotiated.IntersectFeedbackParams(*theirs); - if (IsRtxCodec(negotiated)) { - 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; - } - // 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 - // specific reason, the answerer list formats in the same relative order - // they were present in the offer. - negotiated.preference = theirs->preference; - negotiated_codecs->push_back(negotiated); + for (const C& ours : local_codecs) { + C theirs; + if (FindMatchingCodec(local_codecs, offered_codecs, ours, &theirs)) { + C negotiated = ours; + negotiated.IntersectFeedbackParams(theirs); + if (IsRtxCodec(negotiated)) { + std::string offered_apt_value; + theirs.GetParam(kCodecParamAssociatedPayloadType, &offered_apt_value); + // FindMatchingCodec shouldn't return something with no apt value. + RTC_DCHECK(!offered_apt_value.empty()); + 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 + // specific reason, the answerer list formats in the same relative order + // they were present in the offer. + negotiated.preference = theirs.preference; + negotiated_codecs->push_back(negotiated); } } } +// Finds a codec in |codecs2| that matches |codec_to_match|, which is +// a member of |codecs1|. If |codec_to_match| is an RTX codec, both +// the codecs themselves and their associated codecs must match. template -static bool FindMatchingCodec(const std::vector& codecs, +static bool FindMatchingCodec(const std::vector& codecs1, + const std::vector& codecs2, const C& codec_to_match, C* found_codec) { - for (typename std::vector::const_iterator it = codecs.begin(); - it != codecs.end(); ++it) { - if (it->Matches(codec_to_match)) { - if (found_codec != NULL) { - *found_codec= *it; + for (const C& potential_match : codecs2) { + if (potential_match.Matches(codec_to_match)) { + if (IsRtxCodec(codec_to_match)) { + std::string apt_value_1; + std::string apt_value_2; + if (!codec_to_match.GetParam(kCodecParamAssociatedPayloadType, + &apt_value_1) || + !potential_match.GetParam(kCodecParamAssociatedPayloadType, + &apt_value_2)) { + LOG(LS_WARNING) << "RTX missing associated payload type."; + continue; + } + if (!ReferencedCodecsMatch(codecs1, apt_value_1, codecs2, + apt_value_2)) { + continue; + } + } + if (found_codec) { + *found_codec = potential_match; } return true; } @@ -877,49 +878,64 @@ static void FindCodecsToOffer( std::vector* offered_codecs, UsedPayloadTypes* used_pltypes) { - typedef std::map RtxCodecReferences; - RtxCodecReferences new_rtx_codecs; - - // Find all new RTX codecs. - for (typename std::vector::const_iterator it = reference_codecs.begin(); - it != reference_codecs.end(); ++it) { - if (!FindMatchingCodec(*offered_codecs, *it, NULL) && IsRtxCodec(*it)) { - C rtx_codec = *it; - int referenced_pl_type = - rtc::FromString(0, - rtx_codec.params[kCodecParamAssociatedPayloadType]); - new_rtx_codecs.insert(std::pair(referenced_pl_type, - rtx_codec)); - } - } - // Add all new codecs that are not RTX codecs. - for (typename std::vector::const_iterator it = reference_codecs.begin(); - it != reference_codecs.end(); ++it) { - if (!FindMatchingCodec(*offered_codecs, *it, NULL) && !IsRtxCodec(*it)) { - C codec = *it; - int original_payload_id = codec.id; + for (const C& reference_codec : reference_codecs) { + if (!IsRtxCodec(reference_codec) && + !FindMatchingCodec(reference_codecs, *offered_codecs, + reference_codec, nullptr)) { + C codec = reference_codec; used_pltypes->FindAndSetIdUsed(&codec); offered_codecs->push_back(codec); - - // If this codec is referenced by a new RTX codec, update the reference - // in the RTX codec with the new payload type. - typename RtxCodecReferences::iterator rtx_it = - new_rtx_codecs.find(original_payload_id); - if (rtx_it != new_rtx_codecs.end()) { - C& rtx_codec = rtx_it->second; - rtx_codec.params[kCodecParamAssociatedPayloadType] = - rtc::ToString(codec.id); - } } } // Add all new RTX codecs. - for (typename RtxCodecReferences::iterator it = new_rtx_codecs.begin(); - it != new_rtx_codecs.end(); ++it) { - C& rtx_codec = it->second; - used_pltypes->FindAndSetIdUsed(&rtx_codec); - offered_codecs->push_back(rtx_codec); + for (const C& reference_codec : reference_codecs) { + if (IsRtxCodec(reference_codec) && + !FindMatchingCodec(reference_codecs, *offered_codecs, + reference_codec, nullptr)) { + C rtx_codec = reference_codec; + + std::string associated_pt_str; + if (!rtx_codec.GetParam(kCodecParamAssociatedPayloadType, + &associated_pt_str)) { + LOG(LS_WARNING) << "RTX codec " << rtx_codec.name + << " is missing an associated payload type."; + continue; + } + + int associated_pt; + if (!rtc::FromString(associated_pt_str, &associated_pt)) { + LOG(LS_WARNING) << "Couldn't convert payload type " << associated_pt_str + << " of RTX codec " << rtx_codec.name + << " to an integer."; + continue; + } + + // Find the associated reference codec for the reference RTX codec. + C associated_codec; + if (!FindCodecById(reference_codecs, associated_pt, &associated_codec)) { + LOG(LS_WARNING) << "Couldn't find associated codec with payload type " + << associated_pt << " for RTX codec " << rtx_codec.name + << "."; + continue; + } + + // Find a codec in the offered list that matches the reference codec. + // Its payload type may be different than the reference codec. + C matching_codec; + if (!FindMatchingCodec(reference_codecs, *offered_codecs, + associated_codec, &matching_codec)) { + LOG(LS_WARNING) << "Couldn't find matching " << associated_codec.name + << " codec."; + continue; + } + + rtx_codec.params[kCodecParamAssociatedPayloadType] = + rtc::ToString(matching_codec.id); + used_pltypes->FindAndSetIdUsed(&rtx_codec); + offered_codecs->push_back(rtx_codec); + } } } diff --git a/webrtc/pc/mediasession_unittest.cc b/webrtc/pc/mediasession_unittest.cc index 6887194b58..66204b59c7 100644 --- a/webrtc/pc/mediasession_unittest.cc +++ b/webrtc/pc/mediasession_unittest.cc @@ -1654,6 +1654,48 @@ TEST_F(MediaSessionDescriptionFactoryTest, EXPECT_EQ(new_h264_pl_type, pt_referenced_by_rtx); } +// Create an updated offer with RTX after creating an answer to an offer +// without RTX, and with different default payload types. +// Verify that the added RTX codec references the correct payload type. +TEST_F(MediaSessionDescriptionFactoryTest, + RespondentCreatesOfferWithRtxAfterCreatingAnswerWithoutRtx) { + MediaSessionOptions opts; + opts.recv_video = true; + opts.recv_audio = true; + + std::vector f2_codecs = MAKE_VECTOR(kVideoCodecs2); + // 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, nullptr)); + ASSERT_TRUE(offer.get() != nullptr); + rtc::scoped_ptr answer( + f2_.CreateAnswer(offer.get(), opts, nullptr)); + + const VideoContentDescription* vcd = + GetFirstVideoContentDescription(answer.get()); + + std::vector expected_codecs = MAKE_VECTOR(kVideoCodecsAnswer); + EXPECT_EQ(expected_codecs, vcd->codecs()); + + // Now, ensure that the RTX codec is created correctly when |f2_| creates an + // updated offer, even though the default payload types are different from + // those of |f1_|. + rtc::scoped_ptr updated_offer( + f2_.CreateOffer(opts, answer.get())); + ASSERT_TRUE(updated_offer); + + const VideoContentDescription* updated_vcd = + GetFirstVideoContentDescription(updated_offer.get()); + + // New offer should attempt to add H263, and RTX for H264. + expected_codecs.push_back(kVideoCodecs2[1]); + AddRtxCodec(VideoCodec::CreateRtxCodec(125, kVideoCodecs1[1].id), + &expected_codecs); + EXPECT_EQ(expected_codecs, updated_vcd->codecs()); +} + // Test that RTX is ignored when there is no associated payload type parameter. TEST_F(MediaSessionDescriptionFactoryTest, RtxWithoutApt) { MediaSessionOptions opts; @@ -1762,6 +1804,41 @@ TEST_F(MediaSessionDescriptionFactoryTest, EXPECT_EQ(expected_codecs, vcd->codecs()); } +// Test that after one RTX codec has been negotiated, a new offer can attempt +// to add another. +TEST_F(MediaSessionDescriptionFactoryTest, AddSecondRtxInNewOffer) { + MediaSessionOptions opts; + opts.recv_video = true; + opts.recv_audio = false; + std::vector f1_codecs = MAKE_VECTOR(kVideoCodecs1); + // This creates RTX for H264 for the offerer. + AddRtxCodec(VideoCodec::CreateRtxCodec(126, kVideoCodecs1[1].id), &f1_codecs); + f1_.set_video_codecs(f1_codecs); + + rtc::scoped_ptr offer(f1_.CreateOffer(opts, nullptr)); + ASSERT_TRUE(offer); + const VideoContentDescription* vcd = + GetFirstVideoContentDescription(offer.get()); + + std::vector expected_codecs = MAKE_VECTOR(kVideoCodecs1); + AddRtxCodec(VideoCodec::CreateRtxCodec(126, kVideoCodecs1[1].id), + &expected_codecs); + EXPECT_EQ(expected_codecs, vcd->codecs()); + + // Now, attempt to add RTX for H264-SVC. + AddRtxCodec(VideoCodec::CreateRtxCodec(125, kVideoCodecs1[0].id), &f1_codecs); + f1_.set_video_codecs(f1_codecs); + + rtc::scoped_ptr updated_offer( + f1_.CreateOffer(opts, offer.get())); + ASSERT_TRUE(updated_offer); + vcd = GetFirstVideoContentDescription(updated_offer.get()); + + AddRtxCodec(VideoCodec::CreateRtxCodec(125, kVideoCodecs1[0].id), + &expected_codecs); + EXPECT_EQ(expected_codecs, vcd->codecs()); +} + // Test that when RTX is used in conjunction with simulcast, an RTX ssrc is // generated for each simulcast ssrc and correctly grouped. TEST_F(MediaSessionDescriptionFactoryTest, SimSsrcsGenerateMultipleRtxSsrcs) {