diff --git a/pc/mediasession.cc b/pc/mediasession.cc index 87222bec31..8079ae140c 100644 --- a/pc/mediasession.cc +++ b/pc/mediasession.cc @@ -833,6 +833,12 @@ static bool FindMatchingCodec(const std::vector& codecs1, const std::vector& codecs2, const C& codec_to_match, C* found_codec) { + // |codec_to_match| should be a member of |codecs1|, in order to look up RTX + // codecs' associated codecs correctly. If not, that's a programming error. + RTC_DCHECK(std::find_if(codecs1.begin(), codecs1.end(), + [&codec_to_match](const C& codec) { + return &codec == &codec_to_match; + }) != codecs1.end()); for (const C& potential_match : codecs2) { if (potential_match.Matches(codec_to_match)) { if (IsRtxCodec(codec_to_match)) { @@ -1859,8 +1865,8 @@ bool MediaSessionDescriptionFactory::AddAudioContentForOffer( static_cast( current_content->description); for (const AudioCodec& codec : acd->codecs()) { - if (FindMatchingCodec(supported_audio_codecs, audio_codecs, - codec, nullptr)) { + if (FindMatchingCodec(acd->codecs(), audio_codecs, codec, + nullptr)) { filtered_codecs.push_back(codec); } } @@ -1937,7 +1943,7 @@ bool MediaSessionDescriptionFactory::AddVideoContentForOffer( static_cast( current_content->description); for (const VideoCodec& codec : vcd->codecs()) { - if (FindMatchingCodec(video_codecs_, video_codecs, codec, + if (FindMatchingCodec(vcd->codecs(), video_codecs, codec, nullptr)) { filtered_codecs.push_back(codec); } @@ -2100,8 +2106,8 @@ bool MediaSessionDescriptionFactory::AddAudioContentForAnswer( static_cast( current_content->description); for (const AudioCodec& codec : acd->codecs()) { - if (FindMatchingCodec(supported_audio_codecs, audio_codecs, - codec, nullptr)) { + if (FindMatchingCodec(acd->codecs(), audio_codecs, codec, + nullptr)) { filtered_codecs.push_back(codec); } } @@ -2183,7 +2189,7 @@ bool MediaSessionDescriptionFactory::AddVideoContentForAnswer( static_cast( current_content->description); for (const VideoCodec& codec : vcd->codecs()) { - if (FindMatchingCodec(video_codecs_, video_codecs, codec, + if (FindMatchingCodec(vcd->codecs(), video_codecs, codec, nullptr)) { filtered_codecs.push_back(codec); } diff --git a/pc/mediasession_unittest.cc b/pc/mediasession_unittest.cc index 03ef0d85a5..95c7afff3b 100644 --- a/pc/mediasession_unittest.cc +++ b/pc/mediasession_unittest.cc @@ -2077,6 +2077,64 @@ TEST_F(MediaSessionDescriptionFactoryTest, EXPECT_EQ(expected_codecs, updated_vcd->codecs()); } +// Regression test for: +// https://bugs.chromium.org/p/webrtc/issues/detail?id=8332 +// Existing codecs should always appear before new codecs in re-offers. But +// under a specific set of circumstances, the existing RTX codec was ending up +// added to the end of the list. +TEST_F(MediaSessionDescriptionFactoryTest, + RespondentCreatesOfferAfterCreatingAnswerWithRemappedRtxPayloadType) { + MediaSessionOptions opts; + AddMediaSection(MEDIA_TYPE_VIDEO, "video", cricket::MD_RECVONLY, kActive, + &opts); + // We specifically choose different preferred payload types for VP8 to + // trigger the issue. + cricket::VideoCodec vp8_offerer(100, "VP8"); + cricket::VideoCodec vp8_offerer_rtx = + VideoCodec::CreateRtxCodec(101, vp8_offerer.id); + cricket::VideoCodec vp8_answerer(110, "VP8"); + cricket::VideoCodec vp8_answerer_rtx = + VideoCodec::CreateRtxCodec(111, vp8_answerer.id); + cricket::VideoCodec vp9(120, "VP9"); + cricket::VideoCodec vp9_rtx = VideoCodec::CreateRtxCodec(121, vp9.id); + + std::vector f1_codecs = {vp8_offerer, vp8_offerer_rtx}; + // We also specifically cause the answerer to prefer VP9, such that if it + // *doesn't* honor the existing preferred codec (VP8) we'll notice. + std::vector f2_codecs = {vp9, vp9_rtx, vp8_answerer, + vp8_answerer_rtx}; + + f1_.set_video_codecs(f1_codecs); + f2_.set_video_codecs(f2_codecs); + std::vector audio_codecs; + f1_.set_audio_codecs(audio_codecs, audio_codecs); + f2_.set_audio_codecs(audio_codecs, audio_codecs); + + // Offer will be {VP8, RTX for VP8}. Answer will be the same. + std::unique_ptr offer(f1_.CreateOffer(opts, NULL)); + ASSERT_TRUE(offer.get() != NULL); + std::unique_ptr answer( + f2_.CreateAnswer(offer.get(), opts, NULL)); + + // Updated offer *should* be {VP8, RTX for VP8, VP9, RTX for VP9}. + // But if the bug is triggered, RTX for VP8 ends up last. + std::unique_ptr updated_offer( + f2_.CreateOffer(opts, answer.get())); + + const VideoContentDescription* vcd = + GetFirstVideoContentDescription(updated_offer.get()); + std::vector codecs = vcd->codecs(); + ASSERT_EQ(4u, codecs.size()); + EXPECT_EQ(vp8_offerer, codecs[0]); + EXPECT_EQ(vp8_offerer_rtx, codecs[1]); + EXPECT_EQ(vp9, codecs[2]); + EXPECT_EQ(vp9_rtx, codecs[3]); + LOG(LS_INFO) << "Offer codecs: "; + for (auto codec : codecs) { + LOG(LS_INFO) << codec.ToString(); + } +} + // Create an updated offer that adds video after creating an audio only answer // to the original offer. This test verifies that if a video codec and the RTX // codec have the same default payload type as an audio codec that is already in