diff --git a/media/base/codec.cc b/media/base/codec.cc index 612a994081..c4e1c6f1f3 100644 --- a/media/base/codec.cc +++ b/media/base/codec.cc @@ -62,58 +62,6 @@ bool IsSameCodecSpecific(const std::string& name1, return true; } -bool MatchesMediaSubtype(const Codec& lhs, const Codec& rhs) { - // Match the codec id/name based on the typical static/dynamic name rules. - // Matching is case-insensitive. - - // We support the ranges [96, 127] and more recently [35, 65]. - // https://www.iana.org/assignments/rtp-parameters/rtp-parameters.xhtml#rtp-parameters-1 - // Within those ranges we match by codec name, outside by codec id. - // Since no codecs are assigned an id in the range [66, 95] by us, these will - // never match. - auto id_in_dynamic_range = [](int id) { - const int kLowerDynamicRangeMin = 35; - const int kLowerDynamicRangeMax = 65; - const int kUpperDynamicRangeMin = 96; - const int kUpperDynamicRangeMax = 127; - return (id >= kLowerDynamicRangeMin && id <= kLowerDynamicRangeMax) || - (id >= kUpperDynamicRangeMin && id <= kUpperDynamicRangeMax); - }; - - if (lhs.type != rhs.type) { - return false; - } - - return (id_in_dynamic_range(lhs.id) && id_in_dynamic_range(rhs.id)) - ? absl::EqualsIgnoreCase(lhs.name, rhs.name) - : lhs.id == rhs.id; -} - -bool MatchesMediaParameters(const Codec& lhs, const Codec& rhs) { - switch (lhs.type) { - case Codec::Type::kAudio: - // If a nonzero clockrate is specified, it must match the actual - // clockrate. If a nonzero bitrate is specified, it must match the - // actual bitrate, unless the codec is VBR (0), where we just force - // the supplied value. The number of channels must match exactly, with - // the exception that channels=0 is treated synonymously as - // channels=1, per RFC 4566 section 6: " [The channels] parameter is - // OPTIONAL and may be omitted if the number of channels is one." - // Preference is ignored. - // TODO(juberti): Treat a zero clockrate as 8000Hz, the RTP default - // clockrate. - return ((rhs.clockrate == 0 /*&& lsh.clockrate == 8000*/) || - lhs.clockrate == rhs.clockrate) && - (rhs.bitrate == 0 || lhs.bitrate <= 0 || - lhs.bitrate == rhs.bitrate) && - ((rhs.channels < 2 && lhs.channels < 2) || - lhs.channels == rhs.channels); - - case Codec::Type::kVideo: - return IsSameCodecSpecific(lhs.name, lhs.params, rhs.name, rhs.params); - } -} - } // namespace FeedbackParams::FeedbackParams() = default; @@ -211,14 +159,55 @@ bool Codec::operator==(const Codec& c) const { } bool Codec::Matches(const Codec& codec) const { - return MatchesMediaSubtype(*this, codec) && - MatchesMediaParameters(*this, codec) && - (packetization == codec.packetization); -} + // Match the codec id/name based on the typical static/dynamic name rules. + // Matching is case-insensitive. -bool Codec::MatchesWithoutPacketization(const Codec& codec) const { - return MatchesMediaSubtype(*this, codec) && - MatchesMediaParameters(*this, codec); + // We support the ranges [96, 127] and more recently [35, 65]. + // https://www.iana.org/assignments/rtp-parameters/rtp-parameters.xhtml#rtp-parameters-1 + // Within those ranges we match by codec name, outside by codec id. + // Since no codecs are assigned an id in the range [66, 95] by us, these will + // never match. + const int kLowerDynamicRangeMin = 35; + const int kLowerDynamicRangeMax = 65; + const int kUpperDynamicRangeMin = 96; + const int kUpperDynamicRangeMax = 127; + const bool is_id_in_dynamic_range = + (id >= kLowerDynamicRangeMin && id <= kLowerDynamicRangeMax) || + (id >= kUpperDynamicRangeMin && id <= kUpperDynamicRangeMax); + const bool is_codec_id_in_dynamic_range = + (codec.id >= kLowerDynamicRangeMin && + codec.id <= kLowerDynamicRangeMax) || + (codec.id >= kUpperDynamicRangeMin && codec.id <= kUpperDynamicRangeMax); + bool matches_id = is_id_in_dynamic_range && is_codec_id_in_dynamic_range + ? (absl::EqualsIgnoreCase(name, codec.name)) + : (id == codec.id); + + auto matches_type_specific = [&]() { + switch (type) { + case Type::kAudio: + // If a nonzero clockrate is specified, it must match the actual + // clockrate. If a nonzero bitrate is specified, it must match the + // actual bitrate, unless the codec is VBR (0), where we just force the + // supplied value. The number of channels must match exactly, with the + // exception that channels=0 is treated synonymously as channels=1, per + // RFC 4566 section 6: " [The channels] parameter is OPTIONAL and may be + // omitted if the number of channels is one." + // Preference is ignored. + // TODO(juberti): Treat a zero clockrate as 8000Hz, the RTP default + // clockrate. + return ((codec.clockrate == 0 /*&& clockrate == 8000*/) || + clockrate == codec.clockrate) && + (codec.bitrate == 0 || bitrate <= 0 || + bitrate == codec.bitrate) && + ((codec.channels < 2 && channels < 2) || + channels == codec.channels); + + case Type::kVideo: + return IsSameCodecSpecific(name, params, codec.name, codec.params); + } + }; + + return matches_id && matches_type_specific(); } bool Codec::MatchesRtpCodec(const webrtc::RtpCodec& codec_capability) const { diff --git a/media/base/codec.h b/media/base/codec.h index f40b42fe10..bd4239b251 100644 --- a/media/base/codec.h +++ b/media/base/codec.h @@ -112,10 +112,6 @@ struct RTC_EXPORT Codec { // checking the assigned id and profile values for the relevant video codecs. // H264 levels are not compared. bool Matches(const Codec& codec) const; - - // Like `Matches` but does not consider the packetization. - bool MatchesWithoutPacketization(const Codec& codec) const; - bool MatchesRtpCodec(const webrtc::RtpCodec& capability) const; // Find the parameter for `name` and write the value to `out`. diff --git a/media/base/codec_unittest.cc b/media/base/codec_unittest.cc index f947592509..eb34530c38 100644 --- a/media/base/codec_unittest.cc +++ b/media/base/codec_unittest.cc @@ -210,22 +210,13 @@ TEST(CodecTest, TestVideoCodecMatches) { EXPECT_FALSE(c1.Matches(cricket::CreateVideoCodec(34, "V"))); } -TEST(CodecTest, CodecsWithDifferentPacketizationDoesntMatch) { +TEST(CodecTest, TestVideoCodecMatchesWithDifferentPacketization) { VideoCodec c0 = cricket::CreateVideoCodec(100, cricket::kVp8CodecName); VideoCodec c1 = cricket::CreateVideoCodec(101, cricket::kVp8CodecName); c1.packetization = "raw"; - EXPECT_FALSE(c0.Matches(c1)); - EXPECT_FALSE(c1.Matches(c0)); -} - -TEST(CodecTest, CodecsWithDifferentPacketizationMatchesWithoutPacketization) { - VideoCodec c0 = cricket::CreateVideoCodec(100, cricket::kVp8CodecName); - VideoCodec c1 = cricket::CreateVideoCodec(101, cricket::kVp8CodecName); - c1.packetization = "raw"; - - EXPECT_TRUE(c0.MatchesWithoutPacketization(c1)); - EXPECT_TRUE(c1.MatchesWithoutPacketization(c0)); + EXPECT_TRUE(c0.Matches(c1)); + EXPECT_TRUE(c1.Matches(c0)); } // AV1 codecs compare profile information. diff --git a/pc/channel.cc b/pc/channel.cc index 792f67aa7a..7a1f55d9e9 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -1032,7 +1032,7 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, webrtc::flat_set matched_codecs; for (VideoCodec& send_codec : send_params.codecs) { if (absl::c_any_of(matched_codecs, [&](const VideoCodec* c) { - return send_codec.MatchesWithoutPacketization(*c); + return send_codec.Matches(*c); })) { continue; } @@ -1146,7 +1146,7 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content, webrtc::flat_set matched_codecs; for (Codec& recv_codec : recv_params.codecs) { if (absl::c_any_of(matched_codecs, [&](const Codec* c) { - return recv_codec.MatchesWithoutPacketization(*c); + return recv_codec.Matches(*c); })) { continue; } diff --git a/pc/media_session_unittest.cc b/pc/media_session_unittest.cc index b0dc3ad3de..641f638e72 100644 --- a/pc/media_session_unittest.cc +++ b/pc/media_session_unittest.cc @@ -16,7 +16,6 @@ #include #include #include -#include #include #include #include @@ -63,7 +62,6 @@ using ::rtc::kCsAeadAes256Gcm; using ::rtc::kCsAesCm128HmacSha1_32; using ::rtc::kCsAesCm128HmacSha1_80; using ::rtc::UniqueRandomIdGenerator; -using ::testing::AllOf; using ::testing::Bool; using ::testing::Combine; using ::testing::Contains; @@ -74,10 +72,8 @@ using ::testing::Eq; using ::testing::Field; using ::testing::IsEmpty; using ::testing::IsFalse; -using ::testing::IsSupersetOf; using ::testing::Ne; using ::testing::Not; -using ::testing::NotNull; using ::testing::Pointwise; using ::testing::SizeIs; using ::testing::Values; @@ -4327,45 +4323,6 @@ TEST_F(MediaSessionDescriptionFactoryTest, EXPECT_EQ(vcd1->codecs()[0].id, vcd2->codecs()[0].id); } -TEST_F(MediaSessionDescriptionFactoryTest, - DoesntReusePayloadIdWhenAddingExistingCodecWithDifferentPacketization) { - VideoCodec vp8 = CreateVideoCodec(96, "VP8"); - VideoCodec vp8_raw = CreateVideoCodec(98, "VP8"); - vp8_raw.packetization = "raw"; - VideoCodec vp9 = CreateVideoCodec(98, "VP9"); - - f1_.set_video_codecs({vp8, vp9}, {vp8, vp9}); - MediaSessionOptions opts; - AddMediaDescriptionOptions(MEDIA_TYPE_VIDEO, "video", - RtpTransceiverDirection::kSendRecv, kActive, - &opts); - - std::unique_ptr offer = - f1_.CreateOfferOrError(opts, nullptr).MoveValue(); - ASSERT_THAT(offer, NotNull()); - MediaContentDescription& video = *offer->contents()[0].media_description(); - video.set_codecs({vp8, vp8_raw}); - std::unique_ptr updated_offer = - f1_.CreateOfferOrError(opts, offer.get()).MoveValue(); - ASSERT_THAT(updated_offer, NotNull()); - - MediaContentDescription& updated_video = - *updated_offer->contents()[0].media_description(); - EXPECT_THAT( - updated_video.codecs(), - ElementsAre(AllOf(Field(&cricket::Codec::name, "VP8"), - Field(&cricket::Codec::packetization, absl::nullopt)), - AllOf(Field(&cricket::Codec::name, "VP8"), - Field(&cricket::Codec::packetization, "raw")), - AllOf(Field(&cricket::Codec::name, "VP9"), - Field(&cricket::Codec::packetization, absl::nullopt)))); - std::set used_ids; - for (const VideoCodec& c : updated_video.codecs()) { - used_ids.insert(c.id); - } - EXPECT_THAT(used_ids, AllOf(SizeIs(3), IsSupersetOf({96, 98}))); -} - // Test verifying that negotiating codecs with the same packetization retains // the packetization value. TEST_F(MediaSessionDescriptionFactoryTest, PacketizationIsEqual) { @@ -4402,6 +4359,42 @@ TEST_F(MediaSessionDescriptionFactoryTest, PacketizationIsEqual) { EXPECT_EQ(vcd1->codecs()[0].packetization, "raw"); } +// Test verifying that negotiating codecs with different packetization removes +// the packetization value. +TEST_F(MediaSessionDescriptionFactoryTest, PacketizationIsDifferent) { + std::vector f1_codecs = {CreateVideoCodec(96, "H264")}; + f1_codecs.back().packetization = "raw"; + f1_.set_video_codecs(f1_codecs, f1_codecs); + + std::vector f2_codecs = {CreateVideoCodec(96, "H264")}; + f2_codecs.back().packetization = "notraw"; + f2_.set_video_codecs(f2_codecs, f2_codecs); + + MediaSessionOptions opts; + AddMediaDescriptionOptions(MEDIA_TYPE_VIDEO, "video1", + RtpTransceiverDirection::kSendRecv, kActive, + &opts); + + // Create an offer with two video sections using same codecs. + std::unique_ptr offer = + f1_.CreateOfferOrError(opts, nullptr).MoveValue(); + ASSERT_TRUE(offer); + ASSERT_EQ(1u, offer->contents().size()); + const VideoContentDescription* vcd1 = + offer->contents()[0].media_description()->as_video(); + ASSERT_EQ(1u, vcd1->codecs().size()); + EXPECT_EQ(vcd1->codecs()[0].packetization, "raw"); + + // Create answer and negotiate the codecs. + std::unique_ptr answer = + f2_.CreateAnswerOrError(offer.get(), opts, nullptr).MoveValue(); + ASSERT_TRUE(answer); + ASSERT_EQ(1u, answer->contents().size()); + vcd1 = answer->contents()[0].media_description()->as_video(); + ASSERT_EQ(1u, vcd1->codecs().size()); + EXPECT_EQ(vcd1->codecs()[0].packetization, absl::nullopt); +} + // Test that the codec preference order per media section is respected in // subsequent offer. TEST_F(MediaSessionDescriptionFactoryTest,