Revert 8203 "Reducing locking in OveruseFrameDetector and increa..."

Broke tests in Chrome for some reason:

[ RUN      ] WebRtcAecDumpBrowserTest.CallWithAecDump
[80131:1287:0129/074432:30561723987517:ERROR:vt_video_decode_accelerator.cc(132)] Failed to create VTDecompressionSession: codecOpenErr (-8973)
[80129:1287:0129/074432:30562276677373:INFO:CONSOLE(64)] "Looking at video in element remote-view-1", source: http://127.0.0.1:61401/media/webrtc_test_utilities.js (64)
[80129:1287:0129/074432:30562281435788:INFO:CONSOLE(64)] "Looking at video in element remote-view-2", source: http://127.0.0.1:61401/media/webrtc_test_utilities.js (64)
[80129:1287:0129/074432:30562315329399:INFO:CONSOLE(800)] "Negotiating call...", source: http://127.0.0.1:61401/media/peerconnection-call.html (800)
[80133:29187:0129/074432:30562402039578:FATAL:overuse_frame_detector.cc(388)] Check failed: processing_thread_.CalledOnValidThread().
0   libbase.dylib                       0x000000010dfd688f base::debug::StackTrace::StackTrace() + 47
1   libbase.dylib                       0x000000010dfd68e3 base::debug::StackTrace::StackTrace() + 35
2   libbase.dylib                       0x000000010e030076 logging::LogMessage::~LogMessage() + 70
3   libbase.dylib                       0x000000010e02f0c3 logging::LogMessage::~LogMessage() + 35
4   libcontent.dylib                    0x000000011d8c0cd5 webrtc::OveruseFrameDetector::TimeUntilNextProcess() + 245
5   libcontent.dylib                    0x000000011d31ddfd webrtc::ProcessThreadImpl::Process() + 525
6   libcontent.dylib                    0x000000011d31d836 webrtc::ProcessThreadImpl::Run(void*) + 38
7   libcontent.dylib                    0x000000011d10c390 webrtc::ThreadPosix::Run() + 288
8   libcontent.dylib                    0x000000011d10c076 webrtc::StartThread(void*) + 38
9   libsystem_pthread.dylib             0x00007fff8e667899 _pthread_body + 138
10  libsystem_pthread.dylib             0x00007fff8e66772a _pthread_struct_init + 0
11  libsystem_pthread.dylib             0x00007fff8e66bfc9 thread_start + 13


> 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

TBR=tommi@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/34079004

