Reland "Add recording of PT->Codec mappings on setting SDP for transport"

This reverts commit 6793f831ffdc598e12aced80a4d97956ca50e436.

Reason for revert: Removed the check that caused the error.

Original change's description:
> Revert "Add recording of PT->Codec mappings on setting SDP for transport"
>
> This reverts commit 15717236c8621cb684bb7753acfedbf34d931c80.
>
> Reason for revert: pr-answer
>
> Original change's description:
> > Add recording of PT->Codec mappings on setting SDP for transport
> >
> > Bug: webrtc:360058654
> > Change-Id: I2aa5e0058346cd3fcda47a8ea5115848fbc4f3e2
> > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/360041
> > Commit-Queue: Harald Alvestrand <hta@webrtc.org>
> > Reviewed-by: Florent Castelli <orphis@webrtc.org>
> > Cr-Commit-Position: refs/heads/main@{#42819}
>
> Bug: webrtc:360058654
> Change-Id: I1fea51b3a0cecfa7e7de75f94f47a85fa064be59
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/360380
> Reviewed-by: Jonas Oreland <jonaso@google.com>
> Commit-Queue: Jonas Oreland <jonaso@google.com>
> Reviewed-by: Per Kjellander <perkj@webrtc.org>
> Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
> Reviewed-by: Henrik Boström <hbos@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#42835}

Bug: webrtc:360058654
Change-Id: I2b60ccd60df3bacbeecd848c3cb86f6725b1505a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/360400
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#42847}

pc/BUILD.gn

@@ -206,6 +206,7 @@ rtc_source_set("jsep_transport") {
   deps = [
     ":dtls_srtp_transport",
     ":dtls_transport",
+    ":payload_type_picker",
     ":rtcp_mux_filter",
     ":rtp_transport",
     ":rtp_transport_internal",
@@ -413,6 +414,7 @@ rtc_library("payload_type_picker") {
     "../api:rtc_error",
     "../media:codec",
     "../rtc_base:strong_alias",
+    "//third_party/abseil-cpp/absl/strings",
   ]
 }
 

pc/jsep_transport.cc

@@ -77,7 +77,8 @@ JsepTransport::JsepTransport(
     std::unique_ptr<DtlsTransportInternal> rtp_dtls_transport,
     std::unique_ptr<DtlsTransportInternal> rtcp_dtls_transport,
     std::unique_ptr<SctpTransportInternal> sctp_transport,
-    std::function<void()> rtcp_mux_active_callback)
+    std::function<void()> rtcp_mux_active_callback,
+    webrtc::PayloadTypePicker& suggester)
     : network_thread_(rtc::Thread::Current()),
       mid_(mid),
       local_certificate_(local_certificate),
@@ -99,7 +100,9 @@ JsepTransport::JsepTransport(
                                 std::move(sctp_transport),
                                 rtp_dtls_transport_)
                           : nullptr),
