From b9405c4748cf32b1bc85d74fa3f48f84498409a5 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Thu, 7 Dec 2023 18:00:47 +0100 Subject: [PATCH] Fix list of resiliency mechanisms in setCodecPreferences Add ulpfec and flexfec to list of resiliency mechanisms taken into account and in general exclude Comfort Noise (CN) from media codecs. Also introduce RtpCodecCapability::IsMediaCodec & ::IsResiliencyCodec behaving like the MediaCodec methods. BUG=webrtc:15396 Change-Id: I79041898928190bfdd33a06d8f6975d7556c46b1 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/330424 Reviewed-by: Florent Castelli Commit-Queue: Philipp Hancke Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#41485} --- api/BUILD.gn | 1 + api/rtp_parameters.cc | 9 +++++++++ api/rtp_parameters.h | 2 ++ media/base/codec.cc | 3 ++- pc/peer_connection_encodings_integrationtest.cc | 11 +---------- pc/rtp_transceiver.cc | 14 ++++---------- 6 files changed, 19 insertions(+), 21 deletions(-) diff --git a/api/BUILD.gn b/api/BUILD.gn index d92962c3a8..6cec2974fc 100644 --- a/api/BUILD.gn +++ b/api/BUILD.gn @@ -508,6 +508,7 @@ rtc_library("rtp_parameters") { ":array_view", ":priority", ":rtp_transceiver_direction", + "../media:media_constants", "../rtc_base:checks", "../rtc_base:stringutils", "../rtc_base/system:rtc_export", diff --git a/api/rtp_parameters.cc b/api/rtp_parameters.cc index 54132bcdbb..3ff4b58a2e 100644 --- a/api/rtp_parameters.cc +++ b/api/rtp_parameters.cc @@ -15,6 +15,7 @@ #include #include "api/array_view.h" +#include "media/base/media_constants.h" #include "rtc_base/strings/string_builder.h" namespace webrtc { @@ -47,6 +48,14 @@ RtcpFeedback::~RtcpFeedback() = default; RtpCodec::RtpCodec() = default; RtpCodec::RtpCodec(const RtpCodec&) = default; RtpCodec::~RtpCodec() = default; +bool RtpCodec::IsResiliencyCodec() const { + return name == cricket::kRtxCodecName || name == cricket::kRedCodecName || + name == cricket::kUlpfecCodecName || + name == cricket::kFlexfecCodecName; +} +bool RtpCodec::IsMediaCodec() const { + return !IsResiliencyCodec() && name != cricket::kComfortNoiseCodecName; +} RtpCodecCapability::RtpCodecCapability() = default; RtpCodecCapability::~RtpCodecCapability() = default; diff --git a/api/rtp_parameters.h b/api/rtp_parameters.h index e78cc6667f..025817cf37 100644 --- a/api/rtp_parameters.h +++ b/api/rtp_parameters.h @@ -167,6 +167,8 @@ struct RTC_EXPORT RtpCodec { parameters == o.parameters; } bool operator!=(const RtpCodec& o) const { return !(*this == o); } + bool IsResiliencyCodec() const; + bool IsMediaCodec() const; }; // RtpCodecCapability is to RtpCodecParameters as RtpCapabilities is to diff --git a/media/base/codec.cc b/media/base/codec.cc index fe780548cb..d18baf7132 100644 --- a/media/base/codec.cc +++ b/media/base/codec.cc @@ -311,7 +311,8 @@ webrtc::RtpCodecParameters Codec::ToCodecParameters() const { } bool Codec::IsMediaCodec() const { - return !IsResiliencyCodec(); + return !IsResiliencyCodec() && + !absl::EqualsIgnoreCase(name, kComfortNoiseCodecName); } bool Codec::IsResiliencyCodec() const { diff --git a/pc/peer_connection_encodings_integrationtest.cc b/pc/peer_connection_encodings_integrationtest.cc index ae238671c2..5bf29e4b19 100644 --- a/pc/peer_connection_encodings_integrationtest.cc +++ b/pc/peer_connection_encodings_integrationtest.cc @@ -74,15 +74,6 @@ struct StringParamToString { } }; -// RTX, RED and FEC are reliability mechanisms used in combinations with other -// codecs, but are not themselves a specific codec. Typically you don't want to -// filter these out of the list of codec preferences. -bool IsReliabilityMechanism(const RtpCodecCapability& codec) { - return absl::EqualsIgnoreCase(codec.name, cricket::kRtxCodecName) || - absl::EqualsIgnoreCase(codec.name, cricket::kRedCodecName) || - absl::EqualsIgnoreCase(codec.name, cricket::kUlpfecCodecName); -} - std::string GetCurrentCodecMimeType( rtc::scoped_refptr report, const RTCOutboundRtpStreamStats& outbound_rtp) { @@ -163,7 +154,7 @@ class PeerConnectionEncodingsIntegrationTest : public ::testing::Test { .codecs; codecs.erase(std::remove_if(codecs.begin(), codecs.end(), [&codec_name](const RtpCodecCapability& codec) { - return !IsReliabilityMechanism(codec) && + return !codec.IsResiliencyCodec() && !absl::EqualsIgnoreCase(codec.name, codec_name); }), diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc index ca626cc94b..34d744a3bb 100644 --- a/pc/rtp_transceiver.cc +++ b/pc/rtp_transceiver.cc @@ -51,9 +51,7 @@ RTCError VerifyCodecPreferences( // transceiver.direction. if (!absl::c_any_of(codecs, [&recv_codecs](const RtpCodecCapability& codec) { - return codec.name != cricket::kRtxCodecName && - codec.name != cricket::kRedCodecName && - codec.name != cricket::kFlexfecCodecName && + return codec.IsMediaCodec() && absl::c_any_of(recv_codecs, [&codec](const cricket::Codec& recv_codec) { return recv_codec.MatchesRtpCodec(codec); @@ -65,9 +63,7 @@ RTCError VerifyCodecPreferences( } if (!absl::c_any_of(codecs, [&send_codecs](const RtpCodecCapability& codec) { - return codec.name != cricket::kRtxCodecName && - codec.name != cricket::kRedCodecName && - codec.name != cricket::kFlexfecCodecName && + return codec.IsMediaCodec() && absl::c_any_of(send_codecs, [&codec](const cricket::Codec& send_codec) { return send_codec.MatchesRtpCodec(codec); @@ -101,11 +97,9 @@ RTCError VerifyCodecPreferences( } } - // Check we have a real codec (not just rtx, red or fec) + // Check we have a real codec (not just rtx, red, fec or CN) if (absl::c_all_of(codecs, [](const RtpCodecCapability& codec) { - return codec.name == cricket::kRtxCodecName || - codec.name == cricket::kRedCodecName || - codec.name == cricket::kUlpfecCodecName; + return !codec.IsMediaCodec(); })) { LOG_AND_RETURN_ERROR( RTCErrorType::INVALID_MODIFICATION,