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 <juberti@webrtc.org>
Commit-Queue: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20130}diff --git a/pc/mediasession.cc b/pc/mediasession.cc
index 87222be..8079ae1 100644
--- a/pc/mediasession.cc
+++ b/pc/mediasession.cc
@@ -833,6 +833,12 @@
const std::vector<C>& 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 @@
static_cast<const AudioContentDescription*>(
current_content->description);
for (const AudioCodec& codec : acd->codecs()) {
- if (FindMatchingCodec<AudioCodec>(supported_audio_codecs, audio_codecs,
- codec, nullptr)) {
+ if (FindMatchingCodec<AudioCodec>(acd->codecs(), audio_codecs, codec,
+ nullptr)) {
filtered_codecs.push_back(codec);
}
}
@@ -1937,7 +1943,7 @@
static_cast<const VideoContentDescription*>(
current_content->description);
for (const VideoCodec& codec : vcd->codecs()) {
- if (FindMatchingCodec<VideoCodec>(video_codecs_, video_codecs, codec,
+ if (FindMatchingCodec<VideoCodec>(vcd->codecs(), video_codecs, codec,
nullptr)) {
filtered_codecs.push_back(codec);
}
@@ -2100,8 +2106,8 @@
static_cast<const AudioContentDescription*>(
current_content->description);
for (const AudioCodec& codec : acd->codecs()) {
- if (FindMatchingCodec<AudioCodec>(supported_audio_codecs, audio_codecs,
- codec, nullptr)) {
+ if (FindMatchingCodec<AudioCodec>(acd->codecs(), audio_codecs, codec,
+ nullptr)) {
filtered_codecs.push_back(codec);
}
}
@@ -2183,7 +2189,7 @@
static_cast<const VideoContentDescription*>(
current_content->description);
for (const VideoCodec& codec : vcd->codecs()) {
- if (FindMatchingCodec<VideoCodec>(video_codecs_, video_codecs, codec,
+ if (FindMatchingCodec<VideoCodec>(vcd->codecs(), video_codecs, codec,
nullptr)) {
filtered_codecs.push_back(codec);
}
diff --git a/pc/mediasession_unittest.cc b/pc/mediasession_unittest.cc
index 03ef0d8..95c7aff 100644
--- a/pc/mediasession_unittest.cc
+++ b/pc/mediasession_unittest.cc
@@ -2077,6 +2077,64 @@
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<VideoCodec> 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<VideoCodec> f2_codecs = {vp9, vp9_rtx, vp8_answerer,
+ vp8_answerer_rtx};
+
+ f1_.set_video_codecs(f1_codecs);
+ f2_.set_video_codecs(f2_codecs);
+ std::vector<AudioCodec> 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<SessionDescription> offer(f1_.CreateOffer(opts, NULL));
+ ASSERT_TRUE(offer.get() != NULL);
+ std::unique_ptr<SessionDescription> 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<SessionDescription> updated_offer(
+ f2_.CreateOffer(opts, answer.get()));
+
+ const VideoContentDescription* vcd =
+ GetFirstVideoContentDescription(updated_offer.get());
+ std::vector<cricket::VideoCodec> 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
