Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit a11c879b authored by petsjonkin's avatar petsjonkin
Browse files

DisplayBrightnessController fixing race with updatePowerState in the middle of setBrightness 

setBrightness method delivers brightness to DPC via 2 threads: BrightnessSettings -> mainLopper + DPC -> displayThread
updatePowerState could happen before new brightness is delivered, in this case DisplayManager is not notified about brightness change.

Bug: b/416046693 , b/414262134
Test: atest DisplayPerfTest, atest DisplayBrightnessControllerTest, https://b.corp.google.com/issues/403303379#comment6
Flag: EXEMPT bugfix
Change-Id: I946e219f52a2439ad3405f3b5d7fe5e639d993ac
parent 6cd33417

services/core/java/com/android/server/display/DisplayPowerController.java

+1 −1
Original line number Diff line number Diff line
@@ -1028,7 +1028,7 @@ final class DisplayPowerController implements AutomaticBrightnessController.Call 
        }



        BrightnessSetting.BrightnessSettingListener brightnessSettingListener = brightnessValue -> {

            Message msg = mHandler.obtainMessage(MSG_UPDATE_BRIGHTNESS, brightnessValue);

            Message msg = mHandler.obtainMessage(MSG_UPDATE_BRIGHTNESS);

            mHandler.sendMessageAtTime(msg, mClock.uptimeMillis());

        };

        mDisplayBrightnessController

services/core/java/com/android/server/display/brightness/DisplayBrightnessController.java

