Revert "Reland "Use Payload Type suggester for all codec merging""

This reverts commit f5d13267aee114aa60e9718fc6f5032c8a5450f3.

Reason for revert: Caused downstream test failures

Original change's description:
> Reland "Use Payload Type suggester for all codec merging"
>
> This reverts commit b7abaee819771ca297bac4c51ec0daf62bd9a3fc.
>
> Reason for revert: Suspicion that suspected breakage wasn't real
>
> Original change's description:
> > Revert "Use Payload Type suggester for all codec merging"
> >
> > This reverts commit 0bac2aae596771db020f01a57fee4828081fbc38.
> >
> > Reason for revert: Suspected breakages downstream
> >
> > Original change's description:
> > > Use Payload Type suggester for all codec merging
> > >
> > > Bug: webrtc:360058654
> > > Change-Id: Id475762253c427c1800c2352a60fc0121c2dc388
> > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/364783
> > > Reviewed-by: Florent Castelli <orphis@webrtc.org>
> > > Commit-Queue: Harald Alvestrand <hta@webrtc.org>
> > > Cr-Commit-Position: refs/heads/main@{#43267}
> >
> > Bug: webrtc:360058654, b/375132036
> > Change-Id: Ieda626270193e7e6c93903b3c03a691b2bf0c1e2
> > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/366540
> > Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
> > Commit-Queue: Harald Alvestrand <hta@webrtc.org>
> > Reviewed-by: Florent Castelli <orphis@webrtc.org>
> > Cr-Commit-Position: refs/heads/main@{#43290}
>
> Bug: webrtc:360058654, b/375132036
> Change-Id: Id6e72f7aac81023da43de7627c24dd1a792ea461
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/374304
> Commit-Queue: Harald Alvestrand <hta@webrtc.org>
> Reviewed-by: Henrik Boström <hbos@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#43739}

Bug: webrtc:360058654, b/375132036
Change-Id: I3fb302d6ddb7d9e4b0acc3eefdac74edf55ca01a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/374700
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#43763}

pc/media_session.cc

@@ -508,6 +508,7 @@ webrtc::RTCError AssignCodecIdsAndLinkRed(
           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());
         }
       }
