Set Realtime Audio Thread priority on Android

Set Realtime Audio Thread priority on Android


BUG=


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179545

[Wei James(wistoch), 2012-12-31 08:30:54]

use Android SDK APIs to set audio thread to THREAD_PRIORITY_AUDIO.

[dgreid, 2013-01-07 19:02:40]

Looks like it should work to me.

+qinmin as he might be better suited to thoroughly review this and check if it
fits with how clank does these things.

[qinmin, 2013-01-07 19:16:34]

https://codereview.chromium.org/11729002/diff/1/base/threading/platform_threa...
File base/threading/platform_thread_posix.cc (right):

https://codereview.chromium.org/11729002/diff/1/base/threading/platform_threa...
base/threading/platform_thread_posix.cc:298: const int kThreadPriorityAudio =
-16;
There is no need to define this variable here and pass it to the java side. You
can get the value at java side and set it there. Maybe change the java side call
to setAudioThreadPriority.

[Wei James(wistoch), 2013-01-08 02:42:37]

dgreid, qinmin, 
thanks for reviewing. all issues addressed. thanks

https://codereview.chromium.org/11729002/diff/1/base/threading/platform_threa...
File base/threading/platform_thread_posix.cc (right):

https://codereview.chromium.org/11729002/diff/1/base/threading/platform_threa...
base/threading/platform_thread_posix.cc:298: const int kThreadPriorityAudio =
-16;
On 2013/01/07 19:16:34, qinmin wrote:
> There is no need to define this variable here and pass it to the java side.
You
> can get the value at java side and set it there. Maybe change the java side
call
> to setAudioThreadPriority.

fixed. thanks

[qinmin, 2013-01-08 03:25:13]

lgtm

[commit-bot: I haz the power, 2013-01-08 13:54:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/james.wei@intel.com/11729002/5002

[commit-bot: I haz the power, 2013-01-08 13:54:45]

Presubmit check for 11729002-5002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    base/android
    base/threading
    base

Presubmit checks took 1.0s to calculate.

[Wei James(wistoch), 2013-01-08 14:01:31]

+bulach as base/android OWNER
+mark as base OWNER

[bulach, 2013-01-09 12:57:57]

thanks! android lgtm, just some nits.

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/android/java/s...
File base/android/java/src/org/chromium/base/ThreadUtils.java (right):

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/android/java/s...
base/android/java/src/org/chromium/base/ThreadUtils.java:151: * Set the audio
thread priority.
nit: how about "set thread priority to audio.", so it matches the enum value?
same for the method name: "setThreadPriorityAudio"

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/threading/plat...
File base/threading/platform_thread.h (right):

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/threading/plat...
base/threading/platform_thread.h:24: #if defined(OS_ANDROID)
nit: keep with the platform specific includes above with an #elif

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/threading/plat...
base/threading/platform_thread.h:51: };
nit: add a \n

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/threading/plat...
File base/threading/platform_thread_posix.cc (right):

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/threading/plat...
base/threading/platform_thread_posix.cc:288: return RegisterNativesImpl(env);
nit: unindent -2

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/threading/plat...
base/threading/platform_thread_posix.cc:294: // set the thread to corresponding
cgroups.
this comment is a bit spurious, since it's just mapping
"kThreadPriority_RealtimeAudio" to "Java_ThreadUtils_setThreadPriorityAudio"..

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/threading/plat...
base/threading/platform_thread_posix.cc:297:
Java_ThreadUtils_setAudioThreadPriority(env, PlatformThread::CurrentId());
nit: as above, maybe rename to ..._setThreadPriorityAudio

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/threading/plat...
base/threading/platform_thread_posix.cc:302: #endif
nit:  // defined(OS_ANDROID)

[Wei James(wistoch), 2013-01-10 08:30:05]

bulach, thanks for the comments. 

all addressed except the #elif issue. thanks

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/android/java/s...
File base/android/java/src/org/chromium/base/ThreadUtils.java (right):

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/android/java/s...
base/android/java/src/org/chromium/base/ThreadUtils.java:151: * Set the audio
thread priority.
On 2013/01/09 12:57:58, bulach wrote:
> nit: how about "set thread priority to audio.", so it matches the enum value?
> same for the method name: "setThreadPriorityAudio"

fixed. thanks

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/threading/plat...
File base/threading/platform_thread.h (right):

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/threading/plat...
base/threading/platform_thread.h:24: #if defined(OS_ANDROID)
On 2013/01/09 12:57:58, bulach wrote:
> nit: keep with the platform specific includes above with an #elif

