Reland 8203 "Reducing locking in OveruseFrameDetect..."
The issue that was causing the thread checker to report error, turned out to be unrelated.
> 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
TBR=tommi@webrtc.org
BUG=
Review URL: https://webrtc-codereview.appspot.com/35029004
Cr-Commit-Position: refs/heads/master@{#8287}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8287 4adac7df-926f-26a2-2b94-8c16560cd09d
diff --git a/webrtc/video_engine/overuse_frame_detector.h b/webrtc/video_engine/overuse_frame_detector.h
index 8da22bd..969639c 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_;
@@ -146,13 +157,17 @@
int64_t last_encode_sample_ms_; // Only accessed by one thread.
- 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_);
int64_t last_sample_time_ms_; // Only accessed by one thread.
- 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);
};
