From 84ce5453adee2d62213cee4722d10bf5565f587c Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Mon, 26 Aug 2024 16:02:11 +0000 Subject: [PATCH] Reland "Add PT lookup function to JsepTransportController" This reverts commit 0e3a3266afc50218747134bec7c40f1c6e82ab19. Reason for revert: Ancestor CL fixed Original change's description: > Revert "Add PT lookup function to JsepTransportController" > > This reverts commit d178532ff9416f8b4272b9b8622afa9bab2ed558. > > Reason for revert: break pw-answer > > Original change's description: > > Add PT lookup function to JsepTransportController > > > > Bug: webrtc:360058654 > > Change-Id: I9db58bf872f8659622e9f626fc21ce84993cfdfb > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/360143 > > Commit-Queue: Harald Alvestrand > > Reviewed-by: Florent Castelli > > Cr-Commit-Position: refs/heads/main@{#42829} > > Bug: webrtc:360058654 > Change-Id: Ic082dd3e86ed11d05b65710463fa9e57715bf07a > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/360360 > Bot-Commit: rubber-stamper@appspot.gserviceaccount.com > Commit-Queue: Jonas Oreland > Reviewed-by: Harald Alvestrand > Cr-Commit-Position: refs/heads/main@{#42832} Bug: webrtc:360058654 Change-Id: Ice9c118f9a5d4e0fa2cff89f504a25b80ec625ef Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/360662 Reviewed-by: Florent Castelli Commit-Queue: Harald Alvestrand Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Cr-Commit-Position: refs/heads/main@{#42853} --- pc/jsep_transport.h | 12 ++++++++++++ pc/jsep_transport_collection.cc | 3 +++ pc/jsep_transport_controller.cc | 23 +++++++++++++++++++++++ pc/jsep_transport_controller.h | 7 +++++++ pc/payload_type_picker.cc | 10 +++++----- pc/payload_type_picker.h | 12 ++++++------ pc/payload_type_picker_unittest.cc | 4 ++-- 7 files changed, 58 insertions(+), 13 deletions(-) diff --git a/pc/jsep_transport.h b/pc/jsep_transport.h index b2c7a751aa..3c14f13159 100644 --- a/pc/jsep_transport.h +++ b/pc/jsep_transport.h @@ -242,6 +242,18 @@ class JsepTransport { webrtc::SdpType type, const ContentInfo& content); + const webrtc::PayloadTypeRecorder& remote_payload_types() const { + return remote_payload_types_; + } + const webrtc::PayloadTypeRecorder& local_payload_types() const { + return local_payload_types_; + } + void CommitPayloadTypes() { + RTC_DCHECK_RUN_ON(network_thread_); + local_payload_types_.Commit(); + remote_payload_types_.Commit(); + } + private: bool SetRtcpMux(bool enable, webrtc::SdpType type, ContentSource source); diff --git a/pc/jsep_transport_collection.cc b/pc/jsep_transport_collection.cc index b50d303d77..08ae513698 100644 --- a/pc/jsep_transport_collection.cc +++ b/pc/jsep_transport_collection.cc @@ -306,6 +306,9 @@ void JsepTransportCollection::CommitTransports() { RTC_DCHECK_RUN_ON(&sequence_checker_); stable_mid_to_transport_ = mid_to_transport_; DestroyUnusedTransports(); + for (auto& transport : jsep_transports_by_name_) { + transport.second->CommitPayloadTypes(); + } RTC_DCHECK(IsConsistent()); } diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index 5d6ac95879..7ad8e2d018 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -220,6 +220,29 @@ absl::optional JsepTransportController::GetDtlsRole( return t->GetDtlsRole(); } +RTCErrorOr JsepTransportController::SuggestPayloadType( + const std::string& mid, + cricket::Codec codec) { + RTC_DCHECK_RUN_ON(network_thread_); + const cricket::JsepTransport* transport = GetJsepTransportForMid(mid); + if (transport) { + auto local_result = + transport->local_payload_types().LookupPayloadType(codec); + if (local_result.ok()) { + return local_result; + } + auto remote_result = + transport->remote_payload_types().LookupPayloadType(codec); + if (remote_result.ok()) { + return remote_result; + } + return payload_type_picker_.SuggestMapping( + codec, &transport->local_payload_types()); + } + // If there is no transport, there are no exclusions. + return payload_type_picker_.SuggestMapping(codec, nullptr); +} + bool JsepTransportController::SetLocalCertificate( const rtc::scoped_refptr& certificate) { if (!network_thread_->IsCurrent()) { diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h index 749faddbe2..c4275bde5f 100644 --- a/pc/jsep_transport_controller.h +++ b/pc/jsep_transport_controller.h @@ -235,6 +235,13 @@ class JsepTransportController : public sigslot::has_slots<> { // Get negotiated role, if one has been negotiated. absl::optional GetDtlsRole(const std::string& mid) const; + // Suggest a payload type for a given codec on a given media section. + // Media section is indicated by MID. + // The function will either return a PT already in use on the connection + // or a newly suggested one. + RTCErrorOr SuggestPayloadType(const std::string& mid, + cricket::Codec codec); + // TODO(deadbeef): GetStats isn't const because all the way down to // OpenSSLStreamAdapter, GetSslCipherSuite and GetDtlsSrtpCryptoSuite are not // const. Fix this. diff --git a/pc/payload_type_picker.cc b/pc/payload_type_picker.cc index a06e42ce98..8720b204a8 100644 --- a/pc/payload_type_picker.cc +++ b/pc/payload_type_picker.cc @@ -130,7 +130,7 @@ PayloadTypePicker::PayloadTypePicker() { RTCErrorOr PayloadTypePicker::SuggestMapping( cricket::Codec codec, - PayloadTypeRecorder* excluder) { + const PayloadTypeRecorder* excluder) { // The first matching entry is returned, unless excluder // maps it to something different. for (auto entry : entries_) { @@ -193,12 +193,12 @@ RTCError PayloadTypeRecorder::AddMapping(PayloadType payload_type, } std::vector> -PayloadTypeRecorder::GetMappings() { +PayloadTypeRecorder::GetMappings() const { return std::vector>{}; } RTCErrorOr PayloadTypeRecorder::LookupPayloadType( - cricket::Codec codec) { + cricket::Codec codec) const { // Note that having multiple PTs mapping to the same codec is NOT an error. // In this case, we return the first found (not deterministic). auto result = std::find_if( @@ -212,7 +212,7 @@ RTCErrorOr PayloadTypeRecorder::LookupPayloadType( } RTCErrorOr PayloadTypeRecorder::LookupCodec( - PayloadType payload_type) { + PayloadType payload_type) const { auto result = payload_type_to_codec_.find(payload_type); if (result == payload_type_to_codec_.end()) { return RTCError(RTCErrorType::INVALID_PARAMETER, "No such payload type"); @@ -220,7 +220,7 @@ RTCErrorOr PayloadTypeRecorder::LookupCodec( return result->second; } -void PayloadTypeRecorder::Checkpoint() { +void PayloadTypeRecorder::Commit() { checkpoint_payload_type_to_codec_ = payload_type_to_codec_; } void PayloadTypeRecorder::Rollback() { diff --git a/pc/payload_type_picker.h b/pc/payload_type_picker.h index f29979759b..4726a31635 100644 --- a/pc/payload_type_picker.h +++ b/pc/payload_type_picker.h @@ -42,7 +42,7 @@ class PayloadTypePicker { // Suggest a payload type for the codec. // If the excluder maps it to something different, don't suggest it. RTCErrorOr SuggestMapping(cricket::Codec codec, - PayloadTypeRecorder* excluder); + const PayloadTypeRecorder* excluder); RTCError AddMapping(PayloadType payload_type, cricket::Codec codec); private: @@ -67,12 +67,12 @@ class PayloadTypeRecorder { : suggester_(suggester) {} RTCError AddMapping(PayloadType payload_type, cricket::Codec codec); - std::vector> GetMappings(); - RTCErrorOr LookupPayloadType(cricket::Codec codec); - RTCErrorOr LookupCodec(PayloadType payload_type); + std::vector> GetMappings() const; + RTCErrorOr LookupPayloadType(cricket::Codec codec) const; + RTCErrorOr LookupCodec(PayloadType payload_type) const; // Transaction support. - // Checkpoint() commits previous changes. - void Checkpoint(); + // Commit() commits previous changes. + void Commit(); // Rollback() rolls back to the previous checkpoint. void Rollback(); diff --git a/pc/payload_type_picker_unittest.cc b/pc/payload_type_picker_unittest.cc index e97a2b0ce8..71a338346f 100644 --- a/pc/payload_type_picker_unittest.cc +++ b/pc/payload_type_picker_unittest.cc @@ -75,7 +75,7 @@ TEST(PayloadTypePicker, RollbackAndCommit) { cricket::Codec b_codec = cricket::CreateVideoCodec(0, "vp9"); auto error = recorder.AddMapping(a_payload_type, a_codec); ASSERT_TRUE(error.ok()); - recorder.Checkpoint(); + recorder.Commit(); ASSERT_TRUE(recorder.AddMapping(b_payload_type, b_codec).ok()); { auto result = recorder.LookupCodec(a_payload_type); @@ -99,7 +99,7 @@ TEST(PayloadTypePicker, RollbackAndCommit) { } ASSERT_TRUE(recorder.AddMapping(b_payload_type, b_codec).ok()); // Rollback after a new checkpoint has no effect. - recorder.Checkpoint(); + recorder.Commit(); recorder.Rollback(); { auto result = recorder.LookupCodec(b_payload_type);