From 09ca9a132c0c0f8be866919cd9a25cd9829d9a62 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Mon, 7 Dec 2020 15:42:50 +0100 Subject: [PATCH] allow dynamic payload types <= 95 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit extends the range of allowed dynamic payload types by the lower range [35, 63] from https://www.iana.org/assignments/rtp-parameters/rtp-parameters.xhtml#rtp-parameters-1 The upper limit of that range is chosen to avoid the payload types 64 and 65 which may conflict with RTCP as described in https://tools.ietf.org/html/rfc5761#section-4 A killswitch WebRTC-PayloadTypes-Lower-Dynamic-Range is added to allow turning this off should it result in interoperability issues. BUG=webrtc:12194 Change-Id: I7564cc190e4c18cd5b48510a5b6a467c13b0fae4 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/195821 Reviewed-by: Erik Språng Reviewed-by: Ilya Nikolaevskiy Reviewed-by: Harald Alvestrand Commit-Queue: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/master@{#32790} --- media/base/codec.cc | 31 +++++++++++++++++++++++++++---- media/base/codec_unittest.cc | 33 ++++++++++++++++++++++++--------- 2 files changed, 51 insertions(+), 13 deletions(-) diff --git a/media/base/codec.cc b/media/base/codec.cc index 6b9a052da3..75d0c6cc3f 100644 --- a/media/base/codec.cc +++ b/media/base/codec.cc @@ -18,6 +18,7 @@ #include "rtc_base/logging.h" #include "rtc_base/string_encode.h" #include "rtc_base/strings/string_builder.h" +#include "system_wrappers/include/field_trial.h" namespace cricket { namespace { @@ -143,10 +144,32 @@ 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. - const int kMaxStaticPayloadId = 95; - return (id <= kMaxStaticPayloadId || codec.id <= kMaxStaticPayloadId) - ? (id == codec.id) - : (absl::EqualsIgnoreCase(name, codec.name)); + + // Legacy behaviour with killswitch. + if (webrtc::field_trial::IsDisabled( + "WebRTC-PayloadTypes-Lower-Dynamic-Range")) { + const int kMaxStaticPayloadId = 95; + return (id <= kMaxStaticPayloadId || codec.id <= kMaxStaticPayloadId) + ? (id == codec.id) + : (absl::EqualsIgnoreCase(name, codec.name)); + } + // 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. + 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); + return is_id_in_dynamic_range && is_codec_id_in_dynamic_range + ? (absl::EqualsIgnoreCase(name, codec.name)) + : (id == codec.id); } bool Codec::MatchesCapability( diff --git a/media/base/codec_unittest.cc b/media/base/codec_unittest.cc index ae80113f37..d41ed9bdf9 100644 --- a/media/base/codec_unittest.cc +++ b/media/base/codec_unittest.cc @@ -106,11 +106,12 @@ TEST(CodecTest, TestAudioCodecOperators) { TEST(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))); - EXPECT_TRUE(c0.Matches(AudioCodec(95, "", 44100, 20000, 0))); - EXPECT_TRUE(c0.Matches(AudioCodec(95, "", 44100, 0, 0))); - EXPECT_TRUE(c0.Matches(AudioCodec(95, "", 0, 0, 0))); + AudioCodec c0(34, "A", 44100, 20000, 1); + EXPECT_TRUE(c0.Matches(AudioCodec(34, "", 44100, 20000, 1))); + EXPECT_TRUE(c0.Matches(AudioCodec(34, "", 44100, 20000, 0))); + EXPECT_TRUE(c0.Matches(AudioCodec(34, "", 44100, 0, 0))); + EXPECT_TRUE(c0.Matches(AudioCodec(34, "", 0, 0, 0))); + EXPECT_FALSE(c0.Matches(AudioCodec(96, "A", 44100, 20000, 1))); EXPECT_FALSE(c0.Matches(AudioCodec(96, "", 44100, 20000, 1))); EXPECT_FALSE(c0.Matches(AudioCodec(95, "", 55100, 20000, 1))); EXPECT_FALSE(c0.Matches(AudioCodec(95, "", 44100, 30000, 1))); @@ -123,7 +124,11 @@ TEST(CodecTest, TestAudioCodecMatches) { EXPECT_TRUE(c1.Matches(AudioCodec(97, "A", 0, 0, 0))); EXPECT_TRUE(c1.Matches(AudioCodec(96, "a", 0, 0, 0))); EXPECT_TRUE(c1.Matches(AudioCodec(97, "a", 0, 0, 0))); + EXPECT_TRUE(c1.Matches(AudioCodec(35, "a", 0, 0, 0))); + EXPECT_TRUE(c1.Matches(AudioCodec(42, "a", 0, 0, 0))); + EXPECT_TRUE(c1.Matches(AudioCodec(65, "a", 0, 0, 0))); EXPECT_FALSE(c1.Matches(AudioCodec(95, "A", 0, 0, 0))); + EXPECT_FALSE(c1.Matches(AudioCodec(34, "A", 0, 0, 0))); EXPECT_FALSE(c1.Matches(AudioCodec(96, "", 44100, 20000, 2))); EXPECT_FALSE(c1.Matches(AudioCodec(96, "A", 55100, 30000, 1))); @@ -201,9 +206,10 @@ TEST(CodecTest, TestVideoCodecEqualsWithDifferentPacketization) { TEST(CodecTest, TestVideoCodecMatches) { // Test a codec with a static payload type. - VideoCodec c0(95, "V"); - EXPECT_TRUE(c0.Matches(VideoCodec(95, ""))); + VideoCodec c0(34, "V"); + EXPECT_TRUE(c0.Matches(VideoCodec(34, ""))); EXPECT_FALSE(c0.Matches(VideoCodec(96, ""))); + EXPECT_FALSE(c0.Matches(VideoCodec(96, "V"))); // Test a codec with a dynamic payload type. VideoCodec c1(96, "V"); @@ -211,8 +217,12 @@ TEST(CodecTest, TestVideoCodecMatches) { EXPECT_TRUE(c1.Matches(VideoCodec(97, "V"))); EXPECT_TRUE(c1.Matches(VideoCodec(96, "v"))); EXPECT_TRUE(c1.Matches(VideoCodec(97, "v"))); + EXPECT_TRUE(c1.Matches(VideoCodec(35, "v"))); + EXPECT_TRUE(c1.Matches(VideoCodec(42, "v"))); + EXPECT_TRUE(c1.Matches(VideoCodec(65, "v"))); EXPECT_FALSE(c1.Matches(VideoCodec(96, ""))); EXPECT_FALSE(c1.Matches(VideoCodec(95, "V"))); + EXPECT_FALSE(c1.Matches(VideoCodec(34, "V"))); } TEST(CodecTest, TestVideoCodecMatchesWithDifferentPacketization) { @@ -295,8 +305,9 @@ TEST(CodecTest, TestH264CodecMatches) { TEST(CodecTest, TestDataCodecMatches) { // Test a codec with a static payload type. - DataCodec c0(95, "D"); - EXPECT_TRUE(c0.Matches(DataCodec(95, ""))); + DataCodec c0(34, "D"); + EXPECT_TRUE(c0.Matches(DataCodec(34, ""))); + EXPECT_FALSE(c0.Matches(DataCodec(96, "D"))); EXPECT_FALSE(c0.Matches(DataCodec(96, ""))); // Test a codec with a dynamic payload type. @@ -305,8 +316,12 @@ TEST(CodecTest, TestDataCodecMatches) { EXPECT_TRUE(c1.Matches(DataCodec(97, "D"))); EXPECT_TRUE(c1.Matches(DataCodec(96, "d"))); EXPECT_TRUE(c1.Matches(DataCodec(97, "d"))); + EXPECT_TRUE(c1.Matches(DataCodec(35, "d"))); + EXPECT_TRUE(c1.Matches(DataCodec(42, "d"))); + EXPECT_TRUE(c1.Matches(DataCodec(63, "d"))); EXPECT_FALSE(c1.Matches(DataCodec(96, ""))); EXPECT_FALSE(c1.Matches(DataCodec(95, "D"))); + EXPECT_FALSE(c1.Matches(DataCodec(34, "D"))); } TEST(CodecTest, TestSetParamGetParamAndRemoveParam) {