-      rtcp_mux_active_callback_(std::move(rtcp_mux_active_callback)) {
+      rtcp_mux_active_callback_(std::move(rtcp_mux_active_callback)),
+      remote_payload_types_(suggester),
+      local_payload_types_(suggester) {
   TRACE_EVENT0("webrtc", "JsepTransport::JsepTransport");
   RTC_DCHECK(ice_transport_);
   RTC_DCHECK(rtp_dtls_transport_);
@@ -370,6 +373,24 @@ void JsepTransport::SetActiveResetSrtpParams(bool active_reset_srtp_params) {
   }
 }
 
+webrtc::RTCError JsepTransport::RecordPayloadTypes(bool local,
+                                                   webrtc::SdpType type,
+                                                   const ContentInfo& content) {
+  RTC_DCHECK_RUN_ON(network_thread_);
+  for (auto codec : content.media_description()->codecs()) {
+    webrtc::RTCError result;
+    if (local) {
+      result = local_payload_types_.AddMapping(codec.id, codec);
+    } else {
+      result = remote_payload_types_.AddMapping(codec.id, codec);
+    }
+    if (!result.ok()) {
+      return result;
+    }
+  }
+  return webrtc::RTCError::OK();
+}
+
 void JsepTransport::SetRemoteIceParameters(
     const IceParameters& ice_parameters,
     IceTransportInternal* ice_transport) {

pc/jsep_transport.h

@@ -34,6 +34,7 @@
 #include "p2p/base/transport_info.h"
 #include "pc/dtls_srtp_transport.h"
 #include "pc/dtls_transport.h"
+#include "pc/payload_type_picker.h"
 #include "pc/rtcp_mux_filter.h"
 #include "pc/rtp_transport.h"
 #include "pc/rtp_transport_internal.h"
@@ -97,7 +98,8 @@ class JsepTransport {
       std::unique_ptr<DtlsTransportInternal> rtp_dtls_transport,
       std::unique_ptr<DtlsTransportInternal> rtcp_dtls_transport,
       std::unique_ptr<SctpTransportInternal> sctp_transport,
-      std::function<void()> rtcp_mux_active_callback);
+      std::function<void()> rtcp_mux_active_callback,
+      webrtc::PayloadTypePicker& suggester);
 
   ~JsepTransport();
 
@@ -234,6 +236,12 @@ class JsepTransport {
 
   void SetActiveResetSrtpParams(bool active_reset_srtp_params);
 
+  // Record the PT mappings from a single media section.
+  // This is used to store info needed when generating subsequent SDP.
+  webrtc::RTCError RecordPayloadTypes(bool local,
+                                      webrtc::SdpType type,
+                                      const ContentInfo& content);
+
  private:
   bool SetRtcpMux(bool enable, webrtc::SdpType type, ContentSource source);
 
@@ -313,6 +321,13 @@ class JsepTransport {
   // `rtcp_dtls_transport_` is destroyed. The JsepTransportController will
   // receive the callback and update the aggregate transport states.
   std::function<void()> rtcp_mux_active_callback_;
+
+  // Assigned PTs from the remote description, used when sending.
+  webrtc::PayloadTypeRecorder remote_payload_types_
+      RTC_GUARDED_BY(network_thread_);
+  // Assigned PTs from the local description, used when receiving.
+  webrtc::PayloadTypeRecorder local_payload_types_
+      RTC_GUARDED_BY(network_thread_);
 };
 
 }  // namespace cricket

pc/jsep_transport_controller.cc

@@ -43,6 +43,7 @@ JsepTransportController::JsepTransportController(
     rtc::Thread* network_thread,
     cricket::PortAllocator* port_allocator,
     AsyncDnsResolverFactoryInterface* async_dns_resolver_factory,
+    PayloadTypePicker& payload_type_picker,
     Config config)
     : env_(env),
       network_thread_(network_thread),
@@ -58,7 +59,8 @@ JsepTransportController::JsepTransportController(
           }),
       config_(std::move(config)),
       active_reset_srtp_params_(config.active_reset_srtp_params),
-      bundles_(config.bundle_policy) {
+      bundles_(config.bundle_policy),
+      payload_type_picker_(payload_type_picker) {
   // The `transport_observer` is assumed to be non-null.
   RTC_DCHECK(config_.transport_observer);
   RTC_DCHECK(config_.rtcp_handler);
@@ -681,6 +683,12 @@ RTCError JsepTransportController::ApplyDescription_n(
           "Failed to apply the description for m= section with mid='" +
               content_info.name + "': " + error.message());
     }
+    error = transport->RecordPayloadTypes(local, type, content_info);
+    if (!error.ok()) {
+      RTC_LOG(LS_ERROR) << "RecordPayloadTypes failed: "
+                        << ToString(error.type()) << " - " << error.message();
+      return error;
+    }
   }
   if (type == SdpType::kAnswer) {
     transports_.CommitTransports();
@@ -1099,10 +1107,12 @@ RTCError JsepTransportController::MaybeCreateJsepTransport(
           content_info.name, certificate_, std::move(ice), std::move(rtcp_ice),
           std::move(unencrypted_rtp_transport), std::move(sdes_transport),
           std::move(dtls_srtp_transport), std::move(rtp_dtls_transport),
-          std::move(rtcp_dtls_transport), std::move(sctp_transport), [&]() {
+          std::move(rtcp_dtls_transport), std::move(sctp_transport),
+          [&]() {
             RTC_DCHECK_RUN_ON(network_thread_);
             UpdateAggregateStates_n();
-          });
+          },
+          payload_type_picker_);
 
   jsep_transport->rtp_transport()->SubscribeRtcpPacketReceived(
       this, [this](rtc::CopyOnWriteBuffer* buffer, int64_t packet_time_ms) {

pc/jsep_transport_controller.h

@@ -150,6 +150,7 @@ class JsepTransportController : public sigslot::has_slots<> {
       rtc::Thread* network_thread,
       cricket::PortAllocator* port_allocator,
       AsyncDnsResolverFactoryInterface* async_dns_resolver_factory,
+      PayloadTypePicker& payload_type_picker,
       Config config);
   virtual ~JsepTransportController();
 
@@ -510,6 +511,8 @@ class JsepTransportController : public sigslot::has_slots<> {
   rtc::scoped_refptr<rtc::RTCCertificate> certificate_;
 
   BundleManager bundles_;
+  // Reference to the SdpOfferAnswerHandler's payload type picker.
+  PayloadTypePicker& payload_type_picker_;
 };
 
 }  // namespace webrtc

pc/jsep_transport_controller_unittest.cc

@@ -105,7 +105,8 @@ class JsepTransportControllerTest : public JsepTransportController::Observer,
     config.on_dtls_handshake_error_ = [](rtc::SSLHandshakeError s) {};
     transport_controller_ = std::make_unique<JsepTransportController>(
         env_, network_thread, port_allocator,
-        nullptr /* async_resolver_factory */, std::move(config));
+        nullptr /* async_resolver_factory */, payload_type_picker_,
+        std::move(config));
     SendTask(network_thread, [&] { ConnectTransportControllerSignals(); });
   }
 