@@ -655,10 +656,9 @@ const Codec* GetAssociatedCodecForRed(const CodecList& codec_list,
 // Adds all codecs from `reference_codecs` to `offered_codecs` that don't
 // already exist in `offered_codecs` and ensure the payload types don't
 // collide.
-webrtc::RTCError MergeCodecs(const CodecList& reference_codecs,
+void MergeCodecs(const CodecList& reference_codecs,
-                             CodecList& offered_codecs,
+                 CodecList& offered_codecs,
-                             std::string mid,
+                 UsedPayloadTypes* used_pltypes) {
-                             webrtc::PayloadTypeSuggester& pt_suggester) {
   // Add all new codecs that are not RTX/RED codecs.
   // The two-pass splitting of the loops means preferring payload types
   // of actual codecs with respect to collisions.
@@ -667,11 +667,7 @@ webrtc::RTCError MergeCodecs(const CodecList& reference_codecs,
         reference_codec.GetResiliencyType() != Codec::ResiliencyType::kRed &&
         !FindMatchingCodec(reference_codecs, offered_codecs, reference_codec)) {
       Codec codec = reference_codec;
-      auto id_or_error = pt_suggester.SuggestPayloadType(mid, codec);
+      used_pltypes->FindAndSetIdUsed(&codec);
-      if (!id_or_error.ok()) {
-        return id_or_error.MoveError();
-      }
-      codec.id = id_or_error.value();
       offered_codecs.push_back(codec);
     }
   }
@@ -698,35 +694,15 @@ webrtc::RTCError MergeCodecs(const CodecList& reference_codecs,
 
       rtx_codec.params[kCodecParamAssociatedPayloadType] =
           rtc::ToString(matching_codec->id);
-      auto id_or_error = pt_suggester.SuggestPayloadType(mid, rtx_codec);
+      used_pltypes->FindAndSetIdUsed(&rtx_codec);
-      if (!id_or_error.ok()) {
-        return id_or_error.MoveError();
-      }
-      rtx_codec.id = id_or_error.value();
       offered_codecs.push_back(rtx_codec);
     } else if (reference_codec.GetResiliencyType() ==
                    Codec::ResiliencyType::kRed &&
                !FindMatchingCodec(reference_codecs, offered_codecs,
                                   reference_codec)) {
       Codec red_codec = reference_codec;
-      const Codec* associated_codec = nullptr;
+      const Codec* associated_codec =
-      // Special case: For voice RED, if parameter is not set, look for
+          GetAssociatedCodecForRed(reference_codecs, red_codec);
-      // OPUS as the matching codec.
-      // This is because we add the RED codec in audio engine init, but
-      // can't set the parameter until PTs are assigned.
-      if (red_codec.type == Codec::Type::kAudio &&
-          red_codec.params.find(kCodecParamNotInNameValueFormat) ==
-              red_codec.params.end()) {
-        for (const Codec& codec : reference_codecs) {
-          if (absl::EqualsIgnoreCase(codec.name, kOpusCodecName)) {
-            associated_codec = &codec;
-            break;
-          }
-        }
-      } else {
-        associated_codec =
-            GetAssociatedCodecForRed(reference_codecs, red_codec);
-      }
       if (associated_codec) {
         std::optional<Codec> matching_codec = FindMatchingCodec(
             reference_codecs, offered_codecs, *associated_codec);
@@ -740,15 +716,11 @@ webrtc::RTCError MergeCodecs(const CodecList& reference_codecs,
             rtc::ToString(matching_codec->id) + "/" +
             rtc::ToString(matching_codec->id);
       }
-      auto id_or_error = pt_suggester.SuggestPayloadType(mid, red_codec);
+      used_pltypes->FindAndSetIdUsed(&red_codec);
-      if (!id_or_error.ok()) {
-        return id_or_error.MoveError();
-      }
-      red_codec.id = id_or_error.value();
       offered_codecs.push_back(red_codec);
     }
   }
-  return webrtc::RTCError::OK();
+  offered_codecs.CheckConsistency();
 }
 
 // `codecs` is a full list of codecs with correct payload type mappings, which
@@ -835,28 +807,19 @@ CodecList MatchCodecPreference(
 }
 
 // Compute the union of `codecs1` and `codecs2`.
-webrtc::RTCErrorOr<CodecList> ComputeCodecsUnion(
+CodecList ComputeCodecsUnion(const CodecList codecs1, const CodecList codecs2) {
-    const CodecList& codecs1,
-    const CodecList& codecs2,
-    std::string mid,
-    webrtc::PayloadTypeSuggester& pt_suggester) {
   CodecList all_codecs;
+  UsedPayloadTypes used_payload_types;
   for (const Codec& codec : codecs1) {
     Codec codec_mutable = codec;
-    auto id_or_error = pt_suggester.SuggestPayloadType(mid, codec);
+    used_payload_types.FindAndSetIdUsed(&codec_mutable);
-    if (!id_or_error.ok()) {
-      return id_or_error.MoveError();
-    }
-    codec_mutable.id = id_or_error.value();
     all_codecs.push_back(codec_mutable);
   }
 
   // Use MergeCodecs to merge the second half of our list as it already checks
   // and fixes problems with duplicate payload types.
-  webrtc::RTCError error = MergeCodecs(codecs2, all_codecs, mid, pt_suggester);
+  MergeCodecs(codecs2, all_codecs, &used_payload_types);
-  if (!error.ok()) {
+
-    return error;
-  }
   return all_codecs;
 }
 
@@ -1250,8 +1213,7 @@ webrtc::RTCErrorOr<Codecs> GetNegotiatedCodecsForAnswer(
     const MediaSessionOptions& session_options,
     const ContentInfo* current_content,
     const CodecList& codecs,
-    const CodecList& supported_codecs,
+    const CodecList& supported_codecs) {
-    webrtc::PayloadTypeSuggester& pt_suggester) {
   CodecList filtered_codecs;
 
   if (!media_description_options.codec_preferences.empty()) {
@@ -1290,19 +1252,7 @@ webrtc::RTCErrorOr<Codecs> GetNegotiatedCodecsForAnswer(
 
     // Use ComputeCodecsUnion to avoid having duplicate payload IDs.
     // This is a no-op for audio until RTX is added.
-    // TODO(hta): figure out why current_content is not always there.
+    filtered_codecs = ComputeCodecsUnion(filtered_codecs, other_codecs);
-    std::string mid;
-    if (current_content) {
-      mid = current_content->name;
-    } else {
-      mid = "";
-    }
-    auto codecs_or_error =
-        ComputeCodecsUnion(filtered_codecs, other_codecs, mid, pt_suggester);
-    if (!codecs_or_error.ok()) {
-      return codecs_or_error.MoveError();
-    }
-    filtered_codecs = codecs_or_error.MoveValue();
   }
 
   if (media_description_options.type == MEDIA_TYPE_AUDIO &&
@@ -1379,7 +1329,6 @@ MediaSessionDescriptionFactory::MediaSessionDescriptionFactory(
           transport_desc_factory_->trials().IsEnabled(
               "WebRTC-PayloadTypesInTransport")) {
   RTC_CHECK(transport_desc_factory_);
-  RTC_CHECK(pt_suggester_);
   if (media_engine) {
     audio_send_codecs_ = CodecList(media_engine->voice().send_codecs());
     audio_recv_codecs_ = CodecList(media_engine->voice().recv_codecs());
@@ -1868,29 +1817,20 @@ const CodecList& MediaSessionDescriptionFactory::GetVideoCodecsForAnswer(
   RTC_CHECK_NOTREACHED();
 }
 
-webrtc::RTCError MergeCodecsFromDescription(
+void MergeCodecsFromDescription(
     const std::vector<const ContentInfo*>& current_active_contents,
     CodecList& audio_codecs,
     CodecList& video_codecs,
-    webrtc::PayloadTypeSuggester& pt_suggester) {
+    UsedPayloadTypes* used_pltypes) {
   for (const ContentInfo* content : current_active_contents) {
     if (IsMediaContentOfType(content, MEDIA_TYPE_AUDIO)) {
-      webrtc::RTCError error =
+      MergeCodecs(CodecList{content->media_description()->codecs()},
-          MergeCodecs(CodecList{content->media_description()->codecs()},
+                  audio_codecs, used_pltypes);
-                      audio_codecs, content->name, pt_suggester);
-      if (!error.ok()) {
-        return error;
-      }
     } else if (IsMediaContentOfType(content, MEDIA_TYPE_VIDEO)) {
-      webrtc::RTCError error =
+      MergeCodecs(CodecList{content->media_description()->codecs()},
-          MergeCodecs(CodecList{content->media_description()->codecs()},
+                  video_codecs, used_pltypes);
-                      video_codecs, content->name, pt_suggester);
-      if (!error.ok()) {
-        return error;
-      }
     }
   }
-  return webrtc::RTCError::OK();
 }
 
 // Getting codecs for an offer involves these steps:
@@ -1906,20 +1846,15 @@ void MediaSessionDescriptionFactory::GetCodecsForOffer(
   // First - get all codecs from the current description if the media type
   // is used. Add them to `used_pltypes` so the payload type is not reused if a
   // new media type is added.
+  UsedPayloadTypes used_pltypes;
   CodecList video_codec_list(*video_codecs);
   CodecList audio_codec_list(*audio_codecs);
-  webrtc::RTCError error =
+  MergeCodecsFromDescription(current_active_contents, audio_codec_list,
-      MergeCodecsFromDescription(current_active_contents, audio_codec_list,
+                             video_codec_list, &used_pltypes);
-                                 video_codec_list, *pt_suggester_);
-  RTC_CHECK(error.ok());
 
   // Add our codecs that are not in the current description.
-  error = MergeCodecs(CodecList{all_audio_codecs_}, audio_codec_list, "",
+  MergeCodecs(CodecList{all_audio_codecs_}, audio_codec_list, &used_pltypes);
-                      *pt_suggester_);
+  MergeCodecs(CodecList{all_video_codecs_}, video_codec_list, &used_pltypes);
-  RTC_CHECK(error.ok());
-  error = MergeCodecs(CodecList{all_video_codecs_}, video_codec_list, "",
-                      *pt_suggester_);
-  RTC_CHECK(error.ok());
   *audio_codecs = audio_codec_list.codecs();
   *video_codecs = video_codec_list.codecs();
 }
@@ -1939,12 +1874,11 @@ void MediaSessionDescriptionFactory::GetCodecsForAnswer(
   // First - get all codecs from the current description if the media type
   // is used. Add them to `used_pltypes` so the payload type is not reused if a
   // new media type is added.
+  UsedPayloadTypes used_pltypes;
   CodecList video_codec_list(*video_codecs);
   CodecList audio_codec_list(*audio_codecs);
-  webrtc::RTCError error =
+  MergeCodecsFromDescription(current_active_contents, audio_codec_list,
-      MergeCodecsFromDescription(current_active_contents, audio_codec_list,
+                             video_codec_list, &used_pltypes);
-                                 video_codec_list, *pt_suggester_);
-  RTC_CHECK(error.ok());
 
   // Second - filter out codecs that we don't support at all and should ignore.
   CodecList filtered_offered_audio_codecs;
@@ -1975,12 +1909,8 @@ void MediaSessionDescriptionFactory::GetCodecsForAnswer(
 
   // Add codecs that are not in the current description but were in
   // `remote_offer`.
-  error = MergeCodecs(filtered_offered_audio_codecs, audio_codec_list, "",
+  MergeCodecs(filtered_offered_audio_codecs, audio_codec_list, &used_pltypes);
-                      *pt_suggester_);
+  MergeCodecs(filtered_offered_video_codecs, video_codec_list, &used_pltypes);
-  RTC_CHECK(error.ok());
-  error = MergeCodecs(filtered_offered_video_codecs, video_codec_list, "",
-                      *pt_suggester_);
-  RTC_CHECK(error.ok());
   *audio_codecs = audio_codec_list.codecs();
   *video_codecs = video_codec_list.codecs();
 }
@@ -2293,9 +2223,9 @@ RTCError MediaSessionDescriptionFactory::AddRtpContentForAnswer(
             ? GetAudioCodecsForAnswer(offer_rtd, answer_rtd)
             : GetVideoCodecsForAnswer(offer_rtd, answer_rtd);
     webrtc::RTCErrorOr<std::vector<Codec>> error_or_filtered_codecs =
-        GetNegotiatedCodecsForAnswer(
+        GetNegotiatedCodecsForAnswer(media_description_options, session_options,
-            media_description_options, session_options, current_content,
+                                     current_content, CodecList(codecs),
-            CodecList(codecs), CodecList(supported_codecs), *pt_suggester_);
+                                     CodecList(supported_codecs));
     if (!error_or_filtered_codecs.ok()) {
       return error_or_filtered_codecs.MoveError();
     }
@@ -2527,10 +2457,9 @@ void MediaSessionDescriptionFactory::ComputeVideoCodecsIntersectionAndUnion() {
   video_sendrecv_codecs_.clear();
 
   // Use ComputeCodecsUnion to avoid having duplicate payload IDs
-  auto error_or_codecs = ComputeCodecsUnion(
+  all_video_codecs_ = ComputeCodecsUnion(CodecList(video_recv_codecs_),
-      video_recv_codecs_, video_send_codecs_, "", *pt_suggester_);
+                                         CodecList(video_send_codecs_));
-  RTC_CHECK(error_or_codecs.ok());
+
-  all_video_codecs_ = error_or_codecs.MoveValue();
   // Use NegotiateCodecs to merge our codec lists, since the operation is
   // essentially the same. Put send_codecs as the offered_codecs, which is the
   // order we'd like to follow. The reasoning is that encoding is usually more

pc/test/integration_test_helpers.cc

@@ -20,7 +20,6 @@
 #include "absl/functional/any_invocable.h"
 #include "api/audio/builtin_audio_processing_builder.h"
 #include "api/enable_media_with_defaults.h"
-#include "api/field_trials_view.h"
 #include "api/jsep.h"
 #include "api/peer_connection_interface.h"
 #include "api/rtc_event_log/rtc_event_log_factory.h"

pc/used_ids.h

@@ -14,7 +14,9 @@
 #include <vector>
 
 #include "api/rtp_parameters.h"
+#include "media/base/codec.h"
 #include "rtc_base/checks.h"
+#include "rtc_base/logging.h"
 
 namespace cricket {
 template <typename IdStruct>
@@ -86,6 +88,41 @@ class UsedIds {
   std::set<int> id_set_;
 };
 
+// Helper class used for finding duplicate RTP payload types among audio, video
+// and data codecs. When bundle is used the payload types may not collide.
+class UsedPayloadTypes : public UsedIds<Codec> {
+ public:
+  UsedPayloadTypes()
+      : UsedIds<Codec>(kFirstDynamicPayloadTypeLowerRange,
+                       kLastDynamicPayloadTypeUpperRange) {}
+
+  // Check if a payload type is valid. The range [64-95] is forbidden
+  // when rtcp-mux is used.
+  static bool IsIdValid(Codec codec, bool rtcp_mux) {
+    if (rtcp_mux && (codec.id > kLastDynamicPayloadTypeLowerRange &&
+                     codec.id < kFirstDynamicPayloadTypeUpperRange)) {
+      return false;
+    }
+    return codec.id >= 0 && codec.id <= kLastDynamicPayloadTypeUpperRange;
+  }
+
+ protected:
+  bool IsIdUsed(int new_id) override {
+    // Range marked for RTCP avoidance is "used".
+    if (new_id > kLastDynamicPayloadTypeLowerRange &&
+        new_id < kFirstDynamicPayloadTypeUpperRange)
+      return true;
+    return UsedIds<Codec>::IsIdUsed(new_id);
+  }
+
+ private:
+  static const int kFirstDynamicPayloadTypeLowerRange = 35;
+  static const int kLastDynamicPayloadTypeLowerRange = 63;
+
+  static const int kFirstDynamicPayloadTypeUpperRange = 96;
+  static const int kLastDynamicPayloadTypeUpperRange = 127;
+};
+
 // Helper class used for finding duplicate RTP Header extension ids among
 // audio and video extensions.
 class UsedRtpHeaderExtensionIds : public UsedIds<webrtc::RtpExtension> {