- Add a SetPriority method to ThreadWrapper

- Remove 'priority' from CreateThread and related member variables from implementations
- Make supplying a name for threads, non-optional

BUG=
R=magjed@webrtc.org

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

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

11 files changed, 70 insertions, 69 deletions

diff --git a/webrtc/system_wrappers/interface/thread_wrapper.h b/webrtc/system_wrappers/interface/thread_wrapper.h
index 67ce866d18..742056198a 100644
--- a/webrtc/system_wrappers/interface/thread_wrapper.h
+++ b/webrtc/system_wrappers/interface/thread_wrapper.h
@@ -16,6 +16,10 @@
 #ifndef WEBRTC_SYSTEM_WRAPPERS_INTERFACE_THREAD_WRAPPER_H_
 #define WEBRTC_SYSTEM_WRAPPERS_INTERFACE_THREAD_WRAPPER_H_
 
+#if defined(WEBRTC_WIN)
+#include <windows.h>
+#endif
+
 #include "webrtc/base/scoped_ptr.h"
 #include "webrtc/common_types.h"
 #include "webrtc/typedefs.h"
@@ -28,11 +32,19 @@ namespace webrtc {
 typedef bool(*ThreadRunFunction)(void*);
 
 enum ThreadPriority {
+#ifdef WEBRTC_WIN
+  kLowPriority = THREAD_PRIORITY_BELOW_NORMAL,
+  kNormalPriority = THREAD_PRIORITY_NORMAL,
+  kHighPriority = THREAD_PRIORITY_ABOVE_NORMAL,
+  kHighestPriority = THREAD_PRIORITY_HIGHEST,
+  kRealtimePriority = THREAD_PRIORITY_TIME_CRITICAL
+#else
   kLowPriority = 1,
   kNormalPriority = 2,
   kHighPriority = 3,
   kHighestPriority = 4,
   kRealtimePriority = 5
+#endif
 };
 
 // Represents a simple worker thread.  The implementation must be assumed
@@ -52,11 +64,8 @@ class ThreadWrapper {
   // prio        Thread priority. May require root/admin rights.
   // thread_name  NULL terminated thread name, will be visable in the Windows
   //             debugger.
-  // TODO(tommi): Remove the priority argument and provide a setter instead.
-  // TODO(tommi): Make thread_name non-optional (i.e. no default value).
   static rtc::scoped_ptr<ThreadWrapper> CreateThread(ThreadRunFunction func,
-      void* obj, ThreadPriority prio = kNormalPriority,
-      const char* thread_name = 0);
+      void* obj, const char* thread_name);
 
   // Get the current thread's thread ID.
   // NOTE: This is a static method. It returns the id of the calling thread,
@@ -75,6 +84,10 @@ class ThreadWrapper {
   // Multiple tries to Stop are allowed (e.g. to wait longer than 2 seconds).
   // It's ok to call Stop() even if the spawned thread has been reclaimed.
   virtual bool Stop() = 0;
+
+  // Set the priority of the worker thread.  Must be called when thread
+  // is running.
+  virtual bool SetPriority(ThreadPriority priority) = 0;
 };
 
 }  // namespace webrtc
diff --git a/webrtc/system_wrappers/source/condition_variable_unittest.cc b/webrtc/system_wrappers/source/condition_variable_unittest.cc
index d6907f6475..c34c4ea0d8 100644
--- a/webrtc/system_wrappers/source/condition_variable_unittest.cc
+++ b/webrtc/system_wrappers/source/condition_variable_unittest.cc
@@ -145,7 +145,7 @@ class CondVarTest : public ::testing::Test {
 
   virtual void SetUp() {
     thread_ = ThreadWrapper::CreateThread(&WaitingRunFunction,
-                                          &baton_);
+                                          &baton_, "CondVarTest");
     ASSERT_TRUE(thread_->Start());
   }
 
diff --git a/webrtc/system_wrappers/source/critical_section_unittest.cc b/webrtc/system_wrappers/source/critical_section_unittest.cc
index 9b497b426a..ec639eb673 100644
--- a/webrtc/system_wrappers/source/critical_section_unittest.cc
+++ b/webrtc/system_wrappers/source/critical_section_unittest.cc
@@ -79,7 +79,7 @@ TEST_F(CritSectTest, ThreadWakesOnce) NO_THREAD_SAFETY_ANALYSIS {
       CriticalSectionWrapper::CreateCriticalSection();
   ProtectedCount count(crit_sect);
   rtc::scoped_ptr<ThreadWrapper> thread = ThreadWrapper::CreateThread(
-      &LockUnlockThenStopRunFunction, &count);
+      &LockUnlockThenStopRunFunction, &count, "ThreadWakesOnce");
   crit_sect->Enter();
   ASSERT_TRUE(thread->Start());
   SwitchProcess();
