Payload type allocation: improve stability and test compatibility - Preserve codec parameters (packetization, feedback params) in redesign path. - Interleave media and resiliency codecs to maintain conventional order. - Enhance FakePayloadTypeSuggester to resolve cross-MID PT conflicts. - Guard redesign-specific logic behind the field trial flag. - Improve directional intersection for negotiated codecs. Bug: webrtc:360058654 Change-Id: I7c7516baded9f916899a08f27ffb77a2508d560c Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/473361 Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org> Commit-Queue: Harald Alvestrand <hta@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47735}
diff --git a/api/video_codecs/sdp_video_format.cc b/api/video_codecs/sdp_video_format.cc index 57cdbec..8b62f14 100644 --- a/api/video_codecs/sdp_video_format.cc +++ b/api/video_codecs/sdp_video_format.cc
@@ -161,6 +161,13 @@ builder << "]"; } + if (packetization) { + builder << ", packetization: " << *packetization; + } + if (tx_mode) { + builder << ", tx_mode: " << *tx_mode; + } + return builder.Release(); } @@ -183,7 +190,8 @@ bool operator==(const SdpVideoFormat& a, const SdpVideoFormat& b) { return a.name == b.name && a.parameters == b.parameters && - a.scalability_modes == b.scalability_modes; + a.scalability_modes == b.scalability_modes && + a.packetization == b.packetization && a.tx_mode == b.tx_mode; } const SdpVideoFormat SdpVideoFormat::VP8() {
diff --git a/api/video_codecs/sdp_video_format.h b/api/video_codecs/sdp_video_format.h index 37f03aa..6f1b7a6 100644 --- a/api/video_codecs/sdp_video_format.h +++ b/api/video_codecs/sdp_video_format.h
@@ -69,6 +69,8 @@ std::string name; CodecParameterMap parameters; absl::InlinedVector<ScalabilityMode, kScalabilityModeCount> scalability_modes; + std::optional<std::string> packetization; + std::optional<std::string> tx_mode; // Well-known video codecs and their format parameters. static const SdpVideoFormat VP8();
diff --git a/call/fake_payload_type_suggester.h b/call/fake_payload_type_suggester.h index 9d69b3e..ca4dc73 100644 --- a/call/fake_payload_type_suggester.h +++ b/call/fake_payload_type_suggester.h
@@ -39,13 +39,6 @@ const Codec& codec, bool pick_from_top_of_range = false) override { PayloadTypeRecorder& recorder = LookupRecorder(mid); - if (pick_from_top_of_range) { - RTCErrorOr<PayloadType> result = - MaybeAddMapping(mid, codec, recorder, pick_from_top_of_range); - if (result.ok()) { - return result; - } - } RTCErrorOr<PayloadType> current_pt = recorder.LookupPayloadType(codec); if (current_pt.ok()) { return current_pt; @@ -55,15 +48,19 @@ if (it != fallback_suggestions_.end()) { return it->second; } - RTCErrorOr<PayloadType> result = - MaybeAddMapping(mid, codec, recorder, pick_from_top_of_range); - if (result.ok()) { - return result; + + if (codec.id.IsSet() && !IsPayloadTypeConflict(mid, codec.id, codec)) { + pt_picker_.AddMapping(codec.id, codec); + recorder.AddMapping(codec.id, codec); + return codec.id; } + // There's only one PT picker, but multiple recorders. RTCErrorOr<PayloadType> suggested_result = pt_picker_.SuggestMapping(codec, &recorder, pick_from_top_of_range); + if (suggested_result.ok()) { + pt_picker_.AddMapping(suggested_result.value(), codec); recorder.AddMapping(suggested_result.value(), codec); } return suggested_result; @@ -93,35 +90,20 @@ return rtp_extension_picker_.SuggestMapping( extension.uri, extension.encrypt, extension.id, id_domain, nullptr); } - RTCError AddRtpHeaderExtensionMapping(absl::string_view mid, - const RtpExtension& extension, - bool local) override { + [[nodiscard]] RTCError AddRtpHeaderExtensionMapping( + absl::string_view mid, + const RtpExtension& extension, + bool local) override { return rtp_extension_picker_.AddMapping(extension.id, extension.uri, extension.encrypt); } private: - RTCErrorOr<PayloadType> MaybeAddMapping(absl::string_view mid, - const Codec& codec, - PayloadTypeRecorder& recorder, - bool pick_from_top_of_range) { - if (codec.id.IsSet()) { - if (!IsPayloadTypeConflict(mid, codec.id, codec, - pick_from_top_of_range)) { - pt_picker_.AddMapping(codec.id, codec); - recorder.AddMapping(codec.id, codec); - return codec.id; - } - } - return RTCError(RTCErrorType::INVALID_PARAMETER); - } - bool IsPayloadTypeConflict(absl::string_view mid, PayloadType payload_type, - const Codec& codec, - bool pick_from_top_of_range) const { + const Codec& codec) const { for (const auto& kv : recorders_) { - auto existing = kv.second->LookupCodec(payload_type); + RTCErrorOr<Codec> existing = kv.second->LookupCodec(payload_type); if (existing.ok()) { if (!MatchesWithReferenceAttributes(existing.value(), codec)) { return true; @@ -129,7 +111,7 @@ } } // Also check the global picker - auto global_existing = pt_picker_.LookupCodec(payload_type); + std::optional<Codec> global_existing = pt_picker_.LookupCodec(payload_type); if (global_existing && !MatchesWithReferenceAttributes(*global_existing, codec)) { return true;
diff --git a/g3doc/todo/payload_type_redesign.md b/g3doc/todo/payload_type_redesign.md index 3f3ea16..d1304e6 100644 --- a/g3doc/todo/payload_type_redesign.md +++ b/g3doc/todo/payload_type_redesign.md
@@ -209,22 +209,27 @@ `WebRTC-PayloadTypesInTransport` field trial is being developed, a "Redesign Feedback Loop" strategy is used: +1. **Identify failing tests** Run the tests for this CL with the flag + "force-fieldtrials='WebRTC-PayloadTypesInTransport/Enable'". When using this + with `gtest-parallel`, two dashes must be inserted before the extra argument. +2. **Reproduction and Isolation**: When a failure is identified in step 1, the + specific test case is cloned or ported into a specialized integration test + file (`pc/codec_vendor_redesign_unittest.cc`) on the implementation branch. + This allows for focused debugging and ensures the failure is reproducible in + a clean environment with the trial explicitly enabled. +3. **Surgical Fixes**: Fixes are developed and verified using the isolated + tests. +4. **Full Re-verification**: Once the tests are stable, run all tests without + the field trial flag to ensure there are no regressions, and then either ask + to commit this set of changes or loop back to step 1. + +To ensure that no unit tests are missed, a "canary branch" approach is used. + 1. **Canary Branch (`pt-enable`)**: Maintain a branch where the field trial is forced enabled by default. This branch is used to run the full WebRTC test suite (especially `rtc_pc_unittests` and `peerconnection_unittests`) to identify all edge cases and legacy behaviors that the redesign logic doesn't yet handle. -2. **Reproduction and Isolation**: When a failure is identified on the canary - branch, the specific test case is cloned or ported into a specialized - integration test file (`pc/codec_vendor_redesign_unittest.cc`) on the - implementation branch. This allows for focused debugging and ensures the - failure is reproducible in a clean environment with the trial explicitly - enabled. -3. **Surgical Fixes**: Fixes are developed and verified on the implementation - branch using the isolated tests. -4. **Full Re-verification**: Once the implementation branch is stable, the - canary branch is rebased to include the fixes, and the full test suite is run - again to ensure no remaining failures and to catch new regressions. ## Backwards Compatibility for Unit Testing
diff --git a/media/base/codec.cc b/media/base/codec.cc index 9aaac6e..480f74b 100644 --- a/media/base/codec.cc +++ b/media/base/codec.cc
@@ -135,6 +135,8 @@ : Codec(Type::kVideo, PayloadType::NotSet(), c.name, kVideoCodecClockrate) { params = c.parameters; scalability_modes = c.scalability_modes; + packetization = c.packetization; + tx_mode = c.tx_mode; } Codec::Codec(const Codec& c) = default;
diff --git a/media/base/fake_media_engine.cc b/media/base/fake_media_engine.cc index 3dcec19..02795f0 100644 --- a/media/base/fake_media_engine.cc +++ b/media/base/fake_media_engine.cc
@@ -44,6 +44,7 @@ #include "media/base/codec.h" #include "media/base/media_channel.h" #include "media/base/media_config.h" +#include "media/base/media_constants.h" #include "media/base/media_engine.h" #include "media/base/stream_params.h" #include "rtc_base/checks.h" @@ -722,19 +723,27 @@ std::vector<Codec> FakeVideoEngine::LegacySendCodecs(bool use_rtx) const { if (use_rtx) { return send_codecs_; - } else { - std::vector<Codec> non_rtx_codecs; - for (auto& codec : send_codecs_) { - if (codec.name != "rtx") { - non_rtx_codecs.push_back(codec); - } - } - return non_rtx_codecs; } + std::vector<Codec> out; + for (const auto& codec : send_codecs_) { + if (codec.name != kRtxCodecName) { + out.push_back(codec); + } + } + return out; } -std::vector<Codec> FakeVideoEngine::LegacyRecvCodecs(bool /* use_rtx */) const { - return recv_codecs_; +std::vector<Codec> FakeVideoEngine::LegacyRecvCodecs(bool use_rtx) const { + if (use_rtx) { + return recv_codecs_; + } + std::vector<Codec> out; + for (const auto& codec : recv_codecs_) { + if (codec.name != kRtxCodecName) { + out.push_back(codec); + } + } + return out; } std::vector<SdpVideoFormat> FakeVideoEngine::GetSupportedFormats(
diff --git a/media/base/fake_media_engine.h b/media/base/fake_media_engine.h index 8eb262a..1aee698 100644 --- a/media/base/fake_media_engine.h +++ b/media/base/fake_media_engine.h
@@ -854,7 +854,8 @@ info.supports_network_adaption = false; } specs.push_back(AudioCodecSpec{ - .format = {codec.name, codec.clockrate, channels}, .info = info}); + .format = {codec.name, codec.clockrate, channels, codec.params}, + .info = info}); } return specs; } @@ -889,7 +890,8 @@ info.supports_network_adaption = false; } specs.push_back(AudioCodecSpec{ - .format = {codec.name, codec.clockrate, channels}, .info = info}); + .format = {codec.name, codec.clockrate, channels, codec.params}, + .info = info}); } return specs; } @@ -971,7 +973,11 @@ std::vector<SdpVideoFormat> GetSupportedFormats() const override { std::vector<SdpVideoFormat> formats; for (const auto& codec : owner_->send_codecs_) { - formats.push_back(SdpVideoFormat(codec.name, codec.params)); + SdpVideoFormat format(codec.name, codec.params); + format.packetization = codec.packetization; + format.tx_mode = codec.tx_mode; + format.scalability_modes = codec.scalability_modes; + formats.push_back(format); } return formats; } @@ -991,7 +997,11 @@ std::vector<SdpVideoFormat> GetSupportedFormats() const override { std::vector<SdpVideoFormat> formats; for (const auto& codec : owner_->recv_codecs_) { - formats.push_back(SdpVideoFormat(codec.name, codec.params)); + SdpVideoFormat format(codec.name, codec.params); + format.packetization = codec.packetization; + format.tx_mode = codec.tx_mode; + format.scalability_modes = codec.scalability_modes; + formats.push_back(format); } return formats; }
diff --git a/media/base/media_engine.h b/media/base/media_engine.h index c2be2c9..dc153ab 100644 --- a/media/base/media_engine.h +++ b/media/base/media_engine.h
@@ -117,7 +117,6 @@ // Legacy: Retrieve list of supported codecs. // + protection codecs, and assigns PT numbers that may have to be // reassigned. - // This function is being moved to CodecVendor // TODO: https://issues.webrtc.org/360058654 - remove when all users updated. virtual const std::vector<Codec>& LegacySendCodecs() const = 0; virtual const std::vector<Codec>& LegacyRecvCodecs() const = 0; @@ -134,6 +133,10 @@ virtual void StopAecDump() = 0; virtual std::optional<AudioDeviceModule::Stats> GetAudioDeviceStats() = 0; + + // Returns true if the engine handles built-in codecs like DTMF and CN + // automatically. + virtual bool NeedsAuxiliaryCodecsAdded() const { return false; } }; class VideoEngineInterface : public RtpHeaderExtensionQueryInterface { @@ -177,6 +180,10 @@ virtual std::vector<SdpVideoFormat> GetSupportedFormats( bool is_decoder) const = 0; + + // Returns true if the engine handles built-in codecs like RTX, RED, FEC + // automatically. + virtual bool NeedsAuxiliaryCodecsAdded() const { return false; } }; // MediaEngineInterface is an abstraction of a media engine which can be
diff --git a/media/engine/webrtc_video_engine.h b/media/engine/webrtc_video_engine.h index d19ca1c..4eebf39 100644 --- a/media/engine/webrtc_video_engine.h +++ b/media/engine/webrtc_video_engine.h
@@ -132,16 +132,17 @@ VideoDecoderFactory* decoder_factory() const override { return decoder_factory_.get(); } - std::vector<SdpVideoFormat> GetSupportedFormats( bool is_decoder) const override; + bool NeedsAuxiliaryCodecsAdded() const override { return true; } + + private: std::vector<RtpHeaderExtensionCapability> GetRtpHeaderExtensions( /* optional field trials from PeerConnection that override those from PeerConnectionFactory */ const FieldTrialsView* field_trials) const override; - private: const std::unique_ptr<VideoDecoderFactory> decoder_factory_; const std::unique_ptr<VideoEncoderFactory> encoder_factory_; const FieldTrialsView& trials_; // from PeerConnectionFactory
diff --git a/media/engine/webrtc_voice_engine.h b/media/engine/webrtc_voice_engine.h index 47e09cf..8e2cd90 100644 --- a/media/engine/webrtc_voice_engine.h +++ b/media/engine/webrtc_voice_engine.h
@@ -138,6 +138,8 @@ std::optional<AudioDeviceModule::Stats> GetAudioDeviceStats() override; + bool NeedsAuxiliaryCodecsAdded() const override { return true; } + private: const Environment env_; std::unique_ptr<TaskQueueBase, TaskQueueDeleter> low_priority_worker_queue_;
diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 8054c46..a8e98f9 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn
@@ -457,7 +457,6 @@ "../media:media_constants", "../media:media_engine", "../rtc_base:checks", - "../rtc_base:logging", "../rtc_base/containers:flat_set", "//third_party/abseil-cpp/absl/base:nullability", "//third_party/abseil-cpp/absl/strings",
diff --git a/pc/codec_vendor.cc b/pc/codec_vendor.cc index 96b3ebe..bba1723 100644 --- a/pc/codec_vendor.cc +++ b/pc/codec_vendor.cc
@@ -196,7 +196,7 @@ config.codec.channels}) : CreateVideoCodec(PayloadType::NotSet(), kRtxCodecName); rtx.SetParam(kCodecParamAssociatedPayloadType, primary_codec.id.value()); - auto result = + RTCErrorOr<PayloadType> result = pt_suggester.SuggestPayloadType(mid, rtx, pick_from_top_of_range); if (!result.ok()) { return result.MoveError(); @@ -222,7 +222,7 @@ Codec red = (config.codec.type == Codec::Type::kAudio) ? CreateAudioCodec({kRedCodecName, 48000, 2}) : CreateVideoCodec(kRedCodecName); - auto result = + RTCErrorOr<PayloadType> result = pt_suggester.SuggestPayloadType(mid, red, pick_from_top_of_range); if (!result.ok()) { return result.MoveError(); @@ -234,7 +234,7 @@ // Video RED also gets an RTX codec. Codec red_rtx = CreateVideoCodec(PayloadType::NotSet(), kRtxCodecName); red_rtx.SetParam(kCodecParamAssociatedPayloadType, red.id.value()); - auto rtx_res = + RTCErrorOr<PayloadType> rtx_res = pt_suggester.SuggestPayloadType(mid, red_rtx, pick_from_top_of_range); if (rtx_res.ok()) { red_rtx.id = rtx_res.value(); @@ -265,7 +265,7 @@ }); if (fec_it == offered_codecs.end()) { Codec fec = CreateVideoCodec(kUlpfecCodecName); - auto result = + RTCErrorOr<PayloadType> result = pt_suggester.SuggestPayloadType(mid, fec, pick_from_top_of_range); if (!result.ok()) { return result.MoveError(); @@ -291,7 +291,7 @@ }); if (fec_it == offered_codecs.end()) { Codec fec = CreateVideoCodec(kFlexfecCodecName); - auto result = + RTCErrorOr<PayloadType> result = pt_suggester.SuggestPayloadType(mid, fec, pick_from_top_of_range); if (!result.ok()) { return result.MoveError(); @@ -321,8 +321,8 @@ Codec primary_codec; if (primary_it == offered_codecs.end()) { primary_codec = config.codec; - auto result = pt_suggester.SuggestPayloadType(mid, primary_codec, - pick_from_top_of_range); + RTCErrorOr<PayloadType> result = pt_suggester.SuggestPayloadType( + mid, primary_codec, pick_from_top_of_range); if (!result.ok()) { return result.MoveError(); } @@ -651,7 +651,8 @@ RTCError NegotiateCodecs(const CodecList& local_codecs, const CodecList& offered_codecs, CodecList& negotiated_codecs_out, - bool keep_offer_order) { + bool keep_offer_order, + bool payload_types_in_transport) { RTC_DCHECK_DISALLOW_THREAD_BLOCKING_CALLS(); flat_map<PayloadType, PayloadType> pt_mapping_table; // Since we build the negotiated codec list one entry at a time, @@ -710,11 +711,15 @@ } PayloadType apt_value(apt_int); if (!pt_mapping_table.contains(apt_value)) { - RTC_LOG(LS_WARNING) << "Unmapped apt value " << apt_value; - continue; + if (!payload_types_in_transport) { + RTC_LOG(LS_WARNING) << "Unmapped apt value " << apt_value; + continue; + } } - negotiated.SetParam(kCodecParamAssociatedPayloadType, - pt_mapping_table.at(apt_value).value()); + if (pt_mapping_table.contains(apt_value)) { + negotiated.SetParam(kCodecParamAssociatedPayloadType, + pt_mapping_table.at(apt_value).value()); + } } } if (keep_offer_order) { @@ -796,7 +801,7 @@ for (Codec& codec : codecs) { if (codec.id == PayloadType::NotSet()) { - auto result = + RTCErrorOr<PayloadType> result = pt_suggester.SuggestPayloadType(mid, codec, pick_from_top_of_range); if (!result.ok()) { return result.error(); @@ -845,8 +850,7 @@ absl::string_view mid, CodecList& codecs_out, PayloadTypeSuggester& pt_suggester, - bool pick_from_top_of_range, - bool favor_send_order) { + bool pick_from_top_of_range) { RTC_DCHECK_RUN_ON(&sequence_checker_); RTC_DCHECK_DISALLOW_THREAD_BLOCKING_CALLS(); const std::vector<CodecConfiguration>& send_configs = @@ -860,25 +864,28 @@ case RtpTransceiverDirection::kSendRecv: case RtpTransceiverDirection::kStopped: case RtpTransceiverDirection::kInactive: { - if (favor_send_order) { - RTCError error = MergeCodecsFromConfigurations( - send_configs, mid, codecs_out, pt_suggester, trials_, - pick_from_top_of_range); - if (!error.ok()) - return error; - return MergeCodecsFromConfigurations(recv_configs, mid, codecs_out, - pt_suggester, trials_, - pick_from_top_of_range); - } else { - RTCError error = MergeCodecsFromConfigurations( - recv_configs, mid, codecs_out, pt_suggester, trials_, - pick_from_top_of_range); - if (!error.ok()) - return error; - return MergeCodecsFromConfigurations(send_configs, mid, codecs_out, - pt_suggester, trials_, - pick_from_top_of_range); + // Construct the list of codecs that exist both in the send and + // receive codec lists. We expect that these lists are equal + // most of the time, with some codecs only in the receive configs. + // When there are multiple instances of the same codec, with + // diffferent parameters, we want all the versions of the codec that + // are in the send configuration, since receive configurations are + // often more expansive. + // TODO: issues.webrtc.org/514760523 - write tests to verify outcomes. + std::vector<CodecConfiguration> intersected; + for (const CodecConfiguration& send_config : send_configs) { + for (const CodecConfiguration& recv_config : recv_configs) { + if (absl::EqualsIgnoreCase(send_config.codec.name, + recv_config.codec.name) && + send_config.codec.clockrate == recv_config.codec.clockrate) { + intersected.push_back(send_config); + break; + } + } } + return MergeCodecsFromConfigurations(intersected, mid, codecs_out, + pt_suggester, trials_, + pick_from_top_of_range); } case RtpTransceiverDirection::kSendOnly: return MergeCodecsFromConfigurations(send_configs, mid, codecs_out, @@ -1087,8 +1094,7 @@ } MergeCodecsByDirection(media_description_options.type, RtpTransceiverDirection::kSendRecv, mid, codecs, - pt_suggester, /*pick_from_top_of_range=*/false, - /*favor_send_order=*/true); + pt_suggester, /*pick_from_top_of_range=*/false); } else { // LEGACY path: Assume codecs have PTs. // If current content exists and is not being recycled, use its codecs. @@ -1188,7 +1194,8 @@ } NegotiateCodecs(filtered_codecs, checked_codecs_from_offer.value(), negotiated_codecs, - media_description_options.codec_preferences.empty()); + media_description_options.codec_preferences.empty(), + payload_types_in_transport_); } else { // media_description_options.codecs_to_include contains codecs RTCErrorOr<CodecList> codecs_from_arg = @@ -1345,7 +1352,7 @@ CodecList audio_sendrecv_codecs; RTCError error = NegotiateCodecs(audio_recv_codecs_.codecs(), audio_send_codecs_.codecs(), - audio_sendrecv_codecs, true); + audio_sendrecv_codecs, true, payload_types_in_transport_); RTC_DCHECK(error.ok()); return audio_sendrecv_codecs; } @@ -1364,7 +1371,7 @@ CodecList video_sendrecv_codecs; RTCError error = NegotiateCodecs(video_recv_codecs_.codecs(), video_send_codecs_.codecs(), - video_sendrecv_codecs, true); + video_sendrecv_codecs, true, payload_types_in_transport_); RTC_DCHECK(error.ok()); return video_sendrecv_codecs; }
diff --git a/pc/codec_vendor.h b/pc/codec_vendor.h index a78e0c5..a842e7b 100644 --- a/pc/codec_vendor.h +++ b/pc/codec_vendor.h
@@ -110,8 +110,7 @@ absl::string_view mid, CodecList& codecs_out, PayloadTypeSuggester& pt_suggester, - bool pick_from_top_of_range, - bool favor_send_order = false); + bool pick_from_top_of_range); // Makes sure that modifications and reading data is done on the same thread // and to makessure we consistently make calls to GetNegotiatedCodecsForOffer
diff --git a/pc/codec_vendor_unittest.cc b/pc/codec_vendor_unittest.cc index d86306d..6cfe8bc 100644 --- a/pc/codec_vendor_unittest.cc +++ b/pc/codec_vendor_unittest.cc
@@ -289,7 +289,8 @@ CodecList merged_codecs; FakePayloadTypeSuggester pt_suggester; Codec some_codec = CreateVideoCodec(97, "foo"); - auto pt_or_error = pt_suggester.SuggestPayloadType(mid, some_codec); + RTCErrorOr<PayloadType> pt_or_error = + pt_suggester.SuggestPayloadType(mid, some_codec, false); ASSERT_THAT(pt_or_error.value(), Eq(97)); reference_codecs.push_back(some_codec); merged_codecs.push_back(some_codec); @@ -309,7 +310,8 @@ CodecList merged_codecs; FakePayloadTypeSuggester pt_suggester; Codec some_codec = CreateVideoCodec(97, "foo"); - auto pt_or_error = pt_suggester.SuggestPayloadType(mid, some_codec); + RTCErrorOr<PayloadType> pt_or_error = + pt_suggester.SuggestPayloadType(mid, some_codec, false); ASSERT_THAT(pt_or_error.value(), Eq(97)); merged_codecs.push_back(some_codec); // Use the same PT for a reference codec. This should be renumbered. @@ -428,7 +430,7 @@ // Existing codec with PT 97 Codec some_codec = CreateVideoCodec(97, "foo"); merged_codecs.push_back(some_codec); - pt_suggester.AddLocalMapping(mid, 97, some_codec); + RTC_CHECK(pt_suggester.AddLocalMapping(mid, 97, some_codec).ok()); // New codec in reference that also wants PT 97 Codec some_other_codec = CreateVideoCodec(97, "bar");
diff --git a/pc/media_session_unittest.cc b/pc/media_session_unittest.cc index 664c91a..0292a40 100644 --- a/pc/media_session_unittest.cc +++ b/pc/media_session_unittest.cc
@@ -118,7 +118,7 @@ std::span<const Codec> codecs) { for (const Codec& c : codecs) { if (c.id.IsSet()) { - payload_type_suggester_.AddLocalMapping(mid, c.id, c); + RTC_CHECK(payload_type_suggester_.AddLocalMapping(mid, c.id, c).ok()); } } } @@ -1021,17 +1021,21 @@ const MediaContentDescription* acd = ac->media_description(); const MediaContentDescription* vcd = vc->media_description(); EXPECT_EQ(acd->type(), MediaType::AUDIO); - EXPECT_EQ( - codec_lookup_helper_1_.GetCodecVendor()->audio_sendrecv_codecs().codecs(), - acd->codecs()); + EXPECT_THAT(acd->codecs(), + CodecListsMatch(codec_lookup_helper_1_.GetCodecVendor() + ->audio_sendrecv_codecs() + .codecs(), + &env_.field_trials())); EXPECT_EQ(acd->first_ssrc(), 0U); // no sender is attached EXPECT_EQ(acd->bandwidth(), kAutoBandwidth); // default bandwidth (auto) EXPECT_TRUE(acd->rtcp_mux()); // rtcp-mux defaults on EXPECT_EQ(acd->protocol(), kMediaProtocolDtlsSavpf); EXPECT_EQ(vcd->type(), MediaType::VIDEO); - EXPECT_EQ( - codec_lookup_helper_1_.GetCodecVendor()->video_sendrecv_codecs().codecs(), - vcd->codecs()); + EXPECT_THAT(vcd->codecs(), + CodecListsMatch(codec_lookup_helper_1_.GetCodecVendor() + ->video_sendrecv_codecs() + .codecs(), + &env_.field_trials())); EXPECT_EQ(vcd->first_ssrc(), 0U); // no sender is attached EXPECT_EQ(vcd->bandwidth(), kAutoBandwidth); // default bandwidth (auto) EXPECT_TRUE(vcd->rtcp_mux()); // rtcp-mux defaults on @@ -2854,9 +2858,11 @@ const MediaContentDescription* acd = ac->media_description(); const MediaContentDescription* vcd = vc->media_description(); EXPECT_EQ(acd->type(), MediaType::AUDIO); - EXPECT_EQ( - codec_lookup_helper_1_.GetCodecVendor()->audio_sendrecv_codecs().codecs(), - acd->codecs()); + EXPECT_THAT(acd->codecs(), + CodecListsMatch(codec_lookup_helper_1_.GetCodecVendor() + ->audio_sendrecv_codecs() + .codecs(), + &env_.field_trials())); const StreamParamsVec& audio_streams = acd->streams(); ASSERT_EQ(audio_streams.size(), 2U); @@ -2872,9 +2878,11 @@ EXPECT_TRUE(acd->rtcp_mux()); // rtcp-mux defaults on EXPECT_EQ(vcd->type(), MediaType::VIDEO); - EXPECT_EQ( - codec_lookup_helper_1_.GetCodecVendor()->video_sendrecv_codecs().codecs(), - vcd->codecs()); + EXPECT_THAT(vcd->codecs(), + CodecListsMatch(codec_lookup_helper_1_.GetCodecVendor() + ->video_sendrecv_codecs() + .codecs(), + &env_.field_trials())); const StreamParamsVec& video_streams = vcd->streams(); ASSERT_EQ(video_streams.size(), 1U);
diff --git a/pc/typed_codec_vendor.cc b/pc/typed_codec_vendor.cc index 20b2d66..c29c5ed 100644 --- a/pc/typed_codec_vendor.cc +++ b/pc/typed_codec_vendor.cc
@@ -35,23 +35,18 @@ namespace { -// Create the voice codec configurations. Do not allocate payload types at this -// time. std::vector<CodecConfiguration> CollectAudioCodecConfigurations( - const std::vector<AudioCodecSpec>& specs) { + const std::vector<AudioCodecSpec>& specs, + bool add_auxiliary_codecs) { std::vector<CodecConfiguration> out; - // Audio RED is handled by the engine, not the factory, and is always - // available for Opus. - bool has_red = true; - // Only generate CN payload types for these clockrates: std::map<int, bool, std::greater<int>> generate_cn = {{8000, false}}; // Only generate telephone-event payload types for these clockrates: std::map<int, bool, std::greater<int>> generate_dtmf = {{8000, false}, {48000, false}}; - for (const auto& spec : specs) { + for (const AudioCodecSpec& spec : specs) { if (absl::EqualsIgnoreCase(spec.format.name, kRedCodecName)) { continue; } @@ -63,42 +58,48 @@ FeedbackParam(kRtcpFbParamTransportCc, kParamValueEmpty)); } - if (spec.info.allow_comfort_noise) { - // Generate a CN entry if the decoder allows it and we support the - // clockrate. - auto cn = generate_cn.find(spec.format.clockrate_hz); - if (cn != generate_cn.end()) { - cn->second = true; + if (add_auxiliary_codecs) { + if (spec.info.allow_comfort_noise) { + // Generate a CN entry if the decoder allows it and we support the + // clockrate. + auto cn = generate_cn.find(spec.format.clockrate_hz); + if (cn != generate_cn.end()) { + cn->second = true; + } + } + + // Generate a telephone-event entry if we support the clockrate. + auto dtmf = generate_dtmf.find(spec.format.clockrate_hz); + if (dtmf != generate_dtmf.end()) { + dtmf->second = true; } } - // Generate a telephone-event entry if we support the clockrate. - auto dtmf = generate_dtmf.find(spec.format.clockrate_hz); - if (dtmf != generate_dtmf.end()) { - dtmf->second = true; - } - - if (has_red && config.codec.name == kOpusCodecName) { + if (config.codec.name == kOpusCodecName) { + // Audio RED is handled by the engine, not the factory, and is always + // available for Opus. config.resiliency.red = true; } out.push_back(config); } - // Add CN codecs after "proper" audio codecs. - for (const auto& cn : generate_cn) { - if (cn.second) { - CodecConfiguration cn_config; - cn_config.codec = CreateAudioCodec({kCnCodecName, cn.first, 1}); - out.push_back(cn_config); + if (add_auxiliary_codecs) { + // Add CN codecs after "proper" audio codecs. + for (const auto& cn : generate_cn) { + if (cn.second) { + CodecConfiguration cn_config; + cn_config.codec = CreateAudioCodec({kCnCodecName, cn.first, 1}); + out.push_back(cn_config); + } } - } - // Add telephone-event codecs last. - for (const auto& dtmf : generate_dtmf) { - if (dtmf.second) { - CodecConfiguration dtmf_config; - dtmf_config.codec = CreateAudioCodec({kDtmfCodecName, dtmf.first, 1}); - out.push_back(dtmf_config); + // Add telephone-event codecs last. + for (const auto& dtmf : generate_dtmf) { + if (dtmf.second) { + CodecConfiguration dtmf_config; + dtmf_config.codec = CreateAudioCodec({kDtmfCodecName, dtmf.first, 1}); + out.push_back(dtmf_config); + } } } return out; @@ -111,12 +112,14 @@ RTC_DCHECK(is_sender || voice.decoder_factory()) << "No decoder factory"; return CollectAudioCodecConfigurations( is_sender ? voice.encoder_factory()->GetSupportedEncoders() - : voice.decoder_factory()->GetSupportedDecoders()); + : voice.decoder_factory()->GetSupportedDecoders(), + voice.NeedsAuxiliaryCodecsAdded()); } std::vector<CodecConfiguration> CollectVideoCodecConfigurations( const std::vector<SdpVideoFormat>& formats, bool rtx_enabled, + bool add_auxiliary_codecs, const FieldTrialsView& trials) { if (formats.empty()) { return {}; @@ -127,7 +130,7 @@ bool has_flexfec = false; bool has_rtx = false; - for (const auto& format : formats) { + for (const SdpVideoFormat& format : formats) { if (absl::EqualsIgnoreCase(format.name, kRedCodecName)) { has_red = true; } else if (absl::EqualsIgnoreCase(format.name, kUlpfecCodecName)) { @@ -140,7 +143,7 @@ } std::vector<CodecConfiguration> out; - for (const auto& format : formats) { + for (const SdpVideoFormat& format : formats) { Codec codec = CreateVideoCodec(format); if (codec.IsResiliencyCodec()) { continue; @@ -150,8 +153,7 @@ CodecConfiguration config; config.codec = codec; - config.codec.id = PayloadType::NotSet(); - if (rtx_enabled && has_rtx) { + if (rtx_enabled && (has_rtx || add_auxiliary_codecs)) { Codec::ResiliencyType resiliency_type = codec.GetResiliencyType(); if (resiliency_type != Codec::ResiliencyType::kFlexfec && resiliency_type != Codec::ResiliencyType::kUlpfec) { @@ -160,7 +162,8 @@ } config.resiliency.red = has_red; config.resiliency.ulpfec = has_ulpfec; - if (trials.IsEnabled("WebRTC-FlexFEC-03-Advertised")) { + if (trials.IsEnabled("WebRTC-FlexFEC-03-Advertised") || + trials.IsEnabled("WebRTC-FlexFEC-03")) { config.resiliency.flexfec = has_flexfec; } out.push_back(config); @@ -173,8 +176,9 @@ bool is_sender, bool rtx_enabled, const FieldTrialsView& trials) { - return CollectVideoCodecConfigurations(video.GetSupportedFormats(!is_sender), - rtx_enabled, trials); + return CollectVideoCodecConfigurations( + video.GetSupportedFormats(!is_sender), rtx_enabled, + video.NeedsAuxiliaryCodecsAdded(), trials); } Codecs CodecsFromConfigurations(