From bbaf3633c54e3d49aa4c762b8eaa34e09de01c45 Mon Sep 17 00:00:00 2001 From: Stefan Holmer Date: Thu, 29 Oct 2015 18:53:23 +0100 Subject: [PATCH] Filter overlapping RTP header extensions. This removes unnecessary RTP header extension overhead since only one of these extensions is used at a time. BUG=webrtc:4254 R=pbos@webrtc.org Review URL: https://codereview.webrtc.org/1429753003 . Cr-Commit-Position: refs/heads/master@{#10455} --- talk/media/webrtc/webrtcmediaengine.cc | 40 +++++++++++++++ talk/media/webrtc/webrtcmediaengine.h | 8 +++ talk/media/webrtc/webrtcvideoengine2.cc | 4 +- .../webrtc/webrtcvideoengine2_unittest.cc | 49 ++++++++++++++++--- 4 files changed, 94 insertions(+), 7 deletions(-) diff --git a/talk/media/webrtc/webrtcmediaengine.cc b/talk/media/webrtc/webrtcmediaengine.cc index cf6a5cf2e5..af202bd613 100644 --- a/talk/media/webrtc/webrtcmediaengine.cc +++ b/talk/media/webrtc/webrtcmediaengine.cc @@ -68,4 +68,44 @@ MediaEngineInterface* WebRtcMediaEngineFactory::Create( return CreateWebRtcMediaEngine(adm, encoder_factory, decoder_factory); } +const char* kBweExtensionPriorities[] = { + kRtpTransportSequenceNumberHeaderExtension, + kRtpAbsoluteSenderTimeHeaderExtension, kRtpTimestampOffsetHeaderExtension}; + +const size_t kBweExtensionPrioritiesLength = + ARRAY_SIZE(kBweExtensionPriorities); + +int GetPriority(const RtpHeaderExtension& extension, + const char* extension_prios[], + size_t extension_prios_length) { + for (size_t i = 0; i < extension_prios_length; ++i) { + if (extension.uri == extension_prios[i]) + return static_cast(i); + } + return -1; +} + +std::vector FilterRedundantRtpExtensions( + const std::vector& extensions, + const char* extension_prios[], + size_t extension_prios_length) { + if (extensions.empty()) + return std::vector(); + std::vector filtered; + std::map sorted; + for (auto& extension : extensions) { + int priority = + GetPriority(extension, extension_prios, extension_prios_length); + if (priority == -1) { + filtered.push_back(extension); + continue; + } else { + sorted[priority] = &extension; + } + } + if (!sorted.empty()) + filtered.push_back(*sorted.begin()->second); + return filtered; +} + } // namespace cricket diff --git a/talk/media/webrtc/webrtcmediaengine.h b/talk/media/webrtc/webrtcmediaengine.h index a3af1e73fb..8d7540404d 100644 --- a/talk/media/webrtc/webrtcmediaengine.h +++ b/talk/media/webrtc/webrtcmediaengine.h @@ -48,6 +48,14 @@ class WebRtcMediaEngineFactory { WebRtcVideoDecoderFactory* decoder_factory); }; +extern const char* kBweExtensionPriorities[]; +extern const size_t kBweExtensionPrioritiesLength; + +std::vector FilterRedundantRtpExtensions( + const std::vector& extensions, + const char* extension_prios[], + size_t extension_prios_length); + } // namespace cricket #endif // TALK_MEDIA_WEBRTCMEDIAENGINE_H_ diff --git a/talk/media/webrtc/webrtcvideoengine2.cc b/talk/media/webrtc/webrtcvideoengine2.cc index ed75c0d427..bcd513ee2d 100644 --- a/talk/media/webrtc/webrtcvideoengine2.cc +++ b/talk/media/webrtc/webrtcvideoengine2.cc @@ -36,6 +36,7 @@ #include "talk/media/base/videorenderer.h" #include "talk/media/webrtc/constants.h" #include "talk/media/webrtc/simulcast.h" +#include "talk/media/webrtc/webrtcmediaengine.h" #include "talk/media/webrtc/webrtcvideoencoderfactory.h" #include "talk/media/webrtc/webrtcvideoframe.h" #include "talk/media/webrtc/webrtcvoiceengine.h" @@ -1544,7 +1545,8 @@ bool WebRtcVideoChannel2::SetSendRtpHeaderExtensions( return false; std::vector filtered_extensions = - FilterRtpExtensions(extensions); + FilterRtpExtensions(FilterRedundantRtpExtensions( + extensions, kBweExtensionPriorities, kBweExtensionPrioritiesLength)); if (!RtpExtensionsHaveChanged(send_rtp_extensions_, filtered_extensions)) { LOG(LS_INFO) << "Ignoring call to SetSendRtpHeaderExtensions because " "header extensions haven't changed."; diff --git a/talk/media/webrtc/webrtcvideoengine2_unittest.cc b/talk/media/webrtc/webrtcvideoengine2_unittest.cc index 479dd05f55..c0cd2ffa50 100644 --- a/talk/media/webrtc/webrtcvideoengine2_unittest.cc +++ b/talk/media/webrtc/webrtcvideoengine2_unittest.cc @@ -1066,6 +1066,29 @@ class WebRtcVideoChannel2Test : public WebRtcVideoEngine2Test { EXPECT_EQ(webrtc_ext, recv_stream->GetConfig().rtp.extensions[0].name); } + void TestExtensionFilter(const std::vector& extensions, + const std::string& expected_extension) { + cricket::VideoSendParameters parameters = send_parameters_; + int expected_id = -1; + int id = 1; + for (const std::string& extension : extensions) { + if (extension == expected_extension) + expected_id = id; + parameters.extensions.push_back( + cricket::RtpHeaderExtension(extension, id++)); + } + EXPECT_TRUE(channel_->SetSendParameters(parameters)); + FakeVideoSendStream* send_stream = + AddSendStream(cricket::StreamParams::CreateLegacy(123)); + + // Verify that only one of them has been set, and that it is the one with + // highest priority (transport sequence number). + ASSERT_EQ(1u, send_stream->GetConfig().rtp.extensions.size()); + EXPECT_EQ(expected_id, send_stream->GetConfig().rtp.extensions[0].id); + EXPECT_EQ(expected_extension, + send_stream->GetConfig().rtp.extensions[0].name); + } + void TestCpuAdaptation(bool enable_overuse, bool is_screenshare); void TestReceiverLocalSsrcConfiguration(bool receiver_first); void TestReceiveUnsignalledSsrcPacket(uint8_t payload_type, @@ -1199,6 +1222,23 @@ TEST_F(WebRtcVideoChannel2Test, RecvAbsoluteSendTimeHeaderExtensions) { webrtc::RtpExtension::kAbsSendTime); } +TEST_F(WebRtcVideoChannel2Test, FiltersExtensionsPicksTransportSeqNum) { + // Enable three redundant extensions. + std::vector extensions; + extensions.push_back(kRtpAbsoluteSenderTimeHeaderExtension); + extensions.push_back(kRtpTimestampOffsetHeaderExtension); + extensions.push_back(kRtpTransportSequenceNumberHeaderExtension); + TestExtensionFilter(extensions, kRtpTransportSequenceNumberHeaderExtension); +} + +TEST_F(WebRtcVideoChannel2Test, FiltersExtensionsPicksAbsSendTime) { + // Enable two redundant extensions. + std::vector extensions; + extensions.push_back(kRtpAbsoluteSenderTimeHeaderExtension); + extensions.push_back(kRtpTimestampOffsetHeaderExtension); + TestExtensionFilter(extensions, kRtpAbsoluteSenderTimeHeaderExtension); +} + class WebRtcVideoChannel2WithSendSideBweTest : public WebRtcVideoChannel2Test { public: WebRtcVideoChannel2WithSendSideBweTest() @@ -1230,13 +1270,10 @@ TEST_F(WebRtcVideoChannel2Test, RecvVideoRotationHeaderExtensions) { } TEST_F(WebRtcVideoChannel2Test, IdenticalSendExtensionsDoesntRecreateStream) { - const int kTOffsetId = 1; - const int kAbsSendTimeId = 2; - const int kVideoRotationId = 3; + const int kAbsSendTimeId = 1; + const int kVideoRotationId = 2; send_parameters_.extensions.push_back(cricket::RtpHeaderExtension( kRtpAbsoluteSenderTimeHeaderExtension, kAbsSendTimeId)); - send_parameters_.extensions.push_back(cricket::RtpHeaderExtension( - kRtpTimestampOffsetHeaderExtension, kTOffsetId)); send_parameters_.extensions.push_back(cricket::RtpHeaderExtension( kRtpVideoRotationHeaderExtension, kVideoRotationId)); @@ -1245,7 +1282,7 @@ TEST_F(WebRtcVideoChannel2Test, IdenticalSendExtensionsDoesntRecreateStream) { AddSendStream(cricket::StreamParams::CreateLegacy(123)); EXPECT_EQ(1, fake_call_->GetNumCreatedSendStreams()); - ASSERT_EQ(3u, send_stream->GetConfig().rtp.extensions.size()); + ASSERT_EQ(2u, send_stream->GetConfig().rtp.extensions.size()); // Setting the same extensions (even if in different order) shouldn't // reallocate the stream.