@@ -106,7 +106,7 @@ TEST_F(CritSectTest, ThreadWakesTwice) NO_THREAD_SAFETY_ANALYSIS {
       CriticalSectionWrapper::CreateCriticalSection();
   ProtectedCount count(crit_sect);
   rtc::scoped_ptr<ThreadWrapper> thread = ThreadWrapper::CreateThread(
-      &LockUnlockRunFunction, &count);
+      &LockUnlockRunFunction, &count, "ThreadWakesTwice");
   crit_sect->Enter();  // Make sure counter stays 0 until we wait for it.
   ASSERT_TRUE(thread->Start());
   crit_sect->Leave();
diff --git a/webrtc/system_wrappers/source/data_log.cc b/webrtc/system_wrappers/source/data_log.cc
index 7c2a91b558..653af657dd 100644
--- a/webrtc/system_wrappers/source/data_log.cc
+++ b/webrtc/system_wrappers/source/data_log.cc
@@ -349,13 +349,11 @@ int DataLogImpl::CreateLog() {
 
 int DataLogImpl::Init() {
   file_writer_thread_ = ThreadWrapper::CreateThread(
-                          DataLogImpl::Run,
-                          instance_,
-                          kHighestPriority,
-                          "DataLog");
+      DataLogImpl::Run, instance_, "DataLog");
   bool success = file_writer_thread_->Start();
   if (!success)
     return -1;
+  file_writer_thread_->SetPriority(kHighestPriority);
   return 0;
 }
 
diff --git a/webrtc/system_wrappers/source/event_posix.cc b/webrtc/system_wrappers/source/event_posix.cc
index bbfb0b03b1..6833e0e8af 100644
--- a/webrtc/system_wrappers/source/event_posix.cc
+++ b/webrtc/system_wrappers/source/event_posix.cc
@@ -154,11 +154,11 @@ bool EventPosix::StartTimer(bool periodic, unsigned long time) {
   // Start the timer thread
   timer_event_ = static_cast<EventPosix*>(EventWrapper::Create());
   const char* thread_name = "WebRtc_event_timer_thread";
-  timer_thread_ = ThreadWrapper::CreateThread(Run, this, kRealtimePriority,
-                                              thread_name);
+  timer_thread_ = ThreadWrapper::CreateThread(Run, this, thread_name);
   periodic_ = periodic;
   time_ = time;
   bool started = timer_thread_->Start();
+  timer_thread_->SetPriority(kRealtimePriority);
   pthread_mutex_unlock(&mutex_);
 
   return started;
diff --git a/webrtc/system_wrappers/source/thread.cc b/webrtc/system_wrappers/source/thread.cc
index 957388b885..b469344bd1 100644
--- a/webrtc/system_wrappers/source/thread.cc
+++ b/webrtc/system_wrappers/source/thread.cc
@@ -25,10 +25,9 @@ typedef ThreadPosix ThreadType;
 #endif
 
 rtc::scoped_ptr<ThreadWrapper> ThreadWrapper::CreateThread(
-    ThreadRunFunction func, void* obj, ThreadPriority prio,
-    const char* thread_name) {
+    ThreadRunFunction func, void* obj, const char* thread_name) {
   return rtc::scoped_ptr<ThreadWrapper>(
-      new ThreadType(func, obj, prio, thread_name)).Pass();
+      new ThreadType(func, obj, thread_name)).Pass();
 }
 
 }  // namespace webrtc
diff --git a/webrtc/system_wrappers/source/thread_posix.cc b/webrtc/system_wrappers/source/thread_posix.cc
index c60541175f..dc95cbcd73 100644
--- a/webrtc/system_wrappers/source/thread_posix.cc
+++ b/webrtc/system_wrappers/source/thread_posix.cc
@@ -69,10 +69,9 @@ void* ThreadPosix::StartThread(void* param) {
 }
 
 ThreadPosix::ThreadPosix(ThreadRunFunction func, void* obj,
-                         ThreadPriority prio, const char* thread_name)
+                         const char* thread_name)
     : run_function_(func),
       obj_(obj),