Android also needs the header files for OS_POSIX. so cannot use #elif

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/threading/plat...
base/threading/platform_thread.h:51: };
On 2013/01/09 12:57:58, bulach wrote:
> nit: add a \n

fixed. thanks

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/threading/plat...
File base/threading/platform_thread_posix.cc (right):

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/threading/plat...
base/threading/platform_thread_posix.cc:288: return RegisterNativesImpl(env);
On 2013/01/09 12:57:58, bulach wrote:
> nit: unindent -2

fixed. thanks

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/threading/plat...
base/threading/platform_thread_posix.cc:294: // set the thread to corresponding
cgroups.
On 2013/01/09 12:57:58, bulach wrote:
> this comment is a bit spurious, since it's just mapping
> "kThreadPriority_RealtimeAudio" to "Java_ThreadUtils_setThreadPriorityAudio"..
removed, thanks

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/threading/plat...
base/threading/platform_thread_posix.cc:297:
Java_ThreadUtils_setAudioThreadPriority(env, PlatformThread::CurrentId());
On 2013/01/09 12:57:58, bulach wrote:
> nit: as above, maybe rename to ..._setThreadPriorityAudio

fixed. thanks

https://chromiumcodereview.appspot.com/11729002/diff/5002/base/threading/plat...
base/threading/platform_thread_posix.cc:302: #endif
On 2013/01/09 12:57:58, bulach wrote:
> nit:  // defined(OS_ANDROID)
fixed. thanks

[commit-bot: I haz the power, 2013-01-10 08:31:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/james.wei@intel.com/11729002/15001

[commit-bot: I haz the power, 2013-01-10 08:31:29]

Presubmit check for 11729002-15001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    base
    base/threading

Presubmit checks took 1.1s to calculate.

[Wei James(wistoch), 2013-01-10 08:33:52]

jar or mark, 

could you help to review this patch as base/ OWNER? thanks

[Wei James(wistoch), 2013-01-23 05:07:51]

no base/OWNER response. 

+willchan

willchan, could you kindly review this patch as base OWNER? thanks

[Mark Mentovai, 2013-01-23 13:59:19]

LGTM with this change.

https://chromiumcodereview.appspot.com/11729002/diff/15001/base/threading/pla...
File base/threading/platform_thread_posix.cc (right):

https://chromiumcodereview.appspot.com/11729002/diff/15001/base/threading/pla...
base/threading/platform_thread_posix.cc:300: #endif // defined(OS_ANDROID)
Nit: this needs another space between #endif and //.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem...
(“Line Comments”)

[Wei James(wistoch), 2013-01-24 00:41:59]

mark, thanks for your kind review.

https://chromiumcodereview.appspot.com/11729002/diff/15001/base/threading/pla...
File base/threading/platform_thread_posix.cc (right):

https://chromiumcodereview.appspot.com/11729002/diff/15001/base/threading/pla...
base/threading/platform_thread_posix.cc:300: #endif // defined(OS_ANDROID)
On 2013/01/23 13:59:19, Mark Mentovai wrote:
> Nit: this needs another space between #endif and //.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem...
> (“Line Comments”)

fixed. thanks

[commit-bot: I haz the power, 2013-01-24 00:43:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/james.wei@intel.com/11729002/22001

[commit-bot: I haz the power, 2013-01-24 01:09:49]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-24 03:00:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/james.wei@intel.com/11729002/25003

[commit-bot: I haz the power, 2013-01-24 03:25:09]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Wei James(wistoch), 2013-01-29 07:42:41]

bulach, 

I found the debug build always failed due to the jni definition while release
build is ok. 

I don't know why but the build succeed if I seperate the definition of JNI part
into a seperated header file base/android/thread_utils.h. 

could you help to review the new patch again? thanks 

the following is the build error info for debug build. 

In file included from ../../base/android/jni_android.h:8:0,
                 from ../../base/threading/platform_thread.h:25,
                 from ../../base/synchronization/lock.h:10,
                 from ../../base/threading/thread_checker_impl.h:9,
                 from ../../base/threading/thread_checker.h:24,
                 from ../../base/memory/weak_ptr.h:78,
                 from ../../base/bind_helpers.h:152,
                 from ../../base/bind_internal.h:13,
                 from ../../base/bind.h:13,
                 from ../../base/cancelable_callback.h:46,
                 from ../../net/base/completion_callback.h:9,
                 from ../../net/base/file_stream.h:14,
                 from ../../chrome/common/zip_reader.cc:12:
src/third_party/android_tools/ndk//platforms/android-9/arch-x86/usr/include/jni.h:
In member function 'jbyte _JNIEnv::CallMOZ_Z_ByteMethod(jobject, j
methodID, ...)':
src/third_party/android_tools/ndk//platforms/android-9/arch-x86/usr/include/jni.h:634:1:
error: 'const struct JNINativeInterface' has no member named
 'CallMOZ_Z_ByteMethodV'