@@ -380,7 +381,7 @@ class JsepTransportControllerTest : public JsepTransportController::Observer,
   std::map<std::string, RtpTransportInternal*> changed_rtp_transport_by_mid_;
   std::map<std::string, cricket::DtlsTransportInternal*>
       changed_dtls_transport_by_mid_;
-
+  webrtc::PayloadTypePicker payload_type_picker_;
   // Transport controller needs to be destroyed first, because it may issue
   // callbacks that modify the changed_*_by_mid in the destructor.
   std::unique_ptr<JsepTransportController> transport_controller_;

pc/jsep_transport_unittest.cc

@@ -126,8 +126,8 @@ class JsepTransport2Test : public ::testing::Test, public sigslot::has_slots<> {
     std::unique_ptr<webrtc::RtpTransport> unencrypted_rtp_transport;
     std::unique_ptr<webrtc::SrtpTransport> sdes_transport;
     std::unique_ptr<webrtc::DtlsSrtpTransport> dtls_srtp_transport;
-        dtls_srtp_transport = CreateDtlsSrtpTransport(
+    dtls_srtp_transport = CreateDtlsSrtpTransport(rtp_dtls_transport.get(),
-            rtp_dtls_transport.get(), rtcp_dtls_transport.get());
+                                                  rtcp_dtls_transport.get());
 
     auto jsep_transport = std::make_unique<JsepTransport>(
         kTransportName, /*local_certificate=*/nullptr, std::move(ice),
@@ -135,7 +135,8 @@ class JsepTransport2Test : public ::testing::Test, public sigslot::has_slots<> {
         std::move(sdes_transport), std::move(dtls_srtp_transport),
         std::move(rtp_dtls_transport), std::move(rtcp_dtls_transport),
         /*sctp_transport=*/nullptr,
-        /*rtcp_mux_active_callback=*/[&]() { OnRtcpMuxActive(); });
+        /*rtcp_mux_active_callback=*/[&]() { OnRtcpMuxActive(); },
+        payload_type_picker_);
 
     signal_rtcp_mux_active_received_ = false;
     return jsep_transport;
@@ -179,6 +180,7 @@ class JsepTransport2Test : public ::testing::Test, public sigslot::has_slots<> {
   webrtc::SrtpTransport* sdes_transport_ = nullptr;
 
   webrtc::test::ScopedKeyValueConfig field_trials_;
+  webrtc::PayloadTypePicker payload_type_picker_;
 };
 
 // The parameterized tests cover both cases when RTCP mux is enable and

pc/payload_type_picker.cc

@@ -14,6 +14,7 @@
 #include <utility>
 #include <vector>
 
+#include "absl/strings/match.h"
 #include "api/rtc_error.h"
 #include "media/base/codec.h"
 
@@ -28,8 +29,8 @@ namespace {
 
 bool MatchesForSdp(const cricket::Codec& codec_1,
                    const cricket::Codec& codec_2) {
-  return codec_1.name == codec_2.name && codec_1.type == codec_2.type &&
+  return absl::EqualsIgnoreCase(codec_1.name, codec_2.name) &&
-         codec_1.channels == codec_2.channels &&
+         codec_1.type == codec_2.type && codec_1.channels == codec_2.channels &&
          codec_1.clockrate == codec_2.clockrate &&
          codec_1.params == codec_2.params;
 }
@@ -48,10 +49,24 @@ RTCError PayloadTypePicker::AddMapping(PayloadType payload_type,
 
 RTCError PayloadTypeRecorder::AddMapping(PayloadType payload_type,
                                          cricket::Codec codec) {
-  if (payload_type_to_codec_.find(payload_type) !=
+  auto existing_codec_it = payload_type_to_codec_.find(payload_type);
-      payload_type_to_codec_.end()) {
+  if (existing_codec_it != payload_type_to_codec_.end() &&
-    return RTCError(RTCErrorType::INVALID_PARAMETER,
+      !MatchesForSdp(codec, existing_codec_it->second)) {
-                    "Attempt to insert duplicate mapping for PT");
+    if (absl::EqualsIgnoreCase(codec.name, existing_codec_it->second.name)) {
+      // The difference is in clock rate, channels or FMTP parameters.
+      RTC_LOG(LS_INFO) << "Warning: Attempt to change a codec's parameters";
+      // Some FMTP value changes are harmless, others are harmful.
+      // This is done in production today, so we can't return an error.
+    } else {
+      RTC_LOG(LS_WARNING) << "Warning: You attempted to redefine a codec from "
+                          << existing_codec_it->second.ToString() << " to "
+                          << " new codec " << codec.ToString();
+      // This is a spec violation.
+      // TODO: https://issues.webrtc.org/41480892 - return an error.
+    }
+    // Accept redefinition.
+    payload_type_to_codec_.insert_or_assign(payload_type, codec);
+    return RTCError::OK();
   }
   payload_type_to_codec_.emplace(payload_type, codec);
   suggester_.AddMapping(payload_type, codec);

pc/payload_type_picker.h

@@ -31,6 +31,12 @@ class PayloadType : public StrongAlias<class PayloadTypeTag, uint8_t> {
 
 class PayloadTypePicker {
  public:
+  PayloadTypePicker() = default;
+  PayloadTypePicker(const PayloadTypePicker&) = delete;
+  PayloadTypePicker& operator=(const PayloadTypePicker&) = delete;
+  PayloadTypePicker(PayloadTypePicker&&) = delete;
+  PayloadTypePicker& operator=(PayloadTypePicker&&) = delete;
+
   RTCErrorOr<PayloadType> SuggestMapping(cricket::Codec codec);
   RTCError AddMapping(PayloadType payload_type, cricket::Codec codec);
 };

pc/payload_type_picker_unittest.cc

@@ -46,6 +46,23 @@ TEST(PayloadTypePicker, StoreAndRecall) {
   EXPECT_FALSE(recorder.LookupCodec(not_a_payload_type).ok());
 }
 
+TEST(PayloadTypePicker, ModifyingPtIsIgnored) {
+  // Arguably a spec violation, but happens in production.
+  // To be decided: Whether we should disallow codec change, fmtp change
+  // or both.
+  PayloadTypePicker picker;
+  PayloadTypeRecorder recorder(picker);
+  const PayloadType a_payload_type(123);
+  cricket::Codec a_codec = cricket::CreateVideoCodec(0, "vp8");
+  cricket::Codec b_codec = cricket::CreateVideoCodec(0, "vp9");
+  recorder.AddMapping(a_payload_type, a_codec);
+  auto error = recorder.AddMapping(a_payload_type, b_codec);
+  EXPECT_TRUE(error.ok());
+  auto result = recorder.LookupCodec(a_payload_type);
+  // Redefinition should be accepted.
+  EXPECT_EQ(result.value(), b_codec);
+}
+
 TEST(PayloadTypePicker, RollbackAndCommit) {
   PayloadTypePicker picker;
   PayloadTypeRecorder recorder(picker);

pc/peer_connection.cc

@@ -773,9 +773,10 @@ JsepTransportController* PeerConnection::InitializeTransportController_n(
         }
       };
 
-  transport_controller_.reset(new JsepTransportController(
+  transport_controller_.reset(
-      env_, network_thread(), port_allocator_.get(),
+      new JsepTransportController(env_, network_thread(), port_allocator_.get(),
-      async_dns_resolver_factory_.get(), std::move(config)));
+                                  async_dns_resolver_factory_.get(),
+                                  payload_type_picker_, std::move(config)));
 
   transport_controller_->SubscribeIceConnectionState(
       [this](cricket::IceConnectionState s) {

pc/peer_connection.h

@@ -448,6 +448,9 @@ class PeerConnection : public PeerConnectionInternal,
     RTC_DCHECK(call_);
     return call_->GetTransportControllerSend()->GetNetworkController();
   }
+  PayloadTypePicker& payload_type_picker() override {
+    return payload_type_picker_;
+  }
 
  protected:
   // Available for rtc::scoped_refptr creation
@@ -707,6 +710,7 @@ class PeerConnection : public PeerConnectionInternal,
   // Used to gather metrics only the first such state change.
   bool was_ever_connected_ RTC_GUARDED_BY(signaling_thread()) = false;
 
+  PayloadTypePicker payload_type_picker_;
   // This variable needs to be the last one in the class.
   rtc::WeakPtrFactory<PeerConnection> weak_factory_;
 };

pc/peer_connection_interface_unittest.cc

@@ -3083,7 +3083,13 @@ TEST_P(PeerConnectionInterfaceTest, SetConfigurationNotCausingIceRestart) {
 // 4. Next createOffer should initiate an ICE restart, but only for the other
 //    m= section; it would be pointless to do an ICE restart for the m= section
 //    that was already restarted.
-TEST_P(PeerConnectionInterfaceTest, SetConfigurationCausingPartialIceRestart) {
+// Disabled because work on PT assignment showed that the restart tries
+// to remap an RTX payload type.
+// Tracking bug for PT assignment work: https://issues.webrtc.org/360058654
+// The suspected bug is linked below.
+// TODO(https://issues.webrtc.org/42233461): Fix PT assignment
+TEST_P(PeerConnectionInterfaceTest,
+       DISABLED_SetConfigurationCausingPartialIceRestart) {
   PeerConnectionInterface::RTCConfiguration config;
   config.sdp_semantics = sdp_semantics_;
   config.type = PeerConnectionInterface::kRelay;

pc/peer_connection_internal.h

@@ -127,6 +127,9 @@ class PeerConnectionSdpMethods {
   virtual const FieldTrialsView& trials() const = 0;
 
   virtual void ClearStatsCache() = 0;
+  // Keeps track of assigned payload types and comes up with reasonable
+  // suggestions when new PTs need to be assigned.
+  virtual PayloadTypePicker& payload_type_picker() = 0;
 };
 
 // Functions defined in this class are called by other objects,

pc/test/fake_peer_connection_base.h

@@ -373,10 +373,23 @@ class FakePeerConnectionBase : public PeerConnectionInternal {
     return nullptr;
   }
 
+  PayloadTypePicker& payload_type_picker() override {
+    return payload_type_picker_;
+  }
+
+  cricket::CandidateStatsList GetPooledCandidateStats() const override {
+    return {};
+  }
+
  protected:
   test::ScopedKeyValueConfig field_trials_;
+  PayloadTypePicker payload_type_picker_;
 };
 
+static_assert(
+    !std::is_abstract_v<rtc::RefCountedObject<FakePeerConnectionBase>>,
+    "");
+
 }  // namespace webrtc
 
 #endif  // PC_TEST_FAKE_PEER_CONNECTION_BASE_H_

pc/test/fake_peer_connection_for_stats.h

@@ -498,6 +498,7 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase {
       return nullptr;
     }
   }
+  PayloadTypePicker& payload_type_picker() { return payload_type_picker_; }
 
  private:
   cricket::TransportStats GetTransportStatsByName(
@@ -563,6 +564,7 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase {
       local_certificates_by_transport_;
   std::map<std::string, std::unique_ptr<rtc::SSLCertChain>>
       remote_cert_chains_by_transport_;
+  PayloadTypePicker payload_type_picker_;
 };
 
 }  // namespace webrtc

pc/test/mock_peer_connection_internal.h

@@ -334,6 +334,7 @@ class MockPeerConnectionInternal : public PeerConnectionInternal {
               GetNetworkController,
               (),
               (override));
+  MOCK_METHOD(PayloadTypePicker&, payload_type_picker, (), (override));
 };
 
 }  // namespace webrtc

test/peer_scenario/scenario_connection.cc

@@ -64,6 +64,7 @@ class ScenarioIceConnectionImpl : public ScenarioIceConnection,
       RTC_GUARDED_BY(signaling_thread_);
   std::unique_ptr<cricket::BasicPortAllocator> port_allocator_
       RTC_GUARDED_BY(network_thread_);
+  PayloadTypePicker payload_type_picker_;
   std::unique_ptr<JsepTransportController> jsep_controller_;
   RtpTransportInternal* rtp_transport_ RTC_GUARDED_BY(network_thread_) =
       nullptr;
@@ -107,6 +108,7 @@ ScenarioIceConnectionImpl::ScenarioIceConnectionImpl(
                                       network_thread_,
                                       port_allocator_.get(),
                                       /*async_resolver_factory*/ nullptr,
+                                      payload_type_picker_,
                                       CreateJsepConfig())) {
   SendTask(network_thread_, [this] {
     RTC_DCHECK_RUN_ON(network_thread_);