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}
diff --git a/pc/media_session.cc b/pc/media_session.cc
index 4f297ba..6e85c60 100644
--- a/pc/media_session.cc
+++ b/pc/media_session.cc
@@ -508,6 +508,7 @@
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 @@
// 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,
- CodecList& offered_codecs,
- std::string mid,
- webrtc::PayloadTypeSuggester& pt_suggester) {
+void MergeCodecs(const CodecList& reference_codecs,
+ CodecList& offered_codecs,
+ UsedPayloadTypes* used_pltypes) {
// 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 @@
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);
- if (!id_or_error.ok()) {
- return id_or_error.MoveError();
- }
- codec.id = id_or_error.value();
+ used_pltypes->FindAndSetIdUsed(&codec);
offered_codecs.push_back(codec);
}
}
@@ -698,35 +694,15 @@
rtx_codec.params[kCodecParamAssociatedPayloadType] =
rtc::ToString(matching_codec->id);
- auto id_or_error = pt_suggester.SuggestPayloadType(mid, rtx_codec);
- if (!id_or_error.ok()) {
- return id_or_error.MoveError();
- }
- rtx_codec.id = id_or_error.value();
+ used_pltypes->FindAndSetIdUsed(&rtx_codec);
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;
- // Special case: For voice RED, if parameter is not set, look for
- // 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);
- }
+ const Codec* 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 @@
rtc::ToString(matching_codec->id) + "/" +
rtc::ToString(matching_codec->id);
}
- auto id_or_error = pt_suggester.SuggestPayloadType(mid, red_codec);
- if (!id_or_error.ok()) {
- return id_or_error.MoveError();
- }
- red_codec.id = id_or_error.value();
+ used_pltypes->FindAndSetIdUsed(&red_codec);
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 @@
}
// Compute the union of `codecs1` and `codecs2`.
-webrtc::RTCErrorOr<CodecList> ComputeCodecsUnion(
- const CodecList& codecs1,
- const CodecList& codecs2,
- std::string mid,
- webrtc::PayloadTypeSuggester& pt_suggester) {
+CodecList ComputeCodecsUnion(const CodecList codecs1, const CodecList codecs2) {
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);
- if (!id_or_error.ok()) {
- return id_or_error.MoveError();
- }
- codec_mutable.id = id_or_error.value();
+ used_payload_types.FindAndSetIdUsed(&codec_mutable);
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);
- if (!error.ok()) {
- return error;
- }
+ MergeCodecs(codecs2, all_codecs, &used_payload_types);
+
return all_codecs;
}
@@ -1250,8 +1213,7 @@
const MediaSessionOptions& session_options,
const ContentInfo* current_content,
const CodecList& codecs,
- const CodecList& supported_codecs,
- webrtc::PayloadTypeSuggester& pt_suggester) {
+ const CodecList& supported_codecs) {
CodecList filtered_codecs;
if (!media_description_options.codec_preferences.empty()) {
@@ -1290,19 +1252,7 @@
// 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.
- 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();
+ filtered_codecs = ComputeCodecsUnion(filtered_codecs, other_codecs);
}
if (media_description_options.type == MEDIA_TYPE_AUDIO &&
@@ -1379,7 +1329,6 @@
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 @@
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()},
- audio_codecs, content->name, pt_suggester);
- if (!error.ok()) {
- return error;
- }
+ MergeCodecs(CodecList{content->media_description()->codecs()},
+ audio_codecs, used_pltypes);
} else if (IsMediaContentOfType(content, MEDIA_TYPE_VIDEO)) {
- webrtc::RTCError error =
- MergeCodecs(CodecList{content->media_description()->codecs()},
- video_codecs, content->name, pt_suggester);
- if (!error.ok()) {
- return error;
- }
+ MergeCodecs(CodecList{content->media_description()->codecs()},
+ video_codecs, used_pltypes);
}
}
- return webrtc::RTCError::OK();
}
// Getting codecs for an offer involves these steps:
@@ -1906,20 +1846,15 @@
// 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,
- video_codec_list, *pt_suggester_);
- RTC_CHECK(error.ok());
+ MergeCodecsFromDescription(current_active_contents, audio_codec_list,
+ video_codec_list, &used_pltypes);
// Add our codecs that are not in the current description.
- error = MergeCodecs(CodecList{all_audio_codecs_}, audio_codec_list, "",
- *pt_suggester_);
- RTC_CHECK(error.ok());
- error = MergeCodecs(CodecList{all_video_codecs_}, video_codec_list, "",
- *pt_suggester_);
- RTC_CHECK(error.ok());
+ MergeCodecs(CodecList{all_audio_codecs_}, audio_codec_list, &used_pltypes);
+ MergeCodecs(CodecList{all_video_codecs_}, video_codec_list, &used_pltypes);
*audio_codecs = audio_codec_list.codecs();
*video_codecs = video_codec_list.codecs();
}
@@ -1939,12 +1874,11 @@
// 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,
- video_codec_list, *pt_suggester_);
- RTC_CHECK(error.ok());
+ MergeCodecsFromDescription(current_active_contents, audio_codec_list,
+ video_codec_list, &used_pltypes);
// Second - filter out codecs that we don't support at all and should ignore.
CodecList filtered_offered_audio_codecs;
@@ -1975,12 +1909,8 @@
// Add codecs that are not in the current description but were in
// `remote_offer`.
- error = MergeCodecs(filtered_offered_audio_codecs, audio_codec_list, "",
- *pt_suggester_);
- RTC_CHECK(error.ok());
- error = MergeCodecs(filtered_offered_video_codecs, video_codec_list, "",
- *pt_suggester_);
- RTC_CHECK(error.ok());
+ MergeCodecs(filtered_offered_audio_codecs, audio_codec_list, &used_pltypes);
+ MergeCodecs(filtered_offered_video_codecs, video_codec_list, &used_pltypes);
*audio_codecs = audio_codec_list.codecs();
*video_codecs = video_codec_list.codecs();
}
@@ -2293,9 +2223,9 @@
? GetAudioCodecsForAnswer(offer_rtd, answer_rtd)
: GetVideoCodecsForAnswer(offer_rtd, answer_rtd);
webrtc::RTCErrorOr<std::vector<Codec>> error_or_filtered_codecs =
- GetNegotiatedCodecsForAnswer(
- media_description_options, session_options, current_content,
- CodecList(codecs), CodecList(supported_codecs), *pt_suggester_);
+ GetNegotiatedCodecsForAnswer(media_description_options, session_options,
+ current_content, CodecList(codecs),
+ CodecList(supported_codecs));
if (!error_or_filtered_codecs.ok()) {
return error_or_filtered_codecs.MoveError();
}
@@ -2527,10 +2457,9 @@
video_sendrecv_codecs_.clear();
// Use ComputeCodecsUnion to avoid having duplicate payload IDs
- auto error_or_codecs = ComputeCodecsUnion(
- video_recv_codecs_, video_send_codecs_, "", *pt_suggester_);
- RTC_CHECK(error_or_codecs.ok());
- all_video_codecs_ = error_or_codecs.MoveValue();
+ all_video_codecs_ = ComputeCodecsUnion(CodecList(video_recv_codecs_),
+ CodecList(video_send_codecs_));
+
// 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
diff --git a/pc/test/integration_test_helpers.cc b/pc/test/integration_test_helpers.cc
index 885d654..39588d2 100644
--- a/pc/test/integration_test_helpers.cc
+++ b/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"
diff --git a/pc/used_ids.h b/pc/used_ids.h
index d55eead..42ef00a 100644
--- a/pc/used_ids.h
+++ b/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 @@
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> {