src/third_party/android_tools/ndk//platforms/android-9/arch-x86/usr/include/jni.h:
In member function 'jbyte _JNIEnv::CallMOZ_Z_ByteMethodV(jobject, jmethodID,
va_list)':
src/third_party/android_tools/ndk//platforms/android-9/arch-x86/usr/include/jni.h:634:1:
error: 'const struct JNINativeInterface' has no member named
'CallMOZ_Z_ByteMethodV'
src/third_party/android_tools/ndk//platforms/android-9/arch-x86/usr/include/jni.h:
In member function 'jbyte _JNIEnv::CallMOZ_Z_ByteMethodA(jobject, jmethodID,
jvalue*)':
src/third_party/android_tools/ndk//platforms/android-9/arch-x86/usr/include/jni.h:634:1:
error: 'const struct JNINativeInterface' has no member named
'CallMOZ_Z_ByteMethodA'
src/third_party/android_tools/ndk//platforms/android-9/arch-x86/usr/include/jni.h:
In member function 'jbyte _JNIEnv::CallNonvirtualMOZ_Z_ByteMethod(jobject,
jclass, jmethodID, ...)':
src/third_party/android_tools/ndk//platforms/android-9/arch-x86/usr/include/jni.h:684:1:
error: 'const struct JNINativeInterface' has no member named
'CallNonvirtualMOZ_Z_ByteMethodV'
src/third_party/android_tools/ndk//platforms/android-9/arch-x86/usr/include/jni.h:
In member function 'jbyte _JNIEnv::CallNonvirtualMOZ_Z_ByteMethodV(jobject,
jclass, jmethodID, va_list)':
src/third_party/android_tools/ndk//platforms/android-9/arch-x86/usr/include/jni.h:684:1:
error: 'const struct JNINativeInterface' has no member named
'CallNonvirtualMOZ_Z_ByteMethodV'
src/third_party/android_tools/ndk//platforms/android-9/arch-x86/usr/include/jni.h:
In member function 'jbyte _JNIEnv::CallNonvirtualMOZ_Z_ByteMethodA(jobject,
jclass, jmethodID, jvalue*)':
src/third_party/android_tools/ndk//platforms/android-9/arch-x86/usr/include/jni.h:684:1:
error: 'const struct JNINativeInterface' has no member named
'CallNonvirtualMOZ_Z_ByteMethodA'
src/third_party/android_tools/ndk//platforms/android-9/arch-x86/usr/include/jni.h:
In member function 'jbyte _JNIEnv::CallStaticMOZ_Z_ByteMethod(jclass, jmethodID,
...)':
src/third_party/android_tools/ndk//platforms/android-9/arch-x86/usr/include/jni.h:781:1:
error: 'const struct JNINativeInterface' has no member named
'CallStaticMOZ_Z_ByteMethodV'
src/third_party/android_tools/ndk//platforms/android-9/arch-x86/usr/include/jni.h:
In member function 'jbyte _JNIEnv::CallStaticMOZ_Z_ByteMethodV(jclass,
jmethodID, va_list)':
src/third_party/android_tools/ndk//platforms/android-9/arch-x86/usr/include/jni.h:781:1:
error: 'const struct JNINativeInterface' has no member named
'CallStaticMOZ_Z_ByteMethodV'
src/third_party/android_tools/ndk//platforms/android-9/arch-x86/usr/include/jni.h:
In member function 'jbyte _JNIEnv::CallStaticMOZ_Z_ByteMethodA(jcl

[Wei James(wistoch), 2013-01-30 01:48:10]

it is not a big change to seperate the definition to a seperated file(the same
way with other components)

so check the commit bit. thanks

[commit-bot: I haz the power, 2013-01-30 03:30:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/james.wei@intel.com/11729002/35002

[commit-bot: I haz the power, 2013-01-30 06:24:51]

Change committed as 179545