Reducing locking in OveruseFrameDetector and increasing constness.
I also added a few TODOs there to see what we can do to reduce the chance of contention.
To catch regressions, I've started using the ThreadChecker class on the processing thread but it might also be a good idea to add similar checks for other known threads such as the thread we receive frames on. I'm sure we can reduce locking even further.
BUG=2822
R=asapersson@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/33129004
Cr-Commit-Position: refs/heads/master@{#8203}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8203 4adac7df-926f-26a2-2b94-8c16560cd09d
diff --git a/webrtc/video_engine/overuse_frame_detector.cc b/webrtc/video_engine/overuse_frame_detector.cc
index aed0328..b4e1e36 100644
--- a/webrtc/video_engine/overuse_frame_detector.cc
+++ b/webrtc/video_engine/overuse_frame_detector.cc
@@ -17,9 +17,9 @@
#include <list>
#include <map>
+#include "webrtc/base/checks.h"
#include "webrtc/base/exp_filter.h"
#include "webrtc/system_wrappers/interface/clock.h"
-#include "webrtc/system_wrappers/interface/critical_section_wrapper.h"
#include "webrtc/system_wrappers/interface/logging.h"
namespace webrtc {
@@ -318,8 +318,7 @@
};
OveruseFrameDetector::OveruseFrameDetector(Clock* clock)
- : crit_(CriticalSectionWrapper::CreateCriticalSection()),
- observer_(NULL),
+ : observer_(NULL),
clock_(clock),
next_process_time_(clock_->TimeInMilliseconds()),
num_process_times_(0),
@@ -337,19 +336,20 @@
frame_queue_(new FrameQueue()),
last_sample_time_ms_(0),
capture_queue_delay_(new CaptureQueueDelay()) {
+ processing_thread_.DetachFromThread();
}
OveruseFrameDetector::~OveruseFrameDetector() {
}
void OveruseFrameDetector::SetObserver(CpuOveruseObserver* observer) {
- CriticalSectionScoped cs(crit_.get());
+ rtc::CritScope cs(&crit_);
observer_ = observer;
}
void OveruseFrameDetector::SetOptions(const CpuOveruseOptions& options) {
assert(options.min_frame_samples > 0);
- CriticalSectionScoped cs(crit_.get());
+ rtc::CritScope cs(&crit_);
if (options_.Equals(options)) {
return;
}
@@ -360,23 +360,23 @@
}
int OveruseFrameDetector::CaptureQueueDelayMsPerS() const {
- CriticalSectionScoped cs(crit_.get());
+ rtc::CritScope cs(&crit_);
return capture_queue_delay_->delay_ms();
}
int OveruseFrameDetector::LastProcessingTimeMs() const {
- CriticalSectionScoped cs(crit_.get());
+ rtc::CritScope cs(&crit_);
return frame_queue_->last_processing_time_ms();
}
int OveruseFrameDetector::FramesInQueue() const {
- CriticalSectionScoped cs(crit_.get());
+ rtc::CritScope cs(&crit_);
return frame_queue_->NumFrames();
}
void OveruseFrameDetector::GetCpuOveruseMetrics(
CpuOveruseMetrics* metrics) const {
- CriticalSectionScoped cs(crit_.get());
+ rtc::CritScope cs(&crit_);
metrics->capture_jitter_ms = static_cast<int>(capture_deltas_.StdDev() + 0.5);
metrics->avg_encode_time_ms = encode_time_->Value();
metrics->encode_rsd = 0;
@@ -385,7 +385,7 @@
}
int64_t OveruseFrameDetector::TimeUntilNextProcess() {
- CriticalSectionScoped cs(crit_.get());
+ DCHECK(processing_thread_.CalledOnValidThread());
return next_process_time_ - clock_->TimeInMilliseconds();
}
@@ -416,7 +416,7 @@
void OveruseFrameDetector::FrameCaptured(int width,
int height,
int64_t capture_time_ms) {
- CriticalSectionScoped cs(crit_.get());
+ rtc::CritScope cs(&crit_);
int64_t now = clock_->TimeInMilliseconds();
if (FrameSizeChanged(width * height) || FrameTimeoutDetected(now)) {
@@ -437,12 +437,12 @@
}
void OveruseFrameDetector::FrameProcessingStarted() {
- CriticalSectionScoped cs(crit_.get());
+ rtc::CritScope cs(&crit_);
capture_queue_delay_->FrameProcessingStarted(clock_->TimeInMilliseconds());
}
void OveruseFrameDetector::FrameEncoded(int encode_time_ms) {
- CriticalSectionScoped cs(crit_.get());
+ rtc::CritScope cs(&crit_);
int64_t now = clock_->TimeInMilliseconds();
if (last_encode_sample_ms_ != 0) {
int64_t diff_ms = now - last_encode_sample_ms_;
@@ -456,7 +456,7 @@
}
void OveruseFrameDetector::FrameSent(int64_t capture_time_ms) {
- CriticalSectionScoped cs(crit_.get());
+ rtc::CritScope cs(&crit_);
if (!options_.enable_extended_processing_usage) {
return;
}
@@ -477,7 +477,7 @@
}
int32_t OveruseFrameDetector::Process() {
- CriticalSectionScoped cs(crit_.get());
+ DCHECK(processing_thread_.CalledOnValidThread());
int64_t now = clock_->TimeInMilliseconds();
@@ -487,6 +487,8 @@
int64_t diff_ms = now - next_process_time_ + kProcessIntervalMs;
next_process_time_ = now + kProcessIntervalMs;
+
+ rtc::CritScope cs(&crit_);
++num_process_times_;
capture_queue_delay_->CalculateDelayChange(diff_ms);
diff --git a/webrtc/video_engine/overuse_frame_detector.h b/webrtc/video_engine/overuse_frame_detector.h
index 2c1cd13..57758e2 100644
--- a/webrtc/video_engine/overuse_frame_detector.h
+++ b/webrtc/video_engine/overuse_frame_detector.h
@@ -12,8 +12,10 @@
#define WEBRTC_VIDEO_ENGINE_OVERUSE_FRAME_DETECTOR_H_
#include "webrtc/base/constructormagic.h"
+#include "webrtc/base/criticalsection.h"
#include "webrtc/base/exp_filter.h"
#include "webrtc/base/thread_annotations.h"
+#include "webrtc/base/thread_checker.h"
#include "webrtc/modules/interface/module.h"
#include "webrtc/system_wrappers/interface/scoped_ptr.h"
#include "webrtc/video_engine/include/vie_base.h"
@@ -22,7 +24,6 @@
class Clock;
class CpuOveruseObserver;
-class CriticalSectionWrapper;
// TODO(pbos): Move this somewhere appropriate.
class Statistics {
@@ -108,8 +109,14 @@
class CaptureQueueDelay;
class FrameQueue;
+ // TODO(asapersson): This method is only used on one thread, so it shouldn't
+ // need a guard.
void AddProcessingTime(int elapsed_ms) EXCLUSIVE_LOCKS_REQUIRED(crit_);
+ // TODO(asapersson): This method is always called on the processing thread.
+ // If locking is required, consider doing that locking inside the
+ // implementation and reduce scope as much as possible. We should also
+ // see if we can avoid calling out to other methods while holding the lock.
bool IsOverusing() EXCLUSIVE_LOCKS_REQUIRED(crit_);
bool IsUnderusing(int64_t time_now) EXCLUSIVE_LOCKS_REQUIRED(crit_);
@@ -118,8 +125,11 @@
void ResetAll(int num_pixels) EXCLUSIVE_LOCKS_REQUIRED(crit_);
- // Protecting all members.
- scoped_ptr<CriticalSectionWrapper> crit_;
+ // Protecting all members except const and those that are only accessed on the
+ // processing thread.
+ // TODO(asapersson): See if we can reduce locking. As is, video frame
+ // processing contends with reading stats and the processing thread.
+ mutable rtc::CriticalSection crit_;
// Observer getting overuse reports.
CpuOveruseObserver* observer_ GUARDED_BY(crit_);
@@ -127,12 +137,13 @@
CpuOveruseOptions options_ GUARDED_BY(crit_);
Clock* const clock_;
- int64_t next_process_time_;
+ int64_t next_process_time_; // Only accessed on the processing thread.
int64_t num_process_times_ GUARDED_BY(crit_);
Statistics capture_deltas_ GUARDED_BY(crit_);
int64_t last_capture_time_ GUARDED_BY(crit_);
+ // These six members are only accessed on the processing thread.
int64_t last_overuse_time_;
int checks_above_threshold_;
int num_overuse_detections_;
@@ -145,12 +156,19 @@
int num_pixels_ GUARDED_BY(crit_);
int64_t last_encode_sample_ms_ GUARDED_BY(crit_);
- scoped_ptr<EncodeTimeAvg> encode_time_ GUARDED_BY(crit_);
- scoped_ptr<SendProcessingUsage> usage_ GUARDED_BY(crit_);
- scoped_ptr<FrameQueue> frame_queue_ GUARDED_BY(crit_);
+ // TODO(asapersson): Can these be regular members (avoid separate heap
+ // allocs)?
+ const scoped_ptr<EncodeTimeAvg> encode_time_ GUARDED_BY(crit_);
+ const scoped_ptr<SendProcessingUsage> usage_ GUARDED_BY(crit_);
+ const scoped_ptr<FrameQueue> frame_queue_ GUARDED_BY(crit_);
+
+ // TODO(asapersson): This variable is only used on one thread, so it shouldn't
+ // need a guard.
int64_t last_sample_time_ms_ GUARDED_BY(crit_);
- scoped_ptr<CaptureQueueDelay> capture_queue_delay_ GUARDED_BY(crit_);
+ const scoped_ptr<CaptureQueueDelay> capture_queue_delay_ GUARDED_BY(crit_);
+
+ rtc::ThreadChecker processing_thread_;
DISALLOW_COPY_AND_ASSIGN(OveruseFrameDetector);
};
