diff --git a/webrtc/video_engine/test/auto_test/interface/vie_autotest.h b/webrtc/video_engine/test/auto_test/interface/vie_autotest.h index c584988adf..0ee26a5732 100644 --- a/webrtc/video_engine/test/auto_test/interface/vie_autotest.h +++ b/webrtc/video_engine/test/auto_test/interface/vie_autotest.h @@ -112,6 +112,13 @@ private: TbVideoChannel* video_channel, TbCaptureDevice* capture_device); + // Stops rendering into the two windows as was set up by a call to + // RenderCaptureDeviceAndOutputStream. + void StopRenderCaptureDeviceAndOutputStream( + TbInterfaces* video_engine, + TbVideoChannel* video_channel, + TbCaptureDevice* capture_device); + void* _window1; void* _window2; diff --git a/webrtc/video_engine/test/auto_test/primitives/general_primitives.cc b/webrtc/video_engine/test/auto_test/primitives/general_primitives.cc index 5b2c6b45a9..908345d14e 100644 --- a/webrtc/video_engine/test/auto_test/primitives/general_primitives.cc +++ b/webrtc/video_engine/test/auto_test/primitives/general_primitives.cc @@ -65,6 +65,12 @@ void RenderInWindow(webrtc::ViERender* video_render_interface, EXPECT_EQ(0, video_render_interface->StartRender(frame_provider_id)); } +void StopRenderInWindow(webrtc::ViERender* video_render_interface, + int frame_provider_id) { + EXPECT_EQ(0, video_render_interface->StopRender(frame_provider_id)); + EXPECT_EQ(0, video_render_interface->RemoveRenderer(frame_provider_id)); +} + void RenderToFile(webrtc::ViERender* renderer_interface, int frame_provider_id, ViEToFileRenderer *to_file_renderer) { diff --git a/webrtc/video_engine/test/auto_test/primitives/general_primitives.h b/webrtc/video_engine/test/auto_test/primitives/general_primitives.h index 0c3ebdeeee..cb55eb610a 100644 --- a/webrtc/video_engine/test/auto_test/primitives/general_primitives.h +++ b/webrtc/video_engine/test/auto_test/primitives/general_primitives.h @@ -48,11 +48,17 @@ void FindCaptureDeviceOnSystem(webrtc::ViECapture* capture, // (See vie_window_manager_factory.h for more details on how to make one of // those). The frame provider id is a source of video frames, for instance // a capture device or a video channel. +// NOTE: A call to StopRenderInWindow needs to be done in order to clear +// up the configuration applied by this function. void RenderInWindow(webrtc::ViERender* video_render_interface, - int frame_provider_id, + int frame_provider_id, void* os_window, float z_index); +// Stops rendering into a window as previously set up by calling RenderInWindow. +void StopRenderInWindow(webrtc::ViERender* video_render_interface, + int frame_provider_id); + // Similar in function to RenderInWindow, this function instead renders to // a file using a to-file-renderer. The frame provider id is a source of // video frames, for instance a capture device or a video channel. diff --git a/webrtc/video_engine/test/auto_test/source/vie_autotest.cc b/webrtc/video_engine/test/auto_test/source/vie_autotest.cc index 2240509647..9ae4d4fce7 100644 --- a/webrtc/video_engine/test/auto_test/source/vie_autotest.cc +++ b/webrtc/video_engine/test/auto_test/source/vie_autotest.cc @@ -152,3 +152,11 @@ void ViEAutoTest::RenderCaptureDeviceAndOutputStream( RenderInWindow( video_engine->render, video_channel->videoChannel, _window2, 1); } + +void ViEAutoTest::StopRenderCaptureDeviceAndOutputStream( + TbInterfaces* video_engine, + TbVideoChannel* video_channel, + TbCaptureDevice* capture_device) { + StopRenderInWindow(video_engine->render, capture_device->captureId); + StopRenderInWindow(video_engine->render, video_channel->videoChannel); +} diff --git a/webrtc/video_engine/test/auto_test/source/vie_autotest_base.cc b/webrtc/video_engine/test/auto_test/source/vie_autotest_base.cc index 4d032ef5d8..3a94e051be 100644 --- a/webrtc/video_engine/test/auto_test/source/vie_autotest_base.cc +++ b/webrtc/video_engine/test/auto_test/source/vie_autotest_base.cc @@ -73,6 +73,7 @@ void ViEAutoTest::ViEBaseStandardTest() { // *************************************************************** // Testing finished. Tear down Video Engine // *************************************************************** + EXPECT_EQ(0, capture_interface->DisconnectCaptureDevice(video_channel)); EXPECT_EQ(0, capture_interface->StopCapture(capture_id)); EXPECT_EQ(0, base_interface->StopReceive(video_channel)); diff --git a/webrtc/video_engine/test/auto_test/source/vie_autotest_image_process.cc b/webrtc/video_engine/test/auto_test/source/vie_autotest_image_process.cc index 51ed85bf43..de8d2d1dea 100644 --- a/webrtc/video_engine/test/auto_test/source/vie_autotest_image_process.cc +++ b/webrtc/video_engine/test/auto_test/source/vie_autotest_image_process.cc @@ -48,7 +48,7 @@ void ViEAutoTest::ViEImageProcessStandardTest() //*************************************************************** int rtpPort = 6000; // Create VIE - TbInterfaces ViE("ViEImageProcessAPITest"); + TbInterfaces ViE("ViEImageProcessStandardTest"); // Create a video channel TbVideoChannel tbChannel(ViE, webrtc::kVideoCodecVP8); // Create a capture device @@ -89,8 +89,9 @@ void ViEAutoTest::ViEImageProcessStandardTest() ViETest::Log("Only Window 2 should be black and white"); AutoTestSleep(kAutoTestSleepTimeMs); - EXPECT_EQ(0, ViE.render->StopRender(tbCapture.captureId)); - EXPECT_EQ(0, ViE.render->RemoveRenderer(tbCapture.captureId)); + StopRenderCaptureDeviceAndOutputStream(&ViE, &tbChannel, &tbCapture); + + tbCapture.Disconnect(tbChannel.videoChannel); int rtpPort2 = rtpPort + 100; // Create a video channel @@ -130,6 +131,10 @@ void ViEAutoTest::ViEImageProcessStandardTest() EXPECT_EQ(0, ViE.image_process->DeregisterSendEffectFilter( tbChannel.videoChannel)); + EXPECT_EQ(0, ViE.render->RemoveRenderer(tbChannel2.videoChannel)); + + tbCapture.Disconnect(tbChannel2.videoChannel); + //*************************************************************** // Testing finished. Tear down Video Engine //*************************************************************** @@ -222,4 +227,6 @@ void ViEAutoTest::ViEImageProcessAPITest() tbChannel.videoChannel, false)); EXPECT_NE(0, ViE.image_process->EnableColorEnhancement( tbCapture.captureId, true)); + + tbCapture.Disconnect(tbChannel.videoChannel); } diff --git a/webrtc/video_engine/test/auto_test/source/vie_autotest_render.cc b/webrtc/video_engine/test/auto_test/source/vie_autotest_render.cc index f4faa30803..c30fbf32a4 100644 --- a/webrtc/video_engine/test/auto_test/source/vie_autotest_render.cc +++ b/webrtc/video_engine/test/auto_test/source/vie_autotest_render.cc @@ -297,6 +297,7 @@ void ViEAutoTest::ViERenderAPITest() { // Already started. EXPECT_EQ(-1, ViE.render->SetExpectedRenderDelay(tbChannel.videoChannel, 50)); EXPECT_EQ(0, ViE.render->StopRender(tbChannel.videoChannel)); + // Invalid values. EXPECT_EQ(-1, ViE.render->SetExpectedRenderDelay(tbChannel.videoChannel, 9)); EXPECT_EQ(-1, ViE.render->SetExpectedRenderDelay(tbChannel.videoChannel, @@ -304,4 +305,8 @@ void ViEAutoTest::ViERenderAPITest() { // Valid values. EXPECT_EQ(0, ViE.render->SetExpectedRenderDelay(tbChannel.videoChannel, 11)); EXPECT_EQ(0, ViE.render->SetExpectedRenderDelay(tbChannel.videoChannel, 499)); + + EXPECT_EQ(0, ViE.render->RemoveRenderer(tbChannel.videoChannel)); + EXPECT_EQ(0, ViE.render->RemoveRenderer(tbCapture.captureId)); + tbCapture.Disconnect(tbChannel.videoChannel); } diff --git a/webrtc/video_engine/test/libvietest/testbed/tb_external_transport.cc b/webrtc/video_engine/test/libvietest/testbed/tb_external_transport.cc index 822a4e5ddc..98f290ecb9 100644 --- a/webrtc/video_engine/test/libvietest/testbed/tb_external_transport.cc +++ b/webrtc/video_engine/test/libvietest/testbed/tb_external_transport.cc @@ -227,6 +227,7 @@ int TbExternalTransport::SendPacket(int channel, const void *data, size_t len) } VideoPacket* newPacket = new VideoPacket(); + assert(len <= sizeof(newPacket->packetBuffer)); memcpy(newPacket->packetBuffer, data, len); if (_temporalLayers) @@ -290,6 +291,7 @@ int TbExternalTransport::SendRTCPPacket(int channel, _statCrit.Leave(); VideoPacket* newPacket = new VideoPacket(); + assert(len <= sizeof(newPacket->packetBuffer)); memcpy(newPacket->packetBuffer, data, len); newPacket->length = len; newPacket->channel = channel; diff --git a/webrtc/video_engine/vie_capturer.cc b/webrtc/video_engine/vie_capturer.cc index 06f85df2ff..e777144bdb 100644 --- a/webrtc/video_engine/vie_capturer.cc +++ b/webrtc/video_engine/vie_capturer.cc @@ -554,17 +554,6 @@ void ViECapturer::DeliverI420Frame(I420VideoFrame* video_frame) { ViEFrameProviderBase::DeliverFrame(video_frame, std::vector()); } -int ViECapturer::DeregisterFrameCallback( - const ViEFrameCallback* callbackObject) { - return ViEFrameProviderBase::DeregisterFrameCallback(callbackObject); -} - -bool ViECapturer::IsFrameCallbackRegistered( - const ViEFrameCallback* callbackObject) { - CriticalSectionScoped cs(provider_cs_.get()); - return ViEFrameProviderBase::IsFrameCallbackRegistered(callbackObject); -} - bool ViECapturer::CaptureCapabilityFixed() { return requested_capability_.width != 0 && requested_capability_.height != 0 && diff --git a/webrtc/video_engine/vie_capturer.h b/webrtc/video_engine/vie_capturer.h index ae43a4be65..dcf67df0d1 100644 --- a/webrtc/video_engine/vie_capturer.h +++ b/webrtc/video_engine/vie_capturer.h @@ -67,8 +67,6 @@ class ViECapturer // Implements ViEFrameProviderBase. int FrameCallbackChanged(); - virtual int DeregisterFrameCallback(const ViEFrameCallback* callbackObject); - bool IsFrameCallbackRegistered(const ViEFrameCallback* callbackObject); // Implements ExternalCapture. virtual int IncomingFrame(unsigned char* video_frame, diff --git a/webrtc/video_engine/vie_capturer_unittest.cc b/webrtc/video_engine/vie_capturer_unittest.cc index fe1b3b1b10..ca1f25fcec 100644 --- a/webrtc/video_engine/vie_capturer_unittest.cc +++ b/webrtc/video_engine/vie_capturer_unittest.cc @@ -81,6 +81,7 @@ class ViECapturerTest : public ::testing::Test { } virtual void TearDown() { + vie_capturer_->DeregisterFrameCallback(mock_frame_callback_.get()); // ViECapturer accesses |mock_process_thread_| in destructor and should // be deleted first. vie_capturer_.reset(); diff --git a/webrtc/video_engine/vie_frame_provider_base.cc b/webrtc/video_engine/vie_frame_provider_base.cc index d442cd1626..c7493a4ff2 100644 --- a/webrtc/video_engine/vie_frame_provider_base.cc +++ b/webrtc/video_engine/vie_frame_provider_base.cc @@ -12,6 +12,7 @@ #include +#include "webrtc/base/checks.h" #include "webrtc/common_video/interface/i420_video_frame.h" #include "webrtc/system_wrappers/interface/critical_section_wrapper.h" #include "webrtc/system_wrappers/interface/logging.h" @@ -25,50 +26,49 @@ ViEFrameProviderBase::ViEFrameProviderBase(int Id, int engine_id) engine_id_(engine_id), provider_cs_(CriticalSectionWrapper::CreateCriticalSection()), frame_delay_(0) { + frame_delivery_thread_checker_.DetachFromThread(); } ViEFrameProviderBase::~ViEFrameProviderBase() { - if (frame_callbacks_.size() > 0) { - LOG_F(LS_WARNING) << "FrameCallbacks still exist when Provider deleted: " - << frame_callbacks_.size(); - } + DCHECK(thread_checker_.CalledOnValidThread()); + DCHECK(frame_callbacks_.empty()); - for (FrameCallbacks::iterator it = frame_callbacks_.begin(); - it != frame_callbacks_.end(); ++it) { - (*it)->ProviderDestroyed(id_); + // TODO(tommi): Remove this when we're confident we've fixed the places where + // cleanup wasn't being done. + for (ViEFrameCallback* callback : frame_callbacks_) { + LOG_F(LS_WARNING) << "FrameCallback still registered."; + callback->ProviderDestroyed(id_); } - frame_callbacks_.clear(); } -int ViEFrameProviderBase::Id() { +int ViEFrameProviderBase::Id() const { return id_; } void ViEFrameProviderBase::DeliverFrame(I420VideoFrame* video_frame, const std::vector& csrcs) { + DCHECK(frame_delivery_thread_checker_.CalledOnValidThread()); #ifdef DEBUG_ const TickTime start_process_time = TickTime::Now(); #endif CriticalSectionScoped cs(provider_cs_.get()); // Deliver the frame to all registered callbacks. - if (frame_callbacks_.size() > 0) { - if (frame_callbacks_.size() == 1) { - // We don't have to copy the frame. - frame_callbacks_.front()->DeliverFrame(id_, video_frame, csrcs); - } else { - for (FrameCallbacks::iterator it = frame_callbacks_.begin(); - it != frame_callbacks_.end(); ++it) { - if (video_frame->native_handle() != NULL) { - (*it)->DeliverFrame(id_, video_frame, csrcs); - } else { - // Make a copy of the frame for all callbacks. - if (!extra_frame_.get()) { - extra_frame_.reset(new I420VideoFrame()); - } - extra_frame_->CopyFrame(*video_frame); - (*it)->DeliverFrame(id_, extra_frame_.get(), csrcs); + if (frame_callbacks_.size() == 1) { + // We don't have to copy the frame. + frame_callbacks_.front()->DeliverFrame(id_, video_frame, csrcs); + } else { + for (ViEFrameCallback* callback : frame_callbacks_) { + if (video_frame->native_handle() != NULL) { + callback->DeliverFrame(id_, video_frame, csrcs); + } else { + // Make a copy of the frame for all callbacks. + if (!extra_frame_.get()) { + extra_frame_.reset(new I420VideoFrame()); } + // TODO(mflodman): We can get rid of this frame copy. + extra_frame_->CopyFrame(*video_frame); + callback->DeliverFrame(id_, extra_frame_.get(), csrcs); } } } @@ -83,34 +83,50 @@ void ViEFrameProviderBase::DeliverFrame(I420VideoFrame* video_frame, } void ViEFrameProviderBase::SetFrameDelay(int frame_delay) { + // Called on the capture thread (see OnIncomingCapturedFrame). + // To test, run ViEStandardIntegrationTest.RunsBaseTestWithoutErrors + // in vie_auto_tests. + // In the same test, it appears that it's also called on a thread that's + // neither the ctor thread nor the capture thread. CriticalSectionScoped cs(provider_cs_.get()); frame_delay_ = frame_delay; - for (FrameCallbacks::iterator it = frame_callbacks_.begin(); - it != frame_callbacks_.end(); ++it) { - (*it)->DelayChanged(id_, frame_delay); + for (ViEFrameCallback* callback : frame_callbacks_) { + callback->DelayChanged(id_, frame_delay); } } int ViEFrameProviderBase::FrameDelay() { + // Called on the default thread in WebRtcVideoMediaChannelTest.SetSend + // (libjingle_media_unittest). + + // Called on neither the ctor thread nor the capture thread in + // BitrateEstimatorTest.ImmediatelySwitchToAST (video_engine_tests). + + // Most of the time Called on the capture thread (see OnCaptureDelayChanged). + // To test, run ViEStandardIntegrationTest.RunsBaseTestWithoutErrors + // in vie_auto_tests. return frame_delay_; } int ViEFrameProviderBase::GetBestFormat(int* best_width, int* best_height, int* best_frame_rate) { + DCHECK(thread_checker_.CalledOnValidThread()); int largest_width = 0; int largest_height = 0; int highest_frame_rate = 0; - CriticalSectionScoped cs(provider_cs_.get()); - for (FrameCallbacks::iterator it = frame_callbacks_.begin(); - it != frame_callbacks_.end(); ++it) { + // Here we don't need to grab the provider_cs_ lock to run through the list + // of callbacks. The reason is that we know that we're currently on the same + // thread that is the only thread that will modify the callback list and + // we can be sure that the thread won't race with itself. + for (ViEFrameCallback* callback : frame_callbacks_) { int prefered_width = 0; int prefered_height = 0; int prefered_frame_rate = 0; - if ((*it)->GetPreferedFrameSettings(&prefered_width, &prefered_height, - &prefered_frame_rate) == 0) { + if (callback->GetPreferedFrameSettings(&prefered_width, &prefered_height, + &prefered_frame_rate) == 0) { if (prefered_width > largest_width) { largest_width = prefered_width; } @@ -130,12 +146,13 @@ int ViEFrameProviderBase::GetBestFormat(int* best_width, int ViEFrameProviderBase::RegisterFrameCallback( int observer_id, ViEFrameCallback* callback_object) { - assert(callback_object); + DCHECK(thread_checker_.CalledOnValidThread()); + DCHECK(callback_object); { CriticalSectionScoped cs(provider_cs_.get()); if (std::find(frame_callbacks_.begin(), frame_callbacks_.end(), callback_object) != frame_callbacks_.end()) { - assert(false && "frameObserver already registered"); + DCHECK(false && "frameObserver already registered"); return -1; } frame_callbacks_.push_back(callback_object); @@ -150,33 +167,35 @@ int ViEFrameProviderBase::RegisterFrameCallback( int ViEFrameProviderBase::DeregisterFrameCallback( const ViEFrameCallback* callback_object) { - assert(callback_object); - CriticalSectionScoped cs(provider_cs_.get()); - - FrameCallbacks::iterator it = std::find(frame_callbacks_.begin(), - frame_callbacks_.end(), - callback_object); - if (it == frame_callbacks_.end()) { - return -1; + DCHECK(thread_checker_.CalledOnValidThread()); + DCHECK(callback_object); + { + CriticalSectionScoped cs(provider_cs_.get()); + FrameCallbacks::iterator it = std::find(frame_callbacks_.begin(), + frame_callbacks_.end(), + callback_object); + if (it == frame_callbacks_.end()) { + return -1; + } + frame_callbacks_.erase(it); } - frame_callbacks_.erase(it); // Notify implementer of this class that the callback list have changed. FrameCallbackChanged(); + return 0; } bool ViEFrameProviderBase::IsFrameCallbackRegistered( const ViEFrameCallback* callback_object) { - assert(callback_object); + DCHECK(thread_checker_.CalledOnValidThread()); + DCHECK(callback_object); - CriticalSectionScoped cs(provider_cs_.get()); + // Here we don't need to grab the lock to do this lookup. + // The reason is that we know that we're currently on the same thread that + // is the only thread that will modify the callback list and subsequently the + // thread doesn't race with itself. return std::find(frame_callbacks_.begin(), frame_callbacks_.end(), callback_object) != frame_callbacks_.end(); } - -int ViEFrameProviderBase::NumberOfRegisteredFrameCallbacks() { - CriticalSectionScoped cs(provider_cs_.get()); - return frame_callbacks_.size(); -} } // namespac webrtc diff --git a/webrtc/video_engine/vie_frame_provider_base.h b/webrtc/video_engine/vie_frame_provider_base.h index fd579a45dc..b3ace3bdfc 100644 --- a/webrtc/video_engine/vie_frame_provider_base.h +++ b/webrtc/video_engine/vie_frame_provider_base.h @@ -14,6 +14,7 @@ #include #include "webrtc/base/scoped_ptr.h" +#include "webrtc/base/thread_checker.h" #include "webrtc/common_types.h" #include "webrtc/typedefs.h" @@ -55,18 +56,20 @@ class ViEFrameProviderBase { virtual ~ViEFrameProviderBase(); // Returns the frame provider id. - int Id(); + int Id() const; // Register frame callbacks, i.e. a receiver of the captured frame. - virtual int RegisterFrameCallback(int observer_id, - ViEFrameCallback* callback_object); + // Must be called on the same thread as the provider was constructed on. + int RegisterFrameCallback(int observer_id, ViEFrameCallback* callback); - virtual int DeregisterFrameCallback(const ViEFrameCallback* callback_object); + // Unregisters a previously registered callback. Returns -1 if the callback + // object hasn't been registered. + // Must be called on the same thread as the provider was constructed on. + int DeregisterFrameCallback(const ViEFrameCallback* callback); - virtual bool IsFrameCallbackRegistered( - const ViEFrameCallback* callback_object); - - int NumberOfRegisteredFrameCallbacks(); + // Determines if a callback is currently registered. + // Must be called on the same thread as the provider was constructed on. + bool IsFrameCallbackRegistered(const ViEFrameCallback* callback); // FrameCallbackChanged // Inherited classes should check for new frame_settings and reconfigure @@ -82,13 +85,16 @@ class ViEFrameProviderBase { int* best_height, int* best_frame_rate); - int id_; - int engine_id_; + rtc::ThreadChecker thread_checker_; + rtc::ThreadChecker frame_delivery_thread_checker_; + + const int id_; + const int engine_id_; // Frame callbacks. typedef std::vector FrameCallbacks; FrameCallbacks frame_callbacks_; - rtc::scoped_ptr provider_cs_; + const rtc::scoped_ptr provider_cs_; private: rtc::scoped_ptr extra_frame_; diff --git a/webrtc/video_engine/vie_input_manager.cc b/webrtc/video_engine/vie_input_manager.cc index c790e3866e..aa64f2362f 100644 --- a/webrtc/video_engine/vie_input_manager.cc +++ b/webrtc/video_engine/vie_input_manager.cc @@ -252,12 +252,6 @@ int ViEInputManager::DestroyCaptureDevice(const int capture_id) { LOG(LS_ERROR) << "No such capture device id: " << capture_id; return -1; } - uint32_t num_callbacks = - vie_capture->NumberOfRegisteredFrameCallbacks(); - if (num_callbacks > 0) { - LOG(LS_WARNING) << num_callbacks << " still registered to capture id " - << capture_id << " when destroying capture device."; - } vie_frame_provider_map_.erase(capture_id); ReturnCaptureId(capture_id); // Leave cs before deleting the capture object. This is because deleting the