Add MODIFY_AUDIO_SETTINGS permission in Android manifest and implementation in audio manager.

Add MODIFY_AUDIO_SETTINGS permission in Android manifest and implementation in android audio manager.
                              
MODIFY_AUDIO_SETTINGS allows application to be able to configure audio settings.

In android audio manager, a function SetAudioMode is added to set audio mode.
                
BUG=180328

[leozwang1, 2013-03-08 17:43:57]

This is the follow up of https://codereview.chromium.org/12414005/. I don't why
I have problem uploading a new patch to 12414005, crash happened, I started a
new issue.

Yaron and Shijing, please review manifest and implementation.

[Yaron, 2013-03-09 00:31:18]

https://codereview.chromium.org/12571006/diff/1/media/audio/android/opensles_...
File media/audio/android/opensles_output.cc (right):

https://codereview.chromium.org/12571006/diff/1/media/audio/android/opensles_...
media/audio/android/opensles_output.cc:59: //
context.getSystemService(Context.AUDIO_SERVICE).
We're no longer taking patches with hand-written JNI bindings. Please use the
jni_generator. In this case, you can create a smaller wrapper class that you can
call into, passing it the application context and have it make the appropriate
java calls. See https://codereview.chromium.org/12604003/ for reference.

https://codereview.chromium.org/12571006/diff/1/media/audio/android/opensles_...
media/audio/android/opensles_output.cc:61: jclass context_class =
env->GetObjectClass(j_context);
Examle for why we say this: you're currently leaking this jclass. If you make
enough calls of this nature, dalvik will kill our process with a deadd00d
assertion which gives you no information about where the issue is without
logcat.

[no longer working on chromium, 2013-03-09 18:17:10]

Leo, I am going to defer the review to Yaron.

[leozwang1, 2013-03-10 06:43:45]

rebase

[leozwang1, 2013-03-11 00:40:18]

Thanks Shijing, add Tommi to review it.

Patch 3 is uploaded, there are two major changes
1. hand-writing jni binding code has been removed, a small java wrapper class is
added.
2. moved SetAudioMode to audio_manager. I think audio_manager should control
audio mode, not opensl. Description is updated too.
Please review it again.

[tommi (sloooow) - chröme, 2013-03-11 09:57:18]

lgtm % nits

