From e7c9c0b0408604f9862f8b90fad1ce79a740c3d3 Mon Sep 17 00:00:00 2001 From: Fiona Campbell Date: Thu, 23 Feb 2023 15:28:20 +0000 Subject: [PATCH] Only reset short term model after timeout Here, we create a separate class, used to store the vital short term model information; lux (aka, anchor), brightness and whether it is valid or not. When switching between interactive and idle screen brightness modes, we save out the current short term model to the "mPausedShortTermModel" and send an "invalidate" message, with a delay of the stm timeout. If we do time out, then we invalidate the model, and if not, then we will restore the model upon returning to the appropriate mode. For now, the ShortTermModel class is within ABC, but could be moved to BrightnessMappingStrategy later. Bug: 269557114 Test: atest com.android.server.display Change-Id: Ie3eed5a3acbb56f363f1ca8a61a6ecf64aae85c4 --- .../AutomaticBrightnessController.java | 180 ++++++++++++++---- .../display/BrightnessMappingStrategy.java | 6 +- .../AutomaticBrightnessControllerTest.java | 180 +++++++++++++++++- 3 files changed, 325 insertions(+), 41 deletions(-) diff --git a/services/core/java/com/android/server/display/AutomaticBrightnessController.java b/services/core/java/com/android/server/display/AutomaticBrightnessController.java index af5609a68952e..299f86550efd3 100644 --- a/services/core/java/com/android/server/display/AutomaticBrightnessController.java +++ b/services/core/java/com/android/server/display/AutomaticBrightnessController.java @@ -75,10 +75,11 @@ public class AutomaticBrightnessController { private static final int MSG_UPDATE_AMBIENT_LUX = 1; private static final int MSG_BRIGHTNESS_ADJUSTMENT_SAMPLE = 2; - private static final int MSG_INVALIDATE_SHORT_TERM_MODEL = 3; + private static final int MSG_INVALIDATE_CURRENT_SHORT_TERM_MODEL = 3; private static final int MSG_UPDATE_FOREGROUND_APP = 4; private static final int MSG_UPDATE_FOREGROUND_APP_SYNC = 5; private static final int MSG_RUN_UPDATE = 6; + private static final int MSG_INVALIDATE_PAUSED_SHORT_TERM_MODEL = 7; // Callbacks for requesting updates to the display's power state private final Callbacks mCallbacks; @@ -216,12 +217,11 @@ public class AutomaticBrightnessController { private float mBrightnessAdjustmentSampleOldLux; private float mBrightnessAdjustmentSampleOldBrightness; - // When the short term model is invalidated, we don't necessarily reset it (i.e. clear the - // user's adjustment) immediately, but wait for a drastic enough change in the ambient light. - // The anchor determines what were the light levels when the user has set their preference, and - // we use a relative threshold to determine when to revert to the OEM curve. - private boolean mShortTermModelValid; - private float mShortTermModelAnchor; + // The short term models, current and previous. Eg, we might use the "paused" one to save out + // the interactive short term model when switching to idle screen brightness mode, and + // vice-versa. + private final ShortTermModel mShortTermModel; + private final ShortTermModel mPausedShortTermModel; // Controls High Brightness Mode. private HighBrightnessModeController mHbmController; @@ -309,8 +309,8 @@ public class AutomaticBrightnessController { mAmbientBrightnessThresholdsIdle = ambientBrightnessThresholdsIdle; mScreenBrightnessThresholds = screenBrightnessThresholds; mScreenBrightnessThresholdsIdle = screenBrightnessThresholdsIdle; - mShortTermModelValid = true; - mShortTermModelAnchor = -1; + mShortTermModel = new ShortTermModel(); + mPausedShortTermModel = new ShortTermModel(); mHandler = new AutomaticBrightnessHandler(looper); mAmbientLightRingBuffer = new AmbientLightRingBuffer(mNormalLightSensorRate, mAmbientLightHorizonLong, mClock); @@ -492,10 +492,10 @@ public class AutomaticBrightnessController { Slog.d(TAG, "Display policy transitioning from " + oldPolicy + " to " + policy); } if (!isInteractivePolicy(policy) && isInteractivePolicy(oldPolicy) && !isInIdleMode()) { - mHandler.sendEmptyMessageDelayed(MSG_INVALIDATE_SHORT_TERM_MODEL, + mHandler.sendEmptyMessageDelayed(MSG_INVALIDATE_CURRENT_SHORT_TERM_MODEL, mCurrentBrightnessMapper.getShortTermModelTimeout()); } else if (isInteractivePolicy(policy) && !isInteractivePolicy(oldPolicy)) { - mHandler.removeMessages(MSG_INVALIDATE_SHORT_TERM_MODEL); + mHandler.removeMessages(MSG_INVALIDATE_CURRENT_SHORT_TERM_MODEL); } return true; } @@ -516,25 +516,13 @@ public class AutomaticBrightnessController { private boolean setScreenBrightnessByUser(float lux, float brightness) { mCurrentBrightnessMapper.addUserDataPoint(lux, brightness); - mShortTermModelValid = true; - mShortTermModelAnchor = lux; - if (mLoggingEnabled) { - Slog.d(TAG, "ShortTermModel: anchor=" + mShortTermModelAnchor); - } + mShortTermModel.setUserBrightness(lux, brightness); return true; } public void resetShortTermModel() { mCurrentBrightnessMapper.clearUserDataPoints(); - mShortTermModelValid = true; - mShortTermModelAnchor = -1; - } - - private void invalidateShortTermModel() { - if (mLoggingEnabled) { - Slog.d(TAG, "ShortTermModel: invalidate user data"); - } - mShortTermModelValid = false; + mShortTermModel.reset(); } public boolean setBrightnessConfiguration(BrightnessConfiguration configuration, @@ -595,8 +583,12 @@ public class AutomaticBrightnessController { pw.println(" mShortTermModelTimeout(idle)=" + mIdleModeBrightnessMapper.getShortTermModelTimeout()); } - pw.println(" mShortTermModelAnchor=" + mShortTermModelAnchor); - pw.println(" mShortTermModelValid=" + mShortTermModelValid); + pw.println(" mShortTermModel="); + mShortTermModel.dump(pw); + pw.println(" mPausedShortTermModel="); + mPausedShortTermModel.dump(pw); + + pw.println(); pw.println(" mBrightnessAdjustmentSamplePending=" + mBrightnessAdjustmentSamplePending); pw.println(" mBrightnessAdjustmentSampleOldLux=" + mBrightnessAdjustmentSampleOldLux); pw.println(" mBrightnessAdjustmentSampleOldBrightness=" @@ -740,15 +732,9 @@ public class AutomaticBrightnessController { } mHbmController.onAmbientLuxChange(mAmbientLux); + // If the short term model was invalidated and the change is drastic enough, reset it. - if (!mShortTermModelValid && mShortTermModelAnchor != -1) { - if (mCurrentBrightnessMapper.shouldResetShortTermModel( - mAmbientLux, mShortTermModelAnchor)) { - resetShortTermModel(); - } else { - mShortTermModelValid = true; - } - } + mShortTermModel.maybeReset(mAmbientLux); } private float calculateAmbientLux(long now, long horizon) { @@ -1118,8 +1104,29 @@ public class AutomaticBrightnessController { return; } Slog.i(TAG, "Switching to Idle Screen Brightness Mode"); + // Stash short term model + ShortTermModel tempShortTermModel = new ShortTermModel(); + tempShortTermModel.set(mCurrentBrightnessMapper.getUserLux(), + mCurrentBrightnessMapper.getUserBrightness(), /* valid= */ true); + + // Send delayed timeout + mHandler.sendEmptyMessageAtTime(MSG_INVALIDATE_PAUSED_SHORT_TERM_MODEL, + mClock.uptimeMillis() + + mCurrentBrightnessMapper.getShortTermModelTimeout()); + + Slog.i(TAG, "mPreviousShortTermModel" + mPausedShortTermModel); + // new brightness mapper mCurrentBrightnessMapper = mIdleModeBrightnessMapper; - resetShortTermModel(); + + // if previous stm has been invalidated, and lux has drastically changed, just use + // the new, reset stm. + // if previous stm is still valid then revalidate it + if (mPausedShortTermModel != null && !mPausedShortTermModel.maybeReset(mAmbientLux)) { + setScreenBrightnessByUser(mPausedShortTermModel.mAnchor, + mPausedShortTermModel.mBrightness); + } + mPausedShortTermModel.copyFrom(tempShortTermModel); + update(); } @@ -1128,8 +1135,28 @@ public class AutomaticBrightnessController { return; } Slog.i(TAG, "Switching to Interactive Screen Brightness Mode"); + ShortTermModel tempShortTermModel = new ShortTermModel(); + tempShortTermModel.set(mCurrentBrightnessMapper.getUserLux(), + mCurrentBrightnessMapper.getUserBrightness(), /* valid= */ true); + mHandler.removeMessages(MSG_INVALIDATE_PAUSED_SHORT_TERM_MODEL); + // Send delayed timeout + mHandler.sendEmptyMessageAtTime(MSG_INVALIDATE_PAUSED_SHORT_TERM_MODEL, + mClock.uptimeMillis() + + mCurrentBrightnessMapper.getShortTermModelTimeout()); + Slog.i(TAG, "mPreviousShortTermModel" + mPausedShortTermModel.toString()); + + // restore interactive mapper. mCurrentBrightnessMapper = mInteractiveModeBrightnessMapper; - resetShortTermModel(); + + // if previous stm has been invalidated, and lux has drastically changed, just use + // the new, reset stm. + // if previous stm is still valid then revalidate it + if (!mPausedShortTermModel.maybeReset(mAmbientLux)) { + setScreenBrightnessByUser(mPausedShortTermModel.mAnchor, + mPausedShortTermModel.mBrightness); + } + mPausedShortTermModel.copyFrom(tempShortTermModel); + update(); } @@ -1164,6 +1191,77 @@ public class AutomaticBrightnessController { } } + private class ShortTermModel { + // When the short term model is invalidated, we don't necessarily reset it (i.e. clear the + // user's adjustment) immediately, but wait for a drastic enough change in the ambient + // light. + // The anchor determines what were the light levels when the user has set their preference, + // and we use a relative threshold to determine when to revert to the OEM curve. + private float mAnchor = -1f; + private float mBrightness; + private boolean mIsValid = true; + + private void reset() { + mAnchor = -1f; + mBrightness = -1f; + mIsValid = true; + } + + private void invalidate() { + mIsValid = false; + if (mLoggingEnabled) { + Slog.d(TAG, "ShortTermModel: invalidate user data"); + } + } + + private void setUserBrightness(float lux, float brightness) { + mAnchor = lux; + mBrightness = brightness; + mIsValid = true; + if (mLoggingEnabled) { + Slog.d(TAG, "ShortTermModel: anchor=" + mAnchor); + } + } + + private boolean maybeReset(float currentLux) { + // If the short term model was invalidated and the change is drastic enough, reset it. + // Otherwise, we revalidate it. + if (!mIsValid && mAnchor != -1) { + if (mCurrentBrightnessMapper != null + && mCurrentBrightnessMapper.shouldResetShortTermModel( + currentLux, mAnchor)) { + resetShortTermModel(); + } else { + mIsValid = true; + } + return mIsValid; + } + return false; + } + + private void set(float anchor, float brightness, boolean valid) { + mAnchor = anchor; + mBrightness = brightness; + mIsValid = valid; + } + private void copyFrom(ShortTermModel from) { + mAnchor = from.mAnchor; + mBrightness = from.mBrightness; + mIsValid = from.mIsValid; + } + + public String toString() { + return " mAnchor: " + mAnchor + + "\n mBrightness: " + mBrightness + + "\n mIsValid: " + mIsValid; + } + + void dump(PrintWriter pw) { + pw.println(this); + } + + } + private final class AutomaticBrightnessHandler extends Handler { public AutomaticBrightnessHandler(Looper looper) { super(looper, null, true /*async*/); @@ -1184,8 +1282,8 @@ public class AutomaticBrightnessController { collectBrightnessAdjustmentSample(); break; - case MSG_INVALIDATE_SHORT_TERM_MODEL: - invalidateShortTermModel(); + case MSG_INVALIDATE_CURRENT_SHORT_TERM_MODEL: + mShortTermModel.invalidate(); break; case MSG_UPDATE_FOREGROUND_APP: @@ -1195,6 +1293,10 @@ public class AutomaticBrightnessController { case MSG_UPDATE_FOREGROUND_APP_SYNC: updateForegroundAppSync(); break; + + case MSG_INVALIDATE_PAUSED_SHORT_TERM_MODEL: + mPausedShortTermModel.invalidate(); + break; } } } diff --git a/services/core/java/com/android/server/display/BrightnessMappingStrategy.java b/services/core/java/com/android/server/display/BrightnessMappingStrategy.java index d0471837d79b3..3456e3e262dec 100644 --- a/services/core/java/com/android/server/display/BrightnessMappingStrategy.java +++ b/services/core/java/com/android/server/display/BrightnessMappingStrategy.java @@ -363,13 +363,17 @@ public abstract class BrightnessMappingStrategy { public abstract void recalculateSplines(boolean applyAdjustment, float[] adjustment); /** - * Returns the timeout for the short term model + * Returns the timeout, in milliseconds for the short term model * * Timeout after which we remove the effects any user interactions might've had on the * brightness mapping. This timeout doesn't start until we transition to a non-interactive * display policy so that we don't reset while users are using their devices, but also so that * we don't erroneously keep the short-term model if the device is dozing but the * display is fully on. + * + * This timeout is also used when the device switches from interactive screen brightness mode + * to idle screen brightness mode, to preserve the user's preference when they resume usage of + * the device, within the specified timeframe. */ public abstract long getShortTermModelTimeout(); diff --git a/services/tests/servicestests/src/com/android/server/display/AutomaticBrightnessControllerTest.java b/services/tests/servicestests/src/com/android/server/display/AutomaticBrightnessControllerTest.java index fc251526476e1..3bef413a3e723 100644 --- a/services/tests/servicestests/src/com/android/server/display/AutomaticBrightnessControllerTest.java +++ b/services/tests/servicestests/src/com/android/server/display/AutomaticBrightnessControllerTest.java @@ -126,7 +126,7 @@ public class AutomaticBrightnessControllerTest { return mClock::now; } - }, // pass in test looper instead, pass in offsetable clock + }, // pass in test looper instead, pass in offsettable clock () -> { }, mTestLooper.getLooper(), mSensorManager, lightSensor, mBrightnessMappingStrategy, LIGHT_SENSOR_WARMUP_TIME, BRIGHTNESS_MIN_FLOAT, BRIGHTNESS_MAX_FLOAT, DOZE_SCALE_FACTOR, LIGHT_SENSOR_RATE, @@ -299,6 +299,179 @@ public class AutomaticBrightnessControllerTest { verifyNoMoreInteractions(mBrightnessMappingStrategy); } + @Test + public void testShortTermModelTimesOut() throws Exception { + ArgumentCaptor listenerCaptor = + ArgumentCaptor.forClass(SensorEventListener.class); + verify(mSensorManager).registerListener(listenerCaptor.capture(), eq(mLightSensor), + eq(INITIAL_LIGHT_SENSOR_RATE * 1000), any(Handler.class)); + SensorEventListener listener = listenerCaptor.getValue(); + + // Sensor reads 123 lux, + listener.onSensorChanged(TestUtils.createSensorEvent(mLightSensor, 123)); + // User sets brightness to 100 + mController.configure(AUTO_BRIGHTNESS_ENABLED, /* configuration= */ null, + /* brightness= */ 0.5f, /* userChangedBrightness= */ true, /* adjustment= */ 0, + /* userChanged= */ false, DisplayPowerRequest.POLICY_BRIGHT, + /* shouldResetShortTermModel= */ true); + + when(mBrightnessMappingStrategy.getShortTermModelTimeout()).thenReturn(2000L); + + mController.switchToIdleMode(); + when(mIdleBrightnessMappingStrategy.isForIdleMode()).thenReturn(true); + when(mBrightnessMappingStrategy.shouldResetShortTermModel( + 123f, 0.5f)).thenReturn(true); + + // Sensor reads 1000 lux, + listener.onSensorChanged(TestUtils.createSensorEvent(mLightSensor, 1000)); + mTestLooper.moveTimeForward( + mBrightnessMappingStrategy.getShortTermModelTimeout() + 1000); + mTestLooper.dispatchAll(); + + mController.switchToInteractiveScreenBrightnessMode(); + mTestLooper.moveTimeForward(4000); + mTestLooper.dispatchAll(); + + // Verify only happens on the first configure. (i.e. not again when switching back) + // Intentionally using any() to ensure it's not called whatsoever. + verify(mBrightnessMappingStrategy, times(1)) + .addUserDataPoint(123.0f, 0.5f); + verify(mBrightnessMappingStrategy, times(1)) + .addUserDataPoint(anyFloat(), anyFloat()); + } + + @Test + public void testShortTermModelDoesntTimeOut() throws Exception { + ArgumentCaptor listenerCaptor = + ArgumentCaptor.forClass(SensorEventListener.class); + verify(mSensorManager).registerListener(listenerCaptor.capture(), eq(mLightSensor), + eq(INITIAL_LIGHT_SENSOR_RATE * 1000), any(Handler.class)); + SensorEventListener listener = listenerCaptor.getValue(); + + // Sensor reads 123 lux, + listener.onSensorChanged(TestUtils.createSensorEvent(mLightSensor, 123)); + // User sets brightness to 100 + mController.configure(AUTO_BRIGHTNESS_ENABLED, null /* configuration= */, + 0.51f /* brightness= */, true /* userChangedBrightness= */, 0 /* adjustment= */, + false /* userChanged= */, DisplayPowerRequest.POLICY_BRIGHT, + /* shouldResetShortTermModel= */ true); + + when(mBrightnessMappingStrategy.shouldResetShortTermModel( + anyFloat(), anyFloat())).thenReturn(true); + when(mBrightnessMappingStrategy.getShortTermModelTimeout()).thenReturn(2000L); + when(mBrightnessMappingStrategy.getUserBrightness()).thenReturn(0.51f); + when(mBrightnessMappingStrategy.getUserLux()).thenReturn(123.0f); + + mController.switchToIdleMode(); + when(mIdleBrightnessMappingStrategy.isForIdleMode()).thenReturn(true); + + // Time does not move forward, since clock is doesn't increment naturally. + mTestLooper.dispatchAll(); + + // Sensor reads 100000 lux, + listener.onSensorChanged(TestUtils.createSensorEvent(mLightSensor, 678910)); + mController.switchToInteractiveScreenBrightnessMode(); + + // Verify short term model is not reset. + verify(mBrightnessMappingStrategy, never()).clearUserDataPoints(); + + // Verify that we add the data point once when the user sets it, and again when we return + // interactive mode. + verify(mBrightnessMappingStrategy, times(2)) + .addUserDataPoint(123.0f, 0.51f); + } + + @Test + public void testShortTermModelIsRestoredWhenSwitchingWithinTimeout() throws Exception { + ArgumentCaptor listenerCaptor = + ArgumentCaptor.forClass(SensorEventListener.class); + verify(mSensorManager).registerListener(listenerCaptor.capture(), eq(mLightSensor), + eq(INITIAL_LIGHT_SENSOR_RATE * 1000), any(Handler.class)); + SensorEventListener listener = listenerCaptor.getValue(); + + // Sensor reads 123 lux, + listener.onSensorChanged(TestUtils.createSensorEvent(mLightSensor, 123)); + // User sets brightness to 100 + mController.configure(AUTO_BRIGHTNESS_ENABLED, /* configuration= */ null, + /* brightness= */ 0.5f, /* userChangedBrightness= */ true, /* adjustment= */ 0, + /* userChanged= */ false, DisplayPowerRequest.POLICY_BRIGHT, + /* shouldResetShortTermModel= */ true); + + when(mBrightnessMappingStrategy.getShortTermModelTimeout()).thenReturn(2000L); + when(mBrightnessMappingStrategy.getUserBrightness()).thenReturn(0.5f); + when(mBrightnessMappingStrategy.getUserLux()).thenReturn(123f); + + mController.switchToIdleMode(); + when(mIdleBrightnessMappingStrategy.isForIdleMode()).thenReturn(true); + when(mIdleBrightnessMappingStrategy.getUserBrightness()).thenReturn(-1f); + when(mIdleBrightnessMappingStrategy.getUserLux()).thenReturn(-1f); + when(mBrightnessMappingStrategy.shouldResetShortTermModel( + 123f, 0.5f)).thenReturn(true); + + // Sensor reads 1000 lux, + listener.onSensorChanged(TestUtils.createSensorEvent(mLightSensor, 1000)); + mTestLooper.moveTimeForward( + mBrightnessMappingStrategy.getShortTermModelTimeout() + 1000); + mTestLooper.dispatchAll(); + + mController.switchToInteractiveScreenBrightnessMode(); + mTestLooper.moveTimeForward(4000); + mTestLooper.dispatchAll(); + + // Verify only happens on the first configure. (i.e. not again when switching back) + // Intentionally using any() to ensure it's not called whatsoever. + verify(mBrightnessMappingStrategy, times(1)) + .addUserDataPoint(123.0f, 0.5f); + verify(mBrightnessMappingStrategy, times(1)) + .addUserDataPoint(anyFloat(), anyFloat()); + } + + @Test + public void testShortTermModelNotRestoredAfterTimeout() throws Exception { + ArgumentCaptor listenerCaptor = + ArgumentCaptor.forClass(SensorEventListener.class); + verify(mSensorManager).registerListener(listenerCaptor.capture(), eq(mLightSensor), + eq(INITIAL_LIGHT_SENSOR_RATE * 1000), any(Handler.class)); + SensorEventListener listener = listenerCaptor.getValue(); + + // Sensor reads 123 lux, + listener.onSensorChanged(TestUtils.createSensorEvent(mLightSensor, 123)); + // User sets brightness to 100 + mController.configure(AUTO_BRIGHTNESS_ENABLED, /* configuration= */ null, + /* brightness= */ 0.5f, /* userChangedBrightness= */ true, /* adjustment= */ 0, + /* userChanged= */ false, DisplayPowerRequest.POLICY_BRIGHT, + /* shouldResetShortTermModel= */ true); + + when(mBrightnessMappingStrategy.getShortTermModelTimeout()).thenReturn(2000L); + + when(mBrightnessMappingStrategy.getUserBrightness()).thenReturn(0.5f); + when(mBrightnessMappingStrategy.getUserLux()).thenReturn(123f); + + mController.switchToIdleMode(); + when(mIdleBrightnessMappingStrategy.isForIdleMode()).thenReturn(true); + when(mIdleBrightnessMappingStrategy.getUserBrightness()).thenReturn(-1f); + when(mIdleBrightnessMappingStrategy.getUserLux()).thenReturn(-1f); + + when(mBrightnessMappingStrategy.shouldResetShortTermModel( + 123f, 0.5f)).thenReturn(true); + + // Sensor reads 1000 lux, + listener.onSensorChanged(TestUtils.createSensorEvent(mLightSensor, 1000)); + // Do not fast-forward time. + mTestLooper.dispatchAll(); + + mController.switchToInteractiveScreenBrightnessMode(); + // Do not fast-forward time + mTestLooper.dispatchAll(); + + // Verify this happens on the first configure and again when switching back + // Intentionally using any() to ensure it's not called any other times whatsoever. + verify(mBrightnessMappingStrategy, times(2)) + .addUserDataPoint(123.0f, 0.5f); + verify(mBrightnessMappingStrategy, times(2)) + .addUserDataPoint(anyFloat(), anyFloat()); + } + @Test public void testSwitchToIdleMappingStrategy() throws Exception { ArgumentCaptor listenerCaptor = @@ -326,6 +499,11 @@ public class AutomaticBrightnessControllerTest { // Called once for init, and once when switching, // setAmbientLux() is called twice and once in updateAutoBrightness() verify(mBrightnessMappingStrategy, times(5)).isForIdleMode(); + // Called when switching. + verify(mBrightnessMappingStrategy, times(1)).getShortTermModelTimeout(); + verify(mBrightnessMappingStrategy, times(1)).getUserBrightness(); + verify(mBrightnessMappingStrategy, times(1)).getUserLux(); + // Ensure, after switching, original BMS is not used anymore verifyNoMoreInteractions(mBrightnessMappingStrategy);