RTP Header Extension: record redefinitions and support error return Add UMA histogram "WebRTC.PeerConnection.RtpHeaderExtensionRedefinition" to track how often RTP header extensions are redefined with a different ID. Add field trial "WebRTC-PayloadTypePicker-ErrorOnRtpExtensionRedefinition" to enable returning RTCError::InvalidParameter when such a redefinition occurs. Propagate FieldTrialsView to RtpHeaderExtensionRecorder to support the field trial check. Bug: webrtc:504685269 Change-Id: Ic7a27465c1e08170908b2b49d98494aa6ad96f12 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/473840 Reviewed-by: Danil Chapovalov <danilchap@webrtc.org> Auto-Submit: Harald Alvestrand <hta@webrtc.org> Commit-Queue: Harald Alvestrand <hta@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47744}
diff --git a/call/BUILD.gn b/call/BUILD.gn index 3a9f3fd..7637e68 100644 --- a/call/BUILD.gn +++ b/call/BUILD.gn
@@ -371,10 +371,12 @@ ] deps = [ ":payload_type", + "../api:field_trials_view", "../api:payload_type", "../api:rtc_error", "../api:rtp_parameters", "../api/audio_codecs:audio_codecs_api", + "../api/environment", "../media:codec", "../media:media_constants", "../rtc_base:checks", @@ -382,6 +384,7 @@ "../rtc_base:stringutils", "../rtc_base/containers:flat_map", "../rtc_base/containers:flat_set", + "../system_wrappers:metrics", "//third_party/abseil-cpp/absl/strings", ] } @@ -686,10 +689,14 @@ ":call_interfaces", ":payload_type", ":payload_type_picker", + "../api:field_trials", "../api:payload_type", + "../api:rtc_error", + "../api/environment", "../api/video_codecs:video_codecs_api", "../media:codec", "../media:media_constants", + "../test:create_test_environment", "../test:test_support", "//third_party/abseil-cpp/absl/strings", ]
diff --git a/call/payload_type_picker.cc b/call/payload_type_picker.cc index c89965e..4930a68 100644 --- a/call/payload_type_picker.cc +++ b/call/payload_type_picker.cc
@@ -31,6 +31,7 @@ #include "rtc_base/containers/flat_set.h" #include "rtc_base/logging.h" #include "rtc_base/string_encode.h" +#include "system_wrappers/include/metrics.h" namespace webrtc { @@ -371,8 +372,14 @@ auto it = uri_to_id_.find(std::pair{uri, encrypt}); if (it != uri_to_id_.end()) { if (it->second != id) { - // TODO: https://issues.webrtc.org/41480892 - This will return an error in - // the future. + RTC_HISTOGRAM_BOOLEAN( + "WebRTC.PeerConnection.RtpHeaderExtensionRedefinition", true); + // TODO: bugs.webrtc.org/504685269 - Enable error return by default. + if (env_.field_trials().IsEnabled( + "WebRTC-ErrorOnRtpExtensionRedefinition")) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "Redefining mapping for RTP header extension"); + } RTC_LOG(LS_ERROR) << "RtpHeaderExtensionRecorder: Redefining mapping for " << uri << " (encrypt=" << encrypt << ") from " << it->second << " to " << id;
diff --git a/call/payload_type_picker.h b/call/payload_type_picker.h index 4c0e12a..f3e5674 100644 --- a/call/payload_type_picker.h +++ b/call/payload_type_picker.h
@@ -17,6 +17,7 @@ #include <vector> #include "absl/strings/string_view.h" +#include "api/environment/environment.h" #include "api/payload_type.h" #include "api/rtc_error.h" #include "api/rtp_parameters.h" @@ -116,7 +117,7 @@ class RtpHeaderExtensionRecorder final { public: - RtpHeaderExtensionRecorder() {} + explicit RtpHeaderExtensionRecorder(const Environment& env) : env_(env) {} ~RtpHeaderExtensionRecorder() {} RTCError AddMapping(int id, absl::string_view uri, bool encrypt); @@ -126,6 +127,7 @@ void Rollback(); private: + const Environment env_; // (uri, encrypt) -> id flat_map<std::pair<std::string, bool>, int> uri_to_id_; flat_map<std::pair<std::string, bool>, int> checkpoint_uri_to_id_;
diff --git a/call/payload_type_picker_unittest.cc b/call/payload_type_picker_unittest.cc index 562bb2c..0b6fae7 100644 --- a/call/payload_type_picker_unittest.cc +++ b/call/payload_type_picker_unittest.cc
@@ -14,10 +14,12 @@ #include "absl/strings/str_cat.h" #include "api/payload_type.h" +#include "api/rtc_error.h" #include "api/video_codecs/sdp_video_format.h" #include "call/payload_type.h" #include "media/base/codec.h" #include "media/base/media_constants.h" +#include "test/create_test_environment.h" #include "test/gmock.h" #include "test/gtest.h" @@ -267,4 +269,30 @@ EXPECT_THAT(s, testing::HasSubstr("\n 100:[-1:video/vp8/90000/0]")); } +TEST(RtpHeaderExtensionRecorder, StoreAndRecall) { + RtpHeaderExtensionRecorder recorder(CreateTestEnvironment()); + RTCError error = recorder.AddMapping(1, "uri", false); + EXPECT_TRUE(error.ok()); + RTCErrorOr<int> result = recorder.LookupId("uri", false); + ASSERT_TRUE(result.ok()); + EXPECT_EQ(result.value(), 1); +} + +TEST(RtpHeaderExtensionRecorder, RedefinitionReturnsOkByDefault) { + RtpHeaderExtensionRecorder recorder(CreateTestEnvironment()); + recorder.AddMapping(1, "uri", false); + RTCError error = recorder.AddMapping(2, "uri", false); + EXPECT_TRUE(error.ok()); + EXPECT_EQ(recorder.LookupId("uri", false).value(), 2); +} + +TEST(RtpHeaderExtensionRecorder, RedefinitionReturnsErrorWithFieldTrial) { + RtpHeaderExtensionRecorder recorder(CreateTestEnvironment( + {.field_trials = "WebRTC-ErrorOnRtpExtensionRedefinition/Enabled/"})); + recorder.AddMapping(1, "uri", false); + RTCError error = recorder.AddMapping(2, "uri", false); + EXPECT_FALSE(error.ok()); + EXPECT_EQ(recorder.LookupId("uri", false).value(), 1); +} + } // namespace webrtc
diff --git a/experiments/field_trials.py b/experiments/field_trials.py index 473ad89..8cf948d 100755 --- a/experiments/field_trials.py +++ b/experiments/field_trials.py
@@ -113,6 +113,9 @@ FieldTrial('WebRTC-EnforceTransceiverDirection', 448408148, date(2026, 6, 1)), + FieldTrial('WebRTC-ErrorOnRtpExtensionRedefinition', + 504685269, + date(2027, 1, 1)), FieldTrial('WebRTC-ForceDtls13', 383141571, date(2024,9,1)),
diff --git a/pc/BUILD.gn b/pc/BUILD.gn index faa8cb5..3ade9d8 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn
@@ -1240,6 +1240,7 @@ "../api:peer_connection_interface", "../api:rtc_error", "../api:rtp_parameters", + "../api/environment", "../call:payload_type", "../call:payload_type_picker", "../media:codec", @@ -3684,6 +3685,7 @@ "../api:rtc_error", "../media:codec", "../media:media_constants", + "../test:create_test_environment", "../test:test_support", "//third_party/abseil-cpp/absl/strings:string_view", ]
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index a2bb704..41550e5f 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc
@@ -1623,7 +1623,7 @@ operations_chain_(OperationsChain::Create()), rtcp_cname_(GenerateRtcpCname()), local_ice_credentials_to_replace_(new LocalIceCredentialsToReplace()), - pt_suggester_(pc_->configuration()->bundle_policy), + pt_suggester_(pc_->configuration()->bundle_policy, env_), weak_ptr_factory_(this) { operations_chain_->SetOnChainEmptyCallback( [this_weak_ptr = weak_ptr_factory_.GetWeakPtr()]() {
diff --git a/pc/sdp_payload_type_suggester.cc b/pc/sdp_payload_type_suggester.cc index 3fac8e1..fd4d273 100644 --- a/pc/sdp_payload_type_suggester.cc +++ b/pc/sdp_payload_type_suggester.cc
@@ -177,7 +177,7 @@ } if (!recorder_by_mid_.contains(transport_mapped_name)) { recorder_by_mid_.emplace(std::make_pair( - transport_mapped_name, BundleTypeRecorder(payload_type_picker_))); + transport_mapped_name, BundleTypeRecorder(payload_type_picker_, env_))); } return recorder_by_mid_.at(transport_mapped_name); }
diff --git a/pc/sdp_payload_type_suggester.h b/pc/sdp_payload_type_suggester.h index d499020..1d416ba 100644 --- a/pc/sdp_payload_type_suggester.h +++ b/pc/sdp_payload_type_suggester.h
@@ -18,6 +18,7 @@ #include <string> #include "absl/strings/string_view.h" +#include "api/environment/environment.h" #include "api/jsep.h" #include "api/payload_type.h" #include "api/peer_connection_interface.h" @@ -35,8 +36,9 @@ class SdpPayloadTypeSuggester : public PayloadTypeSuggester { public: explicit SdpPayloadTypeSuggester( - PeerConnectionInterface::BundlePolicy bundle_policy) - : bundle_manager_(bundle_policy) {} + PeerConnectionInterface::BundlePolicy bundle_policy, + const Environment& env) + : env_(env), bundle_manager_(bundle_policy) {} SdpPayloadTypeSuggester(const SdpPayloadTypeSuggester&) = delete; SdpPayloadTypeSuggester& operator=(const SdpPayloadTypeSuggester&) = delete; SdpPayloadTypeSuggester(SdpPayloadTypeSuggester&&) = delete; @@ -69,8 +71,11 @@ // Records the association of local and remote payload types with a bundle. class BundleTypeRecorder { public: - explicit BundleTypeRecorder(PayloadTypePicker& picker) - : local_payload_types_(picker), remote_payload_types_(picker) {} + explicit BundleTypeRecorder(PayloadTypePicker& picker, + const Environment& env) + : local_payload_types_(picker), + remote_payload_types_(picker), + header_extensions_(env) {} PayloadTypeRecorder& local_payload_types() { return local_payload_types_; } PayloadTypeRecorder& remote_payload_types() { @@ -89,6 +94,7 @@ BundleTypeRecorder& LookupBundleRecorder(absl::string_view mid); PayloadTypePicker payload_type_picker_; RtpHeaderExtensionPicker rtp_header_extension_picker_; + const Environment env_; // Record of bundle groups, used for looking up payload type suggesters. // This class also exists on the network thread, in JsepTransportController. BundleManager bundle_manager_;
diff --git a/pc/sdp_payload_type_suggester_unittest.cc b/pc/sdp_payload_type_suggester_unittest.cc index 1610822..ef3e4c2 100644 --- a/pc/sdp_payload_type_suggester_unittest.cc +++ b/pc/sdp_payload_type_suggester_unittest.cc
@@ -22,6 +22,7 @@ #include "media/base/codec.h" #include "media/base/media_constants.h" #include "pc/session_description.h" +#include "test/create_test_environment.h" #include "test/gtest.h" namespace webrtc { @@ -44,7 +45,7 @@ } protected: - SdpPayloadTypeSuggester suggester_{kBundlePolicy}; + SdpPayloadTypeSuggester suggester_{kBundlePolicy, CreateTestEnvironment()}; }; TEST_F(SdpPayloadTypeSuggesterTest, SuggestPayloadTypeBasic) { @@ -59,7 +60,8 @@ TEST_F(SdpPayloadTypeSuggesterTest, SuggestPayloadTypeReusesRemotePayloadType) { const PayloadType remote_lyra_pt(99); Codec remote_lyra_codec = CreateAudioCodec(remote_lyra_pt, "lyra", 8000, 1); - auto offer = std::make_unique<SessionDescription>(); + std::unique_ptr<SessionDescription> offer = + std::make_unique<SessionDescription>(); AddAudioSection(offer.get()); offer->contents()[0].media_description()->set_codecs({remote_lyra_codec}); EXPECT_TRUE( @@ -77,7 +79,8 @@ // libwebrtc will normally allocate 110 to DTMF/48000 const PayloadType remote_opus_pt(110); Codec remote_opus_codec = CreateAudioCodec(remote_opus_pt, "opus", 48000, 2); - auto offer = std::make_unique<SessionDescription>(); + std::unique_ptr<SessionDescription> offer = + std::make_unique<SessionDescription>(); AddAudioSection(offer.get()); offer->contents()[0].media_description()->set_codecs({remote_opus_codec}); EXPECT_TRUE(