Iteratively fix unit tests to work with late assignment.

A number of unit tests assume that payload types will be assigned
without generating an offer. These are flushed out by running tests
with the --force_fieldtrials=WebRTC-PayloadTypesInTransport argument.

Bug: webrtc:360058654
Change-Id: I17cd5bfa275904a9630068190b1cd246e9ce8741
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/362500
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#43127}

media/base/codec.cc

@@ -233,6 +233,7 @@ bool Codec::Matches(const Codec& codec) const {
   // We support the ranges [96, 127] and more recently [35, 65].
   // https://www.iana.org/assignments/rtp-parameters/rtp-parameters.xhtml#rtp-parameters-1
   // Within those ranges we match by codec name, outside by codec id.
+  // We also match by name if either ID is unassigned.
   // Since no codecs are assigned an id in the range [66, 95] by us, these will
   // never match.
   const int kLowerDynamicRangeMin = 35;
@@ -246,9 +247,13 @@ bool Codec::Matches(const Codec& codec) const {
       (codec.id >= kLowerDynamicRangeMin &&
        codec.id <= kLowerDynamicRangeMax) ||
       (codec.id >= kUpperDynamicRangeMin && codec.id <= kUpperDynamicRangeMax);
-  bool matches_id = is_id_in_dynamic_range && is_codec_id_in_dynamic_range
+  bool matches_id;
-                        ? (absl::EqualsIgnoreCase(name, codec.name))
+  if ((is_id_in_dynamic_range && is_codec_id_in_dynamic_range) ||
-                        : (id == codec.id);
+      id == kIdNotSet || codec.id == kIdNotSet) {
+    matches_id = absl::EqualsIgnoreCase(name, codec.name);
+  } else {
+    matches_id = (id == codec.id);
+  }
 
   auto matches_type_specific = [&]() {
     switch (type) {

media/engine/webrtc_voice_engine_unittest.cc

@@ -180,6 +180,30 @@ void AdmSetupExpectations(webrtc::test::MockAudioDeviceModule* adm) {
   EXPECT_CALL(*adm, RegisterAudioCallback(nullptr)).WillOnce(Return(0));
   EXPECT_CALL(*adm, Terminate()).WillOnce(Return(0));
 }
+
+std::vector<cricket::Codec> AddIdToCodecs(
+    webrtc::PayloadTypePicker& pt_mapper,
+    std::vector<cricket::Codec>&& codecs_in) {
+  std::vector<cricket::Codec> codecs = std::move(codecs_in);
+  for (cricket::Codec& codec : codecs) {
+    if (codec.id == cricket::Codec::kIdNotSet) {
+      auto id_or_error = pt_mapper.SuggestMapping(codec, nullptr);
+      EXPECT_TRUE(id_or_error.ok());
+      if (id_or_error.ok()) {
+        codec.id = id_or_error.value();
+      }
+    }
+  }
+  return codecs;
+}
+
+std::vector<cricket::Codec> ReceiveCodecsWithId(
+    cricket::WebRtcVoiceEngine& engine) {
+  webrtc::PayloadTypePicker pt_mapper;
+  std::vector<cricket::Codec> codecs = engine.recv_codecs();
+  return AddIdToCodecs(pt_mapper, std::move(codecs));
+}
+
 }  // namespace
 
 // Tests that our stub library "works".
@@ -501,7 +525,11 @@ class WebRtcVoiceEngineTestFake : public ::testing::TestWithParam<bool> {
   }
 
   std::optional<int> GetCodecBitrate(int32_t ssrc) {
-    return GetSendStreamConfig(ssrc).send_codec_spec->target_bitrate_bps;
+    auto spec = GetSendStreamConfig(ssrc).send_codec_spec;
+    if (!spec.has_value()) {
+      return std::nullopt;
+    }
+    return spec->target_bitrate_bps;
   }
 
   int GetMaxBitrate(int32_t ssrc) {
@@ -872,6 +900,10 @@ class WebRtcVoiceEngineTestFake : public ::testing::TestWithParam<bool> {
     return static_cast<cricket::WebRtcVoiceReceiveChannel*>(
         receive_channel_.get());
   }
+  std::vector<cricket::Codec> SendCodecsWithId() {
+    std::vector<cricket::Codec> codecs = engine_->send_codecs();
+    return AddIdToCodecs(pt_mapper_, std::move(codecs));
+  }
 
  protected:
   rtc::AutoThread main_thread_;
@@ -888,6 +920,7 @@ class WebRtcVoiceEngineTestFake : public ::testing::TestWithParam<bool> {
   cricket::AudioSenderParameter send_parameters_;
   cricket::AudioReceiverParameters recv_parameters_;
   webrtc::AudioProcessing::Config apm_config_;
+  webrtc::PayloadTypePicker pt_mapper_;
 };
 
 INSTANTIATE_TEST_SUITE_P(TestBothWithAndWithoutNullApm,
@@ -1152,7 +1185,7 @@ TEST_P(WebRtcVoiceEngineTestFake, SetMaxSendBandwidthMultiRateAsCallee) {
   EXPECT_TRUE(SetupChannel());
   const int kDesiredBitrate = 128000;
   cricket::AudioSenderParameter parameters;
-  parameters.codecs = engine_->send_codecs();
+  parameters.codecs = SendCodecsWithId();
   parameters.max_bandwidth_bps = kDesiredBitrate;
   SetSenderParameters(parameters);
 
@@ -3889,7 +3922,7 @@ TEST(WebRtcVoiceEngineTest, SetRecvCodecs) {
         webrtc::CryptoOptions(), call.get(),
         webrtc::AudioCodecPairId::Create());
     cricket::AudioReceiverParameters parameters;
-    parameters.codecs = engine.recv_codecs();
+    parameters.codecs = ReceiveCodecsWithId(engine);
     EXPECT_TRUE(channel.SetReceiverParameters(parameters));
   }
 }

pc/media_session.cc

@@ -50,6 +50,7 @@
 #include "rtc_base/checks.h"
 #include "rtc_base/logging.h"
 #include "rtc_base/string_encode.h"
+#include "rtc_base/strings/string_builder.h"
 #include "rtc_base/unique_id_generator.h"
 
 namespace {
@@ -465,6 +466,51 @@ void NegotiateTxMode(const Codec& local_codec,
 }
 #endif
 
+// Update the ID fields of the codec vector.
+// If any codec has an ID with value "kIdNotSet", use the payload type suggester
+// to assign and record a payload type for it.
+// If there is a RED codec without its fmtp parameter, give it the ID of the
+// first OPUS codec in the codec list.
+webrtc::RTCError AssignCodecIdsAndLinkRed(
+    webrtc::PayloadTypeSuggester* pt_suggester,
+    const std::string& mid,
+    std::vector<Codec>& codecs) {
+  int opus_codec = Codec::kIdNotSet;
+  for (cricket::Codec& codec : codecs) {
+    if (codec.id == Codec::kIdNotSet) {
+      // Add payload types to codecs, if needed
+      // This should only happen if WebRTC-PayloadTypesInTransport field trial
+      // is enabled.
+      RTC_CHECK(pt_suggester);
+      auto result = pt_suggester->SuggestPayloadType(mid, codec);
+      if (!result.ok()) {
+        return result.error();
+      }
+      codec.id = result.value();
+    }
+    // record first Opus codec id
+    if (absl::EqualsIgnoreCase(codec.name, kOpusCodecName) &&
+        opus_codec == Codec::kIdNotSet) {
+      opus_codec = codec.id;
+    }
+  }
+  if (opus_codec != Codec::kIdNotSet) {
+    for (cricket::Codec& codec : codecs) {
+      if (codec.type == Codec::Type::kAudio &&
+          absl::EqualsIgnoreCase(codec.name, kRedCodecName)) {
+        if (codec.params.size() == 0) {
+          char buffer[100];
+          rtc::SimpleStringBuilder param(buffer);
+          param << opus_codec << "/" << opus_codec;
+          RTC_LOG(LS_ERROR) << "DEBUG: Setting RED param to " << param.str();
+          codec.SetParam(kCodecParamNotInNameValueFormat, param.str());
+        }
+      }
+    }
+  }
+  return webrtc::RTCError::OK();
+}
+
 // Finds a codec in `codecs2` that matches `codec_to_match`, which is
 // a member of `codecs1`. If `codec_to_match` is an RED or RTX codec, both
 // the codecs themselves and their associated codecs must match.
@@ -1215,7 +1261,7 @@ webrtc::RTCErrorOr<Codecs> GetNegotiatedCodecsForAnswer(
         }
       }
     }
-    // Add other supported video codecs.
+    // Add other supported codecs.
     std::vector<Codec> other_codecs;
     for (const Codec& codec : supported_codecs) {
       if (FindMatchingCodec(supported_codecs, codecs, codec) &&
@@ -1300,7 +1346,10 @@ MediaSessionDescriptionFactory::MediaSessionDescriptionFactory(
     webrtc::PayloadTypeSuggester* pt_suggester)
     : ssrc_generator_(ssrc_generator),
       transport_desc_factory_(transport_desc_factory),
-      pt_suggester_(pt_suggester) {
+      pt_suggester_(pt_suggester),
+      payload_types_in_transport_trial_enabled_(
+          transport_desc_factory_->trials().IsEnabled(
+              "WebRTC-PayloadTypesInTransport")) {
   RTC_CHECK(transport_desc_factory_);
   if (media_engine) {
     audio_send_codecs_ = media_engine->voice().send_codecs();
@@ -1396,6 +1445,7 @@ MediaSessionDescriptionFactory::CreateOfferOrError(
   Codecs offer_video_codecs;
   GetCodecsForOffer(current_active_contents, &offer_audio_codecs,
                     &offer_video_codecs);
+
   AudioVideoRtpHeaderExtensions extensions_with_ids =
       GetOfferedRtpHeaderExtensionsWithIds(
           current_active_contents, session_options.offer_extmap_allow_mixed,
@@ -2001,22 +2051,8 @@ RTCError MediaSessionDescriptionFactory::AddRtpContentForOffer(
     // Ignore both the codecs argument and the Get*CodecsForOffer results.
     codecs_to_include = media_description_options.codecs_to_include;
   }
-  for (cricket::Codec& codec : codecs_to_include) {
+  AssignCodecIdsAndLinkRed(pt_suggester_, media_description_options.mid,
-    if (codec.id == Codec::kIdNotSet) {
+                           codecs_to_include);
-      // Add payload types to codecs, if needed
-      // This should only happen if WebRTC-PayloadTypesInTransport field trial
-      // is enabled.
-      RTC_CHECK(pt_suggester_);
-      RTC_CHECK(transport_desc_factory_->trials().IsEnabled(
-          "WebRTC-PayloadTypesInTransport"));
-      auto result = pt_suggester_->SuggestPayloadType(
-          media_description_options.mid, codec);
-      if (!result.ok()) {
-        return result.error();
-      }
-      codec.id = result.value();
-    }
-  }
   std::unique_ptr<MediaContentDescription> content_description;
   if (media_description_options.type == MEDIA_TYPE_AUDIO) {
     content_description = std::make_unique<AudioContentDescription>();
@@ -2204,20 +2240,8 @@ RTCError MediaSessionDescriptionFactory::AddRtpContentForAnswer(
                     media_description_options.codec_preferences.empty());
     codecs_to_include = negotiated_codecs;
   }
-  for (cricket::Codec& codec : codecs_to_include) {
+  AssignCodecIdsAndLinkRed(pt_suggester_, media_description_options.mid,
-    if (codec.id == Codec::kIdNotSet) {
+                           codecs_to_include);
-      // Add payload types to codecs, if needed
-      RTC_CHECK(pt_suggester_);
-      RTC_CHECK(transport_desc_factory_->trials().IsEnabled(
-          "WebRTC-PayloadTypesInTransport"));
-      auto result = pt_suggester_->SuggestPayloadType(
-          media_description_options.mid, codec);
-      if (!result.ok()) {
-        return result.error();
-      }
-      codec.id = result.value();
-    }
-  }
 
   if (!SetCodecsInAnswer(offer_content_description, codecs_to_include,
                          media_description_options, session_options,

pc/media_session.h

@@ -322,6 +322,7 @@ class MediaSessionDescriptionFactory {
   const TransportDescriptionFactory* transport_desc_factory_;
   // Payoad type tracker interface. Must live longer than this object.
   webrtc::PayloadTypeSuggester* pt_suggester_;
+  bool payload_types_in_transport_trial_enabled_;
 };
 
 // Convenience functions.

pc/media_session_unittest.cc

@@ -92,7 +92,6 @@ const Codec kAudioCodecs1[] = {CreateAudioCodec(111, "opus", 48000, 2),
                                CreateAudioCodec(102, "iLBC", 8000, 1),
                                CreateAudioCodec(0, "PCMU", 8000, 1),
                                CreateAudioCodec(8, "PCMA", 8000, 1),
-                               CreateAudioCodec(117, "red", 8000, 1),
                                CreateAudioCodec(107, "CN", 48000, 1)};
 
 const Codec kAudioCodecs2[] = {
@@ -240,6 +239,24 @@ const char* kMediaProtocolsDtls[] = {"TCP/TLS/RTP/SAVPF", "TCP/TLS/RTP/SAVP",
 constexpr bool kStopped = true;
 constexpr bool kActive = false;
 
+// Helper used for debugging. It reports the media type and the parameters.
+std::string FullMimeType(Codec codec) {
+  rtc::StringBuilder sb;
+  switch (codec.type) {
+    case Codec::Type::kAudio:
+      sb << "audio/";
+      break;
+    case Codec::Type::kVideo:
+      sb << "video/";
+      break;
+  }
+  sb << codec.name;
+  for (auto& param : codec.params) {
+    sb << ";" << param.first << "=" << param.second;
+  }
+  return sb.Release();
+}
+
 bool IsMediaContentOfType(const ContentInfo* content, MediaType media_type) {
   RTC_DCHECK(content);
   return content->media_description()->type() == media_type;
@@ -251,6 +268,7 @@ RtpTransceiverDirection GetMediaDirection(const ContentInfo* content) {
 }
 
 void AddRtxCodec(const Codec& rtx_codec, std::vector<Codec>* codecs) {
+  RTC_LOG(LS_VERBOSE) << "Adding RTX codec " << FullMimeType(rtx_codec);
   ASSERT_FALSE(FindCodecById(*codecs, rtx_codec.id));
   codecs->push_back(rtx_codec);
 }
@@ -636,7 +654,7 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCreateAudioOffer) {
   EXPECT_EQ(MediaProtocolType::kRtp, ac->type);
   const MediaContentDescription* acd = ac->media_description();
   EXPECT_EQ(MEDIA_TYPE_AUDIO, acd->type());
-  EXPECT_EQ(f1_.audio_sendrecv_codecs(), acd->codecs());
+  EXPECT_THAT(f1_.audio_sendrecv_codecs(), ElementsAreArray(acd->codecs()));
   EXPECT_EQ(0U, acd->first_ssrc());             // no sender is attached.
   EXPECT_EQ(kAutoBandwidth, acd->bandwidth());  // default bandwidth (auto)
   EXPECT_TRUE(acd->rtcp_mux());                 // rtcp-mux defaults on

pc/peer_connection_encodings_integrationtest.cc

@@ -224,6 +224,7 @@ class PeerConnectionEncodingsIntegrationTest : public ::testing::Test {
     // Create and set answer for `remote_pc_wrapper`.
     std::unique_ptr<SessionDescriptionInterface> answer =
         CreateAnswer(remote_pc_wrapper);
+    EXPECT_TRUE(answer);
     p1 = SetLocalDescription(remote_pc_wrapper, answer.get());
     // Modify the answer before handoff because `local_pc_wrapper` should still
     // send simulcast.
@@ -1325,8 +1326,8 @@ TEST_F(PeerConnectionEncodingsIntegrationTest,
   EXPECT_EQ(*parameters.encodings[0].codec, *pcmu);
 
   NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper);
-  local_pc_wrapper->WaitForConnection();
+  ASSERT_TRUE(local_pc_wrapper->WaitForConnection());
-  remote_pc_wrapper->WaitForConnection();
+  ASSERT_TRUE(remote_pc_wrapper->WaitForConnection());
 
   rtc::scoped_refptr<const RTCStatsReport> report = GetStats(local_pc_wrapper);
   std::vector<const RTCOutboundRtpStreamStats*> outbound_rtps =
@@ -1945,7 +1946,6 @@ TEST_F(PeerConnectionEncodingsIntegrationTest,
 
   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.
@@ -1957,7 +1957,6 @@ TEST_F(PeerConnectionEncodingsIntegrationTest,
 
   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);
 }
 

pc/test/peer_connection_test_wrapper.cc

@@ -343,15 +343,23 @@ void PeerConnectionTestWrapper::AddIceCandidate(const std::string& sdp_mid,
   EXPECT_TRUE(peer_connection_->AddIceCandidate(owned_candidate.get()));
 }
 
-void PeerConnectionTestWrapper::WaitForCallEstablished() {
+bool PeerConnectionTestWrapper::WaitForCallEstablished() {
-  WaitForConnection();
+  if (!WaitForConnection())
-  WaitForAudio();
+    return false;
-  WaitForVideo();
+  if (!WaitForAudio())
+    return false;
+  if (!WaitForVideo())
+    return false;
+  return true;
 }
 
-void PeerConnectionTestWrapper::WaitForConnection() {
+bool PeerConnectionTestWrapper::WaitForConnection() {
   EXPECT_TRUE_WAIT(CheckForConnection(), kMaxWait);
+  if (testing::Test::HasFailure()) {
+    return false;
+  }
   RTC_LOG(LS_INFO) << "PeerConnectionTestWrapper " << name_ << ": Connected.";
+  return true;
 }
 
 bool PeerConnectionTestWrapper::CheckForConnection() {
@@ -361,10 +369,14 @@ bool PeerConnectionTestWrapper::CheckForConnection() {
           PeerConnectionInterface::kIceConnectionCompleted);
 }
 
-void PeerConnectionTestWrapper::WaitForAudio() {
+bool PeerConnectionTestWrapper::WaitForAudio() {
   EXPECT_TRUE_WAIT(CheckForAudio(), kMaxWait);
+  if (testing::Test::HasFailure()) {
+    return false;
+  }
   RTC_LOG(LS_INFO) << "PeerConnectionTestWrapper " << name_
                    << ": Got enough audio frames.";
+  return true;
 }
 
 bool PeerConnectionTestWrapper::CheckForAudio() {
@@ -372,10 +384,14 @@ bool PeerConnectionTestWrapper::CheckForAudio() {
           kTestAudioFrameCount);
 }
 
-void PeerConnectionTestWrapper::WaitForVideo() {
+bool PeerConnectionTestWrapper::WaitForVideo() {
   EXPECT_TRUE_WAIT(CheckForVideo(), kMaxWait);
+  if (testing::Test::HasFailure()) {
+    return false;
+  }
   RTC_LOG(LS_INFO) << "PeerConnectionTestWrapper " << name_
                    << ": Got enough video frames.";
+  return true;
 }
 
 bool PeerConnectionTestWrapper::CheckForVideo() {

pc/test/peer_connection_test_wrapper.h

@@ -106,10 +106,10 @@ class PeerConnectionTestWrapper
   void AddIceCandidate(const std::string& sdp_mid,
                        int sdp_mline_index,
                        const std::string& candidate);
-  void WaitForCallEstablished();
+  bool WaitForCallEstablished();
-  void WaitForConnection();
+  bool WaitForConnection();
-  void WaitForAudio();
+  bool WaitForAudio();
-  void WaitForVideo();
+  bool WaitForVideo();
   void GetAndAddUserMedia(bool audio,
                           const cricket::AudioOptions& audio_options,
                           bool video);