Remove lock in ViEFrameProviderBase::IsFrameCallbackRegistered.
After looking at how this class is used I discovered that the threading model isn't completely crazy and that we can remove a lock from a method that's causing the most contention in getStats right now (and probably many other places). IsFrameCallbackRegistered() looks innocent enough, but it contented with the frame encoder, which can make it wait for tens of milliseconds, unnecessarily.
In addition to removing the lock, I:
* Documented the threading model
* Added checks to guard against regressions
* Reduced lock scope in the Deregister function (not calling out to the notification while holding the lock).
* made virtual functions non-virtual (since they're not really being overriden) and removed unnecessary method that, if called by other threads than the ctor thread, returned a value that wasn't necessarily correct.
BUG=2822
R=mflodman@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/40859004
Cr-Commit-Position: refs/heads/master@{#8531}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8531 4adac7df-926f-26a2-2b94-8c16560cd09d
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 c584988..0ee26a5 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 @@
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 5b2c6b4..908345d 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 @@
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 0c3ebde..cb55eb6 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 @@
// (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 2240509..9ae4d4f 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 @@
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 4d032ef..3a94e05 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 @@
// ***************************************************************
// 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 51ed85b..de8d2d1 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 @@
//***************************************************************
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 @@
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 @@
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 @@
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 f4faa30..c30fbf3 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 @@
// 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 @@
// 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 822a4e5..98f290e 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 @@
}
VideoPacket* newPacket = new VideoPacket();
+ assert(len <= sizeof(newPacket->packetBuffer));
memcpy(newPacket->packetBuffer, data, len);
if (_temporalLayers)
@@ -290,6 +291,7 @@
_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 06f85df..e777144 100644
--- a/webrtc/video_engine/vie_capturer.cc
+++ b/webrtc/video_engine/vie_capturer.cc
@@ -554,17 +554,6 @@
ViEFrameProviderBase::DeliverFrame(video_frame, std::vector<uint32_t>());
}
-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 ae43a4b..dcf67df 100644
--- a/webrtc/video_engine/vie_capturer.h
+++ b/webrtc/video_engine/vie_capturer.h
@@ -67,8 +67,6 @@
// 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 fe1b3b1..ca1f25f 100644
--- a/webrtc/video_engine/vie_capturer_unittest.cc
+++ b/webrtc/video_engine/vie_capturer_unittest.cc
@@ -81,6 +81,7 @@
}
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 d442cd1..c7493a4 100644
--- a/webrtc/video_engine/vie_frame_provider_base.cc
+++ b/webrtc/video_engine/vie_frame_provider_base.cc
@@ -12,6 +12,7 @@
#include <algorithm>
+#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 @@
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<uint32_t>& 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::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::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::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 fd579a4..b3ace3b 100644
--- a/webrtc/video_engine/vie_frame_provider_base.h
+++ b/webrtc/video_engine/vie_frame_provider_base.h
@@ -14,6 +14,7 @@
#include <vector>
#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 @@
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 @@
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<ViEFrameCallback*> FrameCallbacks;
FrameCallbacks frame_callbacks_;
- rtc::scoped_ptr<CriticalSectionWrapper> provider_cs_;
+ const rtc::scoped_ptr<CriticalSectionWrapper> provider_cs_;
private:
rtc::scoped_ptr<I420VideoFrame> extra_frame_;
diff --git a/webrtc/video_engine/vie_input_manager.cc b/webrtc/video_engine/vie_input_manager.cc
index c790e38..aa64f23 100644
--- a/webrtc/video_engine/vie_input_manager.cc
+++ b/webrtc/video_engine/vie_input_manager.cc
@@ -252,12 +252,6 @@
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
