Refactor audio manager for Android to avoid heavy tasks at startup

media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java

Index: media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
diff --git a/media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java b/media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
index 0c37c0a9e3a51c92e52e907ba449fcc76cd7019b..ad7a86a495bc97dc40ea274de5c47b5e995d5980 100644
--- a/media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
+++ b/media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
@@ -40,7 +40,7 @@ class AudioManagerAndroid {
     private static final String TAG = "AudioManagerAndroid";
 
     // Set to true to enable debug logs. Always check in as false.
-    private static final boolean DEBUG = false;
+    private static final boolean DEBUG = true;

[henrika (OOO until Aug 14), 2013/12/10 16:23:18]

will be restored to false

 
     /** Simple container for device information. */
     private static class AudioDeviceName {
@@ -115,10 +115,12 @@ class AudioManagerAndroid {
     private final Context mContext;
     private final long mNativeAudioManagerAndroid;
 
-    private boolean mHasBluetoothPermission = false;
+    private int mSavedAudioMode = AudioManager.MODE_INVALID;
+
     private boolean mIsInitialized = false;
     private boolean mSavedIsSpeakerphoneOn;
     private boolean mSavedIsMicrophoneMute;
+    private boolean mDefaultDeviceIsSelected = false;
 
     private Integer mAudioDeviceState = STATE_NO_DEVICE_SELECTED;
 
@@ -160,6 +162,7 @@ class AudioManagerAndroid {
      */
     @CalledByNative
     public void init() {
+        if (DEBUG) logd("init");
         if (mIsInitialized)
             return;
 
@@ -169,17 +172,6 @@ class AudioManagerAndroid {
             }
         }
 
-        // Store microphone mute state and speakerphone state so it can
-        // be restored when closing.
-        mSavedIsSpeakerphoneOn = mAudioManager.isSpeakerphoneOn();
-        mSavedIsMicrophoneMute = mAudioManager.isMicrophoneMute();
-
-        // Always enable speaker phone by default. This state might be reset
-        // by the wired headset receiver when it gets its initial sticky
-        // intent, if any.
-        setSpeakerphoneOn(true);
-        mAudioDeviceState = STATE_SPEAKERPHONE_ON;
-
         // Initialize audio device list with things we know is always available.
         synchronized (mLock) {
             if (hasEarpiece()) {
@@ -190,15 +182,8 @@ class AudioManagerAndroid {
 
         // Register receiver for broadcasted intents related to adding/
         // removing a wired headset (Intent.ACTION_HEADSET_PLUG).
-        // Also starts routing to the wired headset/headphone if one is
-        // already attached (can be overridden by a Bluetooth headset).
         registerForWiredHeadsetIntentBroadcast();
 
-        // Start routing to Bluetooth if there's a connected device.
-        // TODO(henrika): the actual routing part is not implemented yet.
-        // All we do currently is to detect if BT headset is attached or not.
-        initBluetooth();
-
         mIsInitialized = true;

[tommi (sloooow) - chröme, 2013/12/10 21:19:11]

nit: set last?

[henrika (OOO until Aug 14), 2013/12/11 13:16:38]

Done.

 
         mSettingsObserverThread = new SettingsObserverThread();
@@ -207,7 +192,7 @@ class AudioManagerAndroid {
             try {
                 mSettingsObserverLock.wait();

[tommi (sloooow) - chröme, 2013/12/10 21:19:11]

I'm not sure we need this locking+waiting actually.  See comment below about
moving mSettingsObserver into the mSettingsObserverThread class.

[henrika (OOO until Aug 14), 2013/12/11 13:16:38]

Wei wrote these things with assistance from reviewer from the Android team. I
woul like to pass on these comments if that is OK for now. Or should I add Wei
to the review list?

             } catch (InterruptedException e) {
-                Log.e(TAG, "unregisterHeadsetReceiver exception: " + e.getMessage());
+                loge("unregisterHeadsetReceiver exception: " + e.getMessage());

[tommi (sloooow) - chröme, 2013/12/10 21:19:11]

nit: log message seems misleading

[henrika (OOO until Aug 14), 2013/12/11 13:16:38]

Changed. Code from Wei.

             }
         }
     }
@@ -218,6 +203,7 @@ class AudioManagerAndroid {
      */
     @CalledByNative
     public void close() {
+        if (DEBUG) logd("close");
         if (!mIsInitialized)
             return;
 
@@ -231,20 +217,53 @@ class AudioManagerAndroid {
 
         unregisterForWiredHeadsetIntentBroadcast();
 
-        // Restore previously stored audio states.
-        setMicrophoneMute(mSavedIsMicrophoneMute);
-        setSpeakerphoneOn(mSavedIsSpeakerphoneOn);
-
         mIsInitialized = false;
     }
 
+    /**
+     * TODO(henrika): add comments...

[Jói, 2013/12/11 10:52:04]

Before landing this patch?

[henrika (OOO until Aug 14), 2013/12/12 10:58:00]

Done.

+     */
     @CalledByNative
-    public void setMode(int mode) {
-        try {
-            mAudioManager.setMode(mode);
-        } catch (SecurityException e) {
-            Log.e(TAG, "setMode exception: " + e.getMessage());
-            logDeviceInfo();
+    public void setCommunicationAudioModeOn(boolean on) {
+        if (DEBUG) logd("setCommunicationAudioModeOn(" + on + ")");
+
+        if (on) {
+            if (mSavedAudioMode == AudioManager.MODE_INVALID) {

[tommi (sloooow) - chröme, 2013/12/10 21:19:11]

What about making it a programmer error if mSavedAudioMode isn't
AudioManager.MODE_INVALID?
Then you can skip this check and assert on the value being correct instead.

Thanks for fixing this bug btw!  Maybe you could file a proper issue just for
tracking and tag this cl with that bug?

[henrika (OOO until Aug 14), 2013/12/12 10:58:00]

Will improve.

+                // Store the current audio mode the first time we try to
+                // switch to communication mode.
+                try {
+                    mSavedAudioMode = mAudioManager.getMode();
+                } catch (SecurityException e) {
+                    loge("getMode exception: " + e.getMessage());

[tommi (sloooow) - chröme, 2013/12/10 21:19:11]

does this happen in practice?

[henrika (OOO until Aug 14), 2013/12/11 13:16:38]

It was added in the original version and I simply maintained it. Don't know the
full history and therefore kept it.

+                    logDeviceInfo();
+                }
+
+                // Store microphone mute state and speakerphone state so it can
+                // be restored when closing.
+                mSavedIsSpeakerphoneOn = mAudioManager.isSpeakerphoneOn();
+                mSavedIsMicrophoneMute = mAudioManager.isMicrophoneMute();
+            }
+            try {
+                mAudioManager.setMode(AudioManager.MODE_IN_COMMUNICATION);
+            } catch (SecurityException e) {
+                loge("setMode exception: " + e.getMessage());
+                logDeviceInfo();
+            }
+        } else {
+            if (mSavedAudioMode != AudioManager.MODE_INVALID) {

[tommi (sloooow) - chröme, 2013/12/10 21:19:11]

assert that mSavedAudioMode != AudioManager.MODE_INVALID?

[henrika (OOO until Aug 14), 2013/12/11 13:16:38]

Done. Assuming my "assert" is OK.

+                // Restore previously stored audio states.
+                setMicrophoneMute(mSavedIsMicrophoneMute);
+                setSpeakerphoneOn(mSavedIsSpeakerphoneOn);
+
+                // Restore the mode that was used before we switched to
+                // communication mode.
+                try {
+                    mAudioManager.setMode(mSavedAudioMode);
+                } catch (SecurityException e) {
+                    loge("setMode exception: " + e.getMessage());
+                    logDeviceInfo();
+                }
+            }

[tommi (sloooow) - chröme, 2013/12/10 21:19:11]

set mSavedAudioMode to AudioManager.MODE_INVALID after restoring the previous
mode.

[henrika (OOO until Aug 14), 2013/12/11 13:16:38]

Done.

         }
     }
 
@@ -256,37 +275,41 @@ class AudioManagerAndroid {
      * default device is selected.
      */
     @CalledByNative
-    public void setDevice(String deviceId) {
+    boolean setDevice(String deviceId) {
         boolean devices[] = null;
         synchronized (mLock) {
             devices = mAudioDevices.clone();
         }
         if (deviceId.isEmpty()) {
-            logd("setDevice: default");
+            if (DEBUG) logd("setDevice: default");
             // Use a special selection scheme if the default device is selected.
-            // The "most unique" device will be selected; Bluetooth first, then
-            // wired headset and last the speaker phone.
-            if (devices[DEVICE_BLUETOOTH_HEADSET]) {
+            // The "most unique" device will be selected; Wired headset first,
+            // then Bluetooth and last the speaker phone.
+            if (devices[DEVICE_WIRED_HEADSET]) {
+                setAudioDevice(DEVICE_WIRED_HEADSET);
+            } else if (devices[DEVICE_BLUETOOTH_HEADSET]) {
                 // TODO(henrika): possibly need improvements here if we are
                 // in a STATE_BLUETOOTH_TURNING_OFF state.
                 setAudioDevice(DEVICE_BLUETOOTH_HEADSET);
-            } else if (devices[DEVICE_WIRED_HEADSET]) {
-                setAudioDevice(DEVICE_WIRED_HEADSET);
             } else {
                 setAudioDevice(DEVICE_SPEAKERPHONE);
             }
-        } else {
-            logd("setDevice: " + deviceId);
+            mDefaultDeviceIsSelected = true;
+            return true;
+        } else if (isNumeric(deviceId)) {
+            if (DEBUG) logd("setDevice: " + deviceId);
             // A non-default device is specified. Verify that it is valid
             // device, and if so, start using it.
             List<Integer> validIds = Arrays.asList(VALID_DEVICES);
             Integer id = Integer.valueOf(deviceId);
-            if (validIds.contains(id)) {
+            if (validIds.contains(id) && mAudioDevices[id.intValue()]) {
                 setAudioDevice(id.intValue());
-            } else {
-                loge("Invalid device ID!");
+                mDefaultDeviceIsSelected = false;
+                return true;
             }
         }
+        loge("Invalid device ID: " + deviceId);
+        return false;
     }
 
     /**
@@ -307,7 +330,7 @@ class AudioManagerAndroid {
                     i++;
                 }
             }
-            logd("getAudioInputDeviceNames: " + devices);
+            if (DEBUG) logd("getAudioInputDeviceNames: " + devices);
             return array;
         }
     }
@@ -433,11 +456,12 @@ class AudioManagerAndroid {
                 int state = intent.getIntExtra("state", STATE_UNPLUGGED);
                 int microphone = intent.getIntExtra("microphone", HAS_NO_MIC);
                 String name = intent.getStringExtra("name");
-                logd("==> onReceive: s=" + state
+                if (DEBUG) {
+                    logd("==> onReceive: s=" + state
                         + ", m=" + microphone
                         + ", n=" + name
                         + ", sb=" + isInitialStickyBroadcast());
-
+                }
                 switch (state) {
                     case STATE_UNPLUGGED:
                         synchronized (mLock) {
@@ -447,27 +471,25 @@ class AudioManagerAndroid {
                                 mAudioDevices[DEVICE_EARPIECE] = true;
                             }
                         }
-                        // If wired headset was used before it was unplugged,
-                        // switch to speaker phone. If it was not in use; just
-                        // log the change.
-                        if (mAudioDeviceState == STATE_WIRED_HEADSET_ON) {
-                            setAudioDevice(DEVICE_SPEAKERPHONE);
-                        } else {
-                            reportUpdate();
-                        }
                         break;
                     case STATE_PLUGGED:
                         synchronized (mLock) {
                             // Wired headset and earpiece are mutually exclusive.
                             mAudioDevices[DEVICE_WIRED_HEADSET] = true;
                             mAudioDevices[DEVICE_EARPIECE] = false;
-                            setAudioDevice(DEVICE_WIRED_HEADSET);
                         }
                         break;
                     default:
                         loge("Invalid state!");
                         break;
                 }
+
+                // Update the existing device selection, but only if a specific
+                // device has already been selected.
+                // TODO(henrika): add lock?

[tommi (sloooow) - chröme, 2013/12/10 21:19:11]

on which thread are we here?
If we're on a different thread than where we test mAudioDevices, won't we need
synchronization for that too?

[henrika (OOO until Aug 14), 2013/12/11 13:16:38]

Will take a second round and consider these issues.

+                if (mAudioDeviceState != STATE_NO_DEVICE_SELECTED) {
+                    updateDeviceSelection();
+                }
             }
         };
 
@@ -484,58 +506,6 @@ class AudioManagerAndroid {
     }
 
     /**
-    * Check if Bluetooth device is connected, register Bluetooth receiver
-    * and start routing to Bluetooth if a device is connected.
-    * TODO(henrika): currently only supports the detecion part at startup.
-    */
-    private void initBluetooth() {
-        // Bail out if we don't have the required permission.
-        mHasBluetoothPermission = mContext.checkPermission(
-            android.Manifest.permission.BLUETOOTH,
-            Process.myPid(),
-            Process.myUid()) == PackageManager.PERMISSION_GRANTED;
-        if (!mHasBluetoothPermission) {
-            loge("BLUETOOTH permission is missing!");
-            return;
-        }
-
-        // To get a BluetoothAdapter representing the local Bluetooth adapter,
-        // when running on JELLY_BEAN_MR1 (4.2) and below, call the static
-        // getDefaultAdapter() method; when running on JELLY_BEAN_MR2 (4.3) and
-        // higher, retrieve it through getSystemService(String) with
-        // BLUETOOTH_SERVICE.
-        // Note: Most methods require the BLUETOOTH permission.
-        BluetoothAdapter btAdapter = null;
-        if (android.os.Build.VERSION.SDK_INT <=
-            android.os.Build.VERSION_CODES.JELLY_BEAN_MR1) {
-            // Use static method for Android 4.2 and below to get the
-            // BluetoothAdapter.
-            btAdapter = BluetoothAdapter.getDefaultAdapter();
-        } else {
-            // Use BluetoothManager to get the BluetoothAdapter for
-            // Android 4.3 and above.
-            BluetoothManager btManager =
-                    (BluetoothManager)mContext.getSystemService(
-                            Context.BLUETOOTH_SERVICE);
-            btAdapter = btManager.getAdapter();
-        }
-
-        if (btAdapter != null &&
-            // android.bluetooth.BluetoothAdapter.getProfileConnectionState
-            // requires BLUETOOTH permission.
-            android.bluetooth.BluetoothProfile.STATE_CONNECTED ==
-                    btAdapter.getProfileConnectionState(
-                            android.bluetooth.BluetoothProfile.HEADSET)) {
-            synchronized (mLock) {
-                mAudioDevices[DEVICE_BLUETOOTH_HEADSET] = true;
-            }
-            // TODO(henrika): ensure that we set the active audio
-            // device to Bluetooth (not trivial).
-            setAudioDevice(DEVICE_BLUETOOTH_HEADSET);
-        }
-    }
-
-    /**
      * Changes selection of the currently active audio device.
      *
      * @param device Specifies the selected audio device.
@@ -578,6 +548,48 @@ class AudioManagerAndroid {
         return count;
     }
 
+    /** TODO(henrika): add comments... */

[Jói, 2013/12/11 10:52:04]

Before landing this patch?

[henrika (OOO until Aug 14), 2013/12/11 13:16:38]

Done.

+    private void updateDeviceSelection() {
+        final String DEFAULT_DEVICE_ID = "";
+        if (DEBUG) logd("updateDeviceSelection");
+
+        // Something has been changed in the device state. A device could
+        // have been added or removed. Take actions given the current state.
+        if (mDefaultDeviceIsSelected) {
+            // Deafault-device mode => shift to new default device.

[tommi (sloooow) - chröme, 2013/12/10 21:19:11]

Default

[henrika (OOO until Aug 14), 2013/12/11 13:16:38]

Done.

+            setDevice(DEFAULT_DEVICE_ID);
+        } else {
+            // Non-default mode => shift to default device mode if the
+            // currently selected device is no longer available.
+            // TODO(henrika): is there anything else we can do here?

[tommi (sloooow) - chröme, 2013/12/10 21:19:11]

Could we keep track of what device was previously selected?
In the case of bluetooth and A2DP, we might have to store more information if we
are not really changing devices but rather switching profiles.

[henrika (OOO until Aug 14), 2013/12/11 13:16:38]

I think I simply have to develop this scheme as we start using the code. Am
unable to come up with a perfect solution right now.

+            if (!selectedDeviceIsInList()) {
+                if (DEBUG) logd("Falling back to default device");
+                setDevice(DEFAULT_DEVICE_ID);
+            } else {
+                if (DEBUG) logd("No action required!");
+            }
+        }
+    }
+
+    private static boolean isNumeric(String str) {

[tommi (sloooow) - chröme, 2013/12/10 21:19:11]

is this necessary?  Would Integer.parseInt() give you what you need?

[Jói, 2013/12/11 10:52:04]

That would throw an exception if it's not an integer, and throwing exceptions is
expensive, reserved for exceptional cases. If it's only in exceptional cases
that the string is not an Integer, then it's fine to do that. If it's a common
case then this seems better.

[tommi (sloooow) - chröme, 2013/12/11 12:34:25]

Yes, I think it would be an exceptional case and should basically never happen
(i.e. I'm not sure we should even attempt to catch that error and let it just
CHECK() in the C++ code.

[henrika (OOO until Aug 14), 2013/12/11 13:16:38]

I tried to avoid exception. Will not happen often. Do you want me to switch to
exception?

[tommi (sloooow) - chröme, 2013/12/11 17:25:05]

yes I think that's OK.

[henrika (OOO until Aug 14), 2013/12/12 10:58:00]

Done.

+        for (char c : str.toCharArray())

[tommi (sloooow) - chröme, 2013/12/10 21:19:11]

for () {
}

[henrika (OOO until Aug 14), 2013/12/11 13:16:38]

Done.

+        {
+            if (!Character.isDigit(c)) return false;

[tommi (sloooow) - chröme, 2013/12/10 21:19:11]

return on a separate line

[henrika (OOO until Aug 14), 2013/12/11 13:16:38]

Done.

+        }
+        return true;
+    }
+
+    private boolean selectedDeviceIsInList() {
+        return (mAudioDeviceState == STATE_SPEAKERPHONE_ON &&

[tommi (sloooow) - chröme, 2013/12/10 21:19:11]

crazy idea:

It seems that all the STATE_FOO_ON constants are duplicates of the DEVICE_FOO
constants.
Can we simply use one of them and set mAudioDeviceState to e.g.
DEVICE_SPEAKERPHONE when the speakerphone is being used?

That makes mAudioDevices and mAudioDeviceState compatible and this function can
be rewritten as:

private boolean selectedDeviceIsInList() {
  return mAudioDeviceState != DEVICE_INVALID &&
mAudioDevices[mAudioDeviceState];
}

If you then rename the mAudioDeviceState variable to e.g. mSelectedAudioDevice,
it's also more inline with selectedDeviceIsInList and to go out on a limb,
arguably more correct since it currently doesn't really represent the state of
the audio device but rather which one is selected.

[henrika (OOO until Aug 14), 2013/12/11 13:16:38]

The idea crossed my mind as well but I had also added STATE_BLUETOOTH_TURNING_ON
and STATE_BLUETOOTH_TURNING_OFF at that stage. I think we will need these as
well in the upcoming BT CL. Perhaps I could use them separately and merge as you
say.

Ahh, it sounds so good when you describe it. Will change. Thanks!

+            mAudioDevices[DEVICE_SPEAKERPHONE]) ||
+            (mAudioDeviceState == STATE_WIRED_HEADSET_ON &&
+            mAudioDevices[DEVICE_WIRED_HEADSET]) ||
+            (mAudioDeviceState == STATE_EARPIECE_ON &&
+            mAudioDevices[DEVICE_EARPIECE]) ||
+            (mAudioDeviceState == STATE_BLUETOOTH_ON &&
+            mAudioDevices[DEVICE_BLUETOOTH_HEADSET]);
+    }
+
     /**
      * For now, just log the state change but the idea is that we should
      * notify a registered state change listener (if any) that there has
@@ -591,8 +603,10 @@ class AudioManagerAndroid {
                 if (mAudioDevices[i])
                     devices.add(DEVICE_NAMES[i]);
             }
-            logd("reportUpdate: state=" + mAudioDeviceState
-                + ", devices=" + devices);
+            if (DEBUG) {
+                logd("reportUpdate: state=" + mAudioDeviceState
+                    + ", devices=" + devices);
+            }
         }
     }