From 1c34974d6fcdf969ba393c6b44dbd9ab3307c4fb Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Tue, 3 Oct 2017 18:25:36 -0700 Subject: [PATCH] Fixing invalid calls to FindMatchingCodec. The first argument of FindMatchingCodec is supposed to be the list that contains the codec to be found, specifically to handle RTX codecs that point to other codecs. But this wasn't being done everywhere, and wasn't noticed because *most of the time* it just results in adding the RTX codec in a different location, which isn't an issue. But, it's still not standards-compliant. And it sometimes is an issue when talking to older endpoints. Adding a regression test, and DCHECK in FindMatchingCodec to ensure this doesn't happen by accident again. Bug: webrtc:8332 Change-Id: I5def056b245c6d00a49a59d429f1dee303fb7cef Reviewed-on: https://webrtc-review.googlesource.com/6240 Reviewed-by: Justin Uberti Commit-Queue: Taylor Brandstetter Cr-Commit-Position: refs/heads/master@{#20130} --- pc/mediasession.cc | 18 ++++++++---- pc/mediasession_unittest.cc | 58 +++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 6 deletions(-) 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