Check H264 packetization mode when using IsSameCodec

H264 requires that the packetization modes are the same in order to
consider the code the same. This logic was added to VideoCodec::Matches
but was not reflected in IsSameCodec. This could manifest itself when a
remote description with an unsupported packetization mode is set.

Bug: webrtc:10693
Change-Id: Icda07f7d56a464895d2267a41cc0f2fd9d5f42ff
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/138983
Commit-Queue: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Amit Hilbuch <amithi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28126}

media/base/codec.cc

@@ -20,6 +20,44 @@
 #include "rtc_base/strings/string_builder.h"
 
 namespace cricket {
+namespace {
+
+std::string GetH264PacketizationModeOrDefault(const CodecParameterMap& params) {
+  auto it = params.find(kH264FmtpPacketizationMode);
+  if (it != params.end()) {
+    return it->second;
+  }
+  // If packetization-mode is not present, default to "0".
+  // https://tools.ietf.org/html/rfc6184#section-6.2
+  return "0";
+}
+
+bool IsSameH264PacketizationMode(const CodecParameterMap& left,
+                                 const CodecParameterMap& right) {
+  return GetH264PacketizationModeOrDefault(left) ==
+         GetH264PacketizationModeOrDefault(right);
+}
+
+// Some (video) codecs are actually families of codecs and rely on parameters
+// to distinguish different incompatible family members.
+bool IsSameCodecSpecific(const std::string& name1,
+                         const CodecParameterMap& params1,
+                         const std::string& name2,
+                         const CodecParameterMap& params2) {
+  // The names might not necessarily match, so check both.
+  auto either_name_matches = [&](const std::string name) {
+    return absl::EqualsIgnoreCase(name, name1) ||
+           absl::EqualsIgnoreCase(name, name2);
+  };
+  if (either_name_matches(kH264CodecName))
+    return webrtc::H264::IsSameH264Profile(params1, params2) &&
+           IsSameH264PacketizationMode(params1, params2);
+  if (either_name_matches(kVp9CodecName))
+    return webrtc::IsSameVP9Profile(params1, params2);
+  return true;
+}
+
+}  // namespace
 
 FeedbackParams::FeedbackParams() = default;
 FeedbackParams::~FeedbackParams() = default;
@@ -259,32 +297,9 @@ 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 (absl::EqualsIgnoreCase(name, kH264CodecName))
-    return webrtc::H264::IsSameH264Profile(params, other.params) &&
-           IsSameH264PacketizationMode(params, other.params);
-  if (absl::EqualsIgnoreCase(name, kVp9CodecName))
-    return webrtc::IsSameVP9Profile(params, other.params);
-  return true;
+  return Codec::Matches(other) &&
+         IsSameCodecSpecific(name, params, other.name, other.params);
 }
 
 VideoCodec VideoCodec::CreateRtxCodec(int rtx_payload_type,
@@ -295,17 +310,16 @@ VideoCodec VideoCodec::CreateRtxCodec(int rtx_payload_type,
 }
 
 VideoCodec::CodecType VideoCodec::GetCodecType() const {
-  const char* payload_name = name.c_str();
-  if (absl::EqualsIgnoreCase(payload_name, kRedCodecName)) {
+  if (absl::EqualsIgnoreCase(name, kRedCodecName)) {
     return CODEC_RED;
   }
-  if (absl::EqualsIgnoreCase(payload_name, kUlpfecCodecName)) {
+  if (absl::EqualsIgnoreCase(name, kUlpfecCodecName)) {
     return CODEC_ULPFEC;
   }
-  if (absl::EqualsIgnoreCase(payload_name, kFlexfecCodecName)) {
+  if (absl::EqualsIgnoreCase(name, kFlexfecCodecName)) {
     return CODEC_FLEXFEC;
   }
-  if (absl::EqualsIgnoreCase(payload_name, kRtxCodecName)) {
+  if (absl::EqualsIgnoreCase(name, kRtxCodecName)) {
     return CODEC_RTX;
   }
 
@@ -394,15 +408,10 @@ bool IsSameCodec(const std::string& name1,
                  const CodecParameterMap& params1,
                  const std::string& name2,
                  const CodecParameterMap& params2) {
-  // If different names (case insensitive), then not same formats.
-  if (!absl::EqualsIgnoreCase(name1, name2))
-    return false;
-  // For every format besides H264 and VP9, comparing names is enough.
-  if (absl::EqualsIgnoreCase(name1, kH264CodecName))
-    return webrtc::H264::IsSameH264Profile(params1, params2);
-  if (absl::EqualsIgnoreCase(name1, kVp9CodecName))
-    return webrtc::IsSameVP9Profile(params1, params2);
-  return true;
+  // Two codecs are considered the same if the name matches (case insensitive)
+  // and certain codec-specific parameters match.
+  return absl::EqualsIgnoreCase(name1, name2) &&
+         IsSameCodecSpecific(name1, params1, name2, params2);
 }
 
 }  // namespace cricket

media/base/codec_unittest.cc

@@ -10,6 +10,8 @@
 
 #include "media/base/codec.h"
 
+#include <tuple>
+
 #include "media/base/vp9_profile.h"
 #include "rtc_base/gunit.h"
 
@@ -403,3 +405,94 @@ TEST(CodecTest, TestToCodecParameters) {
   EXPECT_EQ("p1", codec_params_2.parameters.begin()->first);
   EXPECT_EQ("a1", codec_params_2.parameters.begin()->second);
 }
+
+// Tests that the helper IsSameCodec returns the correct value for codecs that
+// must also be matched on particular parameter values.
+using IsSameCodecParamsTestCase =
+    std::tuple<cricket::CodecParameterMap, cricket::CodecParameterMap>;
+class IsSameCodecParamsTest
+    : public ::testing::TestWithParam<
+          std::tuple<std::string, bool, IsSameCodecParamsTestCase>> {
+ protected:
+  IsSameCodecParamsTest() {
+    name_ = std::get<0>(GetParam());
+    expected_ = std::get<1>(GetParam());
+    const auto& test_case = std::get<2>(GetParam());
+    params_left_ = std::get<0>(test_case);
+    params_right_ = std::get<1>(test_case);
+  }
+
+  std::string name_;
+  bool expected_;
+  cricket::CodecParameterMap params_left_;
+  cricket::CodecParameterMap params_right_;
+};
+
+TEST_P(IsSameCodecParamsTest, Expected) {
+  EXPECT_EQ(expected_,
+            cricket::IsSameCodec(name_, params_left_, name_, params_right_));
+}
+
+TEST_P(IsSameCodecParamsTest, Commutative) {
+  EXPECT_EQ(expected_,
+            cricket::IsSameCodec(name_, params_right_, name_, params_left_));
+}
+
+IsSameCodecParamsTestCase MakeTestCase(cricket::CodecParameterMap left,
+                                       cricket::CodecParameterMap right) {
+  return std::make_tuple(left, right);
+}
+
+const IsSameCodecParamsTestCase kH264ParamsSameTestCases[] = {
+    // Both have the same defaults.
+    MakeTestCase({}, {}),
+    // packetization-mode: 0 is the default.
+    MakeTestCase({{cricket::kH264FmtpPacketizationMode, "0"}}, {}),
+    // Non-default packetization-mode matches.
+    MakeTestCase({{cricket::kH264FmtpPacketizationMode, "1"}},
+                 {{cricket::kH264FmtpPacketizationMode, "1"}}),
+};
+INSTANTIATE_TEST_SUITE_P(
+    H264_Same,
+    IsSameCodecParamsTest,
+    ::testing::Combine(::testing::Values("H264"),
+                       ::testing::Values(true),
+                       ::testing::ValuesIn(kH264ParamsSameTestCases)));
+
+const IsSameCodecParamsTestCase kH264ParamsNotSameTestCases[] = {
+    // packetization-mode does not match the default of "0".
+    MakeTestCase({{cricket::kH264FmtpPacketizationMode, "1"}}, {}),
+};
+INSTANTIATE_TEST_SUITE_P(
+    H264_NotSame,
+    IsSameCodecParamsTest,
+    ::testing::Combine(::testing::Values("H264"),
+                       ::testing::Values(false),
+                       ::testing::ValuesIn(kH264ParamsNotSameTestCases)));
+
+const IsSameCodecParamsTestCase kVP9ParamsSameTestCases[] = {
+    // Both have the same defaults.
+    MakeTestCase({}, {}),
+    // profile-id: 0 is the default.
+    MakeTestCase({{webrtc::kVP9FmtpProfileId, "0"}}, {}),
+    // Non-default profile-id matches.
+    MakeTestCase({{webrtc::kVP9FmtpProfileId, "2"}},
+                 {{webrtc::kVP9FmtpProfileId, "2"}}),
+};
+INSTANTIATE_TEST_SUITE_P(
+    VP9_Same,
+    IsSameCodecParamsTest,
+    ::testing::Combine(::testing::Values("VP9"),
+                       ::testing::Values(true),
+                       ::testing::ValuesIn(kVP9ParamsSameTestCases)));
+
+const IsSameCodecParamsTestCase kVP9ParamsNotSameTestCases[] = {
+    // profile-id missing from right.
+    MakeTestCase({{webrtc::kVP9FmtpProfileId, "2"}}, {}),
+};
+INSTANTIATE_TEST_SUITE_P(
+    VP9_NotSame,
+    IsSameCodecParamsTest,
+    ::testing::Combine(::testing::Values("VP9"),
+                       ::testing::Values(false),
+                       ::testing::ValuesIn(kVP9ParamsNotSameTestCases)));

media/engine/webrtc_video_engine_unittest.cc

@@ -1000,7 +1000,9 @@ TEST_F(WebRtcVideoEngineTest, RegisterH264DecoderIfSupported) {
   // For now we add a FakeWebRtcVideoEncoderFactory to add H264 to supported
   // codecs.
   encoder_factory_->AddSupportedVideoCodecType("H264");
-  decoder_factory_->AddSupportedVideoCodecType(webrtc::SdpVideoFormat("H264"));
+  webrtc::SdpVideoFormat supported_h264("H264");
+  supported_h264.parameters[kH264FmtpPacketizationMode] = "1";
+  decoder_factory_->AddSupportedVideoCodecType(supported_h264);
   std::vector<cricket::VideoCodec> codecs;
   codecs.push_back(GetEngineCodec("H264"));