Handle payload type conflict in SuggestPayloadType
The conflict happened when a remote offer allocated a PT
that was prereserved for another purpose, but used it for
a different codec.
Found while writing tests for something different.
Adds unit tests to SuggestPayloadType and an absl_stringify
to the codec picker.
Bug: None
Change-Id: Icc192c61dc4d7a6f5c84f2a54f4fda67fdc1a1d3
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/383681
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#44265}
diff --git a/call/payload_type_picker.h b/call/payload_type_picker.h
index 5a41c8d..8826ffb 100644
--- a/call/payload_type_picker.h
+++ b/call/payload_type_picker.h
@@ -52,6 +52,13 @@
};
std::vector<MapEntry> entries_;
std::set<PayloadType> seen_payload_types_;
+ template <typename Sink>
+ friend void AbslStringify(Sink& sink, const PayloadTypePicker& picker) {
+ sink.Append("Reserved:");
+ for (PayloadType pt : picker.seen_payload_types_) {
+ absl::Format(&sink, " %v", pt);
+ }
+ }
};
class PayloadTypeRecorder {
diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc
index d435396..55590a6 100644
--- a/pc/jsep_transport_controller.cc
+++ b/pc/jsep_transport_controller.cc
@@ -278,7 +278,21 @@
auto remote_result =
transport->remote_payload_types().LookupPayloadType(codec);
if (remote_result.ok()) {
- return remote_result;
+ RTCErrorOr<cricket::Codec> local_result =
+ transport->local_payload_types().LookupCodec(remote_result.value());
+ if (local_result.ok()) {
+ // Already in use, possibly for something else.
+ // Fall through to SuggestMapping.
+ RTC_LOG(LS_WARNING) << "Ignoring remote suggestion of PT "
+ << static_cast<int>(remote_result.value())
+ << " for " << codec << "; already in use";
+ } else {
+ // Tell the local payload type registry that we've taken this
+ RTC_DCHECK(local_result.error().type() ==
+ RTCErrorType::INVALID_PARAMETER);
+ AddLocalMapping(mid, remote_result.value(), codec);
+ return remote_result;
+ }
}
return payload_type_picker_.SuggestMapping(
codec, &transport->local_payload_types());
diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h
index 836cf27..ed2f7af 100644
--- a/pc/jsep_transport_controller.h
+++ b/pc/jsep_transport_controller.h
@@ -27,7 +27,6 @@
#include "api/candidate.h"
#include "api/crypto/crypto_options.h"
#include "api/environment/environment.h"
-#include "api/ice_transport_factory.h"
#include "api/ice_transport_interface.h"
#include "api/jsep.h"
#include "api/peer_connection_interface.h"
@@ -244,6 +243,9 @@
RTCError AddLocalMapping(const std::string& mid,
PayloadType payload_type,
const cricket::Codec& codec) override;
+ const PayloadTypePicker& PayloadTypePickerForTesting() const {
+ return payload_type_picker_;
+ }
bool GetStats(const std::string& mid, cricket::TransportStats* stats) const;
diff --git a/pc/jsep_transport_controller_unittest.cc b/pc/jsep_transport_controller_unittest.cc
index cf96683..4dedeb6 100644
--- a/pc/jsep_transport_controller_unittest.cc
+++ b/pc/jsep_transport_controller_unittest.cc
@@ -18,6 +18,7 @@
#include <utility>
#include <vector>
+#include "api/candidate.h"
#include "api/crypto/crypto_options.h"
#include "api/dtls_transport_interface.h"
#include "api/environment/environment.h"
@@ -26,13 +27,16 @@
#include "api/jsep.h"
#include "api/make_ref_counted.h"
#include "api/peer_connection_interface.h"
+#include "api/rtc_error.h"
#include "api/scoped_refptr.h"
#include "api/test/rtc_error_matchers.h"
#include "api/transport/data_channel_transport_interface.h"
#include "api/transport/enums.h"
#include "api/units/time_delta.h"
+#include "call/payload_type.h"
#include "call/payload_type_picker.h"
-#include "p2p/base/candidate_pair_interface.h"
+#include "media/base/codec.h"
+#include "media/base/media_constants.h"
#include "p2p/base/ice_transport_internal.h"
#include "p2p/base/p2p_constants.h"
#include "p2p/base/port_allocator.h"
@@ -51,6 +55,7 @@
#include "rtc_base/fake_ssl_identity.h"
#include "rtc_base/logging.h"
#include "rtc_base/net_helper.h"
+#include "rtc_base/rtc_certificate.h"
#include "rtc_base/socket_address.h"
#include "rtc_base/ssl_fingerprint.h"
#include "rtc_base/ssl_identity.h"
@@ -2995,4 +3000,68 @@
.ok());
}
+TEST_F(JsepTransportControllerTest, SuggestPayloadTypeBasic) {
+ auto config = JsepTransportController::Config();
+ CreateJsepTransportController(std::move(config));
+ cricket::Codec pcmu_codec =
+ cricket::CreateAudioCodec(-1, cricket::kPcmuCodecName, 8000, 1);
+ RTCErrorOr<PayloadType> pcmu_pt =
+ transport_controller_->SuggestPayloadType("mid", pcmu_codec);
+ ASSERT_TRUE(pcmu_pt.ok());
+ EXPECT_EQ(pcmu_pt.value(), PayloadType(0));
+}
+
+TEST_F(JsepTransportControllerTest, SuggestPayloadTypeReusesRemotePayloadType) {
+ auto config = JsepTransportController::Config();
+ CreateJsepTransportController(std::move(config));
+ const PayloadType remote_lyra_pt(99);
+ cricket::Codec remote_lyra_codec =
+ cricket::CreateAudioCodec(remote_lyra_pt, "lyra", 8000, 1);
+ auto offer = std::make_unique<SessionDescription>();
+ AddAudioSection(offer.get(), kAudioMid1, kIceUfrag1, kIcePwd1,
+ cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS,
+ nullptr);
+ offer->contents()[0].media_description()->set_codecs({remote_lyra_codec});
+ EXPECT_TRUE(transport_controller_
+ ->SetRemoteDescription(SdpType::kOffer, nullptr, offer.get())
+ .ok());
+ cricket::Codec local_lyra_codec =
+ cricket::CreateAudioCodec(-1, "lyra", 8000, 1);
+ RTCErrorOr<PayloadType> lyra_pt =
+ transport_controller_->SuggestPayloadType(kAudioMid1, local_lyra_codec);
+ ASSERT_TRUE(lyra_pt.ok());
+ EXPECT_EQ(lyra_pt.value(), remote_lyra_pt);
+}
+
+TEST_F(JsepTransportControllerTest,
+ SuggestPayloadTypeAvoidsRemoteLocalConflict) {
+ auto config = JsepTransportController::Config();
+ CreateJsepTransportController(std::move(config));
+ // libwebrtc will normally allocate 110 to DTMF/48000
+ const PayloadType remote_opus_pt(110);
+ cricket::Codec remote_opus_codec =
+ cricket::CreateAudioCodec(remote_opus_pt, "opus", 48000, 2);
+ auto offer = std::make_unique<SessionDescription>();
+ AddAudioSection(offer.get(), kAudioMid1, kIceUfrag1, kIcePwd1,
+ cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS,
+ nullptr);
+ offer->contents()[0].media_description()->set_codecs({remote_opus_codec});
+ EXPECT_TRUE(transport_controller_
+ ->SetRemoteDescription(SdpType::kOffer, nullptr, offer.get())
+ .ok());
+ // Check that we get the Opus codec back with the remote PT
+ cricket::Codec local_opus_codec =
+ cricket::CreateAudioCodec(-1, "opus", 48000, 2);
+ RTCErrorOr<PayloadType> local_opus_pt =
+ transport_controller_->SuggestPayloadType(kAudioMid1, local_opus_codec);
+ EXPECT_EQ(local_opus_pt.value(), remote_opus_pt);
+ // Check that we don't get 110 allocated for DTMF, since it's in use for opus
+ cricket::Codec local_other_codec =
+ cricket::CreateAudioCodec(-1, cricket::kDtmfCodecName, 48000, 1);
+ RTCErrorOr<PayloadType> other_pt =
+ transport_controller_->SuggestPayloadType(kAudioMid1, local_other_codec);
+ ASSERT_TRUE(other_pt.ok());
+ EXPECT_NE(other_pt.value(), remote_opus_pt);
+}
+
} // namespace webrtc
