From e2dff95ac632a979a9f7c180fe7763d4b3d556d0 Mon Sep 17 00:00:00 2001 From: Mirko Bonadei Date: Mon, 17 Jul 2023 10:25:44 +0200 Subject: [PATCH] Revert "Clean up WebRTC-FilterAbsSendTimeExtension field trial" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit ebf71114a326080d523b3bc0c2160b2d848d8910. Reason for revert: Breaks downstream project. Original change's description: > Clean up WebRTC-FilterAbsSendTimeExtension field trial > > which has been enabled by default for a while. Also document the > expected behavior, see > https://groups.google.com/g/discuss-webrtc/c/vfrnxWBVcdA/m/ASf7dBJOGAAJ > for more details. > > BUG=webrtc:10234 > > Change-Id: If793e2b4b6cebb07371bfdf1f94ed8d49bf2bb34 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/311281 > Commit-Queue: Philipp Hancke > Reviewed-by: Konrad Hofbauer > Reviewed-by: Erik Språng > Cr-Commit-Position: refs/heads/main@{#40417} BUG=webrtc:10234 Change-Id: I856991260ff40a24f03f6054a5c2a9e6f37f47da Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/311803 Commit-Queue: Mirko Bonadei Owners-Override: Mirko Bonadei Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#40438} --- media/engine/webrtc_media_engine.cc | 22 +++++++---- media/engine/webrtc_media_engine_unittest.cc | 40 ++++++++++++++++++++ media/engine/webrtc_video_engine_unittest.cc | 11 ++++++ 3 files changed, 65 insertions(+), 8 deletions(-) diff --git a/media/engine/webrtc_media_engine.cc b/media/engine/webrtc_media_engine.cc index 05d6e6e0bd..99d7dd2704 100644 --- a/media/engine/webrtc_media_engine.cc +++ b/media/engine/webrtc_media_engine.cc @@ -178,14 +178,20 @@ std::vector FilterRtpExtensions( }); result.erase(it, result.end()); - // Keep just the highest priority BWE related extension and do not send - // toffset if transport-cc or abs-send-time have been negotiated. - // abs-send-time is sent even if transport-cc is negotiated for logging - // purposes. - static const char* const kBweExtensionPriorities[] = { - webrtc::RtpExtension::kAbsSendTimeUri, - webrtc::RtpExtension::kTimestampOffsetUri}; - DiscardRedundantExtensions(&result, kBweExtensionPriorities); + // Keep just the highest priority extension of any in the following lists. + if (absl::StartsWith(trials.Lookup("WebRTC-FilterAbsSendTimeExtension"), + "Enabled")) { + static const char* const kBweExtensionPriorities[] = { + webrtc::RtpExtension::kTransportSequenceNumberUri, + webrtc::RtpExtension::kAbsSendTimeUri, + webrtc::RtpExtension::kTimestampOffsetUri}; + DiscardRedundantExtensions(&result, kBweExtensionPriorities); + } else { + static const char* const kBweExtensionPriorities[] = { + webrtc::RtpExtension::kAbsSendTimeUri, + webrtc::RtpExtension::kTimestampOffsetUri}; + DiscardRedundantExtensions(&result, kBweExtensionPriorities); + } } return result; } diff --git a/media/engine/webrtc_media_engine_unittest.cc b/media/engine/webrtc_media_engine_unittest.cc index 6b187cca89..4615f03deb 100644 --- a/media/engine/webrtc_media_engine_unittest.cc +++ b/media/engine/webrtc_media_engine_unittest.cc @@ -217,6 +217,23 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensionsRemoveRedundantEncrypted2) { EXPECT_NE(filtered[1].uri, filtered[2].uri); } +TEST(WebRtcMediaEngineTest, FilterRtpExtensionsRemoveRedundantBwe1) { + webrtc::test::ScopedKeyValueConfig trials( + "WebRTC-FilterAbsSendTimeExtension/Enabled/"); + std::vector extensions; + extensions.push_back( + RtpExtension(RtpExtension::kTransportSequenceNumberUri, 3)); + extensions.push_back(RtpExtension(RtpExtension::kTimestampOffsetUri, 9)); + extensions.push_back(RtpExtension(RtpExtension::kAbsSendTimeUri, 6)); + extensions.push_back( + RtpExtension(RtpExtension::kTransportSequenceNumberUri, 1)); + extensions.push_back(RtpExtension(RtpExtension::kTimestampOffsetUri, 14)); + std::vector filtered = + FilterRtpExtensions(extensions, SupportedExtensions2, true, trials); + EXPECT_EQ(1u, filtered.size()); + EXPECT_EQ(RtpExtension::kTransportSequenceNumberUri, filtered[0].uri); +} + TEST(WebRtcMediaEngineTest, FilterRtpExtensionsRemoveRedundantBwe1KeepAbsSendTime) { std::vector extensions; @@ -235,6 +252,29 @@ TEST(WebRtcMediaEngineTest, EXPECT_EQ(RtpExtension::kAbsSendTimeUri, filtered[1].uri); } +TEST(WebRtcMediaEngineTest, FilterRtpExtensionsRemoveRedundantBweEncrypted1) { + webrtc::test::ScopedKeyValueConfig trials( + "WebRTC-FilterAbsSendTimeExtension/Enabled/"); + std::vector extensions; + extensions.push_back( + RtpExtension(RtpExtension::kTransportSequenceNumberUri, 3)); + extensions.push_back( + RtpExtension(RtpExtension::kTransportSequenceNumberUri, 4, true)); + extensions.push_back(RtpExtension(RtpExtension::kTimestampOffsetUri, 9)); + extensions.push_back(RtpExtension(RtpExtension::kAbsSendTimeUri, 6)); + extensions.push_back( + RtpExtension(RtpExtension::kTransportSequenceNumberUri, 1)); + extensions.push_back( + RtpExtension(RtpExtension::kTransportSequenceNumberUri, 2, true)); + extensions.push_back(RtpExtension(RtpExtension::kTimestampOffsetUri, 14)); + std::vector filtered = + FilterRtpExtensions(extensions, SupportedExtensions2, true, trials); + EXPECT_EQ(2u, filtered.size()); + EXPECT_EQ(RtpExtension::kTransportSequenceNumberUri, filtered[0].uri); + EXPECT_EQ(RtpExtension::kTransportSequenceNumberUri, filtered[1].uri); + EXPECT_NE(filtered[0].encrypt, filtered[1].encrypt); +} + TEST(WebRtcMediaEngineTest, FilterRtpExtensionsRemoveRedundantBweEncrypted1KeepAbsSendTime) { std::vector extensions; diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index 10caf667c5..aa59ce4d8d 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -3122,6 +3122,17 @@ TEST_F(WebRtcVideoChannelTest, RecvAbsoluteSendTimeHeaderExtensions) { TestSetRecvRtpHeaderExtensions(RtpExtension::kAbsSendTimeUri); } +TEST_F(WebRtcVideoChannelTest, FiltersExtensionsPicksTransportSeqNum) { + webrtc::test::ScopedKeyValueConfig override_field_trials( + field_trials_, "WebRTC-FilterAbsSendTimeExtension/Enabled/"); + // Enable three redundant extensions. + std::vector extensions; + extensions.push_back(RtpExtension::kAbsSendTimeUri); + extensions.push_back(RtpExtension::kTimestampOffsetUri); + extensions.push_back(RtpExtension::kTransportSequenceNumberUri); + TestExtensionFilter(extensions, RtpExtension::kTransportSequenceNumberUri); +} + TEST_F(WebRtcVideoChannelTest, FiltersExtensionsPicksAbsSendTime) { // Enable two redundant extensions. std::vector extensions;