Add a final lux method to ALSProbe.

This address two remaining issues with with ALS logging since the pointer events were removed from the framework (starting at commit aa07cc3db3). 1) After the pointer events were removed the logging was updated to use wakefullness events as an alternative trigger. However, these events are often too slow when authenticating from the AoD and the sensor may not have a chance to be enabled or respond before the operation completes. The new method allows the sensor to be enabled after the operation has completed to fetch a value. 2) Fixes a bug where the initial wakefullness status was not checked at the start of an auth operation. Fix: 253318983 Test: atest ALSProbeTest FingerprintAuthenticationClientTest BiometricLoggerTest Test: statsd_testdrive 88 (verify lux field is always greater than 0 from AoD and with BP test app) Change-Id: I2b1572579f52dc9dad7238d47e27b0068c7c74c6
2022-10-12 22:21:48 -07:00
parent 26ed84b746
commit 4613aa9552
7 changed files with 293 additions and 18 deletions
--- a/services/core/java/com/android/server/biometrics/log/ALSProbe.java
+++ b/services/core/java/com/android/server/biometrics/log/ALSProbe.java
@@ -30,7 +30,10 @@ import android.util.Slog;
 import com.android.internal.annotations.VisibleForTesting;
 import com.android.server.biometrics.sensors.BaseClientMonitor;

+import java.util.ArrayList;
+import java.util.List;
 import java.util.concurrent.TimeUnit;
