From d153de6d33a79d450c626b9b8e9613df33947282 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Mon, 16 Sep 2024 20:28:14 +0000 Subject: [PATCH] Add payload type assignment to offer/answer generation. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This adds payload types to the codecs at the time when offer is being generated, if they are unassigned at that point. Bug: webrtc:360058654 Change-Id: I231ed057ebaf7fb0fffaf6ff5d600b064ba21f5b Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/362282 Commit-Queue: Harald Alvestrand Reviewed-by: Henrik Boström Cr-Commit-Position: refs/heads/main@{#43033} --- pc/BUILD.gn | 9 ++++++ pc/media_session.cc | 39 ++++++++++++++++++++++-- pc/media_session.h | 12 +++++--- pc/media_session_unittest.cc | 37 ++++++++++++++-------- pc/peer_connection.cc | 5 +-- pc/sdp_offer_answer.cc | 11 ++++--- pc/sdp_offer_answer.h | 7 +++-- pc/webrtc_session_description_factory.cc | 16 ++++++++-- pc/webrtc_session_description_factory.h | 6 ++-- 9 files changed, 111 insertions(+), 31 deletions(-) diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 9a7a783b62..bbd339a753 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -372,6 +372,7 @@ rtc_source_set("media_session") { "../api:rtp_parameters", "../api:rtp_transceiver_direction", "../api/crypto:options", + "../call:payload_type", "../media:codec", "../media:media_constants", "../media:media_engine", @@ -1013,6 +1014,7 @@ rtc_source_set("sdp_offer_answer") { "../api/video:builtin_video_bitrate_allocator_factory", "../api/video:video_bitrate_allocator_factory", "../api/video:video_codec_constants", + "../call:payload_type", "../media:codec", "../media:media_channel", "../media:media_engine", @@ -1346,11 +1348,13 @@ rtc_source_set("webrtc_session_description_factory") { ":media_session", ":sdp_state_provider", ":session_description", + "../api:field_trials_view", "../api:libjingle_peerconnection_api", "../api:rtc_error", "../api:scoped_refptr", "../api:sequence_checker", "../api/task_queue", + "../call:payload_type", "../p2p:rtc_p2p", "../p2p:transport_description", "../p2p:transport_description_factory", @@ -1992,6 +1996,7 @@ if (rtc_include_tests && !build_with_chromium) { ":rtp_transport_internal", ":sctp_transport", ":session_description", + ":simulcast_description", ":srtp_session", ":srtp_transport", ":used_ids", @@ -2008,8 +2013,10 @@ if (rtc_include_tests && !build_with_chromium) { "../api:rtc_error", "../api:rtp_headers", "../api:rtp_parameters", + "../api:rtp_transceiver_direction", "../api:scoped_refptr", "../api:sequence_checker", + "../api/audio_codecs:audio_codecs_api", "../api/environment:environment_factory", "../api/task_queue:pending_task_safety_flag", "../api/task_queue:task_queue", @@ -2027,6 +2034,7 @@ if (rtc_include_tests && !build_with_chromium) { "../media:rid_description", "../media:rtc_data_sctp_transport_internal", "../media:rtc_media_tests_utils", + "../media:stream_params", "../modules/rtp_rtcp:rtp_rtcp_format", "../p2p:candidate_pair_interface", "../p2p:dtls_transport_factory", @@ -2039,6 +2047,7 @@ if (rtc_include_tests && !build_with_chromium) { "../p2p:packet_transport_internal", "../p2p:rtc_p2p", "../p2p:transport_description", + "../p2p:transport_description_factory", "../p2p:transport_info", "../rtc_base:async_packet_socket", "../rtc_base:buffer", diff --git a/pc/media_session.cc b/pc/media_session.cc index 9287155abd..50c4260548 100644 --- a/pc/media_session.cc +++ b/pc/media_session.cc @@ -29,6 +29,7 @@ #include "api/rtc_error.h" #include "api/rtp_parameters.h" #include "api/rtp_transceiver_direction.h" +#include "call/payload_type.h" #include "media/base/codec.h" #include "media/base/media_constants.h" #include "media/base/media_engine.h" @@ -1353,9 +1354,11 @@ MediaSessionDescriptionFactory::MediaSessionDescriptionFactory( cricket::MediaEngineInterface* media_engine, bool rtx_enabled, rtc::UniqueRandomIdGenerator* ssrc_generator, - const TransportDescriptionFactory* transport_desc_factory) + const TransportDescriptionFactory* transport_desc_factory, + webrtc::PayloadTypeSuggester* pt_suggester) : ssrc_generator_(ssrc_generator), - transport_desc_factory_(transport_desc_factory) { + transport_desc_factory_(transport_desc_factory), + pt_suggester_(pt_suggester) { RTC_CHECK(transport_desc_factory_); if (media_engine) { audio_send_codecs_ = media_engine->voice().send_codecs(); @@ -2049,7 +2052,7 @@ RTCError MediaSessionDescriptionFactory::AddRtpContentForOffer( std::vector codecs_to_include; if (media_description_options.codecs_to_include.empty()) { - const std::vector& supported_codecs = + std::vector supported_codecs = media_description_options.type == MEDIA_TYPE_AUDIO ? GetAudioCodecsForOffer(media_description_options.direction) : GetVideoCodecsForOffer(media_description_options.direction); @@ -2064,6 +2067,22 @@ 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) { + 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_); + 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 content_description; if (media_description_options.type == MEDIA_TYPE_AUDIO) { content_description = std::make_unique(); @@ -2251,6 +2270,20 @@ RTCError MediaSessionDescriptionFactory::AddRtpContentForAnswer( media_description_options.codec_preferences.empty()); codecs_to_include = negotiated_codecs; } + for (cricket::Codec& codec : codecs_to_include) { + if (codec.id == Codec::kIdNotSet) { + // 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, diff --git a/pc/media_session.h b/pc/media_session.h index 33c44de71c..3228b4fa9c 100644 --- a/pc/media_session.h +++ b/pc/media_session.h @@ -22,6 +22,7 @@ #include "api/rtc_error.h" #include "api/rtp_parameters.h" #include "api/rtp_transceiver_direction.h" +#include "call/payload_type.h" #include "media/base/codec.h" #include "media/base/rid_description.h" #include "media/base/stream_params.h" @@ -141,13 +142,14 @@ class MediaSessionDescriptionFactory { public: // This constructor automatically sets up the factory to get its configuration // from the specified MediaEngine (when provided). - // The TransportDescriptionFactory and the UniqueRandomIdGenerator are not - // owned by MediaSessionDescriptionFactory, so they must be kept alive by the - // user of this class. + // The TransportDescriptionFactory, the UniqueRandomIdGenerator, and the + // PayloadTypeSuggester are not owned by MediaSessionDescriptionFactory, so + // they must be kept alive by the user of this class. MediaSessionDescriptionFactory(cricket::MediaEngineInterface* media_engine, bool rtx_enabled, rtc::UniqueRandomIdGenerator* ssrc_generator, - const TransportDescriptionFactory* factory); + const TransportDescriptionFactory* factory, + webrtc::PayloadTypeSuggester* pt_suggester); const Codecs& audio_sendrecv_codecs() const; const Codecs& audio_send_codecs() const; @@ -318,6 +320,8 @@ class MediaSessionDescriptionFactory { ssrc_generator_; bool enable_encrypted_rtp_header_extensions_ = false; const TransportDescriptionFactory* transport_desc_factory_; + // Payoad type tracker interface. Must live longer than this object. + webrtc::PayloadTypeSuggester* pt_suggester_; }; // Convenience functions. diff --git a/pc/media_session_unittest.cc b/pc/media_session_unittest.cc index 4ac9a5eb8d..ce81d6ccd8 100644 --- a/pc/media_session_unittest.cc +++ b/pc/media_session_unittest.cc @@ -12,7 +12,6 @@ #include -#include #include #include #include @@ -24,24 +23,31 @@ #include "absl/algorithm/container.h" #include "absl/strings/match.h" #include "absl/strings/string_view.h" +#include "api/audio_codecs/audio_format.h" #include "api/candidate.h" +#include "api/media_types.h" #include "api/rtp_parameters.h" +#include "api/rtp_transceiver_direction.h" #include "media/base/codec.h" #include "media/base/media_constants.h" +#include "media/base/rid_description.h" +#include "media/base/stream_params.h" #include "media/base/test_utils.h" #include "media/sctp/sctp_transport_internal.h" #include "p2p/base/p2p_constants.h" #include "p2p/base/transport_description.h" +#include "p2p/base/transport_description_factory.h" #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 "pc/session_description.h" +#include "pc/simulcast_description.h" #include "rtc_base/checks.h" #include "rtc_base/fake_ssl_identity.h" +#include "rtc_base/logging.h" #include "rtc_base/rtc_certificate.h" #include "rtc_base/ssl_identity.h" -#include "rtc_base/ssl_stream_adapter.h" #include "rtc_base/string_encode.h" #include "rtc_base/strings/string_builder.h" #include "rtc_base/unique_id_generator.h" @@ -382,8 +388,8 @@ class MediaSessionDescriptionFactoryTest : public testing::Test { MediaSessionDescriptionFactoryTest() : tdf1_(field_trials), tdf2_(field_trials), - f1_(nullptr, false, &ssrc_generator1, &tdf1_), - f2_(nullptr, false, &ssrc_generator2, &tdf2_) { + f1_(nullptr, false, &ssrc_generator1, &tdf1_, nullptr), + f2_(nullptr, false, &ssrc_generator2, &tdf2_, nullptr) { f1_.set_audio_codecs(MAKE_VECTOR(kAudioCodecs1), MAKE_VECTOR(kAudioCodecs1)); f1_.set_video_codecs(MAKE_VECTOR(kVideoCodecs1), @@ -742,12 +748,14 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCreateOfferWithCustomCodecs) { webrtc::SdpAudioFormat audio_format("custom-audio", 8000, 2); Codec custom_audio_codec = CreateAudioCodec(audio_format); + custom_audio_codec.id = 123; // picked at random, but valid auto audio_options = MediaDescriptionOptions( MEDIA_TYPE_AUDIO, "0", RtpTransceiverDirection::kSendRecv, kActive); audio_options.codecs_to_include.push_back(custom_audio_codec); opts.media_description_options.push_back(audio_options); Codec custom_video_codec = CreateVideoCodec("custom-video"); + custom_video_codec.id = 124; // picked at random, but valid auto video_options = MediaDescriptionOptions( MEDIA_TYPE_VIDEO, "1", RtpTransceiverDirection::kSendRecv, kActive); video_options.codecs_to_include.push_back(custom_video_codec); @@ -769,6 +777,7 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCreateOfferWithCustomCodecs) { // Fields in codec are set during the gen process, so simple compare // does not work. EXPECT_EQ(acd->codecs()[0].name, custom_audio_codec.name); + RTC_LOG(LS_ERROR) << "DEBUG: audio PT assigned is " << acd->codecs()[0].id; EXPECT_EQ(MEDIA_TYPE_VIDEO, vcd->type()); ASSERT_EQ(vcd->codecs().size(), 1U); @@ -786,12 +795,14 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCreateAnswerWithCustomCodecs) { // on the caller, not on this function. webrtc::SdpAudioFormat audio_format("custom-audio", 8000, 2); Codec custom_audio_codec = CreateAudioCodec(audio_format); + custom_audio_codec.id = 123; // picked at random, but valid auto audio_options = MediaDescriptionOptions( MEDIA_TYPE_AUDIO, "audio", RtpTransceiverDirection::kSendRecv, kActive); audio_options.codecs_to_include.push_back(custom_audio_codec); answer_opts.media_description_options.push_back(audio_options); Codec custom_video_codec = CreateVideoCodec("custom-video"); + custom_video_codec.id = 124; auto video_options = MediaDescriptionOptions( MEDIA_TYPE_VIDEO, "video", RtpTransceiverDirection::kSendRecv, kActive); video_options.codecs_to_include.push_back(custom_video_codec); @@ -4395,8 +4406,8 @@ class MediaProtocolTest : public testing::TestWithParam { MediaProtocolTest() : tdf1_(field_trials_), tdf2_(field_trials_), - f1_(nullptr, false, &ssrc_generator1, &tdf1_), - f2_(nullptr, false, &ssrc_generator2, &tdf2_) { + f1_(nullptr, false, &ssrc_generator1, &tdf1_, nullptr), + f2_(nullptr, false, &ssrc_generator2, &tdf2_, nullptr) { f1_.set_audio_codecs(MAKE_VECTOR(kAudioCodecs1), MAKE_VECTOR(kAudioCodecs1)); f1_.set_video_codecs(MAKE_VECTOR(kVideoCodecs1), @@ -4459,7 +4470,8 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestSetAudioCodecs) { std::unique_ptr(new rtc::FakeSSLIdentity("id")))); UniqueRandomIdGenerator ssrc_generator; - MediaSessionDescriptionFactory sf(nullptr, false, &ssrc_generator, &tdf); + MediaSessionDescriptionFactory sf(nullptr, false, &ssrc_generator, &tdf, + nullptr); std::vector send_codecs = MAKE_VECTOR(kAudioCodecs1); std::vector recv_codecs = MAKE_VECTOR(kAudioCodecs2); @@ -4530,7 +4542,8 @@ void TestAudioCodecsOffer(RtpTransceiverDirection direction) { std::unique_ptr(new rtc::FakeSSLIdentity("id")))); UniqueRandomIdGenerator ssrc_generator; - MediaSessionDescriptionFactory sf(nullptr, false, &ssrc_generator, &tdf); + MediaSessionDescriptionFactory sf(nullptr, false, &ssrc_generator, &tdf, + nullptr); const std::vector send_codecs = MAKE_VECTOR(kAudioCodecs1); const std::vector recv_codecs = MAKE_VECTOR(kAudioCodecs2); const std::vector sendrecv_codecs = MAKE_VECTOR(kAudioCodecsAnswer); @@ -4634,9 +4647,9 @@ void TestAudioCodecsAnswer(RtpTransceiverDirection offer_direction, new rtc::FakeSSLIdentity("answer_id")))); UniqueRandomIdGenerator ssrc_generator1, ssrc_generator2; MediaSessionDescriptionFactory offer_factory(nullptr, false, &ssrc_generator1, - &offer_tdf); - MediaSessionDescriptionFactory answer_factory(nullptr, false, - &ssrc_generator2, &answer_tdf); + &offer_tdf, nullptr); + MediaSessionDescriptionFactory answer_factory( + nullptr, false, &ssrc_generator2, &answer_tdf, nullptr); offer_factory.set_audio_codecs( VectorFromIndices(kOfferAnswerCodecs, kOfferSendCodecs), diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index cbf5a68afc..c4546a9e94 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -764,8 +764,9 @@ RTCError PeerConnection::Initialize( legacy_stats_ = std::make_unique(this); stats_collector_ = RTCStatsCollector::Create(this); - sdp_handler_ = SdpOfferAnswerHandler::Create(this, configuration, - dependencies, context_.get()); + sdp_handler_ = + SdpOfferAnswerHandler::Create(this, configuration, dependencies, + context_.get(), transport_controller_copy_); rtp_manager_ = std::make_unique( env_, IsUnifiedPlan(), context_.get(), &usage_pattern_, observer_, diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 5a508673d1..9d49296b67 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -50,6 +50,7 @@ #include "api/uma_metrics.h" #include "api/video/builtin_video_bitrate_allocator_factory.h" #include "api/video/video_codec_constants.h" +#include "call/payload_type.h" #include "media/base/codec.h" #include "media/base/media_engine.h" #include "media/base/rid_description.h" @@ -1379,16 +1380,18 @@ std::unique_ptr SdpOfferAnswerHandler::Create( PeerConnectionSdpMethods* pc, const PeerConnectionInterface::RTCConfiguration& configuration, PeerConnectionDependencies& dependencies, - ConnectionContext* context) { + ConnectionContext* context, + PayloadTypeSuggester* pt_suggester) { auto handler = absl::WrapUnique(new SdpOfferAnswerHandler(pc, context)); - handler->Initialize(configuration, dependencies, context); + handler->Initialize(configuration, dependencies, context, pt_suggester); return handler; } void SdpOfferAnswerHandler::Initialize( const PeerConnectionInterface::RTCConfiguration& configuration, PeerConnectionDependencies& dependencies, - ConnectionContext* context) { + ConnectionContext* context, + PayloadTypeSuggester* pt_suggester) { RTC_DCHECK_RUN_ON(signaling_thread()); // 100 kbps is used by default, but can be overriden by a non-standard // RTCConfiguration value (not available on Web). @@ -1421,7 +1424,7 @@ void SdpOfferAnswerHandler::Initialize( RTC_DCHECK_RUN_ON(signaling_thread()); transport_controller_s()->SetLocalCertificate(certificate); }, - pc_->trials()); + pt_suggester, pc_->trials()); if (pc_->options()->disable_encryption) { RTC_LOG(LS_INFO) diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h index 13492b66e8..793f2c9770 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h @@ -38,6 +38,7 @@ #include "api/set_remote_description_observer_interface.h" #include "api/uma_metrics.h" #include "api/video/video_bitrate_allocator_factory.h" +#include "call/payload_type.h" #include "media/base/media_channel.h" #include "media/base/stream_params.h" #include "p2p/base/port_allocator.h" @@ -80,7 +81,8 @@ class SdpOfferAnswerHandler : public SdpStateProvider { PeerConnectionSdpMethods* pc, const PeerConnectionInterface::RTCConfiguration& configuration, PeerConnectionDependencies& dependencies, - ConnectionContext* context); + ConnectionContext* context, + PayloadTypeSuggester* pt_suggester); void ResetSessionDescFactory() { RTC_DCHECK_RUN_ON(signaling_thread()); @@ -210,7 +212,8 @@ class SdpOfferAnswerHandler : public SdpStateProvider { void Initialize( const PeerConnectionInterface::RTCConfiguration& configuration, PeerConnectionDependencies& dependencies, - ConnectionContext* context); + ConnectionContext* context, + PayloadTypeSuggester* pt_suggester); rtc::Thread* signaling_thread() const; rtc::Thread* network_thread() const; diff --git a/pc/webrtc_session_description_factory.cc b/pc/webrtc_session_description_factory.cc index 236ec742ca..8b321913f1 100644 --- a/pc/webrtc_session_description_factory.cc +++ b/pc/webrtc_session_description_factory.cc @@ -12,23 +12,33 @@ #include +#include +#include +#include #include #include #include -#include #include #include #include "absl/algorithm/container.h" +#include "absl/functional/any_invocable.h" +#include "api/field_trials_view.h" #include "api/jsep.h" #include "api/jsep_session_description.h" +#include "api/peer_connection_interface.h" #include "api/rtc_error.h" +#include "api/scoped_refptr.h" #include "api/sequence_checker.h" +#include "call/payload_type.h" #include "pc/connection_context.h" +#include "pc/media_session.h" #include "pc/sdp_state_provider.h" #include "pc/session_description.h" #include "rtc_base/checks.h" #include "rtc_base/logging.h" +#include "rtc_base/rtc_certificate.h" +#include "rtc_base/rtc_certificate_generator.h" #include "rtc_base/ssl_identity.h" #include "rtc_base/ssl_stream_adapter.h" #include "rtc_base/string_encode.h" @@ -108,13 +118,15 @@ WebRtcSessionDescriptionFactory::WebRtcSessionDescriptionFactory( rtc::scoped_refptr certificate, std::function&)> on_certificate_ready, + PayloadTypeSuggester* pt_suggester, const FieldTrialsView& field_trials) : signaling_thread_(context->signaling_thread()), transport_desc_factory_(field_trials), session_desc_factory_(context->media_engine(), context->use_rtx(), context->ssrc_generator(), - &transport_desc_factory_), + &transport_desc_factory_, + pt_suggester), // RFC 4566 suggested a Network Time Protocol (NTP) format timestamp // as the session id and session version. To simplify, it should be fine // to just use a random number as session id and start version from diff --git a/pc/webrtc_session_description_factory.h b/pc/webrtc_session_description_factory.h index eed922eda7..6c468acbf2 100644 --- a/pc/webrtc_session_description_factory.h +++ b/pc/webrtc_session_description_factory.h @@ -19,17 +19,18 @@ #include #include "absl/functional/any_invocable.h" +#include "api/field_trials_view.h" #include "api/jsep.h" #include "api/peer_connection_interface.h" +#include "api/rtc_error.h" #include "api/scoped_refptr.h" #include "api/task_queue/task_queue_base.h" -#include "p2p/base/transport_description.h" +#include "call/payload_type.h" #include "p2p/base/transport_description_factory.h" #include "pc/media_session.h" #include "pc/sdp_state_provider.h" #include "rtc_base/rtc_certificate.h" #include "rtc_base/rtc_certificate_generator.h" -#include "rtc_base/unique_id_generator.h" #include "rtc_base/weak_ptr.h" namespace webrtc { @@ -53,6 +54,7 @@ class WebRtcSessionDescriptionFactory { rtc::scoped_refptr certificate, std::function&)> on_certificate_ready, + PayloadTypeSuggester* pt_suggester, const FieldTrialsView& field_trials); ~WebRtcSessionDescriptionFactory();