From 48f90c22e51b2c2c2aca85377e8955785297f52e Mon Sep 17 00:00:00 2001 From: Ytai Ben-Tsvi Date: Thu, 26 Mar 2020 13:09:38 -0700 Subject: [PATCH] Change external capture state notification mechanism Previously, external capture state notifications were delivered from the audio policy manager to the sound trigger service as normal "forward calls". However this creates a cyclic dependency between sound trigger and APM, which is especially problematic during startup, when the audio server is assumed to start prior to the system server and thus cannot block on the sound trigger service becoming available. With this change, the sound trigger service would register a callback with APM for these notifications. A small refactoring was needed in order to keep the internal interface of sound trigger classes having the setExternalCapture() methods, while removing them from the external interface. Test: Manual verification, checking the logs, validating behavior in response to killing the audioserver process. Bug: 146157104 Merged-In: I4b4467fbac3607ee170394dc1b3309e7e3d422d8 Change-Id: Ibf68a5e5681afca9c5b1b821bb8ac18a2eebb4ca --- .../ISoundTriggerMiddlewareService.aidl | 6 - services/core/Android.bp | 1 + .../ExternalCaptureStateTracker.java | 157 ++++++++++++++++++ .../ISoundTriggerMiddlewareInternal.java | 27 +++ .../SoundTriggerMiddlewareImpl.java | 4 +- .../SoundTriggerMiddlewareLogging.java | 14 +- .../SoundTriggerMiddlewareService.java | 16 +- .../SoundTriggerMiddlewareValidation.java | 32 +--- .../SoundTriggerMiddlewareImplTest.java | 16 +- 9 files changed, 219 insertions(+), 54 deletions(-) create mode 100644 services/core/java/com/android/server/soundtrigger_middleware/ExternalCaptureStateTracker.java create mode 100644 services/core/java/com/android/server/soundtrigger_middleware/ISoundTriggerMiddlewareInternal.java diff --git a/media/java/android/media/soundtrigger_middleware/ISoundTriggerMiddlewareService.aidl b/media/java/android/media/soundtrigger_middleware/ISoundTriggerMiddlewareService.aidl index 80333070b7ce0..06c39071cdf50 100644 --- a/media/java/android/media/soundtrigger_middleware/ISoundTriggerMiddlewareService.aidl +++ b/media/java/android/media/soundtrigger_middleware/ISoundTriggerMiddlewareService.aidl @@ -39,10 +39,4 @@ interface ISoundTriggerMiddlewareService { * one of the handles from the returned list. */ ISoundTriggerModule attach(int handle, ISoundTriggerCallback callback); - - /** - * Notify the service that external input capture is taking place. This may cause some of the - * active recognitions to be aborted. - */ - void setExternalCaptureState(boolean active); } \ No newline at end of file diff --git a/services/core/Android.bp b/services/core/Android.bp index 052026c2746aa..5faed43dd6e61 100644 --- a/services/core/Android.bp +++ b/services/core/Android.bp @@ -126,6 +126,7 @@ java_library_static { "android.hardware.rebootescrow-java", "android.hardware.soundtrigger-V2.3-java", "android.hidl.manager-V1.2-java", + "capture_state_listener-aidl-java", "dnsresolver_aidl_interface-V2-java", "netd_event_listener_interface-java", "overlayable_policy_aidl-java", diff --git a/services/core/java/com/android/server/soundtrigger_middleware/ExternalCaptureStateTracker.java b/services/core/java/com/android/server/soundtrigger_middleware/ExternalCaptureStateTracker.java new file mode 100644 index 0000000000000..c341df3926219 --- /dev/null +++ b/services/core/java/com/android/server/soundtrigger_middleware/ExternalCaptureStateTracker.java @@ -0,0 +1,157 @@ +/* + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.soundtrigger_middleware; + +import android.media.ICaptureStateListener; +import android.os.IBinder; +import android.os.Parcel; +import android.os.RemoteException; +import android.os.ServiceManager; +import android.util.Log; + +import java.util.concurrent.Semaphore; +import java.util.function.Consumer; + +/** + * This is a never-give-up listener for sound trigger external capture state notifications, as + * published by the audio policy service. + * + * This class will constantly try to connect to the service over a background thread and tolerate + * its death. The client will be notified by a single provided function that is called in a + * synchronized manner. + * For simplicity, there is currently no way to stop the tracker. This is possible to add if the + * need ever arises. + */ +public class ExternalCaptureStateTracker { + private static final String TAG = "CaptureStateTracker"; + /** Our client's listener. */ + private final Consumer mListener; + /** A lock used to ensure synchronized access to mListener. */ + private final Object mListenerLock = new Object(); + /** + * The binder listener that we're providing to the audio policy service. Ensures synchronized + * access to mListener. + */ + private final Listener mSyncListener = new Listener(); + /** The name of the audio policy service. */ + private final String mAudioPolicyServiceName; + /** This semaphore will get a permit every time we need to reconnect. */ + private final Semaphore mNeedToConnect = new Semaphore(1); + /** + * We must hold a reference to the APM service, even though we're not actually using it after + * installing the callback. Otherwise, binder silently un-links our death listener. + */ + private IBinder mService; + + /** + * Constructor. Will start a background thread to do the work. + * + * @param audioPolicyServiceName The name of the audio policy service to connect to. + * @param listener A client provided listener that will be called on state + * changes. May be + * called multiple consecutive times with the same value. Never + * called + * concurrently. + */ + public ExternalCaptureStateTracker(String audioPolicyServiceName, + Consumer listener) { + mAudioPolicyServiceName = audioPolicyServiceName; + mListener = listener; + new Thread(this::run).start(); + } + + /** + * Routine for the background thread. Keeps trying to reconnect. + */ + private void run() { + while (true) { + mNeedToConnect.acquireUninterruptibly(); + connectWithRetry(); + } + } + + /** + * Try to connect. Retry in case of RemoteException. + */ + private void connectWithRetry() { + while (true) { + try { + connect(); + return; + } catch (RemoteException e) { + Log.w(TAG, "Exception caught trying to connect", e); + } catch (ServiceManager.ServiceNotFoundException e) { + Log.w(TAG, "Service not yet available, waiting", e); + try { + Thread.sleep(1000); + } catch (InterruptedException ex) { + } + } catch (Exception e) { + Log.e(TAG, "Unexpected exception caught trying to connect", e); + throw e; + } + } + } + + /** + * Connect to the service, install listener and death notifier. + * + * @throws RemoteException In case of a binder issue. + */ + private void connect() throws RemoteException, ServiceManager.ServiceNotFoundException { + Log.d(TAG, "Connecting to audio policy service: " + mAudioPolicyServiceName); + mService = ServiceManager.getServiceOrThrow(mAudioPolicyServiceName); + + synchronized (mListenerLock) { + boolean active = registerSoundTriggerCaptureStateListener(mService, mSyncListener); + mListener.accept(active); + } + + mService.linkToDeath(() -> { + Log.w(TAG, "Audio policy service died"); + mNeedToConnect.release(); + }, 0); + } + + /** + * Since the audio policy service does not have an AIDL interface, this method does the + * necessary manual marshalling. + * + * @param service The service binder object. + * @param listener The listener binder object to register. + * @return The active state at the time of registration. + */ + private boolean registerSoundTriggerCaptureStateListener(IBinder service, + ICaptureStateListener listener) throws RemoteException { + Parcel request = Parcel.obtain(); + Parcel response = Parcel.obtain(); + request.writeInterfaceToken("android.media.IAudioPolicyService"); + request.writeStrongBinder(listener.asBinder()); + service.transact(82 /* REGISTER_SOUNDTRIGGER_CAPTURE_STATE_LISTENER */, request, response, + 0); + return response.readBoolean(); + } + + private class Listener extends ICaptureStateListener.Stub { + @Override + public void setCaptureState(boolean active) { + synchronized (mListenerLock) { + mListener.accept(active); + } + } + } +} diff --git a/services/core/java/com/android/server/soundtrigger_middleware/ISoundTriggerMiddlewareInternal.java b/services/core/java/com/android/server/soundtrigger_middleware/ISoundTriggerMiddlewareInternal.java new file mode 100644 index 0000000000000..5def7621c1480 --- /dev/null +++ b/services/core/java/com/android/server/soundtrigger_middleware/ISoundTriggerMiddlewareInternal.java @@ -0,0 +1,27 @@ +/* + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.soundtrigger_middleware; + +import android.media.ICaptureStateListener; +import android.media.soundtrigger_middleware.ISoundTriggerMiddlewareService; + +/** + * This interface unifies ISoundTriggerMiddlewareService with ICaptureStateListener. + */ +public interface ISoundTriggerMiddlewareInternal extends ISoundTriggerMiddlewareService, + ICaptureStateListener { +} diff --git a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareImpl.java b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareImpl.java index 9f4b09a62afff..d76b1bf71a1c1 100644 --- a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareImpl.java +++ b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareImpl.java @@ -50,7 +50,7 @@ import java.util.List; * * @hide */ -public class SoundTriggerMiddlewareImpl implements ISoundTriggerMiddlewareService { +public class SoundTriggerMiddlewareImpl implements ISoundTriggerMiddlewareInternal { static private final String TAG = "SoundTriggerMiddlewareImpl"; private final SoundTriggerModule[] mModules; @@ -124,7 +124,7 @@ public class SoundTriggerMiddlewareImpl implements ISoundTriggerMiddlewareServic } @Override - public void setExternalCaptureState(boolean active) { + public void setCaptureState(boolean active) { for (SoundTriggerModule module : mModules) { module.setExternalCaptureState(active); } diff --git a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareLogging.java b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareLogging.java index fa78cb0931c27..04ba6bfeb4eeb 100644 --- a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareLogging.java +++ b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareLogging.java @@ -62,11 +62,11 @@ import java.util.LinkedList; * String, Object, Object[])}, {@link #logVoidReturnWithObject(Object, String, Object[])} and {@link * #logExceptionWithObject(Object, String, Exception, Object[])}. */ -public class SoundTriggerMiddlewareLogging implements ISoundTriggerMiddlewareService, Dumpable { +public class SoundTriggerMiddlewareLogging implements ISoundTriggerMiddlewareInternal, Dumpable { private static final String TAG = "SoundTriggerMiddlewareLogging"; - private final @NonNull ISoundTriggerMiddlewareService mDelegate; + private final @NonNull ISoundTriggerMiddlewareInternal mDelegate; - public SoundTriggerMiddlewareLogging(@NonNull ISoundTriggerMiddlewareService delegate) { + public SoundTriggerMiddlewareLogging(@NonNull ISoundTriggerMiddlewareInternal delegate) { mDelegate = delegate; } @@ -96,12 +96,12 @@ public class SoundTriggerMiddlewareLogging implements ISoundTriggerMiddlewareSer } @Override - public void setExternalCaptureState(boolean active) throws RemoteException { + public void setCaptureState(boolean active) throws RemoteException { try { - mDelegate.setExternalCaptureState(active); - logVoidReturn("setExternalCaptureState", active); + mDelegate.setCaptureState(active); + logVoidReturn("setCaptureState", active); } catch (Exception e) { - logException("setExternalCaptureState", e, active); + logException("setCaptureState", e, active); throw e; } } diff --git a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareService.java b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareService.java index 0d8fc76e1bd28..33b21416b23f8 100644 --- a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareService.java +++ b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareService.java @@ -63,14 +63,21 @@ public class SoundTriggerMiddlewareService extends ISoundTriggerMiddlewareServic static private final String TAG = "SoundTriggerMiddlewareService"; @NonNull - private final ISoundTriggerMiddlewareService mDelegate; + private final ISoundTriggerMiddlewareInternal mDelegate; /** * Constructor for internal use only. Could be exposed for testing purposes in the future. * Users should access this class via {@link Lifecycle}. */ - private SoundTriggerMiddlewareService(@NonNull ISoundTriggerMiddlewareService delegate) { + private SoundTriggerMiddlewareService(@NonNull ISoundTriggerMiddlewareInternal delegate) { mDelegate = Objects.requireNonNull(delegate); + new ExternalCaptureStateTracker("media.audio_policy", active -> { + try { + mDelegate.setCaptureState(active); + } catch (RemoteException e) { + throw e.rethrowAsRuntimeException(); + } + }); } @Override @@ -86,11 +93,6 @@ public class SoundTriggerMiddlewareService extends ISoundTriggerMiddlewareServic return new ModuleService(mDelegate.attach(handle, callback)); } - @Override - public void setExternalCaptureState(boolean active) throws RemoteException { - mDelegate.setExternalCaptureState(active); - } - @Override protected void dump(FileDescriptor fd, PrintWriter fout, String[] args) { if (mDelegate instanceof Dumpable) { ((Dumpable) mDelegate).dump(fout); diff --git a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareValidation.java b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareValidation.java index 06f2d65c13b29..008933f643dd1 100644 --- a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareValidation.java +++ b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareValidation.java @@ -105,7 +105,7 @@ import java.util.Set; * * {@hide} */ -public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddlewareService, Dumpable { +public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddlewareInternal, Dumpable { private static final String TAG = "SoundTriggerMiddlewareValidation"; private enum ModuleState { @@ -114,12 +114,12 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware DEAD }; - private final @NonNull ISoundTriggerMiddlewareService mDelegate; + private final @NonNull ISoundTriggerMiddlewareInternal mDelegate; private final @NonNull Context mContext; private Map> mModules; public SoundTriggerMiddlewareValidation( - @NonNull ISoundTriggerMiddlewareService delegate, @NonNull Context context) { + @NonNull ISoundTriggerMiddlewareInternal delegate, @NonNull Context context) { mDelegate = delegate; mContext = context; } @@ -213,21 +213,15 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware } @Override - public void setExternalCaptureState(boolean active) { - // Permission check. - checkPreemptPermissions(); - // Input validation (always valid). - - // State validation (always valid). - + public void setCaptureState(boolean active) { + // This is an internal call. No permissions needed. + // // Normally, we would acquire a lock here. However, we do not access any state here so it // is safe to not lock. This call is typically done from a different context than all the // other calls and may result in a deadlock if we lock here (between the audio server and // the system server). - - // From here on, every exception isn't client's fault. try { - mDelegate.setExternalCaptureState(active); + mDelegate.setCaptureState(active); } catch (Exception e) { throw handleException(e); } @@ -249,16 +243,6 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware enforcePermission(Manifest.permission.CAPTURE_AUDIO_HOTWORD); } - /** - * Throws a {@link SecurityException} if caller permanently doesn't have the given permission, - * or a {@link ServiceSpecificException} with a {@link Status#TEMPORARY_PERMISSION_DENIED} if - * caller temporarily doesn't have the right permissions to preempt active sound trigger - * sessions. - */ - void checkPreemptPermissions() { - enforcePermission(Manifest.permission.PREEMPT_SOUND_TRIGGER); - } - /** * Throws a {@link SecurityException} if caller permanently doesn't have the given permission, * or a {@link ServiceSpecificException} with a {@link Status#TEMPORARY_PERMISSION_DENIED} if @@ -806,4 +790,4 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware } } } -} \ No newline at end of file +} diff --git a/services/tests/servicestests/src/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareImplTest.java b/services/tests/servicestests/src/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareImplTest.java index 06b5fe48c4cfa..ebcf10dd019f4 100644 --- a/services/tests/servicestests/src/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareImplTest.java +++ b/services/tests/servicestests/src/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareImplTest.java @@ -1106,7 +1106,7 @@ public class SoundTriggerMiddlewareImplTest { public void testAbortRecognition() throws Exception { // Make sure the HAL doesn't support concurrent capture. initService(false); - mService.setExternalCaptureState(false); + mService.setCaptureState(false); ISoundTriggerCallback callback = createCallbackMock(); ISoundTriggerModule module = mService.attach(0, callback); @@ -1120,7 +1120,7 @@ public class SoundTriggerMiddlewareImplTest { startRecognition(module, handle, hwHandle); // Abort. - mService.setExternalCaptureState(true); + mService.setCaptureState(true); ArgumentCaptor eventCaptor = ArgumentCaptor.forClass( RecognitionEvent.class); @@ -1142,7 +1142,7 @@ public class SoundTriggerMiddlewareImplTest { verifyNotStartRecognition(); // Now enable it and make sure we are notified. - mService.setExternalCaptureState(false); + mService.setCaptureState(false); verify(callback).onRecognitionAvailabilityChange(true); // Unload the model. @@ -1154,7 +1154,7 @@ public class SoundTriggerMiddlewareImplTest { public void testAbortPhraseRecognition() throws Exception { // Make sure the HAL doesn't support concurrent capture. initService(false); - mService.setExternalCaptureState(false); + mService.setCaptureState(false); ISoundTriggerCallback callback = createCallbackMock(); ISoundTriggerModule module = mService.attach(0, callback); @@ -1168,7 +1168,7 @@ public class SoundTriggerMiddlewareImplTest { startRecognition(module, handle, hwHandle); // Abort. - mService.setExternalCaptureState(true); + mService.setCaptureState(true); ArgumentCaptor eventCaptor = ArgumentCaptor.forClass( PhraseRecognitionEvent.class); @@ -1190,7 +1190,7 @@ public class SoundTriggerMiddlewareImplTest { verifyNotStartRecognition(); // Now enable it and make sure we are notified. - mService.setExternalCaptureState(false); + mService.setCaptureState(false); verify(callback).onRecognitionAvailabilityChange(true); // Unload the model. @@ -1216,7 +1216,7 @@ public class SoundTriggerMiddlewareImplTest { startRecognition(module, handle, hwHandle); // Signal concurrent capture. Shouldn't abort. - mService.setExternalCaptureState(true); + mService.setCaptureState(true); verify(callback, never()).onRecognition(anyInt(), any()); verify(callback, never()).onRecognitionAvailabilityChange(anyBoolean()); @@ -1252,7 +1252,7 @@ public class SoundTriggerMiddlewareImplTest { startRecognition(module, handle, hwHandle); // Signal concurrent capture. Shouldn't abort. - mService.setExternalCaptureState(true); + mService.setCaptureState(true); verify(callback, never()).onPhraseRecognition(anyInt(), any()); verify(callback, never()).onRecognitionAvailabilityChange(anyBoolean());