Consider packetization-mode when matching H264 codecs

Bug: webrtc:8808
Change-Id: I90fd6964eccd9fd802e7f6e1cae2c794954e93f9
Reviewed-on: https://webrtc-review.googlesource.com/57820
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Commit-Queue: Steve Anton <steveanton@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22196}

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;
 }
 

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");

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<SessionDescription> offer(f1_.CreateOffer(opts, nullptr));
+  ASSERT_TRUE(offer);
+
+  std::unique_ptr<SessionDescription> 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<const char*> {
  public:
   MediaProtocolTest() : f1_(&tdf1_), f2_(&tdf2_) {