https://codereview.chromium.org/12571006/diff/21001/media/audio/android/audio...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/12571006/diff/21001/media/audio/android/audio...
media/audio/android/audio_manager_android.cc:35: bool
AudioManagerAndroid::RegisterAudioManager(JNIEnv* env) {
try to keep methods in the source file in the same order as they are in the
header file.

https://codereview.chromium.org/12571006/diff/21001/media/audio/android/audio...
File media/audio/android/audio_manager_android.h (right):

https://codereview.chromium.org/12571006/diff/21001/media/audio/android/audio...
media/audio/android/audio_manager_android.h:40: static bool
RegisterAudioManager(JNIEnv* env);
Add whitespace to separate this method from the virtual methods.
Does this need to be public?

https://codereview.chromium.org/12571006/diff/21001/media/audio/android/audio...
media/audio/android/audio_manager_android.h:45: virtual AudioParameters
GetPreferredOutputStreamParameters(
why is this method here and not with the other methods from the same interface?

[leozwang1, 2013-03-11 15:12:24]

thanks Tommi, addressed most of your comments, but have one question.

https://codereview.chromium.org/12571006/diff/21001/media/audio/android/audio...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/12571006/diff/21001/media/audio/android/audio...
media/audio/android/audio_manager_android.cc:35: bool
AudioManagerAndroid::RegisterAudioManager(JNIEnv* env) {
On 2013/03/11 09:57:18, tommi wrote:
> try to keep methods in the source file in the same order as they are in the
> header file.

Done.

https://codereview.chromium.org/12571006/diff/21001/media/audio/android/audio...
File media/audio/android/audio_manager_android.h (right):

https://codereview.chromium.org/12571006/diff/21001/media/audio/android/audio...
media/audio/android/audio_manager_android.h:40: static bool
RegisterAudioManager(JNIEnv* env);
Done.
Yes, it must be public which is called by media_jni_register().

https://codereview.chromium.org/12571006/diff/21001/media/audio/android/audio...
media/audio/android/audio_manager_android.h:45: virtual AudioParameters
GetPreferredOutputStreamParameters(
Tommi, can you please explain more? where shall I move
GetPreferredOutputStreamParameters() to?

[tommi (sloooow) - chröme, 2013-03-11 15:21:32]

Just mean that methods that belong to the same interface should be grouped
together.
Is the odd method out from a different interface than the other virtual
methods?


On Mon, Mar 11, 2013 at 4:12 PM, <leozwang@chromium.org> wrote:

> thanks Tommi, addressed most of your comments, but have one question.
>
>
>
> https://codereview.chromium.**org/12571006/diff/21001/media/**
>
audio/android/audio_manager_**android.cc<https://codereview.chromium.org/12571006/diff/21001/media/audio/android/audio_manager_android.cc>
> File media/audio/android/audio_**manager_android.cc (right):
>
> https://codereview.chromium.**org/12571006/diff/21001/media/**
>
audio/android/audio_manager_**android.cc#newcode35<https://codereview.chromium.org/12571006/diff/21001/media/audio/android/audio_manager_android.cc#newcode35>
> media/audio/android/audio_**manager_android.cc:35: bool
> AudioManagerAndroid::**RegisterAudioManager(JNIEnv* env) {
> On 2013/03/11 09:57:18, tommi wrote:
>
>> try to keep methods in the source file in the same order as they are
>>
> in the
>
>> header file.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/12571006/diff/21001/media/**
>
audio/android/audio_manager_**android.h<https://codereview.chromium.org/12571006/diff/21001/media/audio/android/audio_manager_android.h>
> File media/audio/android/audio_**manager_android.h (right):
>
> https://codereview.chromium.**org/12571006/diff/21001/media/**
>
audio/android/audio_manager_**android.h#newcode40<https://codereview.chromium.org/12571006/diff/21001/media/audio/android/audio_manager_android.h#newcode40>
> media/audio/android/audio_**manager_android.h:40: static bool
> RegisterAudioManager(JNIEnv* env);
> Done.
> Yes, it must be public which is called by media_jni_register().
>
>
> https://codereview.chromium.**org/12571006/diff/21001/media/**
>
audio/android/audio_manager_**android.h#newcode45<https://codereview.chromium.org/12571006/diff/21001/media/audio/android/audio_manager_android.h#newcode45>
> media/audio/android/audio_**manager_android.h:45: virtual AudioParameters
> GetPreferredOutputStreamParame**ters(
> Tommi, can you please explain more? where shall I move
> GetPreferredOutputStreamParame**ters() to?
>
>
https://codereview.chromium.**org/12571006/<https://codereview.chromium.org/1...
>

[leozwang1, 2013-03-11 15:31:06]

On 2013/03/11 15:21:32, tommi wrote:
> Just mean that methods that belong to the same interface should be grouped
> together.
> Is the odd method out from a different interface than the other virtual
> methods?

it was introduced in r186698 and r186180, Shijing, do you have any suggestion to
move this api around?

[leozwang1, 2013-03-11 15:57:33]

GetPreferredOutputStreamParameters() is defined as protected in
audio_manager_base.h.

[no longer working on chromium, 2013-03-11 16:07:54]

https://codereview.chromium.org/12571006/diff/21001/media/audio/android/audio...
File media/audio/android/audio_manager_android.h (right):

https://codereview.chromium.org/12571006/diff/21001/media/audio/android/audio...
media/audio/android/audio_manager_android.h:45: virtual AudioParameters
GetPreferredOutputStreamParameters(
On 2013/03/11 09:57:18, tommi wrote:
> why is this method here and not with the other methods from the same
interface?

You meant GetPreferredOutputStreamParameters and GetInputStreamParameters?

GetInputStreamParameters is a public interface in audio_manager.h, and
GetPreferredOutputStreamParameters is protected interface in
audio_manager_base.h.
The intefaces for input and output are a bit different when it comes to user
cases, because we have resampler only in output side but not input side.

https://codereview.chromium.org/12571006/diff/27001/media/audio/android/audio...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/12571006/diff/27001/media/audio/android/audio...
media/audio/android/audio_manager_android.cc:64: SetAudioMode(kAudioModeNormal);
could you please explain why MakeLinearOutputStream and
MakeLowLatencyOutputStream are using different modes?

https://codereview.chromium.org/12571006/diff/27001/media/audio/android/audio...
File media/audio/android/audio_manager_android.h (right):

https://codereview.chromium.org/12571006/diff/27001/media/audio/android/audio...
media/audio/android/audio_manager_android.h:19: static const int
kAudioModeInCommunication = 0x00000003;
these static variables do not need to be in the interface, move them to the
implementation.

https://codereview.chromium.org/12571006/diff/27001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://codereview.chromium.org/12571006/diff/27001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
nit, you don't need the (c) anymore

[leozwang1, 2013-03-11 20:00:16]

Ping, Yaron, can you take a look?

[Yaron, 2013-03-11 21:56:49]

https://codereview.chromium.org/12571006/diff/37011/media/audio/android/audio...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/12571006/diff/37011/media/audio/android/audio...
media/audio/android/audio_manager_android.cc:129: DCHECK(env);
No need for this DCHECK

https://codereview.chromium.org/12571006/diff/37011/media/audio/android/opens...
File media/audio/android/opensles_output.h (right):

https://codereview.chromium.org/12571006/diff/37011/media/audio/android/opens...
media/audio/android/opensles_output.h:12: #include "base/android/jni_android.h"
This include isn't needed anymore, right?

https://codereview.chromium.org/12571006/diff/37011/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://codereview.chromium.org/12571006/diff/37011/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:14:
public class AudioManagerAndroid {
Nit: need not be public. It can still be accessed via jni

https://codereview.chromium.org/12571006/diff/37011/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/12571006/diff/37011/media/media.gyp#newcode1430
media/media.gyp:1430: 'target_name': 'audio_manager_android_jni_headers',
There's no need to split this into two targets. I don't think you even need to
add these separately for that matter. I think you can just fold these into
player_android_jni_headers and possibly rename media_jni_headers

+Min to comment

https://codereview.chromium.org/12571006/diff/37011/testing/android/AndroidMa...
File testing/android/AndroidManifest.xml (right):

https://codereview.chromium.org/12571006/diff/37011/testing/android/AndroidMa...
testing/android/AndroidManifest.xml:26: <!-- TODO(jrg): add more permissions as
needed by unit tests. -->
Nit: remove this TODO. It's out of date

[qinmin, 2013-03-11 23:09:56]

https://codereview.chromium.org/12571006/diff/37011/media/audio/android/audio...
File media/audio/android/audio_manager_android.h (right):

https://codereview.chromium.org/12571006/diff/37011/media/audio/android/audio...
media/audio/android/audio_manager_android.h:17: static const int
kAudioModeRingtone = 0x00000001;
be specific, i don't think it is proper for chrome to change Ringtone and InCall
volumes.
And you can define the remaining 2 constants in .cc file

[Ami GONE FROM CHROMIUM, 2013-03-12 21:41:07]

drive-by

https://codereview.chromium.org/12571006/diff/37011/media/audio/android/audio...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/12571006/diff/37011/media/audio/android/audio...
media/audio/android/audio_manager_android.cc:64: SetAudioMode(kAudioModeNormal);
Why is this not InCommunication as well?

(this API surface point is legacy and probably never called on android, anyway)

[leozwang1, 2013-03-12 22:22:59]

PTAL

https://codereview.chromium.org/12571006/diff/37011/media/audio/android/audio...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/12571006/diff/37011/media/audio/android/audio...
media/audio/android/audio_manager_android.cc:64: SetAudioMode(kAudioModeNormal);
I don't really know who is using or will be using this lineroutputstream, I'm ok
to switch to communication mode.

https://codereview.chromium.org/12571006/diff/37011/media/audio/android/audio...
media/audio/android/audio_manager_android.cc:129: DCHECK(env);
On 2013/03/11 21:56:50, Yaron wrote:
> No need for this DCHECK

Done.

https://codereview.chromium.org/12571006/diff/37011/media/audio/android/audio...
File media/audio/android/audio_manager_android.h (right):

https://codereview.chromium.org/12571006/diff/37011/media/audio/android/audio...
media/audio/android/audio_manager_android.h:17: static const int
kAudioModeRingtone = 0x00000001;
On 2013/03/11 23:09:56, qinmin wrote:
> be specific, i don't think it is proper for chrome to change Ringtone and
InCall
> volumes.
> And you can define the remaining 2 constants in .cc file

Done.

https://codereview.chromium.org/12571006/diff/37011/media/audio/android/opens...
File media/audio/android/opensles_output.h (right):

https://codereview.chromium.org/12571006/diff/37011/media/audio/android/opens...
media/audio/android/opensles_output.h:12: #include "base/android/jni_android.h"
On 2013/03/11 21:56:50, Yaron wrote:
> This include isn't needed anymore, right?

Done.

https://codereview.chromium.org/12571006/diff/37011/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://codereview.chromium.org/12571006/diff/37011/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:14:
public class AudioManagerAndroid {
On 2013/03/11 21:56:50, Yaron wrote:
> Nit: need not be public. It can still be accessed via jni

Done.

https://codereview.chromium.org/12571006/diff/37011/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/12571006/diff/37011/media/media.gyp#newcode1430
media/media.gyp:1430: 'target_name': 'audio_manager_android_jni_headers',
On 2013/03/11 21:56:50, Yaron wrote:
> There's no need to split this into two targets. I don't think you even need to
> add these separately for that matter. I think you can just fold these into
> player_android_jni_headers and possibly rename media_jni_headers
> 
> +Min to comment

Done.

[Yaron, 2013-03-13 01:15:07]

Sorry for my likley naive question but I'm not really familiar with how media is
supposed to work on Android.

https://codereview.chromium.org/12571006/diff/57001/media/audio/android/audio...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/12571006/diff/57001/media/audio/android/audio...
media/audio/android/audio_manager_android.cc:66:
SetAudioMode(kAudioModeInCommunication);
Ok, but when does this ever get unset? Presumably the stream will end and we
need to unset the audio mode. Or will that happen automatically?

[leozwang1, 2013-03-13 16:46:38]

Ami, when the app is killed, mode will be reset to normal.

Ping, Yaron and Min.

[qinmin, 2013-03-13 17:03:13]

lgtm, do u have access to internal repo? coz u need to add the permission to
AndroidManifest.xml there too.

On 2013/03/13 16:46:38, leozwang1 wrote:
> Ami, when the app is killed, mode will be reset to normal.
> 
> Ping, Yaron and Min.

[leozwang1, 2013-03-13 17:05:18]

On 2013/03/13 17:03:13, qinmin wrote:
> lgtm, do u have access to internal repo? coz u need to add the permission to
> AndroidManifest.xml there too.
Yes, I will do, can you tell me in which folder?

[Ami GONE FROM CHROMIUM, 2013-03-13 17:12:41]

still drive-by

On Wed, Mar 13, 2013 at 9:46 AM, <leozwang@chromium.org> wrote:

> Ami, when the app is killed, mode will be reset to normal.
>

I think you were answering Yaron's q, not mine, but I also think the answer
is not good.
There's no reason to affect the device's audio mode for the remainder of
the process lifetime, just because for a short period a webrtc call was in
effect. I think you need to toggle InCommuncation/Normal modes based on
num_{input,output}_streams_ in AudioManagerBase hitting 0.  (except this is
Android-specific, so you probably need to expose the # of outstanding
streams to subclasses of AMB).

My point was that MakeLinearOutputStream() should probably never be getting
called, and if it *was** *getting called, it should be
setting InCommunication just like MakeLowLatencyOutputStream().

[Yaron, 2013-03-13 18:06:50]

On 2013/03/13 17:12:41, Ami Fischman wrote:
> still drive-by
> 
> On Wed, Mar 13, 2013 at 9:46 AM, <mailto:leozwang@chromium.org> wrote:
> 
> > Ami, when the app is killed, mode will be reset to normal.
> >
> 
> I think you were answering Yaron's q, not mine, but I also think the answer
> is not good.
> There's no reason to affect the device's audio mode for the remainder of
> the process lifetime, just because for a short period a webrtc call was in
> effect. I think you need to toggle InCommuncation/Normal modes based on
> num_{input,output}_streams_ in AudioManagerBase hitting 0.  (except this is
> Android-specific, so you probably need to expose the # of outstanding
> streams to subclasses of AMB).

Agreed that it's unsatisfactory. Since Ami is clearly more qualified to review
this, I'll be waiting for his approval.
> 
> My point was that MakeLinearOutputStream() should probably never be getting
> called, and if it *was** *getting called, it should be
> setting InCommunication just like MakeLowLatencyOutputStream().

[leozwang1, 2013-03-14 04:35:18]

Need to find a better way to configure global settings on Android. Will make a
proposal once I find one.

[qinmin, 2013-03-14 15:03:15]

https://codereview.chromium.org/12571006/diff/57001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://codereview.chromium.org/12571006/diff/57001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:18:
(AudioManager)context.getSystemService(Context.AUDIO_SERVICE);
nits: indent by 8

[leozwang1, 2013-03-19 23:57:32]

The main change in patch 11 is moving SetAudioMode to audio_manager_base, please
take a look.

https://chromiumcodereview.appspot.com/12571006/diff/57001/media/base/android...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://chromiumcodereview.appspot.com/12571006/diff/57001/media/base/android...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:18:
(AudioManager)context.getSystemService(Context.AUDIO_SERVICE);
On 2013/03/14 15:03:16, qinmin wrote:
> nits: indent by 8

Done.

[leozwang1, 2013-03-20 00:14:30]

sorry for the delay, please take a look as soon as you can, I want to land
today. thanks!

[Ami GONE FROM CHROMIUM, 2013-03-20 00:30:27]

https://chromiumcodereview.appspot.com/12571006/diff/74001/media/audio/audio_...
File media/audio/audio_manager_base.cc (right):

https://chromiumcodereview.appspot.com/12571006/diff/74001/media/audio/audio_...
media/audio/audio_manager_base.cc:132: if (num_input_streams_)
I don't get it - here you're triggering off num_input_streams_ but in the
opposite below you trigger off num_output_streams_?

I expected these calls to be in MakeAudioInputStream & ReleaseInputStream,
resp., and both to be keyed off num_input_streams_.

https://chromiumcodereview.appspot.com/12571006/diff/74001/media/base/android...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://chromiumcodereview.appspot.com/12571006/diff/74001/media/base/android...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
No (c).

[Ami GONE FROM CHROMIUM, 2013-03-20 00:45:00]

LGTM

[leozwang1, 2013-03-20 00:49:22]

Yaron and Min, do you have more comments?

[qinmin, 2013-03-20 00:52:40]

lgtm to me

On 2013/03/20 00:49:22, leozwang1 wrote:
> Yaron and Min, do you have more comments?

[Yaron, 2013-03-20 00:59:57]

lgtm

[qinmin, 2013-03-20 01:00:14]

please make sure this change cherrypicked into m26, or the permission change I
made will not get approved

On 2013/03/20 00:52:40, qinmin wrote:
> lgtm to me
> 
> On 2013/03/20 00:49:22, leozwang1 wrote:
> > Yaron and Min, do you have more comments?

[commit-bot: I haz the power, 2013-03-20 01:04:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leozwang@chromium.org/12571006/86002

[commit-bot: I haz the power, 2013-03-20 03:18:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leozwang@chromium.org/12571006/51013

[commit-bot: I haz the power, 2013-03-20 04:01:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leozwang@chromium.org/12571006/88004

[wjia(left Chromium), 2013-03-21 04:31:21]

This patch has been landed as
https://src.chromium.org/viewvc/chrome?view=rev&revision=189210