From ffc61181d860d9686ab769b6d68e1833309d0944 Mon Sep 17 00:00:00 2001 From: brandtr Date: Mon, 28 Nov 2016 06:02:22 -0800 Subject: [PATCH] Don't cache video codec list in VideoEngine2. A WebRtcVideoEngine2 object seems to be reused between PeerConnections, which means that the field trial added in https://codereview.webrtc.org/2511703002/ may not activate/deactivate as intended between calls. This CL removes the caching of video codecs, which gets rid of this problem. BUG=webrtc:5654 Review-Url: https://codereview.webrtc.org/2521393004 Cr-Commit-Position: refs/heads/master@{#15265} --- webrtc/media/base/mediaengine.h | 6 +-- webrtc/media/engine/webrtcvideoengine2.cc | 7 +-- webrtc/media/engine/webrtcvideoengine2.h | 4 +- .../engine/webrtcvideoengine2_unittest.cc | 50 ++++++++++++++++++- webrtc/pc/channelmanager.cc | 10 ++-- 5 files changed, 58 insertions(+), 19 deletions(-) diff --git a/webrtc/media/base/mediaengine.h b/webrtc/media/base/mediaengine.h index d1d162370d..8083c8edac 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 std::vector video_codecs() = 0; virtual RtpCapabilities GetVideoCapabilities() = 0; // Starts AEC dump using existing file, a maximum file size in bytes can be @@ -147,9 +147,7 @@ class CompositeMediaEngine : public MediaEngineInterface { virtual RtpCapabilities GetAudioCapabilities() { return voice_.GetCapabilities(); } - virtual const std::vector& video_codecs() { - return video_.codecs(); - } + virtual std::vector video_codecs() { return video_.codecs(); } virtual RtpCapabilities GetVideoCapabilities() { return video_.GetCapabilities(); } diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index 55e6374a2b..95c7c8c5f5 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -497,7 +497,6 @@ WebRtcVideoEngine2::WebRtcVideoEngine2() external_decoder_factory_(NULL), external_encoder_factory_(NULL) { LOG(LS_INFO) << "WebRtcVideoEngine2::WebRtcVideoEngine2()"; - video_codecs_ = GetSupportedCodecs(external_encoder_factory_); } WebRtcVideoEngine2::~WebRtcVideoEngine2() { @@ -520,8 +519,8 @@ WebRtcVideoChannel2* WebRtcVideoEngine2::CreateChannel( external_decoder_factory_); } -const std::vector& WebRtcVideoEngine2::codecs() const { - return video_codecs_; +std::vector WebRtcVideoEngine2::codecs() const { + return GetSupportedCodecs(external_encoder_factory_); } RtpCapabilities WebRtcVideoEngine2::GetCapabilities() const { @@ -568,8 +567,6 @@ void WebRtcVideoEngine2::SetExternalEncoderFactory( encoder_factory = simulcast_encoder_factory_.get(); } external_encoder_factory_ = encoder_factory; - - video_codecs_ = GetSupportedCodecs(encoder_factory); } // This is a helper function for AppendVideoCodecs below. It will return the diff --git a/webrtc/media/engine/webrtcvideoengine2.h b/webrtc/media/engine/webrtcvideoengine2.h index 20884e8bee..aa116ed318 100644 --- a/webrtc/media/engine/webrtcvideoengine2.h +++ b/webrtc/media/engine/webrtcvideoengine2.h @@ -102,7 +102,7 @@ class WebRtcVideoEngine2 { const MediaConfig& config, const VideoOptions& options); - const std::vector& codecs() const; + std::vector codecs() const; RtpCapabilities GetCapabilities() const; // Set a WebRtcVideoDecoderFactory for external decoding. Video engine does @@ -116,8 +116,6 @@ class WebRtcVideoEngine2 { WebRtcVideoEncoderFactory* encoder_factory); private: - std::vector video_codecs_; - bool initialized_; WebRtcVideoDecoderFactory* external_decoder_factory_; diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index af811216f9..b8cd213f17 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc @@ -79,7 +79,7 @@ bool HasRtxCodec(const std::vector& codecs, return false; } -static rtc::scoped_refptr CreateBlackFrameBuffer( +rtc::scoped_refptr CreateBlackFrameBuffer( int width, int height) { rtc::scoped_refptr buffer = @@ -751,6 +751,32 @@ TEST_F(WebRtcVideoEngine2Test, SimulcastDisabledForH264) { EXPECT_TRUE(channel->SetVideoSend(ssrcs[0], true, nullptr, nullptr)); } +// Test that the FlexFEC field trial properly alters the output of +// WebRtcVideoEngine2::codecs(), for an existing |engine_| object. +// +// TODO(brandtr): Remove this test, when the FlexFEC field trial is gone. +TEST_F(WebRtcVideoEngine2Test, + Flexfec03SupportedAsInternalCodecBehindFieldTrial) { + auto is_flexfec = [](const VideoCodec& codec) { + if (codec.name == "flexfec-03") + return true; + return false; + }; + + // FlexFEC is not active without field trial. + engine_.Init(); + const std::vector codecs_before = engine_.codecs(); + EXPECT_EQ(codecs_before.end(), std::find_if(codecs_before.begin(), + codecs_before.end(), is_flexfec)); + + // FlexFEC is active with field trial. + webrtc::test::ScopedFieldTrials override_field_trials_( + "WebRTC-FlexFEC-03/Enabled/"); + const std::vector codecs_after = engine_.codecs(); + EXPECT_NE(codecs_after.end(), + std::find_if(codecs_after.begin(), codecs_after.end(), is_flexfec)); +} + // Test that external codecs are added to the end of the supported codec list. TEST_F(WebRtcVideoEngine2Test, ReportSupportedExternalCodecs) { cricket::FakeWebRtcVideoEncoderFactory encoder_factory; @@ -763,11 +789,31 @@ TEST_F(WebRtcVideoEngine2Test, ReportSupportedExternalCodecs) { cricket::VideoCodec internal_codec = codecs.front(); cricket::VideoCodec external_codec = codecs.back(); - // The external codec will appear at last. + // The external codec will appear last in the vector. EXPECT_EQ("VP8", internal_codec.name); EXPECT_EQ("FakeExternalCodec", external_codec.name); } +// Test that an external codec that was added after the engine was initialized +// does show up in the codec list after it was added. +TEST_F(WebRtcVideoEngine2Test, ReportSupportedExternalCodecsWithAddedCodec) { + // Set up external encoder factory with first codec, and initialize engine. + cricket::FakeWebRtcVideoEncoderFactory encoder_factory; + encoder_factory.AddSupportedVideoCodecType("FakeExternalCodec1"); + engine_.SetExternalEncoderFactory(&encoder_factory); + engine_.Init(); + + // The first external codec will appear last in the vector. + std::vector codecs_before(engine_.codecs()); + EXPECT_EQ("FakeExternalCodec1", codecs_before.back().name); + + // Add second codec. + encoder_factory.AddSupportedVideoCodecType("FakeExternalCodec2"); + std::vector codecs_after(engine_.codecs()); + EXPECT_EQ(codecs_before.size() + 1, codecs_after.size()); + EXPECT_EQ("FakeExternalCodec2", codecs_after.back().name); +} + TEST_F(WebRtcVideoEngine2Test, RegisterExternalDecodersIfSupported) { cricket::FakeWebRtcVideoDecoderFactory decoder_factory; decoder_factory.AddSupportedVideoCodecType(webrtc::kVideoCodecVP8); diff --git a/webrtc/pc/channelmanager.cc b/webrtc/pc/channelmanager.cc index 06475e9eaa..255730a0ba 100644 --- a/webrtc/pc/channelmanager.cc +++ b/webrtc/pc/channelmanager.cc @@ -141,13 +141,13 @@ 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) { + std::vector video_codecs = media_engine_->video_codecs(); + for (const auto& video_codec : video_codecs) { + if (!enable_rtx_ && + _stricmp(kRtxCodecName, video_codec.name.c_str()) == 0) { continue; } - codecs->push_back(*it); + codecs->push_back(video_codec); } }