From 8dc40270b75e7d83f26e29c96a96a683da4180b8 Mon Sep 17 00:00:00 2001 From: Ytai Ben-Tsvi Date: Wed, 16 Feb 2022 18:08:51 -0800 Subject: [PATCH] Explicitly close sound model file descriptors This is a workaround for the fact that currently, relying on the GC to release those resources is unreliable and on the other hand having an explicit close() that isn't tightly coupled with the internal details of the parcelable is not possible (filed a bug). Test: Manual verification of soundtrigger operation. Test: atest com.android.server.soundtrigger_middleware.SoundHw2CompatTest Fixes: 219411458 Change-Id: I43bd43ca80eadf97f0254678b0e817fc6f8a06b2 Merged-In: I43bd43ca80eadf97f0254678b0e817fc6f8a06b2 (cherry picked from commit 578eadd1b4227c79972eb87a5856a7f6c20fb4ef) --- .../soundtrigger/SoundTriggerModule.java | 32 ++++++++- .../SoundTriggerHw2Compat.java | 24 +++++++ .../SoundHw2CompatTest.java | 29 ++++---- .../soundtrigger_middleware/TestUtil.java | 69 +++++++++++-------- 4 files changed, 111 insertions(+), 43 deletions(-) diff --git a/core/java/android/hardware/soundtrigger/SoundTriggerModule.java b/core/java/android/hardware/soundtrigger/SoundTriggerModule.java index bf4b51431c6fa..eed92c12eb83a 100644 --- a/core/java/android/hardware/soundtrigger/SoundTriggerModule.java +++ b/core/java/android/hardware/soundtrigger/SoundTriggerModule.java @@ -37,6 +37,8 @@ import android.os.Message; import android.os.RemoteException; import android.util.Log; +import java.io.IOException; + /** * The SoundTriggerModule provides APIs to control sound models and sound detection * on a given sound trigger hardware module. @@ -137,13 +139,39 @@ public class SoundTriggerModule { if (model instanceof SoundTrigger.GenericSoundModel) { SoundModel aidlModel = ConversionUtil.api2aidlGenericSoundModel( (SoundTrigger.GenericSoundModel) model); - soundModelHandle[0] = mService.loadModel(aidlModel); + try { + soundModelHandle[0] = mService.loadModel(aidlModel); + } finally { + // TODO(b/219825762): We should be able to use the entire object in a + // try-with-resources + // clause, instead of having to explicitly close internal fields. + if (aidlModel.data != null) { + try { + aidlModel.data.close(); + } catch (IOException e) { + Log.e(TAG, "Failed to close file", e); + } + } + } return SoundTrigger.STATUS_OK; } if (model instanceof SoundTrigger.KeyphraseSoundModel) { PhraseSoundModel aidlModel = ConversionUtil.api2aidlPhraseSoundModel( (SoundTrigger.KeyphraseSoundModel) model); - soundModelHandle[0] = mService.loadPhraseModel(aidlModel); + try { + soundModelHandle[0] = mService.loadPhraseModel(aidlModel); + } finally { + // TODO(b/219825762): We should be able to use the entire object in a + // try-with-resources + // clause, instead of having to explicitly close internal fields. + if (aidlModel.common.data != null) { + try { + aidlModel.common.data.close(); + } catch (IOException e) { + Log.e(TAG, "Failed to close file", e); + } + } + } return SoundTrigger.STATUS_OK; } return SoundTrigger.STATUS_BAD_VALUE; diff --git a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerHw2Compat.java b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerHw2Compat.java index c638201bf8937..9bbae4bd2db5a 100644 --- a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerHw2Compat.java +++ b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerHw2Compat.java @@ -29,7 +29,9 @@ import android.os.IBinder; import android.os.IHwBinder; import android.os.RemoteException; import android.system.OsConstants; +import android.util.Log; +import java.io.IOException; import java.util.HashMap; import java.util.Map; import java.util.Objects; @@ -54,6 +56,8 @@ import java.util.concurrent.atomic.AtomicReference; * */ final class SoundTriggerHw2Compat implements ISoundTriggerHal { + private static final String TAG = "SoundTriggerHw2Compat"; + private final @NonNull Runnable mRebootRunnable; private final @NonNull IHwBinder mBinder; private @NonNull android.hardware.soundtrigger.V2_0.ISoundTriggerHw mUnderlying_2_0; @@ -226,6 +230,16 @@ final class SoundTriggerHw2Compat implements ISoundTriggerHal { return handle.get(); } catch (RemoteException e) { throw e.rethrowAsRuntimeException(); + } finally { + // TODO(b/219825762): We should be able to use the entire object in a try-with-resources + // clause, instead of having to explicitly close internal fields. + if (hidlModel.data != null) { + try { + hidlModel.data.close(); + } catch (IOException e) { + Log.e(TAG, "Failed to close file", e); + } + } } } @@ -252,6 +266,16 @@ final class SoundTriggerHw2Compat implements ISoundTriggerHal { return handle.get(); } catch (RemoteException e) { throw e.rethrowAsRuntimeException(); + } finally { + // TODO(b/219825762): We should be able to use the entire object in a try-with-resources + // clause, instead of having to explicitly close internal fields. + if (hidlModel.common.data != null) { + try { + hidlModel.common.data.close(); + } catch (IOException e) { + Log.e(TAG, "Failed to close file", e); + } + } } } diff --git a/services/tests/servicestests/src/com/android/server/soundtrigger_middleware/SoundHw2CompatTest.java b/services/tests/servicestests/src/com/android/server/soundtrigger_middleware/SoundHw2CompatTest.java index 16cfd13b18c22..6bdd88c6a7126 100644 --- a/services/tests/servicestests/src/com/android/server/soundtrigger_middleware/SoundHw2CompatTest.java +++ b/services/tests/servicestests/src/com/android/server/soundtrigger_middleware/SoundHw2CompatTest.java @@ -31,7 +31,6 @@ import static org.mockito.Mockito.atMost; import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -49,7 +48,6 @@ import android.os.IBinder; import android.os.IHwBinder; import android.os.IHwInterface; import android.os.RemoteException; -import android.system.OsConstants; import org.junit.After; import org.junit.Before; @@ -60,6 +58,7 @@ import org.mockito.ArgumentCaptor; import java.util.LinkedList; import java.util.List; +import java.util.concurrent.atomic.AtomicReference; @RunWith(Parameterized.class) public class SoundHw2CompatTest { @@ -240,14 +239,15 @@ public class SoundHw2CompatTest { (android.hardware.soundtrigger.V2_1.ISoundTriggerHw) mHalDriver; final int handle = 29; - ArgumentCaptor modelCaptor = - ArgumentCaptor.forClass( - android.hardware.soundtrigger.V2_1.ISoundTriggerHw.SoundModel.class); + AtomicReference model = + new AtomicReference<>(); ArgumentCaptor callbackCaptor = ArgumentCaptor.forClass( android.hardware.soundtrigger.V2_1.ISoundTriggerHwCallback.class); doAnswer(invocation -> { + // We need to dup the model, as it gets invalidated after the call returns. + model.set(TestUtil.dupModel_2_1(invocation.getArgument(0))); android.hardware.soundtrigger.V2_1.ISoundTriggerHw.loadSoundModel_2_1Callback resultCallback = invocation.getArgument(3); @@ -259,10 +259,9 @@ public class SoundHw2CompatTest { assertEquals(handle, mCanonical.loadSoundModel(TestUtil.createGenericSoundModel(), canonicalCallback)); - verify(driver_2_1).loadSoundModel_2_1(modelCaptor.capture(), callbackCaptor.capture(), - anyInt(), any()); + verify(driver_2_1).loadSoundModel_2_1(any(), callbackCaptor.capture(), anyInt(), any()); - TestUtil.validateGenericSoundModel_2_1(modelCaptor.getValue()); + TestUtil.validateGenericSoundModel_2_1(model.get()); validateCallback_2_1(callbackCaptor.getValue(), canonicalCallback); return handle; } @@ -355,14 +354,16 @@ public class SoundHw2CompatTest { (android.hardware.soundtrigger.V2_1.ISoundTriggerHw) mHalDriver; final int handle = 29; - ArgumentCaptor - modelCaptor = ArgumentCaptor.forClass( - android.hardware.soundtrigger.V2_1.ISoundTriggerHw.PhraseSoundModel.class); + AtomicReference model = + new AtomicReference<>(); ArgumentCaptor callbackCaptor = ArgumentCaptor.forClass( android.hardware.soundtrigger.V2_1.ISoundTriggerHwCallback.class); doAnswer(invocation -> { + // We need to dup the model, as it gets invalidated after the call returns. + model.set(TestUtil.dupPhraseModel_2_1(invocation.getArgument(0))); + android.hardware.soundtrigger.V2_1.ISoundTriggerHw.loadPhraseSoundModel_2_1Callback resultCallback = invocation.getArgument(3); @@ -374,10 +375,10 @@ public class SoundHw2CompatTest { assertEquals(handle, mCanonical.loadPhraseSoundModel(TestUtil.createPhraseSoundModel(), canonicalCallback)); - verify(driver_2_1).loadPhraseSoundModel_2_1(modelCaptor.capture(), callbackCaptor.capture(), - anyInt(), any()); + verify(driver_2_1).loadPhraseSoundModel_2_1(any(), callbackCaptor.capture(), anyInt(), + any()); - TestUtil.validatePhraseSoundModel_2_1(modelCaptor.getValue()); + TestUtil.validatePhraseSoundModel_2_1(model.get()); validateCallback_2_1(callbackCaptor.getValue(), canonicalCallback); return handle; } diff --git a/services/tests/servicestests/src/com/android/server/soundtrigger_middleware/TestUtil.java b/services/tests/servicestests/src/com/android/server/soundtrigger_middleware/TestUtil.java index e687a2adeb396..30b4a59b32b1a 100644 --- a/services/tests/servicestests/src/com/android/server/soundtrigger_middleware/TestUtil.java +++ b/services/tests/servicestests/src/com/android/server/soundtrigger_middleware/TestUtil.java @@ -47,6 +47,7 @@ import android.os.HidlMemoryUtil; import android.os.ParcelFileDescriptor; import android.os.SharedMemory; +import java.io.IOException; import java.nio.ByteBuffer; import java.util.List; @@ -82,6 +83,19 @@ class TestUtil { HidlMemoryUtil.hidlMemoryToByteArray(model.data)); } + static android.hardware.soundtrigger.V2_1.ISoundTriggerHw.SoundModel dupModel_2_1( + android.hardware.soundtrigger.V2_1.ISoundTriggerHw.SoundModel model) { + android.hardware.soundtrigger.V2_1.ISoundTriggerHw.SoundModel dup = + new android.hardware.soundtrigger.V2_1.ISoundTriggerHw.SoundModel(); + dup.header = model.header; + try { + dup.data = model.data.dup(); + } catch (IOException e) { + throw new RuntimeException(e); + } + return dup; + } + private static void validateSoundModel_2_0( android.hardware.soundtrigger.V2_0.ISoundTriggerHw.SoundModel model, int type) { assertEquals(type, model.type); @@ -121,6 +135,15 @@ class TestUtil { validatePhrases_2_0(model.phrases); } + static android.hardware.soundtrigger.V2_1.ISoundTriggerHw.PhraseSoundModel dupPhraseModel_2_1( + android.hardware.soundtrigger.V2_1.ISoundTriggerHw.PhraseSoundModel model) { + android.hardware.soundtrigger.V2_1.ISoundTriggerHw.PhraseSoundModel dup = + new android.hardware.soundtrigger.V2_1.ISoundTriggerHw.PhraseSoundModel(); + dup.common = dupModel_2_1(model.common); + dup.phrases = model.phrases; + return dup; + } + static void validatePhraseSoundModel_2_0( android.hardware.soundtrigger.V2_0.ISoundTriggerHw.PhraseSoundModel model) { validateSoundModel_2_0(model.common, @@ -190,31 +213,27 @@ class TestUtil { properties.maxKeyPhrases = 567; properties.maxUsers = 678; properties.recognitionModes = - RecognitionMode.VOICE_TRIGGER - | RecognitionMode.USER_IDENTIFICATION - | RecognitionMode.USER_AUTHENTICATION - | RecognitionMode.GENERIC_TRIGGER; + RecognitionMode.VOICE_TRIGGER | RecognitionMode.USER_IDENTIFICATION + | RecognitionMode.USER_AUTHENTICATION | RecognitionMode.GENERIC_TRIGGER; properties.captureTransition = true; properties.maxBufferMs = 321; properties.concurrentCapture = supportConcurrentCapture; properties.triggerInEvent = true; properties.powerConsumptionMw = 432; properties.supportedModelArch = "supportedModelArch"; - properties.audioCapabilities = AudioCapabilities.ECHO_CANCELLATION - | AudioCapabilities.NOISE_SUPPRESSION; + properties.audioCapabilities = + AudioCapabilities.ECHO_CANCELLATION | AudioCapabilities.NOISE_SUPPRESSION; return properties; } - static void validateDefaultProperties(Properties properties, - boolean supportConcurrentCapture) { + static void validateDefaultProperties(Properties properties, boolean supportConcurrentCapture) { validateDefaultProperties(properties, supportConcurrentCapture, AudioCapabilities.ECHO_CANCELLATION | AudioCapabilities.NOISE_SUPPRESSION, "supportedModelArch"); } - static void validateDefaultProperties(Properties properties, - boolean supportConcurrentCapture, @AudioCapabilities int audioCapabilities, - @NonNull String supportedModelArch) { + static void validateDefaultProperties(Properties properties, boolean supportConcurrentCapture, + @AudioCapabilities int audioCapabilities, @NonNull String supportedModelArch) { assertEquals("implementor", properties.implementor); assertEquals("description", properties.description); assertEquals(123, properties.version); @@ -222,10 +241,9 @@ class TestUtil { assertEquals(456, properties.maxSoundModels); assertEquals(567, properties.maxKeyPhrases); assertEquals(678, properties.maxUsers); - assertEquals(RecognitionMode.GENERIC_TRIGGER - | RecognitionMode.USER_AUTHENTICATION - | RecognitionMode.USER_IDENTIFICATION - | RecognitionMode.VOICE_TRIGGER, properties.recognitionModes); + assertEquals(RecognitionMode.GENERIC_TRIGGER | RecognitionMode.USER_AUTHENTICATION + | RecognitionMode.USER_IDENTIFICATION | RecognitionMode.VOICE_TRIGGER, + properties.recognitionModes); assertTrue(properties.captureTransition); assertEquals(321, properties.maxBufferMs); assertEquals(supportConcurrentCapture, properties.concurrentCapture); @@ -246,8 +264,8 @@ class TestUtil { config.phraseRecognitionExtras[0].levels[0].userId = 234; config.phraseRecognitionExtras[0].levels[0].levelPercent = 34; config.data = new byte[]{5, 4, 3, 2, 1}; - config.audioCapabilities = AudioCapabilities.ECHO_CANCELLATION - | AudioCapabilities.NOISE_SUPPRESSION; + config.audioCapabilities = + AudioCapabilities.ECHO_CANCELLATION | AudioCapabilities.NOISE_SUPPRESSION; return config; } @@ -295,13 +313,12 @@ class TestUtil { int captureHandle) { validateRecognitionConfig_2_1(config.base, captureDevice, captureHandle); - assertEquals(AudioCapabilities.ECHO_CANCELLATION - | AudioCapabilities.NOISE_SUPPRESSION, config.audioCapabilities); + assertEquals(AudioCapabilities.ECHO_CANCELLATION | AudioCapabilities.NOISE_SUPPRESSION, + config.audioCapabilities); } static android.hardware.soundtrigger.V2_0.ISoundTriggerHwCallback.RecognitionEvent createRecognitionEvent_2_0( - int hwHandle, - int status) { + int hwHandle, int status) { android.hardware.soundtrigger.V2_0.ISoundTriggerHwCallback.RecognitionEvent halEvent = new android.hardware.soundtrigger.V2_0.ISoundTriggerHwCallback.RecognitionEvent(); halEvent.status = status; @@ -351,8 +368,7 @@ class TestUtil { return event; } - static ISoundTriggerHwCallback.RecognitionEvent createRecognitionEvent_2_1( - int hwHandle, + static ISoundTriggerHwCallback.RecognitionEvent createRecognitionEvent_2_1(int hwHandle, int status) { ISoundTriggerHwCallback.RecognitionEvent halEvent = new ISoundTriggerHwCallback.RecognitionEvent(); @@ -386,8 +402,7 @@ class TestUtil { PhraseRecognitionExtra extra = new PhraseRecognitionExtra(); extra.id = 123; extra.confidenceLevel = 52; - extra.recognitionModes = RecognitionMode.VOICE_TRIGGER - | RecognitionMode.GENERIC_TRIGGER; + extra.recognitionModes = RecognitionMode.VOICE_TRIGGER | RecognitionMode.GENERIC_TRIGGER; ConfidenceLevel level = new ConfidenceLevel(); level.userId = 31; level.levelPercent = 43; @@ -396,8 +411,8 @@ class TestUtil { return event; } - static android.hardware.soundtrigger.V2_0.ISoundTriggerHwCallback.PhraseRecognitionEvent - createPhraseRecognitionEvent_2_0(int hwHandle, int status) { + static android.hardware.soundtrigger.V2_0.ISoundTriggerHwCallback.PhraseRecognitionEvent createPhraseRecognitionEvent_2_0( + int hwHandle, int status) { android.hardware.soundtrigger.V2_0.ISoundTriggerHwCallback.PhraseRecognitionEvent halEvent = new android.hardware.soundtrigger.V2_0.ISoundTriggerHwCallback.PhraseRecognitionEvent(); halEvent.common = createRecognitionEvent_2_0(hwHandle, status);