Revert "Make Codec::Matches also consider packetization"
This reverts commit 1ae700a9233ed647e1b4080c0fcb48f61a0cca0a.
Reason for revert: Potential root cause of crbug.com/1504351
Original change's description:
> Make Codec::Matches also consider packetization
>
> If it's not considered it can lead to payload IDs erroneously being
> reused if the SDP is munged, see https://crbug.com/webrtc/15473#c10.
>
> Bug: webrtc:15473
> Change-Id: I195a06d556e8a57dbeeb946effc4e0f27cc930b0
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/326522
> Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#41153}
Bug: webrtc:15473 chromium:1504351
Change-Id: I87fb671d76c3b17beb65124603cc040bb9bf4fa5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/329201
Commit-Queue: Tony Herre <herre@google.com>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41285}
diff --git a/media/base/codec.cc b/media/base/codec.cc
index 612a994..c4e1c6f 100644
--- a/media/base/codec.cc
+++ b/media/base/codec.cc
@@ -62,58 +62,6 @@
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::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 f40b42f..bd4239b 100644
--- a/media/base/codec.h
+++ b/media/base/codec.h
@@ -112,10 +112,6 @@
// 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 f947592..eb34530 100644
--- a/media/base/codec_unittest.cc
+++ b/media/base/codec_unittest.cc
@@ -210,22 +210,13 @@
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 792f67a..7a1f55d 100644
--- a/pc/channel.cc
+++ b/pc/channel.cc
@@ -1032,7 +1032,7 @@
webrtc::flat_set<const VideoCodec*> 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 @@
webrtc::flat_set<const Codec*> 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 b0dc3ad..641f638 100644
--- a/pc/media_session_unittest.cc
+++ b/pc/media_session_unittest.cc
@@ -16,7 +16,6 @@
#include <cstdint>
#include <map>
#include <memory>
-#include <set>
#include <string>
#include <tuple>
#include <vector>
@@ -63,7 +62,6 @@
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::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 @@
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<SessionDescription> 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<SessionDescription> 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<int> 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 @@
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<SessionDescription> 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<SessionDescription> 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,
