From 7233df975189c83ddb326a7eeb46fa941aabc688 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Fri, 14 Feb 2025 11:43:11 -0800 Subject: [PATCH] Filter fmtp parameters from RED capabilities MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit and ensure there is only one, similar to what is done with RTX. This avoids exposing a payload type there. See also https://github.com/w3c/webrtc-pc/issues/2696 BUG=webrtc:42221750,webrtc:360058654 Change-Id: Id7c2ddeaf47a3169db9be43c9c5b8e59346f1d57 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/376760 Reviewed-by: Henrik Boström Commit-Queue: Philipp Hancke Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#43929} --- media/base/codec_comparators.cc | 7 +++++++ media/engine/webrtc_voice_engine.cc | 2 +- pc/rtp_parameters_conversion.cc | 26 +++++++++++++----------- pc/rtp_parameters_conversion_unittest.cc | 19 +++++++++-------- 4 files changed, 32 insertions(+), 22 deletions(-) diff --git a/media/base/codec_comparators.cc b/media/base/codec_comparators.cc index a1edd9a97c..54201168d8 100644 --- a/media/base/codec_comparators.cc +++ b/media/base/codec_comparators.cc @@ -19,6 +19,7 @@ #include "absl/functional/any_invocable.h" #include "absl/strings/match.h" #include "absl/strings/string_view.h" +#include "api/media_types.h" #include "api/rtp_parameters.h" #include "api/video_codecs/av1_profile.h" #include "api/video_codecs/h264_profile_level_id.h" @@ -407,6 +408,12 @@ bool IsSameRtpCodecIgnoringLevel(const Codec& codec, return IsSameCodecSpecific(rtp_codec.name, params1, rtp_codec2.name, params2); } + // audio/RED should ignore the parameters which specify payload types so + // can not be compared. + if (rtp_codec.kind == cricket::MediaType::MEDIA_TYPE_AUDIO && + rtp_codec.name == cricket::kRedCodecName) { + return true; + } return params1 == params2; } diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc index d9f138263e..934fa27940 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/media/engine/webrtc_voice_engine.cc @@ -333,7 +333,7 @@ bool CheckRedParameters( // Check the FMTP line for the empty parameter which should match // /[/...] - auto red_parameters = red_codec.params.find(""); + auto red_parameters = red_codec.params.find(kCodecParamNotInNameValueFormat); if (red_parameters == red_codec.params.end()) { RTC_LOG(LS_WARNING) << "audio/RED missing fmtp parameters."; return false; diff --git a/pc/rtp_parameters_conversion.cc b/pc/rtp_parameters_conversion.cc index f6b83da6a6..415eac6480 100644 --- a/pc/rtp_parameters_conversion.cc +++ b/pc/rtp_parameters_conversion.cc @@ -10,21 +10,16 @@ #include "pc/rtp_parameters_conversion.h" -#include -#include +#include #include -#include -#include +#include -#include "api/array_view.h" #include "api/media_types.h" -#include "api/rtc_error.h" +#include "api/rtp_parameters.h" #include "media/base/codec.h" #include "media/base/media_constants.h" -#include "media/base/rtp_utils.h" -#include "rtc_base/checks.h" +#include "pc/session_description.h" #include "rtc_base/logging.h" -#include "rtc_base/strings/string_builder.h" namespace webrtc { @@ -118,6 +113,10 @@ RtpCapabilities ToRtpCapabilities( bool have_rtx = false; for (const cricket::Codec& cricket_codec : cricket_codecs) { if (cricket_codec.name == cricket::kRedCodecName) { + if (have_red) { + // There should only be one RED codec entry in caps. + continue; + } have_red = true; } else if (cricket_codec.name == cricket::kUlpfecCodecName) { have_ulpfec = true; @@ -125,14 +124,17 @@ RtpCapabilities ToRtpCapabilities( have_flexfec = true; } else if (cricket_codec.name == cricket::kRtxCodecName) { if (have_rtx) { - // There should only be one RTX codec entry + // There should only be one RTX codec entry in caps. continue; } have_rtx = true; } auto codec_capability = ToRtpCodecCapability(cricket_codec); - if (cricket_codec.name == cricket::kRtxCodecName) { - // RTX codec should not have any parameter + if (cricket_codec.name == cricket::kRtxCodecName || + cricket_codec.name == cricket::kRedCodecName) { + // For RTX this removes the APT which points to a payload type. + // For RED this removes the redundancy spec which points to a payload + // type. codec_capability.parameters.clear(); } capabilities.codecs.push_back(codec_capability); diff --git a/pc/rtp_parameters_conversion_unittest.cc b/pc/rtp_parameters_conversion_unittest.cc index e1b6ac2608..337313e646 100644 --- a/pc/rtp_parameters_conversion_unittest.cc +++ b/pc/rtp_parameters_conversion_unittest.cc @@ -10,12 +10,15 @@ #include "pc/rtp_parameters_conversion.h" -#include #include +#include #include #include "api/media_types.h" +#include "api/rtp_parameters.h" #include "media/base/codec.h" +#include "media/base/media_constants.h" +#include "pc/session_description.h" #include "test/gmock.h" #include "test/gtest.h" @@ -137,19 +140,15 @@ TEST(RtpParametersConversionTest, ToRtpCodecCapabilityUnknownFeedbackParam) { // the "fec" list is assembled correctly. TEST(RtpParametersConversionTest, ToRtpCapabilities) { cricket::Codec vp8 = cricket::CreateVideoCodec(101, "VP8"); - vp8.clockrate = 90000; cricket::Codec red = cricket::CreateVideoCodec(102, "red"); - red.clockrate = 90000; + // Note: fmtp not usually done for video-red but we want it filtered. + red.SetParam(cricket::kCodecParamNotInNameValueFormat, "101/101"); + cricket::Codec red2 = cricket::CreateVideoCodec(127, "red"); cricket::Codec ulpfec = cricket::CreateVideoCodec(103, "ulpfec"); - ulpfec.clockrate = 90000; - cricket::Codec flexfec = cricket::CreateVideoCodec(102, "flexfec-03"); - flexfec.clockrate = 90000; - cricket::Codec rtx = cricket::CreateVideoRtxCodec(014, 101); - cricket::Codec rtx2 = cricket::CreateVideoRtxCodec(105, 109); RtpCapabilities capabilities = @@ -166,7 +165,7 @@ TEST(RtpParametersConversionTest, ToRtpCapabilities) { EXPECT_EQ(3, capabilities.header_extensions[1].preferred_id); EXPECT_EQ(0u, capabilities.fec.size()); - capabilities = ToRtpCapabilities({vp8, red, ulpfec, rtx}, + capabilities = ToRtpCapabilities({vp8, red, red2, ulpfec, rtx}, cricket::RtpHeaderExtensions()); EXPECT_EQ(4u, capabilities.codecs.size()); EXPECT_THAT( @@ -178,6 +177,8 @@ TEST(RtpParametersConversionTest, ToRtpCapabilities) { EXPECT_EQ(3u, capabilities.codecs.size()); EXPECT_THAT(capabilities.fec, UnorderedElementsAre(FecMechanism::RED, FecMechanism::FLEXFEC)); + EXPECT_EQ(capabilities.codecs[1].name, "red"); + EXPECT_TRUE(capabilities.codecs[1].parameters.empty()); } } // namespace webrtc