From 5c4c836ac4daf51c389ba8bb88e95223fb2228b0 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Tue, 8 Dec 2020 13:55:12 +0100 Subject: [PATCH] use [35,65] rtp payload type range for new codecs Changes the video payload type allocation to use the lower dynamic payload type range [35,65] described in https://www.iana.org/assignments/rtp-parameters/rtp-parameters.xhtml#rtp-parameters-1 for new codecs such as FlexFEC. BUG=webrtc:12194 Change-Id: I71782199074e0fc94fa6aa8c36afeb68221a2839 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/195822 Commit-Queue: Harald Alvestrand Reviewed-by: Ilya Nikolaevskiy Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#32799} --- media/engine/webrtc_video_engine.cc | 69 +++++++++++++++----- media/engine/webrtc_video_engine_unittest.cc | 20 ++++++ 2 files changed, 71 insertions(+), 18 deletions(-) diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 5837899bf3..a7ca96719d 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -101,18 +101,24 @@ void AddDefaultFeedbackParams(VideoCodec* codec, } } -// This function will assign dynamic payload types (in the range [96, 127]) to -// the input codecs, and also add ULPFEC, RED, FlexFEC, and associated RTX -// codecs for recognized codecs (VP8, VP9, H264, and RED). It will also add -// default feedback params to the codecs. +// This function will assign dynamic payload types (in the range [96, 127] and +// for new additions the [35, 65] range) to the input codecs, and also add +// ULPFEC, RED, FlexFEC, and associated RTX codecs for recognized codecs (VP8, +// VP9, H264 and RED). It will also add default feedback params to the codecs. std::vector AssignPayloadTypesAndDefaultCodecs( std::vector input_formats, const webrtc::WebRtcKeyValueConfig& trials) { if (input_formats.empty()) return std::vector(); - static const int kFirstDynamicPayloadType = 96; - static const int kLastDynamicPayloadType = 127; - int payload_type = kFirstDynamicPayloadType; + // Due to interoperability issues with old Chrome/WebRTC versions only use + // the lower range for new codecs. + static const int kFirstDynamicPayloadTypeLowerRange = 35; + static const int kLastDynamicPayloadTypeLowerRange = 65; + + static const int kFirstDynamicPayloadTypeUpperRange = 96; + static const int kLastDynamicPayloadTypeUpperRange = 127; + int payload_type_upper = kFirstDynamicPayloadTypeUpperRange; + int payload_type_lower = kFirstDynamicPayloadTypeLowerRange; input_formats.push_back(webrtc::SdpVideoFormat(kRedCodecName)); input_formats.push_back(webrtc::SdpVideoFormat(kUlpfecCodecName)); @@ -130,27 +136,54 @@ std::vector AssignPayloadTypesAndDefaultCodecs( std::vector output_codecs; for (const webrtc::SdpVideoFormat& format : input_formats) { VideoCodec codec(format); - codec.id = payload_type; + bool isCodecValidForLowerRange = + absl::EqualsIgnoreCase(codec.name, kFlexfecCodecName); + if (!isCodecValidForLowerRange) { + codec.id = payload_type_upper++; + } else { + codec.id = payload_type_lower++; + } AddDefaultFeedbackParams(&codec, trials); output_codecs.push_back(codec); - // Increment payload type. - ++payload_type; - if (payload_type > kLastDynamicPayloadType) { - RTC_LOG(LS_ERROR) << "Out of dynamic payload types, skipping the rest."; + if (payload_type_upper > kLastDynamicPayloadTypeUpperRange) { + RTC_LOG(LS_ERROR) + << "Out of dynamic payload types [96,127], skipping the rest."; + // TODO(https://bugs.chromium.org/p/webrtc/issues/detail?id=12194): + // continue in lower range. + break; + } + if (payload_type_lower > kLastDynamicPayloadTypeLowerRange) { + // TODO(https://bugs.chromium.org/p/webrtc/issues/detail?id=12248): + // return an error. + RTC_LOG(LS_ERROR) + << "Out of dynamic payload types [35,65], skipping the rest."; break; } // Add associated RTX codec for non-FEC codecs. if (!absl::EqualsIgnoreCase(codec.name, kUlpfecCodecName) && !absl::EqualsIgnoreCase(codec.name, kFlexfecCodecName)) { - output_codecs.push_back( - VideoCodec::CreateRtxCodec(payload_type, codec.id)); + if (!isCodecValidForLowerRange) { + output_codecs.push_back( + VideoCodec::CreateRtxCodec(payload_type_upper++, codec.id)); + } else { + output_codecs.push_back( + VideoCodec::CreateRtxCodec(payload_type_lower++, codec.id)); + } - // Increment payload type. - ++payload_type; - if (payload_type > kLastDynamicPayloadType) { - RTC_LOG(LS_ERROR) << "Out of dynamic payload types, skipping the rest."; + if (payload_type_upper > kLastDynamicPayloadTypeUpperRange) { + RTC_LOG(LS_ERROR) + << "Out of dynamic payload types [96,127], skipping rtx."; + // TODO(https://bugs.chromium.org/p/webrtc/issues/detail?id=12194): + // continue in lower range. + break; + } + if (payload_type_lower > kLastDynamicPayloadTypeLowerRange) { + // TODO(https://bugs.chromium.org/p/webrtc/issues/detail?id=12248): + // return an error. + RTC_LOG(LS_ERROR) + << "Out of dynamic payload types [35,65], skipping rtx."; break; } } diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index d4b26218c5..63d134d9b7 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -971,6 +971,26 @@ TEST_F(WebRtcVideoEngineTest, EXPECT_THAT(engine_.send_codecs(), Contains(flexfec)); } +// Test that the FlexFEC "codec" gets assigned to the lower payload type range +TEST_F(WebRtcVideoEngineTest, Flexfec03LowerPayloadTypeRange) { + encoder_factory_->AddSupportedVideoCodecType("VP8"); + + auto flexfec = Field("name", &VideoCodec::name, "flexfec-03"); + + // FlexFEC is active with field trial. + RTC_DCHECK(!override_field_trials_); + override_field_trials_ = std::make_unique( + "WebRTC-FlexFEC-03-Advertised/Enabled/"); + auto send_codecs = engine_.send_codecs(); + auto it = std::find_if(send_codecs.begin(), send_codecs.end(), + [](const cricket::VideoCodec& codec) { + return codec.name == "flexfec-03"; + }); + ASSERT_NE(it, send_codecs.end()); + EXPECT_LE(35, it->id); + EXPECT_GE(65, it->id); +} + // Test that codecs are added in the order they are reported from the factory. TEST_F(WebRtcVideoEngineTest, ReportSupportedCodecs) { encoder_factory_->AddSupportedVideoCodecType("VP8");