Makes sure that RED is not added twice to the list of codecs when it is used with Opus.
Bug: webrtc:15606
Change-Id: I3ab3ee287f5d2e3a0a46520608e5c0931e0bff90
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/325180
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Tomas Lundqvist <tomasl@google.com>
Cr-Commit-Position: refs/heads/main@{#41028}
diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index e9a49ca..7c22a26 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -2106,6 +2106,7 @@
":rtc_pc",
":rtcp_mux_filter",
":rtp_media_utils",
+ ":rtp_parameters_conversion",
":rtp_transport",
":rtp_transport_internal",
":sctp_transport",
diff --git a/pc/media_session.cc b/pc/media_session.cc
index bc5d80b..a763919 100644
--- a/pc/media_session.cc
+++ b/pc/media_session.cc
@@ -1045,6 +1045,7 @@
want_red = true;
}
}
+ bool red_was_added = false;
for (const auto& codec_preference : codec_preferences) {
auto found_codec = absl::c_find_if(
supported_codecs, [&codec_preference](const Codec& codec) {
@@ -1062,7 +1063,13 @@
absl::optional<Codec> found_codec_with_correct_pt = FindMatchingCodec(
supported_codecs, codecs, *found_codec, field_trials);
if (found_codec_with_correct_pt) {
- filtered_codecs.push_back(*found_codec_with_correct_pt);
+ // RED may already have been added if its primary codec is before RED
+ // in the codec list.
+ bool is_red_codec = IsRedCodec(*found_codec_with_correct_pt);
+ if (!is_red_codec || !red_was_added) {
+ filtered_codecs.push_back(*found_codec_with_correct_pt);
+ red_was_added = is_red_codec ? true : red_was_added;
+ }
std::string id = rtc::ToString(found_codec_with_correct_pt->id);
// Search for the matching rtx or red codec.
if (want_red || want_rtx) {
@@ -1083,11 +1090,11 @@
if (fmtp != codec.params.end()) {
std::vector<absl::string_view> redundant_payloads =
rtc::split(fmtp->second, '/');
- if (redundant_payloads.size() > 0 &&
+ if (!redundant_payloads.empty() &&
redundant_payloads[0] == id) {
- if (std::find(filtered_codecs.begin(), filtered_codecs.end(),
- codec) == filtered_codecs.end()) {
+ if (!red_was_added) {
filtered_codecs.push_back(codec);
+ red_was_added = true;
}
break;
}
diff --git a/pc/media_session_unittest.cc b/pc/media_session_unittest.cc
index 9351c06..a1770c1 100644
--- a/pc/media_session_unittest.cc
+++ b/pc/media_session_unittest.cc
@@ -26,6 +26,7 @@
#include "absl/types/optional.h"
#include "api/candidate.h"
#include "api/crypto_params.h"
+#include "api/rtp_parameters.h"
#include "media/base/codec.h"
#include "media/base/media_constants.h"
#include "media/base/test_utils.h"
@@ -35,6 +36,7 @@
#include "p2p/base/transport_info.h"
#include "pc/media_protocol_names.h"
#include "pc/rtp_media_utils.h"
+#include "pc/rtp_parameters_conversion.h"
#include "rtc_base/arraysize.h"
#include "rtc_base/checks.h"
#include "rtc_base/fake_ssl_identity.h"
@@ -111,8 +113,16 @@
using webrtc::RtpExtension;
using webrtc::RtpTransceiverDirection;
+static AudioCodec createRedAudioCodec(absl::string_view encoding_id) {
+ AudioCodec red = cricket::CreateAudioCodec(63, "red", 48000, 2);
+ red.SetParam(cricket::kCodecParamNotInNameValueFormat,
+ std::string(encoding_id) + '/' + std::string(encoding_id));
+ return red;
+}
+
static const AudioCodec kAudioCodecs1[] = {
- cricket::CreateAudioCodec(103, "ISAC", 16000, 1),
+ cricket::CreateAudioCodec(111, "opus", 48000, 2),
+ createRedAudioCodec("111"),
cricket::CreateAudioCodec(102, "iLBC", 8000, 1),
cricket::CreateAudioCodec(0, "PCMU", 8000, 1),
cricket::CreateAudioCodec(8, "PCMA", 8000, 1),
@@ -820,6 +830,65 @@
EXPECT_EQ(cricket::kMediaProtocolSavpf, acd->protocol());
}
+// Create an offer with just Opus and RED.
+TEST_F(MediaSessionDescriptionFactoryTest,
+ TestCreateAudioOfferWithJustOpusAndRed) {
+ f1_.set_secure(SEC_ENABLED);
+ // First, prefer to only use opus and red.
+ std::vector<webrtc::RtpCodecCapability> preferences;
+ preferences.push_back(
+ webrtc::ToRtpCodecCapability(f1_.audio_sendrecv_codecs()[0]));
+ preferences.push_back(
+ webrtc::ToRtpCodecCapability(f1_.audio_sendrecv_codecs()[1]));
+ EXPECT_EQ("opus", preferences[0].name);
+ EXPECT_EQ("red", preferences[1].name);
+
+ auto opts = CreatePlanBMediaSessionOptions();
+ opts.media_description_options.at(0).codec_preferences = preferences;
+ std::unique_ptr<SessionDescription> offer =
+ f1_.CreateOfferOrError(opts, nullptr).MoveValue();
+ ASSERT_TRUE(offer.get());
+ const ContentInfo* ac = offer->GetContentByName("audio");
+ const ContentInfo* vc = offer->GetContentByName("video");
+ ASSERT_TRUE(ac != NULL);
+ ASSERT_TRUE(vc == NULL);
+ EXPECT_EQ(MediaProtocolType::kRtp, ac->type);
+ const AudioContentDescription* acd = ac->media_description()->as_audio();
+ EXPECT_EQ(MEDIA_TYPE_AUDIO, acd->type());
+ EXPECT_EQ(2U, acd->codecs().size());
+ EXPECT_EQ("opus", acd->codecs()[0].name);
+ EXPECT_EQ("red", acd->codecs()[1].name);
+}
+
+// Create an offer with RED before Opus, which enables RED with Opus encoding.
+TEST_F(MediaSessionDescriptionFactoryTest, TestCreateAudioOfferWithRedForOpus) {
+ f1_.set_secure(SEC_ENABLED);
+ // First, prefer to only use opus and red.
+ std::vector<webrtc::RtpCodecCapability> preferences;
+ preferences.push_back(
+ webrtc::ToRtpCodecCapability(f1_.audio_sendrecv_codecs()[1]));
+ preferences.push_back(
+ webrtc::ToRtpCodecCapability(f1_.audio_sendrecv_codecs()[0]));
+ EXPECT_EQ("red", preferences[0].name);
+ EXPECT_EQ("opus", preferences[1].name);
+
+ auto opts = CreatePlanBMediaSessionOptions();
+ opts.media_description_options.at(0).codec_preferences = preferences;
+ std::unique_ptr<SessionDescription> offer =
+ f1_.CreateOfferOrError(opts, nullptr).MoveValue();
+ ASSERT_TRUE(offer.get());
+ const ContentInfo* ac = offer->GetContentByName("audio");
+ const ContentInfo* vc = offer->GetContentByName("video");
+ ASSERT_TRUE(ac != NULL);
+ ASSERT_TRUE(vc == NULL);
+ EXPECT_EQ(MediaProtocolType::kRtp, ac->type);
+ const AudioContentDescription* acd = ac->media_description()->as_audio();
+ EXPECT_EQ(MEDIA_TYPE_AUDIO, acd->type());
+ EXPECT_EQ(2U, acd->codecs().size());
+ EXPECT_EQ("red", acd->codecs()[0].name);
+ EXPECT_EQ("opus", acd->codecs()[1].name);
+}
+
// Create a typical video offer, and ensure it matches what we expect.
TEST_F(MediaSessionDescriptionFactoryTest, TestCreateVideoOffer) {
MediaSessionOptions opts;
@@ -4663,13 +4732,13 @@
MAKE_VECTOR(kAudioCodecsAnswer);
const std::vector<AudioCodec> no_codecs;
- RTC_CHECK_EQ(send_codecs[1].name, "iLBC")
+ RTC_CHECK_EQ(send_codecs[2].name, "iLBC")
<< "Please don't change shared test data!";
RTC_CHECK_EQ(recv_codecs[2].name, "iLBC")
<< "Please don't change shared test data!";
// Alter iLBC send codec to have zero channels, to test that that is handled
// properly.
- send_codecs[1].channels = 0;
+ send_codecs[2].channels = 0;
// Alter iLBC receive codec to be lowercase, to test that case conversions
// are handled properly.
diff --git a/pc/peer_connection_encodings_integrationtest.cc b/pc/peer_connection_encodings_integrationtest.cc
index 4a786e0..c7181c5 100644
--- a/pc/peer_connection_encodings_integrationtest.cc
+++ b/pc/peer_connection_encodings_integrationtest.cc
@@ -1679,6 +1679,67 @@
}
TEST_F(PeerConnectionEncodingsIntegrationTest,
+ EncodingParametersRedEnabledBeforeNegotiationAudio) {
+ rtc::scoped_refptr<PeerConnectionTestWrapper> local_pc_wrapper = CreatePc();
+ rtc::scoped_refptr<PeerConnectionTestWrapper> remote_pc_wrapper = CreatePc();
+ ExchangeIceCandidates(local_pc_wrapper, remote_pc_wrapper);
+
+ std::vector<webrtc::RtpCodecCapability> send_codecs =
+ local_pc_wrapper->pc_factory()
+ ->GetRtpSenderCapabilities(cricket::MEDIA_TYPE_AUDIO)
+ .codecs;
+
+ absl::optional<webrtc::RtpCodecCapability> opus =
+ local_pc_wrapper->FindFirstSendCodecWithName(cricket::MEDIA_TYPE_AUDIO,
+ "opus");
+ ASSERT_TRUE(opus);
+
+ absl::optional<webrtc::RtpCodecCapability> red =
+ local_pc_wrapper->FindFirstSendCodecWithName(cricket::MEDIA_TYPE_AUDIO,
+ "red");
+ ASSERT_TRUE(red);
+
+ webrtc::RtpTransceiverInit init;
+ init.direction = webrtc::RtpTransceiverDirection::kSendOnly;
+ webrtc::RtpEncodingParameters encoding_parameters;
+ encoding_parameters.codec = opus;
+ init.send_encodings.push_back(encoding_parameters);
+
+ auto transceiver_or_error =
+ local_pc_wrapper->pc()->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, init);
+ ASSERT_TRUE(transceiver_or_error.ok());
+ rtc::scoped_refptr<RtpTransceiverInterface> audio_transceiver =
+ transceiver_or_error.MoveValue();
+
+ // Preferring RED over Opus should enable RED with Opus encoding.
+ send_codecs[0] = red.value();
+ send_codecs[1] = opus.value();
+
+ ASSERT_TRUE(audio_transceiver->SetCodecPreferences(send_codecs).ok());
+ NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper);
+ local_pc_wrapper->WaitForConnection();
+ remote_pc_wrapper->WaitForConnection();
+
+ webrtc::RtpParameters parameters =
+ audio_transceiver->sender()->GetParameters();
+ EXPECT_EQ(parameters.encodings[0].codec, opus);
+ EXPECT_EQ(parameters.codecs[0].payload_type, red->preferred_payload_type);
+ EXPECT_EQ(parameters.codecs[0].name, red->name);
+
+ // Check that it's possible to switch back to Opus without RED.
+ send_codecs[0] = opus.value();
+ send_codecs[1] = red.value();
+
+ ASSERT_TRUE(audio_transceiver->SetCodecPreferences(send_codecs).ok());
+ NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper);
+
+ parameters = audio_transceiver->sender()->GetParameters();
+ EXPECT_EQ(parameters.encodings[0].codec, opus);
+ EXPECT_EQ(parameters.codecs[0].payload_type, opus->preferred_payload_type);
+ EXPECT_EQ(parameters.codecs[0].name, opus->name);
+}
+
+TEST_F(PeerConnectionEncodingsIntegrationTest,
SetParametersRejectsScalabilityModeForSelectedCodec) {
rtc::scoped_refptr<PeerConnectionTestWrapper> local_pc_wrapper = CreatePc();
