From 9f71ec5a3e3175751f4475b126cfda89767363f2 Mon Sep 17 00:00:00 2001 From: magjed Date: Wed, 9 Nov 2016 23:45:11 -0800 Subject: [PATCH] Stop caching supported codecs in WebRtcVideoEngine2 We currently cache the result of GetSupportedCodecs in a member variable |video_codecs_| in WebRtcVideoEngine2. This means we need to keep |video_codecs_| and the result of GetSupportedCodecs in sync, which is error prone. It's simpler to just call GetSupportedCodecs when we need it, and we actually end up making fewer calls, so it's faster as well. This CL also returns all std::vectors by-value instead of by-ref. Move semantic together with in-place filtering of codecs actually end up with fewer copies, and it's also simpler to not return references. BUG=webrtc:6337 Review-Url: https://codereview.webrtc.org/2492473002 Cr-Commit-Position: refs/heads/master@{#15007} --- webrtc/media/base/mediaengine.h | 4 ++-- webrtc/media/engine/webrtcvideoengine2.cc | 7 ++----- webrtc/media/engine/webrtcvideoengine2.h | 4 +--- webrtc/pc/channelmanager.cc | 20 +++++++++----------- webrtc/pc/channelmanager.h | 2 +- webrtc/pc/channelmanager_unittest.cc | 8 ++++---- webrtc/pc/mediasession.cc | 2 +- 7 files changed, 20 insertions(+), 27 deletions(-) diff --git a/webrtc/media/base/mediaengine.h b/webrtc/media/base/mediaengine.h index d1d162370d..edf0cb0f30 100644 --- a/webrtc/media/base/mediaengine.h +++ b/webrtc/media/base/mediaengine.h @@ -74,7 +74,7 @@ class MediaEngineInterface { virtual const std::vector& audio_send_codecs() = 0; virtual const std::vector& audio_recv_codecs() = 0; virtual RtpCapabilities GetAudioCapabilities() = 0; - virtual const std::vector& video_codecs() = 0; + virtual const std::vector video_codecs() = 0; virtual RtpCapabilities GetVideoCapabilities() = 0; // Starts AEC dump using existing file, a maximum file size in bytes can be @@ -147,7 +147,7 @@ class CompositeMediaEngine : public MediaEngineInterface { virtual RtpCapabilities GetAudioCapabilities() { return voice_.GetCapabilities(); } - virtual const std::vector& video_codecs() { + virtual const std::vector video_codecs() { return video_.codecs(); } virtual RtpCapabilities GetVideoCapabilities() { diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index 3cacc7da82..dfdae5ea99 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -543,7 +543,6 @@ WebRtcVideoEngine2::WebRtcVideoEngine2() external_decoder_factory_(NULL), external_encoder_factory_(NULL) { LOG(LS_INFO) << "WebRtcVideoEngine2::WebRtcVideoEngine2()"; - video_codecs_ = GetSupportedCodecs(external_encoder_factory_); } WebRtcVideoEngine2::~WebRtcVideoEngine2() { @@ -566,8 +565,8 @@ WebRtcVideoChannel2* WebRtcVideoEngine2::CreateChannel( external_decoder_factory_); } -const std::vector& WebRtcVideoEngine2::codecs() const { - return video_codecs_; +const std::vector WebRtcVideoEngine2::codecs() const { + return GetSupportedCodecs(external_encoder_factory_); } RtpCapabilities WebRtcVideoEngine2::GetCapabilities() const { @@ -614,8 +613,6 @@ void WebRtcVideoEngine2::SetExternalEncoderFactory( encoder_factory = simulcast_encoder_factory_.get(); } external_encoder_factory_ = encoder_factory; - - video_codecs_ = GetSupportedCodecs(encoder_factory); } static std::vector GetSupportedCodecs( diff --git a/webrtc/media/engine/webrtcvideoengine2.h b/webrtc/media/engine/webrtcvideoengine2.h index ef893d2e98..1032e9c7e0 100644 --- a/webrtc/media/engine/webrtcvideoengine2.h +++ b/webrtc/media/engine/webrtcvideoengine2.h @@ -105,7 +105,7 @@ class WebRtcVideoEngine2 { const MediaConfig& config, const VideoOptions& options); - const std::vector& codecs() const; + const std::vector codecs() const; RtpCapabilities GetCapabilities() const; // Set a WebRtcVideoDecoderFactory for external decoding. Video engine does @@ -119,8 +119,6 @@ class WebRtcVideoEngine2 { WebRtcVideoEncoderFactory* encoder_factory); private: - std::vector video_codecs_; - bool initialized_; WebRtcVideoDecoderFactory* external_decoder_factory_; diff --git a/webrtc/pc/channelmanager.cc b/webrtc/pc/channelmanager.cc index 06475e9eaa..fb6f44f846 100644 --- a/webrtc/pc/channelmanager.cc +++ b/webrtc/pc/channelmanager.cc @@ -29,6 +29,9 @@ namespace cricket { +static bool IsRtxCodec(const VideoCodec& codec) { + return _stricmp(kRtxCodecName, codec.name.c_str()) == 0; +} using rtc::Bind; @@ -137,18 +140,13 @@ void ChannelManager::GetSupportedAudioRtpHeaderExtensions( *ext = media_engine_->GetAudioCapabilities().header_extensions; } -void ChannelManager::GetSupportedVideoCodecs( - std::vector* codecs) const { - codecs->clear(); - - std::vector::const_iterator it; - for (it = media_engine_->video_codecs().begin(); - it != media_engine_->video_codecs().end(); ++it) { - if (!enable_rtx_ && _stricmp(kRtxCodecName, it->name.c_str()) == 0) { - continue; - } - codecs->push_back(*it); +std::vector ChannelManager::GetSupportedVideoCodecs() const { + std::vector codecs = media_engine_->video_codecs(); + if (!enable_rtx_) { + codecs.erase(std::remove_if(codecs.begin(), codecs.end(), &IsRtxCodec), + codecs.end()); } + return codecs; } void ChannelManager::GetSupportedVideoRtpHeaderExtensions( diff --git a/webrtc/pc/channelmanager.h b/webrtc/pc/channelmanager.h index 15a3752c44..68b1ab2feb 100644 --- a/webrtc/pc/channelmanager.h +++ b/webrtc/pc/channelmanager.h @@ -75,7 +75,7 @@ class ChannelManager { void GetSupportedAudioSendCodecs(std::vector* codecs) const; void GetSupportedAudioReceiveCodecs(std::vector* codecs) const; void GetSupportedAudioRtpHeaderExtensions(RtpHeaderExtensions* ext) const; - void GetSupportedVideoCodecs(std::vector* codecs) const; + std::vector GetSupportedVideoCodecs() const; void GetSupportedVideoRtpHeaderExtensions(RtpHeaderExtensions* ext) const; void GetSupportedDataCodecs(std::vector* codecs) const; diff --git a/webrtc/pc/channelmanager_unittest.cc b/webrtc/pc/channelmanager_unittest.cc index 174d064b05..34b347b3ff 100644 --- a/webrtc/pc/channelmanager_unittest.cc +++ b/webrtc/pc/channelmanager_unittest.cc @@ -173,17 +173,17 @@ TEST_F(ChannelManagerTest, SetVideoRtxEnabled) { const VideoCodec rtx_codec(96, "rtx"); // By default RTX is disabled. - cm_->GetSupportedVideoCodecs(&codecs); + codecs = cm_->GetSupportedVideoCodecs(); EXPECT_FALSE(ContainsMatchingCodec(codecs, rtx_codec)); // Enable and check. EXPECT_TRUE(cm_->SetVideoRtxEnabled(true)); - cm_->GetSupportedVideoCodecs(&codecs); + codecs = cm_->GetSupportedVideoCodecs(); EXPECT_TRUE(ContainsMatchingCodec(codecs, rtx_codec)); // Disable and check. EXPECT_TRUE(cm_->SetVideoRtxEnabled(false)); - cm_->GetSupportedVideoCodecs(&codecs); + codecs = cm_->GetSupportedVideoCodecs(); EXPECT_FALSE(ContainsMatchingCodec(codecs, rtx_codec)); // Cannot toggle rtx after initialization. @@ -194,7 +194,7 @@ TEST_F(ChannelManagerTest, SetVideoRtxEnabled) { // Can set again after terminate. cm_->Terminate(); EXPECT_TRUE(cm_->SetVideoRtxEnabled(true)); - cm_->GetSupportedVideoCodecs(&codecs); + codecs = cm_->GetSupportedVideoCodecs(); EXPECT_TRUE(ContainsMatchingCodec(codecs, rtx_codec)); } diff --git a/webrtc/pc/mediasession.cc b/webrtc/pc/mediasession.cc index ebba9acbcb..0ebc766039 100644 --- a/webrtc/pc/mediasession.cc +++ b/webrtc/pc/mediasession.cc @@ -1274,7 +1274,7 @@ MediaSessionDescriptionFactory::MediaSessionDescriptionFactory( channel_manager->GetSupportedAudioReceiveCodecs(&audio_recv_codecs_); channel_manager->GetSupportedAudioSendCodecs(&audio_send_codecs_); channel_manager->GetSupportedAudioRtpHeaderExtensions(&audio_rtp_extensions_); - channel_manager->GetSupportedVideoCodecs(&video_codecs_); + video_codecs_ = channel_manager->GetSupportedVideoCodecs(); channel_manager->GetSupportedVideoRtpHeaderExtensions(&video_rtp_extensions_); channel_manager->GetSupportedDataCodecs(&data_codecs_); NegotiateCodecs(audio_recv_codecs_, audio_send_codecs_,