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}

pc/BUILD.gn

@@ -2106,6 +2106,7 @@ if (rtc_include_tests && !build_with_chromium) {
       ":rtc_pc",
       ":rtcp_mux_filter",
       ":rtp_media_utils",
+      ":rtp_parameters_conversion",
       ":rtp_transport",
       ":rtp_transport_internal",
       ":sctp_transport",

pc/media_session.cc

@@ -1045,6 +1045,7 @@ std::vector<Codec> MatchCodecPreference(
       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 @@ std::vector<Codec> MatchCodecPreference(
       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 @@ std::vector<Codec> MatchCodecPreference(
               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;
                 }

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 ::testing::SizeIs;
 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 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCreateAudioOffer) {
   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 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestSetAudioCodecs) {
       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.

pc/peer_connection_encodings_integrationtest.cc

@@ -1678,6 +1678,67 @@ TEST_F(PeerConnectionEncodingsIntegrationTest,
   EXPECT_FALSE(parameters.encodings[0].codec);
 }
 
+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();