From cdf7bb6c7bf23481f123303ba1612755c3f88319 Mon Sep 17 00:00:00 2001 From: Marin Shalamanov Date: Tue, 7 Jan 2020 14:19:50 +0100 Subject: [PATCH] Update default and active mode when new modes are added. If after a display change new modes are added and the current active mode has changed we assume the current default mode may be invalid. This change updates the default mode to the current active mode. Test: manual Test: atest LocalDisplayAdapterTest Bug: 146824575 Change-Id: Ic41c44a1e82c2a7a72c739335c7fe22bb2654f53 --- .../server/display/DisplayAdapter.java | 18 +-- .../server/display/LocalDisplayAdapter.java | 67 ++++++----- .../display/LocalDisplayAdapterTest.java | 110 ++++++++++++++++-- 3 files changed, 145 insertions(+), 50 deletions(-) diff --git a/services/core/java/com/android/server/display/DisplayAdapter.java b/services/core/java/com/android/server/display/DisplayAdapter.java index 6ba25a53248de..838dc84d4a0a6 100644 --- a/services/core/java/com/android/server/display/DisplayAdapter.java +++ b/services/core/java/com/android/server/display/DisplayAdapter.java @@ -109,24 +109,14 @@ abstract class DisplayAdapter { */ protected final void sendDisplayDeviceEventLocked( final DisplayDevice device, final int event) { - mHandler.post(new Runnable() { - @Override - public void run() { - mListener.onDisplayDeviceEvent(device, event); - } - }); + mHandler.post(() -> mListener.onDisplayDeviceEvent(device, event)); } /** * Sends a request to perform traversals. */ protected final void sendTraversalRequestLocked() { - mHandler.post(new Runnable() { - @Override - public void run() { - mListener.onTraversalRequested(); - } - }); + mHandler.post(() -> mListener.onTraversalRequested()); } public static Display.Mode createMode(int width, int height, float refreshRate) { @@ -135,7 +125,7 @@ abstract class DisplayAdapter { } public interface Listener { - public void onDisplayDeviceEvent(DisplayDevice device, int event); - public void onTraversalRequested(); + void onDisplayDeviceEvent(DisplayDevice device, int event); + void onTraversalRequested(); } } diff --git a/services/core/java/com/android/server/display/LocalDisplayAdapter.java b/services/core/java/com/android/server/display/LocalDisplayAdapter.java index 704cbff44388b..fc9542a33a787 100644 --- a/services/core/java/com/android/server/display/LocalDisplayAdapter.java +++ b/services/core/java/com/android/server/display/LocalDisplayAdapter.java @@ -65,8 +65,9 @@ final class LocalDisplayAdapter extends DisplayAdapter { private static final String PROPERTY_EMULATOR_CIRCULAR = "ro.emulator.circular"; - private final LongSparseArray mDevices = - new LongSparseArray(); + private static final int NO_DISPLAY_MODE_ID = 0; + + private final LongSparseArray mDevices = new LongSparseArray<>(); @SuppressWarnings("unused") // Becomes active at instantiation time. private PhysicalDisplayEventReceiver mPhysicalDisplayEventReceiver; @@ -133,14 +134,9 @@ final class LocalDisplayAdapter extends DisplayAdapter { hdrCapabilities, isInternal); mDevices.put(physicalDisplayId, device); sendDisplayDeviceEventLocked(device, DISPLAY_DEVICE_EVENT_ADDED); - } else { - boolean changed = device.updateDisplayConfigsLocked(configs, activeConfig, - configSpecs); - changed |= device.updateColorModesLocked(colorModes, activeColorMode); - changed |= device.updateHdrCapabilitiesLocked(hdrCapabilities); - if (changed) { - sendDisplayDeviceEventLocked(device, DISPLAY_DEVICE_EVENT_CHANGED); - } + } else if (device.updateDisplayProperties(configs, activeConfig, + configSpecs, colorModes, activeColorMode, hdrCapabilities)) { + sendDisplayDeviceEventLocked(device, DISPLAY_DEVICE_EVENT_CHANGED); } } else { // The display is no longer available. Ignore the attempt to add it. @@ -215,10 +211,8 @@ final class LocalDisplayAdapter extends DisplayAdapter { mPhysicalDisplayId = physicalDisplayId; mIsInternal = isInternal; mDisplayInfo = info; - - updateDisplayConfigsLocked(configs, activeConfigId, configSpecs); - updateColorModesLocked(colorModes, activeColorMode); - updateHdrCapabilitiesLocked(hdrCapabilities); + updateDisplayProperties(configs, activeConfigId, configSpecs, colorModes, + activeColorMode, hdrCapabilities); mSidekickInternal = LocalServices.getService(SidekickInternal.class); if (mIsInternal) { LightsManager lights = LocalServices.getService(LightsManager.class); @@ -240,13 +234,25 @@ final class LocalDisplayAdapter extends DisplayAdapter { return true; } + /** + * Returns true if there is a change. + **/ + public boolean updateDisplayProperties(SurfaceControl.DisplayConfig[] configs, + int activeConfigId, SurfaceControl.DesiredDisplayConfigSpecs configSpecs, + int[] colorModes, int activeColorMode, Display.HdrCapabilities hdrCapabilities) { + boolean changed = updateDisplayConfigsLocked(configs, activeConfigId, configSpecs); + changed |= updateColorModesLocked(colorModes, activeColorMode); + changed |= updateHdrCapabilitiesLocked(hdrCapabilities); + return changed; + } + public boolean updateDisplayConfigsLocked( SurfaceControl.DisplayConfig[] configs, int activeConfigId, SurfaceControl.DesiredDisplayConfigSpecs configSpecs) { mDisplayConfigs = Arrays.copyOf(configs, configs.length); mActiveConfigId = activeConfigId; // Build an updated list of all existing modes. - ArrayList records = new ArrayList(); + ArrayList records = new ArrayList<>(); boolean modesAdded = false; for (int i = 0; i < configs.length; i++) { SurfaceControl.DisplayConfig config = configs[i]; @@ -286,7 +292,7 @@ final class LocalDisplayAdapter extends DisplayAdapter { // Check whether surface flinger spontaneously changed modes out from under us. // Schedule traversals to ensure that the correct state is reapplied if necessary. - if (mActiveModeId != 0 + if (mActiveModeId != NO_DISPLAY_MODE_ID && mActiveModeId != activeRecord.mMode.getModeId()) { mActiveModeInvalid = true; sendTraversalRequestLocked(); @@ -294,12 +300,12 @@ final class LocalDisplayAdapter extends DisplayAdapter { // Check whether surface flinger spontaneously changed display config specs out from // under us. If so, schedule a traversal to reapply our display config specs. - if (mDisplayModeSpecs.baseModeId != 0) { + if (mDisplayModeSpecs.baseModeId != NO_DISPLAY_MODE_ID) { int activeBaseMode = findMatchingModeIdLocked(configSpecs.defaultConfig); // If we can't map the defaultConfig index to a mode, then the physical display // configs must have changed, and the code below for handling changes to the // list of available modes will take care of updating display config specs. - if (activeBaseMode != 0) { + if (activeBaseMode != NO_DISPLAY_MODE_ID) { if (mDisplayModeSpecs.baseModeId != activeBaseMode || mDisplayModeSpecs.refreshRateRange.min != configSpecs.minRefreshRate || mDisplayModeSpecs.refreshRateRange.max @@ -323,18 +329,23 @@ final class LocalDisplayAdapter extends DisplayAdapter { mSupportedModes.put(record.mMode.getModeId(), record); } - // Update the default mode, if needed. - if (findDisplayConfigIdLocked(mDefaultModeId) < 0) { - if (mDefaultModeId != 0) { - Slog.w(TAG, "Default display mode no longer available, using currently" - + " active mode as default."); - } + // For a new display, we need to initialize the default mode ID. + if (mDefaultModeId == NO_DISPLAY_MODE_ID) { + mDefaultModeId = activeRecord.mMode.getModeId(); + } else if (modesAdded && mActiveModeId != activeRecord.mMode.getModeId()) { + Slog.d(TAG, "New display modes are added and the active mode has changed, " + + "use active mode as default mode."); + mActiveModeId = activeRecord.mMode.getModeId(); + mDefaultModeId = activeRecord.mMode.getModeId(); + } else if (findDisplayConfigIdLocked(mDefaultModeId) < 0) { + Slog.w(TAG, "Default display mode no longer available, using currently" + + " active mode as default."); mDefaultModeId = activeRecord.mMode.getModeId(); } // Determine whether the display mode specs' base mode is still there. if (mSupportedModes.indexOfKey(mDisplayModeSpecs.baseModeId) < 0) { - if (mDisplayModeSpecs.baseModeId != 0) { + if (mDisplayModeSpecs.baseModeId != NO_DISPLAY_MODE_ID) { Slog.w(TAG, "DisplayModeSpecs base mode no longer available, using currently" + " active mode."); @@ -345,7 +356,7 @@ final class LocalDisplayAdapter extends DisplayAdapter { // Determine whether the active mode is still there. if (mSupportedModes.indexOfKey(mActiveModeId) < 0) { - if (mActiveModeId != 0) { + if (mActiveModeId != NO_DISPLAY_MODE_ID) { Slog.w(TAG, "Active display mode no longer available, reverting to default" + " mode."); } @@ -797,7 +808,7 @@ final class LocalDisplayAdapter extends DisplayAdapter { } mActiveConfigId = activeConfigId; mActiveModeId = findMatchingModeIdLocked(activeConfigId); - mActiveModeInvalid = mActiveModeId == 0; + mActiveModeInvalid = mActiveModeId == NO_DISPLAY_MODE_ID; if (mActiveModeInvalid) { Slog.w(TAG, "In unknown mode after setting allowed configs" + ", activeConfigId=" + mActiveConfigId); @@ -922,7 +933,7 @@ final class LocalDisplayAdapter extends DisplayAdapter { return record.mMode.getModeId(); } } - return 0; + return NO_DISPLAY_MODE_ID; } private void updateDeviceInfoLocked() { diff --git a/services/tests/mockingservicestests/src/com/android/server/display/LocalDisplayAdapterTest.java b/services/tests/mockingservicestests/src/com/android/server/display/LocalDisplayAdapterTest.java index 4a40b80a85c14..6d15302193726 100644 --- a/services/tests/mockingservicestests/src/com/android/server/display/LocalDisplayAdapterTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/display/LocalDisplayAdapterTest.java @@ -20,6 +20,8 @@ import static com.android.dx.mockito.inline.extended.ExtendedMockito.doReturn; import static com.android.dx.mockito.inline.extended.ExtendedMockito.mockitoSession; import static com.android.dx.mockito.inline.extended.ExtendedMockito.spyOn; +import static com.google.common.truth.Truth.assertThat; + import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; @@ -29,6 +31,7 @@ import android.os.Binder; import android.os.Handler; import android.os.IBinder; import android.os.Looper; +import android.view.Display; import android.view.DisplayAddress; import android.view.SurfaceControl; @@ -47,6 +50,7 @@ import org.mockito.Mock; import org.mockito.quality.Strictness; import java.util.ArrayList; +import java.util.Arrays; import java.util.LinkedList; @@ -167,6 +171,7 @@ public class LocalDisplayAdapterTest { */ @Test public void testDpiValues() throws Exception { + // needs default one always setUpDisplay(new FakeDisplay(PORT_A)); setUpDisplay(new FakeDisplay(PORT_B)); updateAvailableDisplays(); @@ -182,6 +187,67 @@ public class LocalDisplayAdapterTest { 16000); } + @Test + public void testAfterDisplayChange_ModesAreUpdated() throws Exception { + SurfaceControl.DisplayConfig displayInfo = createFakeDisplayConfig(1920, 1080, 60f); + SurfaceControl.DisplayConfig[] configs = + new SurfaceControl.DisplayConfig[]{displayInfo}; + FakeDisplay display = new FakeDisplay(PORT_A, configs, 0); + setUpDisplay(display); + updateAvailableDisplays(); + mAdapter.registerLocked(); + waitForHandlerToComplete(mHandler, HANDLER_WAIT_MS); + + assertThat(mListener.addedDisplays.size()).isEqualTo(1); + assertThat(mListener.changedDisplays).isEmpty(); + + DisplayDeviceInfo displayDeviceInfo = mListener.addedDisplays.get( + 0).getDisplayDeviceInfoLocked(); + + assertThat(displayDeviceInfo.supportedModes.length).isEqualTo(configs.length); + assertModeIsSupported(displayDeviceInfo.supportedModes, displayInfo); + + Display.Mode defaultMode = getModeById(displayDeviceInfo, displayDeviceInfo.defaultModeId); + assertThat(defaultMode.matches(displayInfo.width, displayInfo.height, + displayInfo.refreshRate)).isTrue(); + + Display.Mode activeMode = getModeById(displayDeviceInfo, displayDeviceInfo.modeId); + assertThat(activeMode.matches(displayInfo.width, displayInfo.height, + displayInfo.refreshRate)).isTrue(); + + // Change the display + SurfaceControl.DisplayConfig addedDisplayInfo = createFakeDisplayConfig(3840, 2160, + 60f); + configs = new SurfaceControl.DisplayConfig[]{displayInfo, addedDisplayInfo}; + display.configs = configs; + display.activeConfig = 1; + setUpDisplay(display); + mAdapter.registerLocked(); + waitForHandlerToComplete(mHandler, HANDLER_WAIT_MS); + + assertThat(SurfaceControl.getActiveConfig(display.token)).isEqualTo(1); + assertThat(SurfaceControl.getDisplayConfigs(display.token).length).isEqualTo(2); + + assertThat(mListener.addedDisplays.size()).isEqualTo(1); + assertThat(mListener.changedDisplays.size()).isEqualTo(1); + + DisplayDevice displayDevice = mListener.changedDisplays.get(0); + displayDevice.applyPendingDisplayDeviceInfoChangesLocked(); + displayDeviceInfo = displayDevice.getDisplayDeviceInfoLocked(); + + assertThat(displayDeviceInfo.supportedModes.length).isEqualTo(configs.length); + assertModeIsSupported(displayDeviceInfo.supportedModes, displayInfo); + assertModeIsSupported(displayDeviceInfo.supportedModes, addedDisplayInfo); + + activeMode = getModeById(displayDeviceInfo, displayDeviceInfo.modeId); + assertThat(activeMode.matches(addedDisplayInfo.width, addedDisplayInfo.height, + addedDisplayInfo.refreshRate)).isTrue(); + + defaultMode = getModeById(displayDeviceInfo, displayDeviceInfo.defaultModeId); + assertThat(defaultMode.matches(addedDisplayInfo.width, addedDisplayInfo.height, + addedDisplayInfo.refreshRate)).isTrue(); + } + private void assertDisplayDpi(DisplayDeviceInfo info, int expectedPort, float expectedXdpi, float expectedYDpi, @@ -194,16 +260,40 @@ public class LocalDisplayAdapterTest { assertEquals(expectedDensityDpi, info.densityDpi); } + private Display.Mode getModeById(DisplayDeviceInfo displayDeviceInfo, int modeId) { + return Arrays.stream(displayDeviceInfo.supportedModes) + .filter(mode -> mode.getModeId() == modeId) + .findFirst() + .get(); + } + + private void assertModeIsSupported(Display.Mode[] supportedModes, + SurfaceControl.DisplayConfig mode) { + assertThat(Arrays.stream(supportedModes).anyMatch( + x -> x.matches(mode.width, mode.height, mode.refreshRate))).isTrue(); + } + private static class FakeDisplay { public final DisplayAddress.Physical address; public final IBinder token = new Binder(); public final SurfaceControl.DisplayInfo info; - public final SurfaceControl.DisplayConfig config; + public SurfaceControl.DisplayConfig[] configs; + public int activeConfig; private FakeDisplay(int port) { this.address = createDisplayAddress(port); this.info = createFakeDisplayInfo(); - this.config = createFakeDisplayConfig(); + this.configs = new SurfaceControl.DisplayConfig[]{ + createFakeDisplayConfig(800, 600, 60f) + }; + this.activeConfig = 0; + } + + private FakeDisplay(int port, SurfaceControl.DisplayConfig[] configs, int activeConfig) { + this.address = createDisplayAddress(port); + this.info = createFakeDisplayInfo(); + this.configs = configs; + this.activeConfig = activeConfig; } } @@ -212,9 +302,9 @@ public class LocalDisplayAdapterTest { doReturn(display.token).when(() -> SurfaceControl.getPhysicalDisplayToken(display.address.getPhysicalDisplayId())); doReturn(display.info).when(() -> SurfaceControl.getDisplayInfo(display.token)); - doReturn(new SurfaceControl.DisplayConfig[] { display.config }).when( + doReturn(display.configs).when( () -> SurfaceControl.getDisplayConfigs(display.token)); - doReturn(0).when(() -> SurfaceControl.getActiveConfig(display.token)); + doReturn(display.activeConfig).when(() -> SurfaceControl.getActiveConfig(display.token)); doReturn(0).when(() -> SurfaceControl.getActiveColorMode(display.token)); doReturn(new int[] { 0 }).when( () -> SurfaceControl.getDisplayColorModes(display.token)); @@ -242,10 +332,12 @@ public class LocalDisplayAdapterTest { return info; } - private static SurfaceControl.DisplayConfig createFakeDisplayConfig() { + private static SurfaceControl.DisplayConfig createFakeDisplayConfig(int width, int height, + float refreshRate) { final SurfaceControl.DisplayConfig config = new SurfaceControl.DisplayConfig(); - config.width = 800; - config.height = 600; + config.width = width; + config.height = height; + config.refreshRate = refreshRate; config.xDpi = 100; config.yDpi = 100; return config; @@ -266,17 +358,19 @@ public class LocalDisplayAdapterTest { private class TestListener implements DisplayAdapter.Listener { public ArrayList addedDisplays = new ArrayList<>(); + public ArrayList changedDisplays = new ArrayList<>(); @Override public void onDisplayDeviceEvent(DisplayDevice device, int event) { if (event == DisplayAdapter.DISPLAY_DEVICE_EVENT_ADDED) { addedDisplays.add(device); + } else if (event == DisplayAdapter.DISPLAY_DEVICE_EVENT_CHANGED) { + changedDisplays.add(device); } } @Override public void onTraversalRequested() { - } } }