Makes it such that calling ThreadWrapper::Start(..) only returns false if a thread was never started. I.e. it will not return false if it was unable to configure the thread (however it does log such failures).

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

git-svn-id: http://webrtc.googlecode.com/svn/trunk@2879 4adac7df-926f-26a2-2b94-8c16560cd09d

src/system_wrappers/interface/thread_wrapper.h

@@ -65,8 +65,12 @@ public:
     // to delete this class until the spawned thread has been reclaimed.
     virtual void SetNotAlive() = 0;
 
-    // Spawns the thread. This will start the triggering of the callback
-    // function.
+    // Tries to spawns a thread and returns true if that was successful.
+    // Additionally, it tries to set thread priority according to the priority
+    // from when CreateThread was called. However, failure to set priority will
+    // not result in a false return value.
+    // TODO(henrike): add a function for polling whether priority was set or
+    //                not.
     virtual bool Start(unsigned int& id) = 0;
 
     // Sets the threads CPU affinity. CPUs are listed 0 - (number of CPUs - 1).

src/system_wrappers/source/system_wrappers.gyp

@@ -216,6 +216,7 @@
             'data_log_c_helpers_unittest.c',
             'data_log_c_helpers_unittest.h',
             'thread_unittest.cc',
+            'thread_posix_unittest.cc',
             'trace_unittest.cc',
             'unittest_utilities_unittest.cc',
           ],
@@ -225,6 +226,9 @@
             }, {
               'sources!': [ 'data_log_unittest.cc', ],
             }],
+            ['os_posix!=1', {
+              'sources!': [ 'thread_posix_unittest.cc', ],
+            }],
           ],
         },
       ], # targets

src/system_wrappers/source/thread_posix.cc

@@ -44,6 +44,9 @@
 
 #include "thread_posix.h"
 
+#include <algorithm>
+
+#include <assert.h>
 #include <errno.h>
 #include <string.h> // strncpy
 #include <time.h>   // nanosleep