-      prio_(prio),
       stop_event_(false, false),
       name_(thread_name ? thread_name : "webrtc"),
       thread_(0) {
@@ -112,16 +111,10 @@ bool ThreadPosix::Stop() {
   return true;
 }
 
-void ThreadPosix::Run() {
-  if (!name_.empty()) {
-    // Setting the thread name may fail (harmlessly) if running inside a
-    // sandbox. Ignore failures if they happen.
-#if defined(WEBRTC_LINUX) || defined(WEBRTC_ANDROID)
-    prctl(PR_SET_NAME, reinterpret_cast<unsigned long>(name_.c_str()));
-#elif defined(WEBRTC_MAC) || defined(WEBRTC_IOS)
-    pthread_setname_np(name_.substr(0, 63).c_str());
-#endif
-  }
+bool ThreadPosix::SetPriority(ThreadPriority priority) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (!thread_)
+    return false;
 
 #ifdef WEBRTC_THREAD_RR
   const int policy = SCHED_RR;
@@ -130,18 +123,35 @@ void ThreadPosix::Run() {
 #endif
   const int min_prio = sched_get_priority_min(policy);
   const int max_prio = sched_get_priority_max(policy);
-  if ((min_prio == -1) || (max_prio == -1)) {
+  if (min_prio == -1 || max_prio == -1) {
     WEBRTC_TRACE(kTraceError, kTraceUtility, -1,
                  "unable to retreive min or max priority for threads");
+    return false;
   }
 
-  if (max_prio - min_prio > 2) {
-    sched_param param;
-    param.sched_priority = ConvertToSystemPriority(prio_, min_prio, max_prio);
-    if (pthread_setschedparam(pthread_self(), policy, &param) != 0) {
-      WEBRTC_TRACE(
-          kTraceError, kTraceUtility, -1, "unable to set thread priority");
-    }
+  if (max_prio - min_prio <= 2)
+    return false;
+
+  sched_param param;
+  param.sched_priority = ConvertToSystemPriority(priority, min_prio, max_prio);
+  if (pthread_setschedparam(thread_, policy, &param) != 0) {
+    WEBRTC_TRACE(
+        kTraceError, kTraceUtility, -1, "unable to set thread priority");
+    return false;
+  }
+
+  return true;
+}
+
+void ThreadPosix::Run() {
+  if (!name_.empty()) {
+    // Setting the thread name may fail (harmlessly) if running inside a
+    // sandbox. Ignore failures if they happen.
+#if defined(WEBRTC_LINUX) || defined(WEBRTC_ANDROID)
+    prctl(PR_SET_NAME, reinterpret_cast<unsigned long>(name_.c_str()));
+#elif defined(WEBRTC_MAC) || defined(WEBRTC_IOS)
+    pthread_setname_np(name_.substr(0, 63).c_str());
+#endif
   }
 
   // It's a requirement that for successful thread creation that the run
diff --git a/webrtc/system_wrappers/source/thread_posix.h b/webrtc/system_wrappers/source/thread_posix.h
index ccc52b642a..c726e480cf 100644
--- a/webrtc/system_wrappers/source/thread_posix.h
+++ b/webrtc/system_wrappers/source/thread_posix.h
@@ -25,14 +25,15 @@ int ConvertToSystemPriority(ThreadPriority priority, int min_prio,
 
 class ThreadPosix : public ThreadWrapper {
  public:
-  ThreadPosix(ThreadRunFunction func, void* obj, ThreadPriority prio,
-              const char* thread_name);
+  ThreadPosix(ThreadRunFunction func, void* obj, const char* thread_name);
   ~ThreadPosix() override;
 
   // From ThreadWrapper.
   bool Start() override;
   bool Stop() override;
 
+  bool SetPriority(ThreadPriority priority) override;
+
  private:
   static void* StartThread(void* param);
 
@@ -41,7 +42,6 @@ class ThreadPosix : public ThreadWrapper {
   rtc::ThreadChecker thread_checker_;
   ThreadRunFunction const run_function_;
   void* const obj_;
-  ThreadPriority prio_;
   rtc::Event stop_event_;
   const std::string name_;
 
diff --git a/webrtc/system_wrappers/source/thread_unittest.cc b/webrtc/system_wrappers/source/thread_unittest.cc
index f5de7d2099..854f98bbd9 100644
--- a/webrtc/system_wrappers/source/thread_unittest.cc
+++ b/webrtc/system_wrappers/source/thread_unittest.cc
@@ -24,7 +24,7 @@ bool NullRunFunction(void* obj) {
 
 TEST(ThreadTest, StartStop) {
   rtc::scoped_ptr<ThreadWrapper> thread = ThreadWrapper::CreateThread(
-      &NullRunFunction, NULL);
+      &NullRunFunction, nullptr, "ThreadTest");
   ASSERT_TRUE(thread->Start());
   EXPECT_TRUE(thread->Stop());
 }
@@ -40,7 +40,7 @@ bool SetFlagRunFunction(void* obj) {
 TEST(ThreadTest, RunFunctionIsCalled) {
   bool flag = false;
   rtc::scoped_ptr<ThreadWrapper> thread = ThreadWrapper::CreateThread(
-      &SetFlagRunFunction, &flag);
+      &SetFlagRunFunction, &flag, "RunFunctionIsCalled");
   ASSERT_TRUE(thread->Start());
 
   // At this point, the flag may be either true or false.
diff --git a/webrtc/system_wrappers/source/thread_win.cc b/webrtc/system_wrappers/source/thread_win.cc
index 0c1a8f209e..aa03b91809 100644
--- a/webrtc/system_wrappers/source/thread_win.cc
+++ b/webrtc/system_wrappers/source/thread_win.cc
@@ -55,10 +55,9 @@ void SetThreadName(DWORD dwThreadID, LPCSTR szThreadName) {
 }
 
 ThreadWindows::ThreadWindows(ThreadRunFunction func, void* obj,
-                             ThreadPriority prio, const char* thread_name)
+                             const char* thread_name)
     : run_function_(func),
       obj_(obj),
-      prio_(prio),
       stop_(false),
       thread_(NULL),
       name_(thread_name ? thread_name : "webrtc") {
@@ -98,28 +97,6 @@ bool ThreadWindows::Start() {
     return false;
   }
 
-  if (prio_ != kNormalPriority) {
-    int priority = THREAD_PRIORITY_NORMAL;
-    switch (prio_) {
-      case kLowPriority:
-        priority = THREAD_PRIORITY_BELOW_NORMAL;
-        break;
-      case kHighPriority:
-        priority = THREAD_PRIORITY_ABOVE_NORMAL;
-        break;
-      case kHighestPriority:
-        priority = THREAD_PRIORITY_HIGHEST;
-        break;
-      case kRealtimePriority:
-        priority = THREAD_PRIORITY_TIME_CRITICAL;
-        break;
-      default:
-        break;
-    }
-
-    SetThreadPriority(thread_, priority);
-  }
-
   return true;
 }
 
@@ -136,6 +113,11 @@ bool ThreadWindows::Stop() {
   return true;
 }
 
+bool ThreadWindows::SetPriority(ThreadPriority priority) {
+  DCHECK(main_thread_.CalledOnValidThread());
+  return thread_ && SetThreadPriority(thread_, priority);
+}
+
 void ThreadWindows::Run() {
   if (!name_.empty())
     SetThreadName(static_cast<DWORD>(-1), name_.c_str());
diff --git a/webrtc/system_wrappers/source/thread_win.h b/webrtc/system_wrappers/source/thread_win.h
index 1652551a7b..741ae1ed05 100644
--- a/webrtc/system_wrappers/source/thread_win.h
+++ b/webrtc/system_wrappers/source/thread_win.h
@@ -21,13 +21,14 @@ namespace webrtc {
 
 class ThreadWindows : public ThreadWrapper {
  public:
-  ThreadWindows(ThreadRunFunction func, void* obj, ThreadPriority prio,
-                const char* thread_name);
+  ThreadWindows(ThreadRunFunction func, void* obj, const char* thread_name);
   ~ThreadWindows() override;
 
   bool Start() override;
   bool Stop() override;
 
+  bool SetPriority(ThreadPriority priority) override;
+
  protected:
   void Run();
 
@@ -37,8 +38,6 @@ class ThreadWindows : public ThreadWrapper {
   ThreadRunFunction const run_function_;
   void* const obj_;
   bool stop_;
-  // TODO(tommi): Consider having a SetPriority method instead of this variable.
-  ThreadPriority prio_;
   HANDLE thread_;
   const std::string name_;
   rtc::ThreadChecker main_thread_;