Filter fmtp parameters from RED capabilities 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 <hbos@webrtc.org> Commit-Queue: Philipp Hancke <phancke@meta.com> Reviewed-by: Harald Alvestrand <hta@webrtc.org> Cr-Commit-Position: refs/heads/main@{#43929}
diff --git a/media/base/codec_comparators.cc b/media/base/codec_comparators.cc index a1edd9a..5420116 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 @@ 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 d9f138263..934fa27 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/media/engine/webrtc_voice_engine.cc
@@ -333,7 +333,7 @@ // Check the FMTP line for the empty parameter which should match // <primary codec>/<primary codec>[/...] - 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 f6b83da..415eac6 100644 --- a/pc/rtp_parameters_conversion.cc +++ b/pc/rtp_parameters_conversion.cc
@@ -10,21 +10,16 @@ #include "pc/rtp_parameters_conversion.h" -#include <cstdint> -#include <set> +#include <optional> #include <string> -#include <type_traits> -#include <utility> +#include <vector> -#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 @@ 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 @@ 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 e1b6ac2..337313e 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 <cstdint> #include <map> +#include <optional> #include <string> #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 @@ // 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 @@ 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 @@ 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