@@ -65,6 +68,32 @@
 #include "system_wrappers/interface/trace.h"
 
 namespace webrtc {
+
+int ConvertToSystemPriority(ThreadPriority priority, int minPrio, int maxPrio)
+{
+    assert(maxPrio - minPrio > 2);
+    const int topPrio = maxPrio - 1;
+    const int lowPrio = minPrio + 1;
+
+    switch (priority)
+    {
+    case kLowPriority:
+        return lowPrio;
+    case kNormalPriority:
+        // The -1 ensures that the kHighPriority is always greater or equal to
+        // kNormalPriority.
+        return (lowPrio + topPrio - 1) / 2;
+    case kHighPriority:
+        return std::max(topPrio - 2, lowPrio);
+    case kHighestPriority:
+        return std::max(topPrio - 1, lowPrio);
+    case kRealtimePriority:
+        return topPrio;
+    }
+    assert(false);
+    return lowPrio;
+}
+
 extern "C"
 {
     static void* StartThread(void* lpParameter)
@@ -177,6 +206,11 @@ bool ThreadPosix::Start(unsigned int& /*threadID*/)
     const int policy = SCHED_FIFO;
 #endif
     _event->Reset();
+    // If pthread_create was successful, a thread was created and is running.
+    // Don't return false if it was successful since if there are any other
+    // failures the state will be: thread was started but not configured as
+    // asked for. However, the caller of this API will assume that a false
+    // return value means that the thread never started.
     result |= pthread_create(&_thread, &_attr, &StartThread, this);
     if (result != 0)
     {
@@ -187,9 +221,11 @@ bool ThreadPosix::Start(unsigned int& /*threadID*/)
     // race condition if Stop() is called too quickly after start.
     if (kEventSignaled != _event->Wait(WEBRTC_EVENT_10_SEC))
     {
+        WEBRTC_TRACE(kTraceError, kTraceUtility, -1,
+                     "posix thread event never triggered");
         // Timed out. Something went wrong.
         _runFunction = NULL;
-        return false;
+        return true;
     }
 
 #if HAS_THREAD_ID
@@ -201,31 +237,21 @@ bool ThreadPosix::Start(unsigned int& /*threadID*/)
     const int maxPrio = sched_get_priority_max(policy);
     if ((minPrio == EINVAL) || (maxPrio == EINVAL))
     {
-        return false;
+        WEBRTC_TRACE(kTraceError, kTraceUtility, -1,
+                     "unable to retreive min or max priority for threads");
+        return true;
     }
-
-    switch (_prio)
+    if (maxPrio - minPrio <= 2)
     {
-    case kLowPriority:
-        param.sched_priority = minPrio + 1;
-        break;
-    case kNormalPriority:
-        param.sched_priority = (minPrio + maxPrio) / 2;
-        break;
-    case kHighPriority:
-        param.sched_priority = maxPrio - 3;
-        break;
-    case kHighestPriority:
-        param.sched_priority = maxPrio - 2;
-        break;
-    case kRealtimePriority:
-        param.sched_priority = maxPrio - 1;
-        break;
+      // There is no room for setting priorities with any granularity.
+      return true;
     }
+    param.sched_priority = ConvertToSystemPriority(_prio, minPrio, maxPrio);
     result = pthread_setschedparam(_thread, policy, &param);
     if (result == EINVAL)
     {
-        return false;
+        WEBRTC_TRACE(kTraceError, kTraceUtility, -1,
+                     "unable to set thread priority");
     }
     return true;
 }
@@ -344,7 +370,7 @@ void ThreadPosix::Run()
 #ifdef WEBRTC_LINUX
         prctl(PR_SET_NAME, (unsigned long)_name, 0, 0, 0);
 #endif
-        WEBRTC_TRACE(kTraceStateInfo, kTraceUtility,-1,
+        WEBRTC_TRACE(kTraceStateInfo, kTraceUtility, -1,
                      "Thread with name:%s started ", _name);
     } else
     {
@@ -382,13 +408,13 @@ void ThreadPosix::Run()
         // coupling the thread and the trace class like this.
         if (strcmp(_name, "Trace"))
         {
-            WEBRTC_TRACE(kTraceStateInfo, kTraceUtility,-1,
+            WEBRTC_TRACE(kTraceStateInfo, kTraceUtility, -1,
                          "Thread with name:%s stopped", _name);
         }
     }
     else
     {
-        WEBRTC_TRACE(kTraceStateInfo, kTraceUtility,-1,
+        WEBRTC_TRACE(kTraceStateInfo, kTraceUtility, -1,
                      "Thread without name stopped");
     }
     {

src/system_wrappers/source/thread_posix.h

@@ -19,6 +19,8 @@ namespace webrtc {
 class CriticalSectionWrapper;
 class EventWrapper;
 
+int ConvertToSystemPriority(ThreadPriority priority, int minPrio, int maxPrio);
+
 class ThreadPosix : public ThreadWrapper
 {
 public:

src/system_wrappers/source/thread_posix_unittest.cc

@@ -0,0 +1,30 @@
+/*
+ *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "system_wrappers/source/thread_posix.h"
+
+#include "gtest/gtest.h"
+
+TEST(ThreadTestPosix, PrioritySettings) {
+  // API assumes that maxPrio - minPrio > 2. Test the extreme case.
+  const int kMinPrio = -1;
+  const int kMaxPrio = 2;
+
+  int last_priority = kMinPrio;
+  for (int priority = webrtc::kLowPriority;
+       priority <= webrtc::kRealtimePriority; ++priority) {
+    int system_priority = webrtc::ConvertToSystemPriority(
+        static_cast<webrtc::ThreadPriority>(priority), kMinPrio, kMaxPrio);
+    EXPECT_GT(system_priority, kMinPrio);
+    EXPECT_LT(system_priority, kMaxPrio);
+    EXPECT_GE(system_priority, last_priority);
+    last_priority = system_priority;
+  }
+}

src/system_wrappers/source/thread_unittest.cc

@@ -11,6 +11,7 @@
 #include "system_wrappers/interface/thread_wrapper.h"
 
 #include "gtest/gtest.h"
+#include "system_wrappers/interface/scoped_ptr.h"
 #include "system_wrappers/interface/trace.h"
 
 namespace webrtc {
@@ -60,6 +61,13 @@ class ThreadTest : public ::testing::Test {
   TestTraceCallback trace_;
 };
 
+TEST_F(ThreadTest, NullFunctionPointer) {
+  webrtc::scoped_ptr<ThreadWrapper> thread(
+      webrtc::ThreadWrapper::CreateThread());
+  unsigned int id = 42;
+  EXPECT_FALSE(thread->Start(id));
+}
+
 // Function that does nothing, and reports success.
 bool NullRunFunction(void* /* obj */) {
   return true;

src/system_wrappers/source/thread_win.cc

@@ -75,6 +75,9 @@ unsigned int WINAPI ThreadWindows::StartThread(LPVOID lpParameter)
 
 bool ThreadWindows::Start(unsigned int& threadID)
 {
+    if (!_runFunction) {
+      return false;
+    }
     _doNotCloseHandle = false;
 
     // Set stack size to 1M