From 25ec8c6dc4f74b384f9825056b8fdb4f089dcfb6 Mon Sep 17 00:00:00 2001 From: Ytai Ben-Tsvi Date: Tue, 15 Mar 2022 10:05:48 -0700 Subject: [PATCH 1/3] Extract some event utilities Those will be used in several places. Test: Compile Change-Id: I9d08cdf68bc6093d0a03d9149a607c8992813232 --- .../soundtrigger_middleware/AidlUtil.java | 71 +++++++++++++++++++ ...undTriggerHalConcurrentCaptureHandler.java | 36 ++-------- 2 files changed, 78 insertions(+), 29 deletions(-) create mode 100644 services/core/java/com/android/server/soundtrigger_middleware/AidlUtil.java diff --git a/services/core/java/com/android/server/soundtrigger_middleware/AidlUtil.java b/services/core/java/com/android/server/soundtrigger_middleware/AidlUtil.java new file mode 100644 index 0000000000000..f3457f5a221ba --- /dev/null +++ b/services/core/java/com/android/server/soundtrigger_middleware/AidlUtil.java @@ -0,0 +1,71 @@ +/* + * Copyright (C) 2022 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.soundtrigger.PhraseRecognitionEvent; +import android.media.soundtrigger.PhraseRecognitionExtra; +import android.media.soundtrigger.RecognitionEvent; +import android.media.soundtrigger.RecognitionStatus; +import android.media.soundtrigger.SoundModelType; + +/** + * Utilities for working with sound trigger related AIDL generated types. + */ +public class AidlUtil { + /** + * Initialize a new recognition event. + * @return The new event. + */ + static RecognitionEvent newEmptyRecognitionEvent() { + RecognitionEvent result = new RecognitionEvent(); + result.data = new byte[0]; + return result; + } + + /** + * Initialize a new phrase recognition event. + * @return The new event. + */ + static PhraseRecognitionEvent newEmptyPhraseRecognitionEvent() { + PhraseRecognitionEvent result = new PhraseRecognitionEvent(); + result.common = newEmptyRecognitionEvent(); + result.phraseExtras = new PhraseRecognitionExtra[0]; + return result; + } + + /** + * Creates a new generic abort event. + * @return The new event. + */ + static RecognitionEvent newAbortEvent() { + RecognitionEvent event = newEmptyRecognitionEvent(); + event.type = SoundModelType.GENERIC; + event.status = RecognitionStatus.ABORTED; + return event; + } + + /** + * Creates a new generic phrase event. + * @return The new event. + */ + static PhraseRecognitionEvent newAbortPhraseEvent() { + PhraseRecognitionEvent event = newEmptyPhraseRecognitionEvent(); + event.common.type = SoundModelType.KEYPHRASE; + event.common.status = RecognitionStatus.ABORTED; + return event; + } +} diff --git a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerHalConcurrentCaptureHandler.java b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerHalConcurrentCaptureHandler.java index 1cc05391b497c..c0ab65a3215c8 100644 --- a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerHalConcurrentCaptureHandler.java +++ b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerHalConcurrentCaptureHandler.java @@ -20,12 +20,10 @@ import android.annotation.NonNull; import android.media.permission.SafeCloseable; import android.media.soundtrigger.ModelParameterRange; import android.media.soundtrigger.PhraseRecognitionEvent; -import android.media.soundtrigger.PhraseRecognitionExtra; import android.media.soundtrigger.PhraseSoundModel; import android.media.soundtrigger.Properties; import android.media.soundtrigger.RecognitionConfig; import android.media.soundtrigger.RecognitionEvent; -import android.media.soundtrigger.RecognitionStatus; import android.media.soundtrigger.SoundModel; import android.media.soundtrigger.SoundModelType; import android.media.soundtrigger.Status; @@ -392,21 +390,14 @@ public class SoundTriggerHalConcurrentCaptureHandler implements ISoundTriggerHal /** Notify the client that recognition has been aborted. */ private static void notifyAbort(int modelHandle, LoadedModel model) { switch (model.type) { - case SoundModelType.GENERIC: { - RecognitionEvent event = newEmptyRecognitionEvent(); - event.status = RecognitionStatus.ABORTED; - event.type = SoundModelType.GENERIC; - model.callback.recognitionCallback(modelHandle, event); - } - break; + case SoundModelType.GENERIC: + model.callback.recognitionCallback(modelHandle, AidlUtil.newAbortEvent()); + break; - case SoundModelType.KEYPHRASE: { - PhraseRecognitionEvent event = newEmptyPhraseRecognitionEvent(); - event.common.status = RecognitionStatus.ABORTED; - event.common.type = SoundModelType.KEYPHRASE; - model.callback.phraseRecognitionCallback(modelHandle, event); - } - break; + case SoundModelType.KEYPHRASE: + model.callback.phraseRecognitionCallback(modelHandle, + AidlUtil.newAbortPhraseEvent()); + break; } } @@ -416,19 +407,6 @@ public class SoundTriggerHalConcurrentCaptureHandler implements ISoundTriggerHal mNotifier.unregisterListener(this); } - private static PhraseRecognitionEvent newEmptyPhraseRecognitionEvent() { - PhraseRecognitionEvent result = new PhraseRecognitionEvent(); - result.common = newEmptyRecognitionEvent(); - result.phraseExtras = new PhraseRecognitionExtra[0]; - return result; - } - - private static RecognitionEvent newEmptyRecognitionEvent() { - RecognitionEvent result = new RecognitionEvent(); - result.data = new byte[0]; - return result; - } - //////////////////////////////////////////////////////////////////////////////////////////////// // All methods below do trivial delegation - no interesting logic. @Override From 3b660525f37bdb6c549d9ce483658d4a8f189438 Mon Sep 17 00:00:00 2001 From: Ytai Ben-Tsvi Date: Mon, 14 Mar 2022 17:10:01 -0700 Subject: [PATCH 2/3] Generate an abort event when stopping Because the client callbacks from SoundTriggerMiddleware service are async, if a client quickly stops and starts a model, then receives a detection event for that model, it is impossible for it to tell whether the event corresponding to the previous or current session, and thus is unable to reason about the resulting state of the model. To mitigate that, we will always send an abort event if a model is stopped while it was in an active state. This way the client can consider the model active (even after stop()) until an event is received for that model. Bug: 191935600 Test: Manual verification of soundtrigger use-cases. Test: atest SoundTriggerMiddlewareImplTest Change-Id: I59555b024df54ca90ae7573acb38e114dac0c130 --- .../SoundTriggerModule.java | 48 ++++++++++++++++--- .../SoundTriggerMiddlewareImplTest.java | 10 ++++ 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerModule.java b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerModule.java index 934b0e46ee957..fd8dee8416f62 100644 --- a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerModule.java +++ b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerModule.java @@ -25,6 +25,7 @@ import android.media.soundtrigger.Properties; import android.media.soundtrigger.RecognitionConfig; import android.media.soundtrigger.RecognitionEvent; import android.media.soundtrigger.SoundModel; +import android.media.soundtrigger.SoundModelType; import android.media.soundtrigger.Status; import android.media.soundtrigger_middleware.ISoundTriggerCallback; import android.media.soundtrigger_middleware.ISoundTriggerModule; @@ -305,9 +306,12 @@ class SoundTriggerModule implements IBinder.DeathRecipient, ISoundTriggerHal.Glo @Override public void stopRecognition(int modelHandle) { + Model model; synchronized (SoundTriggerModule.this) { - mLoadedModels.get(modelHandle).stopRecognition(); + checkValid(); + model = mLoadedModels.get(modelHandle); } + model.stopRecognition(); } @Override @@ -374,6 +378,7 @@ class SoundTriggerModule implements IBinder.DeathRecipient, ISoundTriggerHal.Glo private class Model implements ISoundTriggerHal.ModelCallback { public int mHandle; private ModelState mState = ModelState.INIT; + private int mType = SoundModelType.INVALID; private SoundTriggerMiddlewareImpl.AudioSessionProvider.AudioSession mSession; private @NonNull @@ -390,6 +395,7 @@ class SoundTriggerModule implements IBinder.DeathRecipient, ISoundTriggerHal.Glo SoundTriggerMiddlewareImpl.AudioSessionProvider.AudioSession audioSession) { mSession = audioSession; mHandle = mHalService.loadSoundModel(model, this); + mType = SoundModelType.GENERIC; setState(ModelState.LOADED); mLoadedModels.put(mHandle, this); return mHandle; @@ -399,7 +405,7 @@ class SoundTriggerModule implements IBinder.DeathRecipient, ISoundTriggerHal.Glo SoundTriggerMiddlewareImpl.AudioSessionProvider.AudioSession audioSession) { mSession = audioSession; mHandle = mHalService.loadPhraseSoundModel(model, this); - + mType = SoundModelType.KEYPHRASE; setState(ModelState.LOADED); mLoadedModels.put(mHandle, this); return mHandle; @@ -422,12 +428,41 @@ class SoundTriggerModule implements IBinder.DeathRecipient, ISoundTriggerHal.Glo } private void stopRecognition() { - if (getState() == ModelState.LOADED) { - // This call is idempotent in order to avoid races. - return; + synchronized (SoundTriggerModule.this) { + if (getState() == ModelState.LOADED) { + // This call is idempotent in order to avoid races. + return; + } } + // This must be invoked outside the lock. mHalService.stopRecognition(mHandle); - setState(ModelState.LOADED); + + // No more callbacks for this model after this point. + synchronized (SoundTriggerModule.this) { + // Generate an abortion callback to the client if the model is still active. + if (getState() == ModelState.ACTIVE) { + if (mCallback != null) { + try { + switch (mType) { + case SoundModelType.GENERIC: + mCallback.onRecognition(mHandle, AidlUtil.newAbortEvent(), + mSession.mSessionHandle); + break; + case SoundModelType.KEYPHRASE: + mCallback.onPhraseRecognition(mHandle, + AidlUtil.newAbortPhraseEvent(), + mSession.mSessionHandle); + break; + default: + throw new RuntimeException( + "Unexpected model type: " + mType); + } + } catch (RemoteException e) { + } + } + setState(ModelState.LOADED); + } + } } /** Request a forced recognition event. Will do nothing if recognition is inactive. */ @@ -518,4 +553,5 @@ class SoundTriggerModule implements IBinder.DeathRecipient, ISoundTriggerHal.Glo } } } + } 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 0eba6a335d006..0187e34cbc5f9 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 @@ -224,6 +224,11 @@ public class SoundTriggerMiddlewareImplTest { // Stop the recognition. stopRecognition(module, handle, hwHandle); + ArgumentCaptor eventCaptor = ArgumentCaptor.forClass( + RecognitionEvent.class); + verify(callback).onRecognition(eq(handle), eventCaptor.capture(), eq(101)); + assertEquals(RecognitionStatus.ABORTED, eventCaptor.getValue().status); + // Unload the model. unloadModel(module, handle, hwHandle); module.detach(); @@ -268,6 +273,11 @@ public class SoundTriggerMiddlewareImplTest { // Stop the recognition. stopRecognition(module, handle, hwHandle); + ArgumentCaptor eventCaptor = ArgumentCaptor.forClass( + PhraseRecognitionEvent.class); + verify(callback).onPhraseRecognition(eq(handle), eventCaptor.capture(), eq(101)); + assertEquals(RecognitionStatus.ABORTED, eventCaptor.getValue().common.status); + // Unload the model. unloadModel(module, handle, hwHandle); module.detach(); From 6e2eb81ac4687ac5340673ff996c0591154def7f Mon Sep 17 00:00:00 2001 From: Ytai Ben-Tsvi Date: Tue, 15 Mar 2022 11:34:18 -0700 Subject: [PATCH 3/3] Wait for an abort event when stopping a model Because the client callbacks from SoundTriggerMiddleware service are async, if a client quickly stops and starts a model, then receives a detection event for that model, it is impossible for it to tell whether the event corresponding to the previous or current session, and thus is unable to reason about the resulting state of the model. To mitigate that, we will always wait for an abort event when a model is stopped before any other operation is attempted. This way the model state stays synchronized. Fixes: 191935600 Test: Manual verification of soundtrigger use-cases. Specifically, quick toggling of Assitant and Now Playing via Settings. Change-Id: I010558e8fa1891922f20dcdb7b0ca04c9500c2e1 --- .../soundtrigger/SoundTriggerHelper.java | 82 ++++++++++++------- 1 file changed, 53 insertions(+), 29 deletions(-) diff --git a/services/voiceinteraction/java/com/android/server/soundtrigger/SoundTriggerHelper.java b/services/voiceinteraction/java/com/android/server/soundtrigger/SoundTriggerHelper.java index 24ce7e76a49c6..c8bcb83cae74e 100644 --- a/services/voiceinteraction/java/com/android/server/soundtrigger/SoundTriggerHelper.java +++ b/services/voiceinteraction/java/com/android/server/soundtrigger/SoundTriggerHelper.java @@ -84,6 +84,9 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { private static final int INVALID_VALUE = Integer.MIN_VALUE; + /** Maximum time to wait for a model stop confirmation before giving up. */ + private static final long STOP_TIMEOUT_MS = 5000; + /** The {@link ModuleProperties} for the system, or null if none exists. */ final ModuleProperties mModuleProperties; @@ -831,7 +834,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { } if (!event.recognitionStillActive) { - model.setStopped(); + model.setStoppedLocked(); } try { @@ -918,7 +921,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { MetricsLogger.count(mContext, "sth_recognition_aborted", 1); ModelData modelData = getModelDataForLocked(event.soundModelHandle); if (modelData != null && modelData.isModelStarted()) { - modelData.setStopped(); + modelData.setStoppedLocked(); try { modelData.getCallback().onRecognitionPaused(); } catch (DeadObjectException e) { @@ -972,7 +975,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { } if (!event.recognitionStillActive) { - modelData.setStopped(); + modelData.setStoppedLocked(); } try { @@ -1200,7 +1203,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { if (modelData.isModelStarted()) { Slog.d(TAG, "Stopping previously started dangling model " + modelData.getHandle()); if (mModule.stopRecognition(modelData.getHandle()) == STATUS_OK) { - modelData.setStopped(); + modelData.setStoppedLocked(); modelData.setRequested(false); } else { Slog.e(TAG, "Failed to stop model " + modelData.getHandle()); @@ -1249,7 +1252,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { private ModelData getOrCreateGenericModelDataLocked(UUID modelId) { ModelData modelData = mModelDataMap.get(modelId); if (modelData == null) { - modelData = ModelData.createGenericModelData(modelId); + modelData = createGenericModelData(modelId); mModelDataMap.put(modelId, modelData); } else if (!modelData.isGenericModel()) { Slog.e(TAG, "UUID already used for non-generic model."); @@ -1281,7 +1284,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { mKeyphraseUuidMap.remove(keyphraseId); mModelDataMap.remove(modelId); mKeyphraseUuidMap.put(keyphraseId, modelId); - ModelData modelData = ModelData.createKeyphraseModelData(modelId); + ModelData modelData = createKeyphraseModelData(modelId); mModelDataMap.put(modelId, modelData); return modelData; } @@ -1413,18 +1416,26 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { Slog.w(TAG, "RemoteException in onError", e); } } - } else { - modelData.setStopped(); - MetricsLogger.count(mContext, "sth_stop_recognition_success", 1); - // Notify of pause if needed. - if (notify) { - try { - callback.onRecognitionPaused(); - } catch (DeadObjectException e) { - forceStopAndUnloadModelLocked(modelData, e); - } catch (RemoteException e) { - Slog.w(TAG, "RemoteException in onRecognitionPaused", e); - } + return status; + } + + // Wait for model to be stopped. + try { + modelData.waitStoppedLocked(STOP_TIMEOUT_MS); + } catch (InterruptedException e) { + Slog.e(TAG, "Didn't receive model stop callback"); + return SoundTrigger.STATUS_ERROR; + } + + MetricsLogger.count(mContext, "sth_stop_recognition_success", 1); + // Notify of pause if needed. + if (notify) { + try { + callback.onRecognitionPaused(); + } catch (DeadObjectException e) { + forceStopAndUnloadModelLocked(modelData, e); + } catch (RemoteException e) { + Slog.w(TAG, "RemoteException in onRecognitionPaused", e); } } if (DBG) { @@ -1459,7 +1470,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { // This class encapsulates the callbacks, state, handles and any other information that // represents a model. - private static class ModelData { + private class ModelData { // Model not loaded (and hence not started). static final int MODEL_NOTLOADED = 0; @@ -1516,17 +1527,9 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { mModelType = modelType; } - static ModelData createKeyphraseModelData(UUID modelId) { - return new ModelData(modelId, SoundModel.TYPE_KEYPHRASE); - } - - static ModelData createGenericModelData(UUID modelId) { - return new ModelData(modelId, SoundModel.TYPE_GENERIC_SOUND); - } - // Note that most of the functionality in this Java class will not work for // SoundModel.TYPE_UNKNOWN nevertheless we have it since lower layers support it. - static ModelData createModelDataOfUnknownType(UUID modelId) { + ModelData createModelDataOfUnknownType(UUID modelId) { return new ModelData(modelId, SoundModel.TYPE_UNKNOWN); } @@ -1550,8 +1553,20 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { mModelState = MODEL_STARTED; } - synchronized void setStopped() { + synchronized void setStoppedLocked() { mModelState = MODEL_LOADED; + mLock.notifyAll(); + } + + void waitStoppedLocked(long timeoutMs) throws InterruptedException { + long deadline = System.currentTimeMillis() + timeoutMs; + while (mModelState == MODEL_STARTED) { + long waitTime = deadline - System.currentTimeMillis(); + if (waitTime <= 0) { + throw new InterruptedException(); + } + mLock.wait(waitTime); + } } synchronized void setLoaded() { @@ -1571,6 +1586,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { mRecognitionConfig = null; mRequested = false; mCallback = null; + notifyAll(); } synchronized void clearCallback() { @@ -1675,4 +1691,12 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { return "Model type: " + type + "\n"; } } + + ModelData createKeyphraseModelData(UUID modelId) { + return new ModelData(modelId, SoundModel.TYPE_KEYPHRASE); + } + + ModelData createGenericModelData(UUID modelId) { + return new ModelData(modelId, SoundModel.TYPE_GENERIC_SOUND); + } }