From f1f0d9a4cd53f4eacbf791cb7317612fa7382a45 Mon Sep 17 00:00:00 2001 From: "pbos@webrtc.org" Date: Mon, 2 Mar 2015 13:30:15 +0000 Subject: [PATCH] Remove WebRtcVideoEngine::SetVoiceEngine. Instead enforcing that a voice engine is set on construction. Apart from simplifying the class this permits tracing to be set up in the constructor without worrying about racing sets from SetVoiceEngine later. BUG= R=tommi@webrtc.org Review URL: https://webrtc-codereview.appspot.com/44489004 Cr-Commit-Position: refs/heads/master@{#8555} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8555 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/media/base/fakemediaengine.h | 4 ++- talk/media/base/mediaengine.h | 2 +- talk/media/base/videoengine_unittest.h | 2 +- talk/media/webrtc/webrtcmediaengine.cc | 2 -- talk/media/webrtc/webrtcvideoengine.cc | 13 ++-------- talk/media/webrtc/webrtcvideoengine.h | 4 +-- talk/media/webrtc/webrtcvideoengine2.cc | 13 ++-------- talk/media/webrtc/webrtcvideoengine2.h | 5 +--- .../webrtc/webrtcvideoengine2_unittest.cc | 26 ++++++++++++------- 9 files changed, 28 insertions(+), 43 deletions(-) diff --git a/talk/media/base/fakemediaengine.h b/talk/media/base/fakemediaengine.h index 391ec37215..5228298670 100644 --- a/talk/media/base/fakemediaengine.h +++ b/talk/media/base/fakemediaengine.h @@ -860,7 +860,9 @@ class FakeVoiceEngine : public FakeBaseEngine { class FakeVideoEngine : public FakeBaseEngine { public: - FakeVideoEngine() : capture_(false), processor_(NULL) { + FakeVideoEngine() : FakeVideoEngine(nullptr) {} + explicit FakeVideoEngine(FakeVoiceEngine* voice) + : capture_(false), processor_(NULL) { // Add a fake video codec. Note that the name must not be "" as there are // sanity checks against that. codecs_.push_back(VideoCodec(0, "fake_video_codec", 0, 0, 0, 0)); diff --git a/talk/media/base/mediaengine.h b/talk/media/base/mediaengine.h index b8d661c444..61424d225d 100644 --- a/talk/media/base/mediaengine.h +++ b/talk/media/base/mediaengine.h @@ -172,7 +172,7 @@ class MediaEngineFactory { template class CompositeMediaEngine : public MediaEngineInterface { public: - CompositeMediaEngine() {} + CompositeMediaEngine() : video_(&voice_) {} virtual ~CompositeMediaEngine() {} virtual bool Init(rtc::Thread* worker_thread) { if (!voice_.Init(worker_thread)) diff --git a/talk/media/base/videoengine_unittest.h b/talk/media/base/videoengine_unittest.h index 128ec80108..201c35235d 100644 --- a/talk/media/base/videoengine_unittest.h +++ b/talk/media/base/videoengine_unittest.h @@ -100,7 +100,7 @@ inline int TimeBetweenSend(const cricket::VideoCodec& codec) { template class VideoEngineOverride : public T { public: - VideoEngineOverride() { + VideoEngineOverride() : T(nullptr) { } virtual ~VideoEngineOverride() { } diff --git a/talk/media/webrtc/webrtcmediaengine.cc b/talk/media/webrtc/webrtcmediaengine.cc index cf0fcdf506..0f3a240795 100644 --- a/talk/media/webrtc/webrtcmediaengine.cc +++ b/talk/media/webrtc/webrtcmediaengine.cc @@ -49,7 +49,6 @@ class WebRtcMediaEngine : WebRtcVideoEncoderFactory* encoder_factory, WebRtcVideoDecoderFactory* decoder_factory) { voice_.SetAudioDeviceModule(adm, adm_sc); - video_.SetVoiceEngine(&voice_); video_.SetExternalEncoderFactory(encoder_factory); video_.SetExternalDecoderFactory(decoder_factory); } @@ -66,7 +65,6 @@ class WebRtcMediaEngine2 : voice_.SetAudioDeviceModule(adm, adm_sc); video_.SetExternalDecoderFactory(decoder_factory); video_.SetExternalEncoderFactory(encoder_factory); - video_.SetVoiceEngine(&voice_); } }; #endif // WEBRTC_CHROMIUM_BUILD diff --git a/talk/media/webrtc/webrtcvideoengine.cc b/talk/media/webrtc/webrtcvideoengine.cc index 2685252955..a86e6107bf 100644 --- a/talk/media/webrtc/webrtcvideoengine.cc +++ b/talk/media/webrtc/webrtcvideoengine.cc @@ -1066,8 +1066,8 @@ static bool GetCpuOveruseOptions(const VideoOptions& options, return true; } -WebRtcVideoEngine::WebRtcVideoEngine() { - Construct(new ViEWrapper(), new ViETraceWrapper(), NULL, +WebRtcVideoEngine::WebRtcVideoEngine(WebRtcVoiceEngine* voice_engine) { + Construct(new ViEWrapper(), new ViETraceWrapper(), voice_engine, new rtc::CpuMonitor(NULL)); } @@ -1473,15 +1473,6 @@ void WebRtcVideoEngine::UnregisterChannel(WebRtcVideoMediaChannel *channel) { channels_.end()); } -bool WebRtcVideoEngine::SetVoiceEngine(WebRtcVoiceEngine* voice_engine) { - if (initialized_) { - LOG(LS_WARNING) << "SetVoiceEngine can not be called after Init"; - return false; - } - voice_engine_ = voice_engine; - return true; -} - bool WebRtcVideoEngine::EnableTimedRender() { if (initialized_) { LOG(LS_WARNING) << "EnableTimedRender can not be called after Init"; diff --git a/talk/media/webrtc/webrtcvideoengine.h b/talk/media/webrtc/webrtcvideoengine.h index 0b0374568f..25db0a966a 100644 --- a/talk/media/webrtc/webrtcvideoengine.h +++ b/talk/media/webrtc/webrtcvideoengine.h @@ -98,7 +98,7 @@ class WebRtcVideoEngine : public sigslot::has_slots<>, public webrtc::TraceCallback { public: // Creates the WebRtcVideoEngine with internal VideoCaptureModule. - WebRtcVideoEngine(); + explicit WebRtcVideoEngine(WebRtcVoiceEngine* voice_engine); // For testing purposes. Allows the WebRtcVoiceEngine, // ViEWrapper and CpuMonitor to be mocks. // TODO(juberti): Remove the 3-arg ctor once fake tracing is implemented. @@ -131,8 +131,6 @@ class WebRtcVideoEngine : public sigslot::has_slots<>, sigslot::repeater2 SignalCaptureStateChange; - // Set the VoiceEngine for A/V sync. This can only be called before Init. - bool SetVoiceEngine(WebRtcVoiceEngine* voice_engine); // Set a WebRtcVideoDecoderFactory for external decoding. Video engine does // not take the ownership of |decoder_factory|. The caller needs to make sure // that |decoder_factory| outlives the video engine. diff --git a/talk/media/webrtc/webrtcvideoengine2.cc b/talk/media/webrtc/webrtcvideoengine2.cc index ebf05fa58b..2e56db7b54 100644 --- a/talk/media/webrtc/webrtcvideoengine2.cc +++ b/talk/media/webrtc/webrtcvideoengine2.cc @@ -315,9 +315,9 @@ void DefaultUnsignalledSsrcHandler::SetDefaultRenderer( } } -WebRtcVideoEngine2::WebRtcVideoEngine2() +WebRtcVideoEngine2::WebRtcVideoEngine2(WebRtcVoiceEngine* voice_engine) : worker_thread_(NULL), - voice_engine_(NULL), + voice_engine_(voice_engine), default_codec_format_(kDefaultVideoMaxWidth, kDefaultVideoMaxHeight, FPS_TO_INTERVAL(kDefaultVideoMaxFramerate), @@ -524,15 +524,6 @@ bool WebRtcVideoEngine2::CanSendCodec(const VideoCodec& requested, return out->width > 0 && out->height > 0; } -bool WebRtcVideoEngine2::SetVoiceEngine(WebRtcVoiceEngine* voice_engine) { - if (initialized_) { - LOG(LS_WARNING) << "SetVoiceEngine can not be called after Init"; - return false; - } - voice_engine_ = voice_engine; - return true; -} - // Ignore spammy trace messages, mostly from the stats API when we haven't // gotten RTCP info yet from the remote side. bool WebRtcVideoEngine2::ShouldIgnoreTrace(const std::string& trace) { diff --git a/talk/media/webrtc/webrtcvideoengine2.h b/talk/media/webrtc/webrtcvideoengine2.h index 98fa8e6579..fdc8443de2 100644 --- a/talk/media/webrtc/webrtcvideoengine2.h +++ b/talk/media/webrtc/webrtcvideoengine2.h @@ -111,7 +111,7 @@ class WebRtcCallFactory { // WebRtcVideoEngine2 is used for the new native WebRTC Video API (webrtc:1667). class WebRtcVideoEngine2 : public sigslot::has_slots<> { public: - WebRtcVideoEngine2(); + explicit WebRtcVideoEngine2(WebRtcVoiceEngine* voice_engine); virtual ~WebRtcVideoEngine2(); // Used for testing to be able to check and use the webrtc::Call config. @@ -145,9 +145,6 @@ class WebRtcVideoEngine2 : public sigslot::has_slots<> { // This is currently ignored. sigslot::repeater2 SignalCaptureStateChange; - // Set the VoiceEngine for A/V sync. This can only be called before Init. - bool SetVoiceEngine(WebRtcVoiceEngine* voice_engine); - bool FindCodec(const VideoCodec& in); bool CanSendCodec(const VideoCodec& in, const VideoCodec& current, diff --git a/talk/media/webrtc/webrtcvideoengine2_unittest.cc b/talk/media/webrtc/webrtcvideoengine2_unittest.cc index 261bc78670..3c4f112a43 100644 --- a/talk/media/webrtc/webrtcvideoengine2_unittest.cc +++ b/talk/media/webrtc/webrtcvideoengine2_unittest.cc @@ -363,7 +363,9 @@ void FakeCall::SignalNetworkState(webrtc::Call::NetworkState state) { class WebRtcVideoEngine2Test : public ::testing::Test { public: - WebRtcVideoEngine2Test() { + WebRtcVideoEngine2Test() : WebRtcVideoEngine2Test(nullptr) {} + WebRtcVideoEngine2Test(WebRtcVoiceEngine* voice_engine) + : engine_(voice_engine) { std::vector engine_codecs = engine_.codecs(); assert(!engine_codecs.empty()); bool codec_set = false; @@ -408,6 +410,9 @@ class WebRtcVideoEngine2Test : public ::testing::Test { cricket::WebRtcVideoDecoderFactory* decoder_factory, const std::vector& codecs); + // Used in WebRtcVideoEngine2VoiceTest, but defined here so it's properly + // initialized when the constructor is called. + WebRtcVoiceEngine voice_engine_; WebRtcVideoEngine2 engine_; VideoCodec default_codec_; VideoCodec default_red_codec_; @@ -415,17 +420,20 @@ class WebRtcVideoEngine2Test : public ::testing::Test { VideoCodec default_rtx_codec_; }; -TEST_F(WebRtcVideoEngine2Test, ConfiguresAvSyncForFirstReceiveChannel) { +class WebRtcVideoEngine2VoiceTest : public WebRtcVideoEngine2Test { + public: + WebRtcVideoEngine2VoiceTest() : WebRtcVideoEngine2Test(&voice_engine_) {} +}; + +TEST_F(WebRtcVideoEngine2VoiceTest, ConfiguresAvSyncForFirstReceiveChannel) { FakeCallFactory call_factory; engine_.SetCallFactory(&call_factory); - WebRtcVoiceEngine voice_engine; - engine_.SetVoiceEngine(&voice_engine); - voice_engine.Init(rtc::Thread::Current()); + voice_engine_.Init(rtc::Thread::Current()); engine_.Init(rtc::Thread::Current()); rtc::scoped_ptr voice_channel( - voice_engine.CreateChannel()); + voice_engine_.CreateChannel()); ASSERT_TRUE(voice_channel.get() != NULL); WebRtcVoiceMediaChannel* webrtc_voice_channel = static_cast(voice_channel.get()); @@ -438,8 +446,8 @@ TEST_F(WebRtcVideoEngine2Test, ConfiguresAvSyncForFirstReceiveChannel) { webrtc::Call::Config call_config = fake_call->GetConfig(); - ASSERT_TRUE(voice_engine.voe()->engine() != NULL); - ASSERT_EQ(voice_engine.voe()->engine(), call_config.voice_engine); + ASSERT_TRUE(voice_engine_.voe()->engine() != NULL); + ASSERT_EQ(voice_engine_.voe()->engine(), call_config.voice_engine); EXPECT_TRUE(channel->AddRecvStream(StreamParams::CreateLegacy(kSsrc))); EXPECT_TRUE(channel->AddRecvStream(StreamParams::CreateLegacy(kSsrc + 1))); @@ -2282,7 +2290,7 @@ TEST_F(WebRtcVideoChannel2Test, TranslatesSenderBitrateStatsCorrectly) { class WebRtcVideoEngine2SimulcastTest : public testing::Test { public: WebRtcVideoEngine2SimulcastTest() - : engine_codecs_(engine_.codecs()) { + : engine_(nullptr), engine_codecs_(engine_.codecs()) { assert(!engine_codecs_.empty()); bool codec_set = false;