+54 −43
Original line number Diff line number Diff line
@@ -65,11 +65,11 @@ public final class DisplayBrightnessController { 
    // mCurrentScreenBrightness is updated.

    private final Runnable mOnBrightnessChangeRunnable;



    // The screen brightness that has changed but not taken effect yet. If this is different

    // from the current screen brightness then this is coming from something other than us

    // and should be considered a user interaction.

    // The screen brightness that has changed but not taken effect yet. If after applying min

    // and max constraints this is different from the current  screen brightness then this

    // is coming from something other than us and should be considered a user interaction.

    @GuardedBy("mLock")

    private float mPendingScreenBrightness;

    private float mPendingUnthrottledScreenBrightness;



    // The last observed screen brightness, either set by us or by the settings app on

    // behalf of the user.
@@ -142,7 +142,7 @@ public final class DisplayBrightnessController { 
        mDisplayId = displayId;

        // TODO: b/186428377 update brightness setting when display changes

        mBrightnessSetting = brightnessSetting;

        mPendingScreenBrightness = PowerManager.BRIGHTNESS_INVALID_FLOAT;

        mPendingUnthrottledScreenBrightness = PowerManager.BRIGHTNESS_INVALID_FLOAT;

        mScreenBrightnessDefault = BrightnessUtils.clampAbsoluteBrightness(defaultScreenBrightness);

        mCurrentScreenBrightness = getScreenBrightnessSetting();

        mCurrentUnthrottledBrightness = mCurrentScreenBrightness;
@@ -267,10 +267,9 @@ public final class DisplayBrightnessController { 
    public void setAndNotifyCurrentScreenBrightness(float brightnessValue) {

        final boolean hasBrightnessChanged;

        synchronized (mLock) {

            float brightnessAdjusted = MathUtils

                    .constrain(brightnessValue, mCurrentMinBrightness, mCurrentMaxBrightness);

            float brightnessAdjusted = getBrightnessConstrainedLocked(brightnessValue);

            hasBrightnessChanged = (brightnessAdjusted != mCurrentScreenBrightness);

            setCurrentScreenBrightnessLocked(brightnessAdjusted);

            mCurrentScreenBrightness = brightnessAdjusted;

            mCurrentUnthrottledBrightness = brightnessValue;

        }

        if (hasBrightnessChanged) {
@@ -287,12 +286,10 @@ public final class DisplayBrightnessController { 
        }

    }



    /**

     * Returns the screen brightness which has changed but has not taken any effect so far.

     */

    public float getPendingScreenBrightness() {

    @VisibleForTesting

    float getPendingScreenBrightness() {

        synchronized (mLock) {

            return mPendingScreenBrightness;

            return mPendingUnthrottledScreenBrightness;

        }

    }
@@ -302,8 +299,7 @@ public final class DisplayBrightnessController { 
    public void handleSettingsChange() {

        float brightness = getScreenBrightnessSetting();

        synchronized (mLock) {

            mPendingScreenBrightness = MathUtils.constrain(

                    brightness, mCurrentMinBrightness, mCurrentMaxBrightness);

            mPendingUnthrottledScreenBrightness = brightness;

        }

    }
@@ -353,13 +349,15 @@ public final class DisplayBrightnessController { 
     * Returns the current screen brightnessSetting constrained by current min and max values

     */

    public float getScreenBrightnessSettingConstrained() {

        float brightness = getScreenBrightnessSetting();

        float maxBrightness, minBrightness;

        float brightness = mBrightnessSetting.getBrightness();

        if (Float.isNaN(brightness)) {

            brightness = mScreenBrightnessDefault;

        }

        float brightnessAdjusted;

        synchronized (mLock) {

            maxBrightness = mCurrentMaxBrightness;

            minBrightness = mCurrentMinBrightness;

            brightnessAdjusted = getBrightnessConstrainedLocked(brightness);

        }

        return MathUtils.constrain(brightness, minBrightness, maxBrightness);

        return brightnessAdjusted;

    }



    /**
@@ -397,7 +395,6 @@ public final class DisplayBrightnessController { 
        float maxBrightness;

        synchronized (mLock) {

            maxBrightness = mCurrentMaxBrightness;

            mCurrentUnthrottledBrightness = brightnessValue;

        }

        setBrightnessInternal(brightnessValue, maxBrightness);

    }
@@ -414,21 +411,34 @@ public final class DisplayBrightnessController { 
     */

    public void updateScreenBrightnessSetting(float brightnessValue,

            float minBrightness, float maxBrightness) {

        float adjustedBrightness = MathUtils

        float constrainedBrightness = MathUtils

                .constrain(brightnessValue, minBrightness, maxBrightness);

        boolean constrainedBrightnessChanged = false;

        boolean rawBrightnessChanged = false;

        synchronized (mLock) {

            mCurrentMaxBrightness = maxBrightness;

            mCurrentMinBrightness = minBrightness;

            if (!BrightnessUtils.isValidBrightnessValue(adjustedBrightness)

                    || adjustedBrightness == mCurrentScreenBrightness) {

            if (!BrightnessUtils.isValidBrightnessValue(brightnessValue)) {

                return;

            }

            if (constrainedBrightness != mCurrentScreenBrightness) {

                constrainedBrightnessChanged = true;

            }

            if (brightnessValue != mCurrentUnthrottledBrightness) {

                rawBrightnessChanged = true;

            }



            mCurrentScreenBrightness = constrainedBrightness;

            mCurrentUnthrottledBrightness = brightnessValue;

            setCurrentScreenBrightnessLocked(adjustedBrightness);

        }



        if (constrainedBrightnessChanged) {

            notifyCurrentScreenBrightness();

        }

        if (rawBrightnessChanged) {

            setBrightnessInternal(brightnessValue, maxBrightness);

        }

    }



    /**

     * Sets up the auto brightness and the relevant state for the associated display
@@ -555,7 +565,8 @@ public final class DisplayBrightnessController { 
        synchronized (mLock) {

            writer.println("  mCurrentMinBrightness=" + mCurrentMinBrightness);

            writer.println("  mCurrentMaxBrightness=" + mCurrentMaxBrightness);

            writer.println("  mPendingScreenBrightness=" + mPendingScreenBrightness);

            writer.println("  mPendingUnthrottledScreenBrightness="

                    + mPendingUnthrottledScreenBrightness);

            writer.println("  mCurrentScreenBrightness=" + mCurrentScreenBrightness);

            writer.println("  mCurrentUnthrottledBrightness=" + mCurrentUnthrottledBrightness);

            writer.println("  mLastUserSetScreenBrightness="
@@ -578,18 +589,20 @@ public final class DisplayBrightnessController { 
    boolean updateUserSetScreenBrightness() {

        mUserSetScreenBrightnessUpdated = false;

        synchronized (mLock) {

            if (!BrightnessUtils.isValidBrightnessValue(mPendingScreenBrightness)) {

            if (!BrightnessUtils.isValidBrightnessValue(mPendingUnthrottledScreenBrightness)) {

                return false;

            }

            if (mCurrentScreenBrightness == mPendingScreenBrightness) {

                mPendingScreenBrightness = PowerManager.BRIGHTNESS_INVALID_FLOAT;

            float pendingBrightnessConstrained = getBrightnessConstrainedLocked(

                    mPendingUnthrottledScreenBrightness);

            mCurrentUnthrottledBrightness = mPendingUnthrottledScreenBrightness;

            mPendingUnthrottledScreenBrightness = PowerManager.BRIGHTNESS_INVALID_FLOAT;

            setTemporaryBrightnessLocked(PowerManager.BRIGHTNESS_INVALID_FLOAT);



            if (mCurrentScreenBrightness == pendingBrightnessConstrained) {

                return false;

            }

            setCurrentScreenBrightnessLocked(mPendingScreenBrightness);

            mLastUserSetScreenBrightness = mPendingScreenBrightness;

            mPendingScreenBrightness = PowerManager.BRIGHTNESS_INVALID_FLOAT;

            setTemporaryBrightnessLocked(PowerManager.BRIGHTNESS_INVALID_FLOAT);

            mCurrentScreenBrightness = pendingBrightnessConstrained;

            mLastUserSetScreenBrightness = pendingBrightnessConstrained;

        }

        notifyCurrentScreenBrightness();

        mUserSetScreenBrightnessUpdated = true;
@@ -677,13 +690,6 @@ public final class DisplayBrightnessController { 
                .setTemporaryScreenBrightness(temporaryBrightness);

    }



    @GuardedBy("mLock")

    private void setCurrentScreenBrightnessLocked(float brightnessValue) {

        if (brightnessValue != mCurrentScreenBrightness) {

            mCurrentScreenBrightness = brightnessValue;

        }

    }



    private void notifyCurrentScreenBrightness() {

        mBrightnessChangeExecutor.execute(mOnBrightnessChangeRunnable);

    }
@@ -738,4 +744,9 @@ public final class DisplayBrightnessController { 
        return new StrategyExecutionRequest(displayPowerRequest, mCurrentUnthrottledBrightness,

                mUserSetScreenBrightnessUpdated, mIsStylusBeingUsed, offloadSession);

    }



    @GuardedBy("mLock")

    private float getBrightnessConstrainedLocked(float brightness) {

        return MathUtils.constrain(brightness, mCurrentMinBrightness, mCurrentMaxBrightness);

    }

}

services/tests/displayservicetests/src/com/android/server/display/brightness/DisplayBrightnessControllerTest.java

+55 −1
Original line number Diff line number Diff line
@@ -40,8 +40,8 @@ import android.os.HandlerExecutor; 
import android.os.PowerManager;

import android.view.Display;



import androidx.test.ext.junit.runners.AndroidJUnit4;

import androidx.test.filters.SmallTest;

import androidx.test.runner.AndroidJUnit4;



import com.android.server.display.AutomaticBrightnessController;

import com.android.server.display.BrightnessMappingStrategy;
@@ -64,6 +64,7 @@ import org.mockito.MockitoAnnotations; 
@SmallTest

@RunWith(AndroidJUnit4.class)

public final class DisplayBrightnessControllerTest {

    private static final float FLOAT_TOLERANCE = 0.001f;

    private static final int DISPLAY_ID = Display.DEFAULT_DISPLAY;

    private static final float DEFAULT_BRIGHTNESS = 0.15f;
@@ -641,4 +642,57 @@ public final class DisplayBrightnessControllerTest { 
        mDisplayBrightnessController.setStylusBeingUsed(true);

        assertTrue(mDisplayBrightnessController.isStylusBeingUsed());

    }



    @Test

    public void testBrightnessSet_withUpdatePowerStateInTheMiddle() {

        TemporaryBrightnessStrategy temporaryBrightnessStrategy = mock(

                TemporaryBrightnessStrategy.class);

        when(mDisplayBrightnessStrategySelector.getTemporaryDisplayBrightnessStrategy()).thenReturn(

                temporaryBrightnessStrategy);

        float currentMin = 0.1f;

        float currentMax = 0.8f;

        float currentBrightness = 0.3f;

        float newBrightness = 0.5f;

        // initial screen brightness

        mDisplayBrightnessController.updateScreenBrightnessSetting(

                currentBrightness, currentMin, currentMax);

        when(mBrightnessSetting.getBrightness()).thenReturn(currentBrightness);

        assertEquals(currentBrightness, mDisplayBrightnessController.getCurrentBrightness(),

                FLOAT_TOLERANCE);

        assertEquals(PowerManager.BRIGHTNESS_INVALID_FLOAT,

                mDisplayBrightnessController.getPendingScreenBrightness(), FLOAT_TOLERANCE);



        // request to change brightness. Persist but don't process.

        mDisplayBrightnessController.setBrightness(newBrightness);

        verify(mBrightnessSetting).setBrightness(newBrightness);

        when(mBrightnessSetting.getBrightness()).thenReturn(newBrightness);

        assertEquals(currentBrightness, mDisplayBrightnessController.getCurrentBrightness(),

                FLOAT_TOLERANCE);

        assertEquals(PowerManager.BRIGHTNESS_INVALID_FLOAT,

                mDisplayBrightnessController.getPendingScreenBrightness(), FLOAT_TOLERANCE);



        // updatePowerState loop, with current brightness

        clearInvocations(mBrightnessSetting, mBrightnessChangeExecutor);

        mDisplayBrightnessController.updateScreenBrightnessSetting(

                currentBrightness, currentMin, currentMax);

        verifyNoMoreInteractions(mBrightnessSetting, mBrightnessChangeExecutor);

        assertEquals(currentBrightness, mDisplayBrightnessController.getCurrentBrightness(),

                FLOAT_TOLERANCE);

        assertEquals(PowerManager.BRIGHTNESS_INVALID_FLOAT,

                mDisplayBrightnessController.getPendingScreenBrightness(), FLOAT_TOLERANCE);



        // process brightness change request

        mDisplayBrightnessController.handleSettingsChange();

        assertEquals(currentBrightness, mDisplayBrightnessController.getCurrentBrightness(),

                FLOAT_TOLERANCE);

        assertEquals(newBrightness, mDisplayBrightnessController.getPendingScreenBrightness(),

                FLOAT_TOLERANCE);



        mDisplayBrightnessController.updateUserSetScreenBrightness();

        assertEquals(newBrightness, mDisplayBrightnessController.getCurrentBrightness(),

                FLOAT_TOLERANCE);

        assertEquals(PowerManager.BRIGHTNESS_INVALID_FLOAT,

                mDisplayBrightnessController.getPendingScreenBrightness(), FLOAT_TOLERANCE);

        verify(mBrightnessChangeExecutor).execute(mOnBrightnessChangeRunnable);

    }

}