From 06c8e1eaa7eb35c519c499f1d22d718213f71cc0 Mon Sep 17 00:00:00 2001 From: Magnus Jedvert Date: Tue, 25 Oct 2016 09:53:52 +0200 Subject: [PATCH] Revert of H264 codec: Check profile-level-id when matching (patchset #2 id:60001 of https://codereview.webrtc.org/2347863003/ ) Reason for revert: Breaks H264 for external encoders in WebRTC as well as breaking H264 interop with e.g. Edge. Original issue's description: > H264 codec: Check profile-level-id when matching > > For the H264 video codec, comparing the codec name is not enough > for determining a match. The profile-level-id must also match. > This CL: > - Specializes the VideoCodec::Matches function with extra logic for > matching H264 codecs. > - Adds unittests for matching H264 video codecs. > - Removes the unnecessary CodecTest fixture class. > > BUG=webrtc:6337,chromium:645599 > > Committed: https://crrev.com/68979ab7dd971ab6e983b23c83154ba05e183fb8 > Cr-Commit-Position: refs/heads/master@{#14546} TBR=kthelgason@webrtc.org,hta@webrtc.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=webrtc:6337,chromium:645599,webrtc:6552,webrtc:6402 Review URL: https://codereview.webrtc.org/2440123002 . Cr-Commit-Position: refs/heads/master@{#14759} --- webrtc/media/base/codec.cc | 38 ------------ webrtc/media/base/codec.h | 3 - webrtc/media/base/codec_unittest.cc | 92 +++++------------------------ webrtc/media/base/mediaconstants.cc | 3 - webrtc/media/base/mediaconstants.h | 1 - 5 files changed, 16 insertions(+), 121 deletions(-) diff --git a/webrtc/media/base/codec.cc b/webrtc/media/base/codec.cc index 26d1fafc00..1e06e8e595 100644 --- a/webrtc/media/base/codec.cc +++ b/webrtc/media/base/codec.cc @@ -18,19 +18,6 @@ #include "webrtc/base/stringencode.h" #include "webrtc/base/stringutils.h" -namespace { - -// Return the contained value for |key| if available, and |default_value| -// otherwise. -std::string GetParamOrDefault(const cricket::Codec& codec, - const std::string& key, - const std::string& default_value) { - cricket::CodecParameterMap::const_iterator iter = codec.params.find(key); - return (iter == codec.params.end()) ? default_value : iter->second; -} - -} // anonymous namespace - namespace cricket { const int kMaxPayloadId = 127; @@ -240,31 +227,6 @@ bool VideoCodec::operator==(const VideoCodec& c) const { return Codec::operator==(c); } -bool VideoCodec::Matches(const VideoCodec& codec) const { - if (!Codec::Matches(codec)) - return false; - // TODO(magjed): It would be better to have this logic in a H264 subclass. See - // http://crbug/webrtc/6385 for more info. - if (!CodecNamesEq(name, kH264CodecName)) - return true; - // H264 codecs need to have matching profile-level-id. - const std::string our_profile_level_id = GetParamOrDefault( - *this, kH264FmtpProfileLevelId, kH264FmtpDefaultProfileLevelId); - const std::string their_profile_level_id = GetParamOrDefault( - codec, kH264FmtpProfileLevelId, kH264FmtpDefaultProfileLevelId); - if (our_profile_level_id == their_profile_level_id) - return true; - // At this point, profile-level-id is not an exact match, but that is still ok - // if only level_idc differs and level asymmetry is allowed. - const bool level_asymmetry_allowed = - GetParamOrDefault(*this, kH264FmtpLevelAsymmetryAllowed, "0") == "1" && - GetParamOrDefault(codec, kH264FmtpLevelAsymmetryAllowed, "0") == "1"; - // Ignore level_idc and compare only profile_idc and profile_iop. - const bool is_profile_match = (our_profile_level_id.substr(0, 4) == - their_profile_level_id.substr(0, 4)); - return level_asymmetry_allowed && is_profile_match; -} - VideoCodec VideoCodec::CreateRtxCodec(int rtx_payload_type, int associated_payload_type) { VideoCodec rtx_codec(rtx_payload_type, kRtxCodecName); diff --git a/webrtc/media/base/codec.h b/webrtc/media/base/codec.h index 807e4b6e5a..facdfefebc 100644 --- a/webrtc/media/base/codec.h +++ b/webrtc/media/base/codec.h @@ -146,9 +146,6 @@ struct VideoCodec : public Codec { VideoCodec(const VideoCodec& c); virtual ~VideoCodec() = default; - // Indicates if this codec is compatible with the specified codec. - bool Matches(const VideoCodec& codec) const; - std::string ToString() const; VideoCodec& operator=(const VideoCodec& c); diff --git a/webrtc/media/base/codec_unittest.cc b/webrtc/media/base/codec_unittest.cc index 230cf08fa6..7ad9733dda 100644 --- a/webrtc/media/base/codec_unittest.cc +++ b/webrtc/media/base/codec_unittest.cc @@ -20,7 +20,12 @@ using cricket::kCodecParamAssociatedPayloadType; using cricket::kCodecParamMaxBitrate; using cricket::kCodecParamMinBitrate; -TEST(CodecTest, TestCodecOperators) { +class CodecTest : public testing::Test { + public: + CodecTest() {} +}; + +TEST_F(CodecTest, TestCodecOperators) { Codec c0(96, "D", 1000); c0.SetParam("a", 1); @@ -53,7 +58,7 @@ TEST(CodecTest, TestCodecOperators) { EXPECT_TRUE(c5 == c6); } -TEST(CodecTest, TestAudioCodecOperators) { +TEST_F(CodecTest, TestAudioCodecOperators) { AudioCodec c0(96, "A", 44100, 20000, 2); AudioCodec c1(95, "A", 44100, 20000, 2); AudioCodec c2(96, "x", 44100, 20000, 2); @@ -90,7 +95,7 @@ TEST(CodecTest, TestAudioCodecOperators) { EXPECT_TRUE(c13 == c10); } -TEST(CodecTest, TestAudioCodecMatches) { +TEST_F(CodecTest, TestAudioCodecMatches) { // Test a codec with a static payload type. AudioCodec c0(95, "A", 44100, 20000, 1); EXPECT_TRUE(c0.Matches(AudioCodec(95, "", 44100, 20000, 1))); @@ -130,7 +135,7 @@ TEST(CodecTest, TestAudioCodecMatches) { EXPECT_FALSE(c3.Matches(AudioCodec(96, "A", 44100, 20000, 0))); } -TEST(CodecTest, TestVideoCodecOperators) { +TEST_F(CodecTest, TestVideoCodecOperators) { VideoCodec c0(96, "V"); VideoCodec c1(95, "V"); VideoCodec c2(96, "x"); @@ -162,7 +167,7 @@ TEST(CodecTest, TestVideoCodecOperators) { EXPECT_TRUE(c13 == c10); } -TEST(CodecTest, TestVideoCodecMatches) { +TEST_F(CodecTest, TestVideoCodecMatches) { // Test a codec with a static payload type. VideoCodec c0(95, "V"); EXPECT_TRUE(c0.Matches(VideoCodec(95, ""))); @@ -178,71 +183,6 @@ TEST(CodecTest, TestVideoCodecMatches) { EXPECT_FALSE(c1.Matches(VideoCodec(95, "V"))); } -TEST(CodecTest, TestVideoCodecMatchesH264Baseline) { - const VideoCodec no_params(96, cricket::kH264CodecName); - - VideoCodec baseline(96, cricket::kH264CodecName); - baseline.SetParam(cricket::kH264FmtpProfileLevelId, - cricket::kH264FmtpDefaultProfileLevelId); - - EXPECT_TRUE(baseline.Matches(baseline)); - EXPECT_TRUE(baseline.Matches(no_params)); - EXPECT_TRUE(no_params.Matches(baseline)); - EXPECT_TRUE(no_params.Matches(no_params)); -} - -TEST(CodecTest, TestVideoCodecMatchesH264Profiles) { - VideoCodec baseline(96, cricket::kH264CodecName); - baseline.SetParam(cricket::kH264FmtpProfileLevelId, - cricket::kH264FmtpDefaultProfileLevelId); - baseline.SetParam(cricket::kH264FmtpLevelAsymmetryAllowed, "1"); - - VideoCodec constrained_baseline(96, cricket::kH264CodecName); - constrained_baseline.SetParam(cricket::kH264FmtpProfileLevelId, - cricket::kH264ProfileLevelConstrainedBaseline); - constrained_baseline.SetParam(cricket::kH264FmtpLevelAsymmetryAllowed, "1"); - - EXPECT_TRUE(baseline.Matches(baseline)); - EXPECT_FALSE(baseline.Matches(constrained_baseline)); - EXPECT_FALSE(constrained_baseline.Matches(baseline)); - EXPECT_TRUE(constrained_baseline.Matches(constrained_baseline)); -} - -TEST(CodecTest, TestVideoCodecMatchesH264LevelAsymmetry) { - // Constrained Baseline Profile Level 1.0. - VideoCodec cbp_1_0(96, cricket::kH264CodecName); - cbp_1_0.SetParam(cricket::kH264FmtpProfileLevelId, - "42e00a"); - - VideoCodec cbp_1_0_asymmetry_allowed = cbp_1_0; - cbp_1_0_asymmetry_allowed.SetParam(cricket::kH264FmtpLevelAsymmetryAllowed, - "1"); - - // Constrained Baseline Profile Level 3.1. - VideoCodec cbp_3_1(96, cricket::kH264CodecName); - cbp_3_1.SetParam(cricket::kH264FmtpProfileLevelId, "42e01f"); - - VideoCodec cbp_3_1_asymmetry_allowed = cbp_3_1; - cbp_3_1_asymmetry_allowed.SetParam(cricket::kH264FmtpLevelAsymmetryAllowed, - "1"); - - // It's ok to differ in level-asymmetry-allowed param as long as the level is - // the same. - EXPECT_TRUE(cbp_1_0.Matches(cbp_1_0_asymmetry_allowed)); - EXPECT_TRUE(cbp_3_1.Matches(cbp_3_1_asymmetry_allowed)); - - // Both codecs need to accept level asymmetry if levels differ. - EXPECT_FALSE(cbp_1_0.Matches(cbp_3_1_asymmetry_allowed)); - EXPECT_FALSE(cbp_1_0_asymmetry_allowed.Matches(cbp_3_1)); - EXPECT_TRUE(cbp_1_0_asymmetry_allowed.Matches(cbp_3_1_asymmetry_allowed)); - - // Test explicitly disabling level asymmetry. It should have the same behavior - // as missing the param. - cbp_1_0.SetParam(cricket::kH264FmtpLevelAsymmetryAllowed, "0"); - EXPECT_TRUE(cbp_1_0.Matches(cbp_1_0_asymmetry_allowed)); - EXPECT_FALSE(cbp_1_0.Matches(cbp_3_1_asymmetry_allowed)); -} - TEST(CodecTest, TestDataCodecMatches) { // Test a codec with a static payload type. DataCodec c0(95, "D"); @@ -259,7 +199,7 @@ TEST(CodecTest, TestDataCodecMatches) { EXPECT_FALSE(c1.Matches(DataCodec(95, "D"))); } -TEST(CodecTest, TestSetParamGetParamAndRemoveParam) { +TEST_F(CodecTest, TestSetParamGetParamAndRemoveParam) { AudioCodec codec; codec.SetParam("a", "1"); codec.SetParam("b", "x"); @@ -280,7 +220,7 @@ TEST(CodecTest, TestSetParamGetParamAndRemoveParam) { EXPECT_FALSE(codec.RemoveParam("c")); } -TEST(CodecTest, TestIntersectFeedbackParams) { +TEST_F(CodecTest, TestIntersectFeedbackParams) { const FeedbackParam a1("a", "1"); const FeedbackParam b2("b", "2"); const FeedbackParam b3("b", "3"); @@ -299,7 +239,7 @@ TEST(CodecTest, TestIntersectFeedbackParams) { EXPECT_FALSE(c1.HasFeedbackParam(c3)); } -TEST(CodecTest, TestGetCodecType) { +TEST_F(CodecTest, TestGetCodecType) { // Codec type comparison should be case insenstive on names. const VideoCodec codec(96, "V"); const VideoCodec rtx_codec(96, "rTx"); @@ -311,7 +251,7 @@ TEST(CodecTest, TestGetCodecType) { EXPECT_EQ(VideoCodec::CODEC_RED, red_codec.GetCodecType()); } -TEST(CodecTest, TestCreateRtxCodec) { +TEST_F(CodecTest, TestCreateRtxCodec) { VideoCodec rtx_codec = VideoCodec::CreateRtxCodec(96, 120); EXPECT_EQ(96, rtx_codec.id); EXPECT_EQ(VideoCodec::CODEC_RTX, rtx_codec.GetCodecType()); @@ -321,7 +261,7 @@ TEST(CodecTest, TestCreateRtxCodec) { EXPECT_EQ(120, associated_payload_type); } -TEST(CodecTest, TestValidateCodecFormat) { +TEST_F(CodecTest, TestValidateCodecFormat) { const VideoCodec codec(96, "V"); ASSERT_TRUE(codec.ValidateCodecFormat()); @@ -362,7 +302,7 @@ TEST(CodecTest, TestValidateCodecFormat) { EXPECT_TRUE(different_bitrates.ValidateCodecFormat()); } -TEST(CodecTest, TestToCodecParameters) { +TEST_F(CodecTest, TestToCodecParameters) { const VideoCodec v(96, "V"); webrtc::RtpCodecParameters codec_params_1 = v.ToCodecParameters(); EXPECT_EQ(96, codec_params_1.payload_type); diff --git a/webrtc/media/base/mediaconstants.cc b/webrtc/media/base/mediaconstants.cc index 0bde3f9332..2bbca7c632 100644 --- a/webrtc/media/base/mediaconstants.cc +++ b/webrtc/media/base/mediaconstants.cc @@ -97,9 +97,6 @@ const char kH264CodecName[] = "H264"; // RFC 6184 RTP Payload Format for H.264 video const char kH264FmtpProfileLevelId[] = "profile-level-id"; -// If no profile-level-id is present as a parameter, the Baseline Profile -// without additional constraints at Level 1 is implied. -const char kH264FmtpDefaultProfileLevelId[] = "42000a"; const char kH264FmtpLevelAsymmetryAllowed[] = "level-asymmetry-allowed"; const char kH264FmtpPacketizationMode[] = "packetization-mode"; const char kH264ProfileLevelConstrainedBaseline[] = "42e01f"; diff --git a/webrtc/media/base/mediaconstants.h b/webrtc/media/base/mediaconstants.h index 03a19de9f6..57ddcbd437 100644 --- a/webrtc/media/base/mediaconstants.h +++ b/webrtc/media/base/mediaconstants.h @@ -123,7 +123,6 @@ extern const char kH264CodecName[]; // RFC 6184 RTP Payload Format for H.264 video extern const char kH264FmtpProfileLevelId[]; -extern const char kH264FmtpDefaultProfileLevelId[]; extern const char kH264FmtpLevelAsymmetryAllowed[]; extern const char kH264FmtpPacketizationMode[]; extern const char kH264ProfileLevelConstrainedBaseline[];