+import java.util.function.Consumer;

 /** Probe for ambient light. */
 final class ALSProbe implements Probe {
@@ -47,12 +50,18 @@ final class ALSProbe implements Probe {

    private boolean mEnabled = false;
    private boolean mDestroyed = false;
+    private boolean mDestroyRequested = false;
+    private boolean mDisableRequested = false;
+    private volatile NextConsumer mNextConsumer = null;
    private volatile float mLastAmbientLux = -1;

    private final SensorEventListener mLightSensorListener = new SensorEventListener() {
        @Override
        public void onSensorChanged(SensorEvent event) {
            mLastAmbientLux = event.values[0];
+            if (mNextConsumer != null) {
+                completeNextConsumer(mLastAmbientLux);
+            }
        }

        @Override
@@ -102,29 +111,84 @@ final class ALSProbe implements Probe {

    @Override
    public synchronized void enable() {
-        if (!mDestroyed) {
+        if (!mDestroyed && !mDestroyRequested) {
+            mDisableRequested = false;
            enableLightSensorLoggingLocked();
        }
    }

    @Override
    public synchronized void disable() {
-        if (!mDestroyed) {
+        mDisableRequested = true;
+
+        // if a final consumer is set it will call destroy/disable on the next value if requested
+        if (!mDestroyed && mNextConsumer == null) {
            disableLightSensorLoggingLocked();
        }
    }

    @Override
    public synchronized void destroy() {
-        disable();
-        mDestroyed = true;
+        mDestroyRequested = true;
+
+        // if a final consumer is set it will call destroy/disable on the next value if requested
+        if (!mDestroyed && mNextConsumer == null) {
+            disable();
+            mDestroyed = true;
+        }
    }

    /** The most recent lux reading. */
-    public float getCurrentLux() {
+    public float getMostRecentLux() {
        return mLastAmbientLux;
    }

+    /**
+     * Register a listener for the next available ALS reading, which will be reported to the given
+     * consumer even if this probe is {@link #disable()}'ed or {@link #destroy()}'ed before a value
+     * is available.
+     *
+     * This method is intended to be used for event logs that occur when the screen may be
+     * off and sampling may have been {@link #disable()}'ed. In these cases, this method will turn
+     * on the sensor (if needed), fetch & report the first value, and then destroy or disable this
+     * probe (if needed).
+     *
+     * @param consumer consumer to notify when the data is available
+     * @param handler handler for notifying the consumer, or null
+     */
+    public synchronized void awaitNextLux(@NonNull Consumer<Float> consumer,
+            @Nullable Handler handler) {
+        final NextConsumer nextConsumer = new NextConsumer(consumer, handler);
+        final float current = mLastAmbientLux;
+        if (current > 0) {
+            nextConsumer.consume(current);
+        } else if (mDestroyed) {
+            nextConsumer.consume(-1f);
+        } else if (mNextConsumer != null) {
+            mNextConsumer.add(nextConsumer);
+        } else {
+            mNextConsumer = nextConsumer;
+            enableLightSensorLoggingLocked();
+        }
+    }
+
+    private synchronized void completeNextConsumer(float value) {
+        Slog.v(TAG, "Finishing next consumer");
+
+        final NextConsumer consumer = mNextConsumer;
+        mNextConsumer = null;
+
+        if (mDestroyRequested) {
+            destroy();
+        } else if (mDisableRequested) {
+            disable();
+        }
+
+        if (consumer != null) {
+            consumer.consume(value);
+        }
+    }
+
    private void enableLightSensorLoggingLocked() {
        if (!mEnabled) {
            mEnabled = true;
@@ -160,4 +224,30 @@ final class ALSProbe implements Probe {
                + mLightSensorListener.hashCode());
        disable();
    }
+
+    private static class NextConsumer {
+        @NonNull private final Consumer<Float> mConsumer;
+        @Nullable private final Handler mHandler;
+        @NonNull private final List<NextConsumer> mOthers = new ArrayList<>();
+
+        private NextConsumer(@NonNull Consumer<Float> consumer, @Nullable Handler handler) {
+            mConsumer = consumer;
+            mHandler = handler;
+        }
+
+        public void consume(float value) {
+            if (mHandler != null) {
+                mHandler.post(() -> mConsumer.accept(value));
+            } else {
+                mConsumer.accept(value);
+            }
+            for (NextConsumer c : mOthers) {
+                c.consume(value);
+            }
+        }
+
+        public void add(NextConsumer consumer) {
+            mOthers.add(consumer);
+        }
+    }
 }
--- a/services/core/java/com/android/server/biometrics/log/BiometricFrameworkStatsLogger.java
+++ b/services/core/java/com/android/server/biometrics/log/BiometricFrameworkStatsLogger.java
@@ -62,8 +62,7 @@ public class BiometricFrameworkStatsLogger {
    /** {@see FrameworkStatsLog.BIOMETRIC_AUTHENTICATED}. */
    public void authenticate(OperationContext operationContext,
            int statsModality, int statsAction, int statsClient, boolean isDebug, long latency,
-            int authState, boolean requireConfirmation,
-            int targetUserId, float ambientLightLux) {
+            int authState, boolean requireConfirmation, int targetUserId, float ambientLightLux) {
        FrameworkStatsLog.write(FrameworkStatsLog.BIOMETRIC_AUTHENTICATED,
                statsModality,
                targetUserId,
@@ -80,6 +79,16 @@ public class BiometricFrameworkStatsLogger {
                operationContext.isAod);
    }

+    /** {@see FrameworkStatsLog.BIOMETRIC_AUTHENTICATED}. */
+    public void authenticate(OperationContext operationContext,
+            int statsModality, int statsAction, int statsClient, boolean isDebug, long latency,
+            int authState, boolean requireConfirmation, int targetUserId, ALSProbe alsProbe) {
+        alsProbe.awaitNextLux((ambientLightLux) -> {
+            authenticate(operationContext, statsModality, statsAction, statsClient, isDebug,
+                    latency, authState, requireConfirmation, targetUserId, ambientLightLux);
+        }, null /* handler */);
+    }
+
    /** {@see FrameworkStatsLog.BIOMETRIC_ENROLLED}. */
    public void enroll(int statsModality, int statsAction, int statsClient,
            int targetUserId, long latency, boolean enrollSuccessful, float ambientLightLux) {
--- a/services/core/java/com/android/server/biometrics/log/BiometricLogger.java
+++ b/services/core/java/com/android/server/biometrics/log/BiometricLogger.java
@@ -220,7 +220,7 @@ public class BiometricLogger {
                    + ", RequireConfirmation: " + requireConfirmation
                    + ", State: " + authState
                    + ", Latency: " + latency
-                    + ", Lux: " + mALSProbe.getCurrentLux());
+                    + ", Lux: " + mALSProbe.getMostRecentLux());
        } else {
            Slog.v(TAG, "Authentication latency: " + latency);
        }
@@ -231,7 +231,7 @@ public class BiometricLogger {

        mSink.authenticate(operationContext, mStatsModality, mStatsAction, mStatsClient,
                Utils.isDebugEnabled(context, targetUserId),
-                latency, authState, requireConfirmation, targetUserId, mALSProbe.getCurrentLux());
+                latency, authState, requireConfirmation, targetUserId, mALSProbe);
    }

    /** Log enrollment outcome. */
@@ -245,7 +245,7 @@ public class BiometricLogger {
                    + ", User: " + targetUserId
                    + ", Client: " + mStatsClient
                    + ", Latency: " + latency
-                    + ", Lux: " + mALSProbe.getCurrentLux()
+                    + ", Lux: " + mALSProbe.getMostRecentLux()
                    + ", Success: " + enrollSuccessful);
        } else {
            Slog.v(TAG, "Enroll latency: " + latency);
@@ -256,7 +256,7 @@ public class BiometricLogger {
        }

        mSink.enroll(mStatsModality, mStatsAction, mStatsClient,
-                targetUserId, latency, enrollSuccessful, mALSProbe.getCurrentLux());
+                targetUserId, latency, enrollSuccessful, mALSProbe.getMostRecentLux());
    }

    /** Report unexpected enrollment reported by the HAL. */
--- a/services/core/java/com/android/server/biometrics/sensors/fingerprint/aidl/FingerprintAuthenticationClient.java
+++ b/services/core/java/com/android/server/biometrics/sensors/fingerprint/aidl/FingerprintAuthenticationClient.java
@@ -333,6 +333,9 @@ class FingerprintAuthenticationClient extends AuthenticationClient<AidlSession>
                mALSProbeCallback.getProbe().disable();
            }
        });
+        if (getBiometricContext().isAwake()) {
+            mALSProbeCallback.getProbe().enable();
+        }

        if (session.hasContextMethods()) {
            return session.getSession().authenticateWithContext(mOperationId, opContext);
--- a/services/tests/servicestests/src/com/android/server/biometrics/log/ALSProbeTest.java
+++ b/services/tests/servicestests/src/com/android/server/biometrics/log/ALSProbeTest.java
@@ -23,6 +23,7 @@ import static org.mockito.Mockito.anyInt;
 import static org.mockito.Mockito.eq;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.reset;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
@@ -50,6 +51,9 @@ import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnit;
 import org.mockito.junit.MockitoRule;

+import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
+
@Presubmit
@SmallTest
@RunWith(AndroidTestingRunner.class)
@@ -93,7 +97,7 @@ public class ALSProbeTest {
        mSensorEventListenerCaptor.getValue().onSensorChanged(
                new SensorEvent(mLightSensor, 1, 2, new float[]{value}));

-        assertThat(mProbe.getCurrentLux()).isEqualTo(value);
+        assertThat(mProbe.getMostRecentLux()).isEqualTo(value);
    }

    @Test
@@ -121,13 +125,17 @@ public class ALSProbeTest {
        mProbe.destroy();
        mProbe.enable();

+        AtomicInteger lux = new AtomicInteger(10);
+        mProbe.awaitNextLux((v) -> lux.set(Math.round(v)), null /* handler */);
+
        verify(mSensorManager, never()).registerListener(any(), any(), anyInt());
        verifyNoMoreInteractions(mSensorManager);
+        assertThat(lux.get()).isLessThan(0);
    }

    @Test
    public void testDisabledReportsNegativeValue() {
-        assertThat(mProbe.getCurrentLux()).isLessThan(0f);
+        assertThat(mProbe.getMostRecentLux()).isLessThan(0f);

        mProbe.enable();
        verify(mSensorManager).registerListener(
@@ -136,7 +144,7 @@ public class ALSProbeTest {
                new SensorEvent(mLightSensor, 1, 1, new float[]{4.0f}));
        mProbe.disable();

-        assertThat(mProbe.getCurrentLux()).isLessThan(0f);
+        assertThat(mProbe.getMostRecentLux()).isLessThan(0f);
    }

    @Test
@@ -150,7 +158,7 @@ public class ALSProbeTest {

        verify(mSensorManager).unregisterListener(eq(mSensorEventListenerCaptor.getValue()));
        verifyNoMoreInteractions(mSensorManager);
-        assertThat(mProbe.getCurrentLux()).isLessThan(0f);
+        assertThat(mProbe.getMostRecentLux()).isLessThan(0f);
    }

    @Test
@@ -166,7 +174,148 @@ public class ALSProbeTest {

        verify(mSensorManager).unregisterListener(any(SensorEventListener.class));
        verifyNoMoreInteractions(mSensorManager);
-        assertThat(mProbe.getCurrentLux()).isLessThan(0f);
+        assertThat(mProbe.getMostRecentLux()).isLessThan(0f);
+    }
+
+    @Test
+    public void testNextLuxWhenAlreadyEnabledAndNotAvailable() {
+        testNextLuxWhenAlreadyEnabled(false /* dataIsAvailable */);
+    }
+
+    @Test
+    public void testNextLuxWhenAlreadyEnabledAndAvailable() {
+        testNextLuxWhenAlreadyEnabled(true /* dataIsAvailable */);
+    }
+
+    private void testNextLuxWhenAlreadyEnabled(boolean dataIsAvailable) {
+        final List<Integer> values = List.of(1, 2, 3, 4, 6);
+        mProbe.enable();
+
+        verify(mSensorManager).registerListener(
+                mSensorEventListenerCaptor.capture(), any(), anyInt());
+
+        if (dataIsAvailable) {
+            for (int v : values) {
+                mSensorEventListenerCaptor.getValue().onSensorChanged(
+                        new SensorEvent(mLightSensor, 1, 1, new float[]{v}));
+            }
+        }
+        AtomicInteger lux = new AtomicInteger(-1);
+        mProbe.awaitNextLux((v) -> lux.set(Math.round(v)), null /* handler */);
+        if (!dataIsAvailable) {
+            for (int v : values) {
+                mSensorEventListenerCaptor.getValue().onSensorChanged(
+                        new SensorEvent(mLightSensor, 1, 1, new float[]{v}));
+            }
+        }
+
+        mSensorEventListenerCaptor.getValue().onSensorChanged(
+                new SensorEvent(mLightSensor, 1, 1, new float[]{200f}));
+
+        // should remain enabled
+        assertThat(lux.get()).isEqualTo(values.get(dataIsAvailable ? values.size() - 1 : 0));
+        verify(mSensorManager, never()).unregisterListener(any(SensorEventListener.class));
+        verifyNoMoreInteractions(mSensorManager);
+
+        final int anotherValue = 12;
+        mSensorEventListenerCaptor.getValue().onSensorChanged(
+                new SensorEvent(mLightSensor, 1, 1, new float[]{12}));
+        assertThat(mProbe.getMostRecentLux()).isEqualTo(anotherValue);
+    }
+
+    @Test
+    public void testNextLuxWhenNotEnabled() {
+        testNextLuxWhenNotEnabled(false /* enableWhileWaiting */);
+    }
+
+    @Test
+    public void testNextLuxWhenNotEnabledButEnabledLater() {
+        testNextLuxWhenNotEnabled(true /* enableWhileWaiting */);
+    }
+
+    private void testNextLuxWhenNotEnabled(boolean enableWhileWaiting) {
+        final List<Integer> values = List.of(1, 2, 3, 4, 6);
+        mProbe.disable();
+
+        AtomicInteger lux = new AtomicInteger(-1);
+        mProbe.awaitNextLux((v) -> lux.set(Math.round(v)), null /* handler */);
+
+        if (enableWhileWaiting) {
+            mProbe.enable();
+        }
+
+        verify(mSensorManager).registerListener(
+                mSensorEventListenerCaptor.capture(), any(), anyInt());
+        for (int v : values) {
+            mSensorEventListenerCaptor.getValue().onSensorChanged(
+                    new SensorEvent(mLightSensor, 1, 1, new float[]{v}));
+        }
+
+        // should restore the disabled state
+        assertThat(lux.get()).isEqualTo(values.get(0));
+        verify(mSensorManager, enableWhileWaiting ? never() : times(1)).unregisterListener(
+                any(SensorEventListener.class));
+        verifyNoMoreInteractions(mSensorManager);
+    }
+
+    @Test
+    public void testNextLuxIsNotCanceledByDisableOrDestroy() {
+        final int value = 7;
+        AtomicInteger lux = new AtomicInteger(-1);
+        mProbe.awaitNextLux((v) -> lux.set(Math.round(v)), null /* handler */);
+
+        verify(mSensorManager).registerListener(
+                mSensorEventListenerCaptor.capture(), any(), anyInt());
+
+        mProbe.destroy();
+        mProbe.disable();
+
+        assertThat(lux.get()).isEqualTo(-1);
+
+        mSensorEventListenerCaptor.getValue().onSensorChanged(
+                new SensorEvent(mLightSensor, 1, 1, new float[]{value}));
+
+        assertThat(lux.get()).isEqualTo(value);
+
+        mSensorEventListenerCaptor.getValue().onSensorChanged(
+                new SensorEvent(mLightSensor, 1, 1, new float[]{value + 1}));
+
+        // should remain destroyed
+        mProbe.enable();
+
+        assertThat(lux.get()).isEqualTo(value);
+        verify(mSensorManager).unregisterListener(any(SensorEventListener.class));
+        verifyNoMoreInteractions(mSensorManager);
+    }
+
+    @Test
+    public void testMultipleNextConsumers() {
+        final int value = 7;
+        AtomicInteger lux = new AtomicInteger(-1);
+        AtomicInteger lux2 = new AtomicInteger(-1);
+        mProbe.awaitNextLux((v) -> lux.set(Math.round(v)), null /* handler */);
+        mProbe.awaitNextLux((v) -> lux2.set(Math.round(v)), null /* handler */);
+
+        verify(mSensorManager).registerListener(
+                mSensorEventListenerCaptor.capture(), any(), anyInt());
+        mSensorEventListenerCaptor.getValue().onSensorChanged(
+                new SensorEvent(mLightSensor, 1, 1, new float[]{value}));
+
+        assertThat(lux.get()).isEqualTo(value);
+        assertThat(lux2.get()).isEqualTo(value);
+    }
+
+    @Test
+    public void testNoNextLuxWhenDestroyed() {
+        mProbe.destroy();
+
+        AtomicInteger lux = new AtomicInteger(-20);
+        mProbe.awaitNextLux((v) -> lux.set(Math.round(v)), null /* handler */);
+
+        assertThat(lux.get()).isEqualTo(-1);
+        verify(mSensorManager, never()).registerListener(
+                mSensorEventListenerCaptor.capture(), any(), anyInt());
+        verifyNoMoreInteractions(mSensorManager);
    }

    private void moveTimeBy(long millis) {
--- a/services/tests/servicestests/src/com/android/server/biometrics/log/BiometricLoggerTest.java
+++ b/services/tests/servicestests/src/com/android/server/biometrics/log/BiometricLoggerTest.java
@@ -121,7 +121,7 @@ public class BiometricLoggerTest {
        verify(mSink).authenticate(eq(mOpContext),
                eq(DEFAULT_MODALITY), eq(DEFAULT_ACTION), eq(DEFAULT_CLIENT), anyBoolean(),
                anyLong(), anyInt(), eq(requireConfirmation),
-                eq(targetUserId), anyFloat());
+                eq(targetUserId), any());
    }

    @Test
--- a/services/tests/servicestests/src/com/android/server/biometrics/sensors/fingerprint/aidl/FingerprintAuthenticationClientTest.java
+++ b/services/tests/servicestests/src/com/android/server/biometrics/sensors/fingerprint/aidl/FingerprintAuthenticationClientTest.java
@@ -28,6 +28,7 @@ import static org.mockito.Mockito.inOrder;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.same;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;

@@ -215,7 +216,7 @@ public class FingerprintAuthenticationClientTest {

    @Test
    public void luxProbeWhenAwake() throws RemoteException {
-        when(mBiometricContext.isAwake()).thenReturn(false, true, false);
+        when(mBiometricContext.isAwake()).thenReturn(false);
        when(mBiometricContext.isAod()).thenReturn(false);
        final FingerprintAuthenticationClient client = createClient();
        client.start(mCallback);
@@ -228,14 +229,37 @@ public class FingerprintAuthenticationClientTest {
        verify(mLuxProbe, never()).enable();

        reset(mLuxProbe);
+        when(mBiometricContext.isAwake()).thenReturn(true);
+
        mContextInjector.getValue().accept(opContext);
        verify(mLuxProbe).enable();
        verify(mLuxProbe, never()).disable();

+        when(mBiometricContext.isAwake()).thenReturn(false);
+
        mContextInjector.getValue().accept(opContext);
        verify(mLuxProbe).disable();
    }

+    @Test
+    public void luxProbeEnabledOnStartWhenWake() throws RemoteException {
+        luxProbeEnabledOnStart(true /* isAwake */);
+    }
+
+    @Test
+    public void luxProbeNotEnabledOnStartWhenNotWake() throws RemoteException {
+        luxProbeEnabledOnStart(false /* isAwake */);
+    }
+
+    private void luxProbeEnabledOnStart(boolean isAwake) throws RemoteException {
+        when(mBiometricContext.isAwake()).thenReturn(isAwake);
+        when(mBiometricContext.isAod()).thenReturn(false);
+        final FingerprintAuthenticationClient client = createClient();
+        client.start(mCallback);
+
+        verify(mLuxProbe, isAwake ? times(1) : never()).enable();
+    }
+
    @Test
    public void luxProbeDisabledOnAod() throws RemoteException {
        when(mBiometricContext.isAwake()).thenReturn(false);