Cr-Commit-Position: refs/heads/master@{#8206}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8206 4adac7df-926f-26a2-2b94-8c16560cd09d

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,7 +318,8 @@ class OveruseFrameDetector::CaptureQueueDelay {
 };
 
 OveruseFrameDetector::OveruseFrameDetector(Clock* clock)
-    : observer_(NULL),
+    : crit_(CriticalSectionWrapper::CreateCriticalSection()),
+      observer_(NULL),
       clock_(clock),
       next_process_time_(clock_->TimeInMilliseconds()),
       num_process_times_(0),
@@ -336,20 +337,19 @@ OveruseFrameDetector::OveruseFrameDetector(Clock* clock)
       frame_queue_(new FrameQueue()),
       last_sample_time_ms_(0),
       capture_queue_delay_(new CaptureQueueDelay()) {
-  processing_thread_.DetachFromThread();
 }
 
 OveruseFrameDetector::~OveruseFrameDetector() {
 }
 
 void OveruseFrameDetector::SetObserver(CpuOveruseObserver* observer) {
-  rtc::CritScope cs(&crit_);
+  CriticalSectionScoped cs(crit_.get());
   observer_ = observer;
 }
 
 void OveruseFrameDetector::SetOptions(const CpuOveruseOptions& options) {
   assert(options.min_frame_samples > 0);
-  rtc::CritScope cs(&crit_);
+  CriticalSectionScoped cs(crit_.get());
   if (options_.Equals(options)) {
     return;
   }
@@ -360,23 +360,23 @@ void OveruseFrameDetector::SetOptions(const CpuOveruseOptions& options) {
 }
 
 int OveruseFrameDetector::CaptureQueueDelayMsPerS() const {
-  rtc::CritScope cs(&crit_);
+  CriticalSectionScoped cs(crit_.get());
   return capture_queue_delay_->delay_ms();
 }
 
 int OveruseFrameDetector::LastProcessingTimeMs() const {
-  rtc::CritScope cs(&crit_);
+  CriticalSectionScoped cs(crit_.get());
   return frame_queue_->last_processing_time_ms();
 }
 
 int OveruseFrameDetector::FramesInQueue() const {
-  rtc::CritScope cs(&crit_);
+  CriticalSectionScoped cs(crit_.get());
   return frame_queue_->NumFrames();
 }
 
 void OveruseFrameDetector::GetCpuOveruseMetrics(
     CpuOveruseMetrics* metrics) const {
-  rtc::CritScope cs(&crit_);
+  CriticalSectionScoped cs(crit_.get());
   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 @@ void OveruseFrameDetector::GetCpuOveruseMetrics(
 }
 
 int64_t OveruseFrameDetector::TimeUntilNextProcess() {
-  DCHECK(processing_thread_.CalledOnValidThread());
+  CriticalSectionScoped cs(crit_.get());
   return next_process_time_ - clock_->TimeInMilliseconds();
 }
 
@@ -416,7 +416,7 @@ void OveruseFrameDetector::ResetAll(int num_pixels) {
 void OveruseFrameDetector::FrameCaptured(int width,
                                          int height,
                                          int64_t capture_time_ms) {
-  rtc::CritScope cs(&crit_);
+  CriticalSectionScoped cs(crit_.get());
 
   int64_t now = clock_->TimeInMilliseconds();
   if (FrameSizeChanged(width * height) || FrameTimeoutDetected(now)) {
@@ -437,12 +437,12 @@ void OveruseFrameDetector::FrameCaptured(int width,
 }
 
 void OveruseFrameDetector::FrameProcessingStarted() {
-  rtc::CritScope cs(&crit_);
+  CriticalSectionScoped cs(crit_.get());
   capture_queue_delay_->FrameProcessingStarted(clock_->TimeInMilliseconds());
 }
 
 void OveruseFrameDetector::FrameEncoded(int encode_time_ms) {
-  rtc::CritScope cs(&crit_);
+  CriticalSectionScoped cs(crit_.get());
   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::FrameEncoded(int encode_time_ms) {
 }
 
 void OveruseFrameDetector::FrameSent(int64_t capture_time_ms) {
-  rtc::CritScope cs(&crit_);
+  CriticalSectionScoped cs(crit_.get());
   if (!options_.enable_extended_processing_usage) {
     return;
   }
@@ -477,7 +477,7 @@ void OveruseFrameDetector::AddProcessingTime(int elapsed_ms) {
 }
 
 int32_t OveruseFrameDetector::Process() {
-  DCHECK(processing_thread_.CalledOnValidThread());
+  CriticalSectionScoped cs(crit_.get());
 
   int64_t now = clock_->TimeInMilliseconds();
 
@@ -487,8 +487,6 @@ int32_t OveruseFrameDetector::Process() {
 
   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);

webrtc/video_engine/overuse_frame_detector.h

@@ -12,10 +12,8 @@
 #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"
@@ -24,6 +22,7 @@ namespace webrtc {
 
 class Clock;
 class CpuOveruseObserver;
+class CriticalSectionWrapper;
 
 // TODO(pbos): Move this somewhere appropriate.
 class Statistics {
@@ -109,14 +108,8 @@ class OveruseFrameDetector : public Module {
   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_);
 
@@ -125,11 +118,8 @@ class OveruseFrameDetector : public Module {
 
   void ResetAll(int num_pixels) EXCLUSIVE_LOCKS_REQUIRED(crit_);
 
-  // Protecting all members except const and those that are only accessed on the
+  // Protecting all members.
-  // processing thread.
+  scoped_ptr<CriticalSectionWrapper> crit_;
-  // 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_);
@@ -137,13 +127,12 @@ class OveruseFrameDetector : public Module {
   CpuOveruseOptions options_ GUARDED_BY(crit_);
 
   Clock* const clock_;
-  int64_t next_process_time_;  // Only accessed on the processing thread.
+  int64_t next_process_time_;
   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_;
@@ -156,19 +145,12 @@ class OveruseFrameDetector : public Module {
   int num_pixels_ GUARDED_BY(crit_);
 
   int64_t last_encode_sample_ms_ GUARDED_BY(crit_);
-  // TODO(asapersson): Can these be regular members (avoid separate heap
+  scoped_ptr<EncodeTimeAvg> encode_time_ GUARDED_BY(crit_);
-  // allocs)?
+  scoped_ptr<SendProcessingUsage> usage_ GUARDED_BY(crit_);
-  const scoped_ptr<EncodeTimeAvg> encode_time_ GUARDED_BY(crit_);
+  scoped_ptr<FrameQueue> frame_queue_ 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_);
 
-  const scoped_ptr<CaptureQueueDelay> capture_queue_delay_ GUARDED_BY(crit_);
+  scoped_ptr<CaptureQueueDelay> capture_queue_delay_ GUARDED_BY(crit_);
-
-  rtc::ThreadChecker processing_thread_;
 
   DISALLOW_COPY_AND_ASSIGN(OveruseFrameDetector);
 };