From c37e72e890cb1c769af9006dbd2e582c1a2e2a50 Mon Sep 17 00:00:00 2001 From: "pbos@webrtc.org" Date: Mon, 5 Jan 2015 18:51:13 +0000 Subject: [PATCH] Make setting identical RTP extensions a no-op. Setting extensions are responsible for a lot of stream tear-downs causing substantial slowdowns in SetRemoteDescription. BUG=1788,4077 R=pthatcher@webrtc.org Review URL: https://webrtc-codereview.appspot.com/40389004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@7998 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/media/webrtc/webrtcvideoengine2.cc | 42 +++++++++- .../webrtc/webrtcvideoengine2_unittest.cc | 76 ++++++++++++++++++- .../webrtc/webrtcvideoengine2_unittest.h | 5 ++ 3 files changed, 120 insertions(+), 3 deletions(-) diff --git a/talk/media/webrtc/webrtcvideoengine2.cc b/talk/media/webrtc/webrtcvideoengine2.cc index 0ad35f3316..d066eb9c9e 100644 --- a/talk/media/webrtc/webrtcvideoengine2.cc +++ b/talk/media/webrtc/webrtcvideoengine2.cc @@ -28,6 +28,7 @@ #ifdef HAVE_WEBRTC_VIDEO #include "talk/media/webrtc/webrtcvideoengine2.h" +#include #include #include @@ -165,6 +166,13 @@ static bool ValidateRtpHeaderExtensionIds( return true; } +static bool CompareRtpHeaderExtensionIds( + const webrtc::RtpExtension& extension1, + const webrtc::RtpExtension& extension2) { + // Sorting on ID is sufficient, more than one extension per ID is unsupported. + return extension1.id > extension2.id; +} + static std::vector FilterRtpExtensions( const std::vector& extensions) { std::vector webrtc_extensions; @@ -177,9 +185,28 @@ static std::vector FilterRtpExtensions( LOG(LS_WARNING) << "Unsupported RTP extension: " << extensions[i].uri; } } + + // Sort filtered headers to make sure that they can later be compared + // regardless of in which order they were entered. + std::sort(webrtc_extensions.begin(), webrtc_extensions.end(), + CompareRtpHeaderExtensionIds); return webrtc_extensions; } +static bool RtpExtensionsHaveChanged( + const std::vector& before, + const std::vector& after) { + if (before.size() != after.size()) + return true; + for (size_t i = 0; i < before.size(); ++i) { + if (before[i].id != after[i].id) + return true; + if (before[i].name != after[i].name) + return true; + } + return false; +} + WebRtcVideoEncoderFactory2::~WebRtcVideoEncoderFactory2() { } @@ -1156,7 +1183,13 @@ bool WebRtcVideoChannel2::SetRecvRtpHeaderExtensions( if (!ValidateRtpHeaderExtensionIds(extensions)) return false; - recv_rtp_extensions_ = FilterRtpExtensions(extensions); + std::vector filtered_extensions = + FilterRtpExtensions(extensions); + if (!RtpExtensionsHaveChanged(recv_rtp_extensions_, filtered_extensions)) + return true; + + recv_rtp_extensions_ = filtered_extensions; + rtc::CritScope stream_lock(&stream_crit_); for (std::map::iterator it = receive_streams_.begin(); @@ -1174,7 +1207,12 @@ bool WebRtcVideoChannel2::SetSendRtpHeaderExtensions( if (!ValidateRtpHeaderExtensionIds(extensions)) return false; - send_rtp_extensions_ = FilterRtpExtensions(extensions); + std::vector filtered_extensions = + FilterRtpExtensions(extensions); + if (!RtpExtensionsHaveChanged(send_rtp_extensions_, filtered_extensions)) + return true; + + send_rtp_extensions_ = filtered_extensions; rtc::CritScope stream_lock(&stream_crit_); for (std::map::iterator it = diff --git a/talk/media/webrtc/webrtcvideoengine2_unittest.cc b/talk/media/webrtc/webrtcvideoengine2_unittest.cc index 158040fd2f..93660a5722 100644 --- a/talk/media/webrtc/webrtcvideoengine2_unittest.cc +++ b/talk/media/webrtc/webrtcvideoengine2_unittest.cc @@ -25,6 +25,7 @@ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ +#include #include #include @@ -188,7 +189,10 @@ void FakeVideoReceiveStream::Stop() { } FakeCall::FakeCall(const webrtc::Call::Config& config) - : config_(config), network_state_(kNetworkUp) { + : config_(config), + network_state_(kNetworkUp), + num_created_send_streams_(0), + num_created_receive_streams_(0) { SetVideoCodecs(GetDefaultVideoCodecs()); } @@ -265,6 +269,7 @@ webrtc::VideoSendStream* FakeCall::CreateVideoSendStream( FakeVideoSendStream* fake_stream = new FakeVideoSendStream(config, encoder_config); video_send_streams_.push_back(fake_stream); + ++num_created_send_streams_; return fake_stream; } @@ -284,6 +289,7 @@ void FakeCall::DestroyVideoSendStream(webrtc::VideoSendStream* send_stream) { webrtc::VideoReceiveStream* FakeCall::CreateVideoReceiveStream( const webrtc::VideoReceiveStream::Config& config) { video_receive_streams_.push_back(new FakeVideoReceiveStream(config)); + ++num_created_receive_streams_; return video_receive_streams_[video_receive_streams_.size() - 1]; } @@ -310,6 +316,14 @@ void FakeCall::SetStats(const webrtc::Call::Stats& stats) { stats_ = stats; } +int FakeCall::GetNumCreatedSendStreams() const { + return num_created_send_streams_; +} + +int FakeCall::GetNumCreatedReceiveStreams() const { + return num_created_receive_streams_; +} + webrtc::Call::Stats FakeCall::GetStats() const { return stats_; } @@ -1049,6 +1063,66 @@ TEST_F(WebRtcVideoChannel2Test, RecvAbsoluteSendTimeHeaderExtensions) { webrtc::RtpExtension::kAbsSendTime); } +TEST_F(WebRtcVideoChannel2Test, IdenticalSendExtensionsDoesntRecreateStream) { + const int kTOffsetId = 1; + const int kAbsSendTimeId = 2; + std::vector extensions; + extensions.push_back(cricket::RtpHeaderExtension( + kRtpAbsoluteSenderTimeHeaderExtension, kAbsSendTimeId)); + extensions.push_back(cricket::RtpHeaderExtension( + kRtpTimestampOffsetHeaderExtension, kTOffsetId)); + + EXPECT_TRUE(channel_->SetSendRtpHeaderExtensions(extensions)); + FakeVideoSendStream* send_stream = + AddSendStream(cricket::StreamParams::CreateLegacy(123)); + + EXPECT_EQ(1, fake_call_->GetNumCreatedSendStreams()); + ASSERT_EQ(2u, send_stream->GetConfig().rtp.extensions.size()); + + // Setting the same extensions (even if in different order) shouldn't + // reallocate the stream. + std::reverse(extensions.begin(), extensions.end()); + EXPECT_TRUE(channel_->SetSendRtpHeaderExtensions(extensions)); + + EXPECT_EQ(1, fake_call_->GetNumCreatedSendStreams()); + + // Setting different extensions should recreate the stream. + extensions.resize(1); + EXPECT_TRUE(channel_->SetSendRtpHeaderExtensions(extensions)); + + EXPECT_EQ(2, fake_call_->GetNumCreatedSendStreams()); +} + +TEST_F(WebRtcVideoChannel2Test, IdenticalRecvExtensionsDoesntRecreateStream) { + const int kTOffsetId = 1; + const int kAbsSendTimeId = 2; + std::vector extensions; + extensions.push_back(cricket::RtpHeaderExtension( + kRtpAbsoluteSenderTimeHeaderExtension, kAbsSendTimeId)); + extensions.push_back(cricket::RtpHeaderExtension( + kRtpTimestampOffsetHeaderExtension, kTOffsetId)); + + EXPECT_TRUE(channel_->SetRecvRtpHeaderExtensions(extensions)); + FakeVideoReceiveStream* send_stream = + AddRecvStream(cricket::StreamParams::CreateLegacy(123)); + + EXPECT_EQ(1, fake_call_->GetNumCreatedReceiveStreams()); + ASSERT_EQ(2u, send_stream->GetConfig().rtp.extensions.size()); + + // Setting the same extensions (even if in different order) shouldn't + // reallocate the stream. + std::reverse(extensions.begin(), extensions.end()); + EXPECT_TRUE(channel_->SetRecvRtpHeaderExtensions(extensions)); + + EXPECT_EQ(1, fake_call_->GetNumCreatedReceiveStreams()); + + // Setting different extensions should recreate the stream. + extensions.resize(1); + EXPECT_TRUE(channel_->SetRecvRtpHeaderExtensions(extensions)); + + EXPECT_EQ(2, fake_call_->GetNumCreatedReceiveStreams()); +} + TEST_F(WebRtcVideoChannel2Test, SetSendRtpHeaderExtensionsExcludeUnsupportedExtensions) { const int kUnsupportedId = 1; diff --git a/talk/media/webrtc/webrtcvideoengine2_unittest.h b/talk/media/webrtc/webrtcvideoengine2_unittest.h index 40d3973f81..e53a30b06a 100644 --- a/talk/media/webrtc/webrtcvideoengine2_unittest.h +++ b/talk/media/webrtc/webrtcvideoengine2_unittest.h @@ -113,6 +113,8 @@ class FakeCall : public webrtc::Call { std::vector GetDefaultVideoCodecs(); webrtc::Call::NetworkState GetNetworkState() const; + int GetNumCreatedSendStreams() const; + int GetNumCreatedReceiveStreams() const; void SetStats(const webrtc::Call::Stats& stats); private: @@ -142,6 +144,9 @@ class FakeCall : public webrtc::Call { std::vector codecs_; std::vector video_send_streams_; std::vector video_receive_streams_; + + int num_created_send_streams_; + int num_created_receive_streams_; }; } // namespace cricket