diff --git a/media/base/codec.cc b/media/base/codec.cc index 98e52d6848..e4b6b7899b 100644 --- a/media/base/codec.cc +++ b/media/base/codec.cc @@ -244,11 +244,29 @@ bool VideoCodec::operator==(const VideoCodec& c) const { return Codec::operator==(c); } +static bool IsSameH264PacketizationMode(const CodecParameterMap& ours, + const CodecParameterMap& theirs) { + // If packetization-mode is not present, default to "0". + // https://tools.ietf.org/html/rfc6184#section-6.2 + std::string our_packetization_mode = "0"; + std::string their_packetization_mode = "0"; + auto ours_it = ours.find(kH264FmtpPacketizationMode); + if (ours_it != ours.end()) { + our_packetization_mode = ours_it->second; + } + auto theirs_it = theirs.find(kH264FmtpPacketizationMode); + if (theirs_it != theirs.end()) { + their_packetization_mode = theirs_it->second; + } + return our_packetization_mode == their_packetization_mode; +} + bool VideoCodec::Matches(const VideoCodec& other) const { if (!Codec::Matches(other)) return false; if (CodecNamesEq(name.c_str(), kH264CodecName)) - return webrtc::H264::IsSameH264Profile(params, other.params); + return webrtc::H264::IsSameH264Profile(params, other.params) && + IsSameH264PacketizationMode(params, other.params); return true; } diff --git a/media/base/codec_unittest.cc b/media/base/codec_unittest.cc index 03d8684c64..d771a80281 100644 --- a/media/base/codec_unittest.cc +++ b/media/base/codec_unittest.cc @@ -186,6 +186,45 @@ TEST(CodecTest, TestVideoCodecMatches) { EXPECT_FALSE(c1.Matches(VideoCodec(95, "V"))); } +// Matching H264 codecs also need to have matching profile-level-id and +// packetization-mode. +TEST(CodecTest, TestH264CodecMatches) { + const char kProfileLevelId1[] = "42e01f"; + const char kProfileLevelId2[] = "42a01e"; + + VideoCodec pli_1_pm_0(95, "H264"); + pli_1_pm_0.params[cricket::kH264FmtpProfileLevelId] = kProfileLevelId1; + pli_1_pm_0.params[cricket::kH264FmtpPacketizationMode] = "0"; + + { + VideoCodec pli_1_pm_blank(95, "H264"); + pli_1_pm_blank.params[cricket::kH264FmtpProfileLevelId] = kProfileLevelId1; + pli_1_pm_blank.params.erase( + pli_1_pm_blank.params.find(cricket::kH264FmtpPacketizationMode)); + + // Matches since if packetization-mode is not specified it defaults to "0". + EXPECT_TRUE(pli_1_pm_0.Matches(pli_1_pm_blank)); + } + + { + VideoCodec pli_1_pm_1(95, "H264"); + pli_1_pm_1.params[cricket::kH264FmtpProfileLevelId] = kProfileLevelId1; + pli_1_pm_1.params[cricket::kH264FmtpPacketizationMode] = "1"; + + // Does not match since packetization-mode is different. + EXPECT_FALSE(pli_1_pm_0.Matches(pli_1_pm_1)); + } + + { + VideoCodec pli_2_pm_0(95, "H264"); + pli_2_pm_0.params[cricket::kH264FmtpProfileLevelId] = kProfileLevelId2; + pli_2_pm_0.params[cricket::kH264FmtpPacketizationMode] = "0"; + + // Does not match since profile-level-id is different. + EXPECT_FALSE(pli_1_pm_0.Matches(pli_2_pm_0)); + } +} + TEST(CodecTest, TestDataCodecMatches) { // Test a codec with a static payload type. DataCodec c0(95, "D"); diff --git a/pc/mediasession_unittest.cc b/pc/mediasession_unittest.cc index 971e568889..af6b695024 100644 --- a/pc/mediasession_unittest.cc +++ b/pc/mediasession_unittest.cc @@ -3380,6 +3380,47 @@ TEST_F(MediaSessionDescriptionFactoryTest, CreateAnswerWithLocalCodecParams) { EXPECT_EQ(video_value2, value); } +// Test that matching packetization-mode is part of the criteria for matching +// H264 codecs (in addition to profile-level-id). Previously, this was not the +// case, so the first H264 codec with the same profile-level-id would match and +// the payload type in the answer would be incorrect. +// This is a regression test for bugs.webrtc.org/8808 +TEST_F(MediaSessionDescriptionFactoryTest, + H264MatchCriteriaIncludesPacketizationMode) { + // Create two H264 codecs with the same profile level ID and different + // packetization modes. + VideoCodec h264_pm0(96, "H264"); + h264_pm0.params[cricket::kH264FmtpProfileLevelId] = "42c01f"; + h264_pm0.params[cricket::kH264FmtpPacketizationMode] = "0"; + VideoCodec h264_pm1(97, "H264"); + h264_pm1.params[cricket::kH264FmtpProfileLevelId] = "42c01f"; + h264_pm1.params[cricket::kH264FmtpPacketizationMode] = "1"; + + // Offerer will send both codecs, answerer should choose the one with matching + // packetization mode (and not the first one it sees). + f1_.set_video_codecs({h264_pm0, h264_pm1}); + f2_.set_video_codecs({h264_pm1}); + + MediaSessionOptions opts; + AddMediaSection(MEDIA_TYPE_VIDEO, "video", RtpTransceiverDirection::kSendRecv, + kActive, &opts); + + std::unique_ptr offer(f1_.CreateOffer(opts, nullptr)); + ASSERT_TRUE(offer); + + std::unique_ptr answer( + f2_.CreateAnswer(offer.get(), opts, nullptr)); + ASSERT_TRUE(answer); + + // Answer should have one negotiated codec with packetization-mode=1 using the + // offered payload type. + ASSERT_EQ(1u, answer->contents().size()); + auto answer_vcd = answer->contents()[0].media_description()->as_video(); + ASSERT_EQ(1u, answer_vcd->codecs().size()); + auto answer_codec = answer_vcd->codecs()[0]; + EXPECT_EQ(h264_pm1.id, answer_codec.id); +} + class MediaProtocolTest : public ::testing::TestWithParam { public: MediaProtocolTest() : f1_(&tdf1_), f2_(&tdf2_) {