From 7a37bfc2407c59b55eff9da19743d4479db5db91 Mon Sep 17 00:00:00 2001 From: "tommi@webrtc.org" Date: Thu, 29 Jan 2015 16:08:20 +0000 Subject: [PATCH] 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 | 32 ++++++++--------- webrtc/video_engine/overuse_frame_detector.h | 34 +++++-------------- 2 files changed, 23 insertions(+), 43 deletions(-) diff --git a/webrtc/video_engine/overuse_frame_detector.cc b/webrtc/video_engine/overuse_frame_detector.cc index b4e1e36b0..aed03284c 100644 --- a/webrtc/video_engine/overuse_frame_detector.cc +++ b/webrtc/video_engine/overuse_frame_detector.cc @@ -17,9 +17,9 @@ #include #include -#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(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); diff --git a/webrtc/video_engine/overuse_frame_detector.h b/webrtc/video_engine/overuse_frame_detector.h index 57758e2ed..2c1cd1395 100644 --- a/webrtc/video_engine/overuse_frame_detector.h +++ b/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 - // 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_; + // Protecting all members. + scoped_ptr 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 - // allocs)? - const scoped_ptr encode_time_ GUARDED_BY(crit_); - const scoped_ptr usage_ GUARDED_BY(crit_); - const scoped_ptr frame_queue_ GUARDED_BY(crit_); - - // TODO(asapersson): This variable is only used on one thread, so it shouldn't - // need a guard. + scoped_ptr encode_time_ GUARDED_BY(crit_); + scoped_ptr usage_ GUARDED_BY(crit_); + scoped_ptr frame_queue_ GUARDED_BY(crit_); int64_t last_sample_time_ms_ GUARDED_BY(crit_); - const scoped_ptr capture_queue_delay_ GUARDED_BY(crit_); - - rtc::ThreadChecker processing_thread_; + scoped_ptr capture_queue_delay_ GUARDED_BY(crit_); DISALLOW_COPY_AND_ASSIGN(OveruseFrameDetector); };