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}

media/base/codec.cc

@@ -62,6 +62,58 @@ 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;
@@ -159,55 +211,14 @@ bool Codec::operator==(const Codec& c) const {
 }
 
 bool Codec::Matches(const Codec& codec) const {
-  // Match the codec id/name based on the typical static/dynamic name rules.
-  // Matching is case-insensitive.
+  return MatchesMediaSubtype(*this, codec) &&
+         MatchesMediaParameters(*this, codec) &&
+         (packetization == codec.packetization);
+}
 
-  // 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::MatchesWithoutPacketization(const Codec& codec) const {
+  return MatchesMediaSubtype(*this, codec) &&
+         MatchesMediaParameters(*this, codec);
 }
 
 bool Codec::MatchesRtpCodec(const webrtc::RtpCodec& codec_capability) const {

media/base/codec.h

@@ -112,6 +112,10 @@ 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`.

media/base/codec_unittest.cc

@@ -210,13 +210,22 @@ TEST(CodecTest, TestVideoCodecMatches) {
   EXPECT_FALSE(c1.Matches(cricket::CreateVideoCodec(34, "V")));
 }
 
-TEST(CodecTest, TestVideoCodecMatchesWithDifferentPacketization) {
+TEST(CodecTest, CodecsWithDifferentPacketizationDoesntMatch) {
   VideoCodec c0 = cricket::CreateVideoCodec(100, cricket::kVp8CodecName);
   VideoCodec c1 = cricket::CreateVideoCodec(101, cricket::kVp8CodecName);
   c1.packetization = "raw";
 
-  EXPECT_TRUE(c0.Matches(c1));
-  EXPECT_TRUE(c1.Matches(c0));
+  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));
 }
 
 // AV1 codecs compare profile information.

pc/channel.cc

@@ -1032,7 +1032,7 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content,
     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.Matches(*c);
+            return send_codec.MatchesWithoutPacketization(*c);
           })) {
         continue;
       }
@@ -1148,7 +1148,7 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content,
     webrtc::flat_set<const VideoCodec*> matched_codecs;
     for (VideoCodec& recv_codec : recv_params.codecs) {
       if (absl::c_any_of(matched_codecs, [&](const VideoCodec* c) {
-            return recv_codec.Matches(*c);
+            return recv_codec.MatchesWithoutPacketization(*c);
           })) {
         continue;
       }

pc/media_session_unittest.cc

@@ -16,6 +16,7 @@
 #include <cstdint>
 #include <map>
 #include <memory>
+#include <set>
 #include <string>
 #include <tuple>
 #include <vector>
@@ -62,6 +63,7 @@ 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;
@@ -72,8 +74,10 @@ 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;
@@ -4333,6 +4337,46 @@ 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<SessionDescription> offer =
+      f1_.CreateOfferOrError(opts, nullptr).MoveValue();
+  ASSERT_THAT(offer, NotNull());
+  VideoContentDescription& video =
+      *offer->contents()[0].media_description()->as_video();
+  video.set_codecs({vp8, vp8_raw});
+  std::unique_ptr<SessionDescription> updated_offer =
+      f1_.CreateOfferOrError(opts, offer.get()).MoveValue();
+  ASSERT_THAT(updated_offer, NotNull());
+
+  VideoContentDescription& updated_video =
+      *updated_offer->contents()[0].media_description()->as_video();
+  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) {
@@ -4369,42 +4413,6 @@ 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<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,