From 2dbc627aa07e410ee914afbbf1805d2da1ee6ddd Mon Sep 17 00:00:00 2001 From: Steve Anton Date: Fri, 31 May 2019 13:08:58 -0700 Subject: [PATCH] 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 Reviewed-by: Amit Hilbuch Cr-Commit-Position: refs/heads/master@{#28126} --- media/base/codec.cc | 87 ++++++++++-------- media/base/codec_unittest.cc | 93 ++++++++++++++++++++ media/engine/webrtc_video_engine_unittest.cc | 4 +- 3 files changed, 144 insertions(+), 40 deletions(-) diff --git a/media/base/codec.cc b/media/base/codec.cc index 5b1bc9d972..e95517dd3a 100644 --- a/media/base/codec.cc +++ b/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 diff --git a/media/base/codec_unittest.cc b/media/base/codec_unittest.cc index 54396a9244..b3cda813b2 100644 --- a/media/base/codec_unittest.cc +++ b/media/base/codec_unittest.cc @@ -10,6 +10,8 @@ #include "media/base/codec.h" +#include + #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; +class IsSameCodecParamsTest + : public ::testing::TestWithParam< + std::tuple> { + 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))); diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index 2aa10e640b..f2b6e373ab 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/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 codecs; codecs.push_back(GetEngineCodec("H264"));