diff options
5 files changed, 298 insertions, 7 deletions
diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java index 2b29e7878a5d..6bd9538a9f65 100644 --- a/core/java/android/view/ViewRootImpl.java +++ b/core/java/android/view/ViewRootImpl.java @@ -885,6 +885,16 @@ public final class ViewRootImpl implements ViewParent, */ private SurfaceSyncGroup mActiveSurfaceSyncGroup; + + private final Object mPreviousSyncSafeguardLock = new Object(); + + /** + * Wraps the TransactionCommitted callback for the previous SSG so it can be added to the next + * SSG if started before previous has completed. + */ + @GuardedBy("mPreviousSyncSafeguardLock") + private SurfaceSyncGroup mPreviousSyncSafeguard; + private static final Object sSyncProgressLock = new Object(); // The count needs to be static since it's used to enable or disable RT animations which is // done at a global level per process. If any VRI syncs are in progress, we can't enable RT @@ -11329,6 +11339,61 @@ public final class ViewRootImpl implements ViewParent, }); } + /** + * This code will ensure that if multiple SurfaceSyncGroups are created for the same + * ViewRootImpl the SurfaceSyncGroups will maintain an order. The scenario that could occur + * is the following: + * <p> + * 1. SSG1 is created that includes the target VRI. There could be other VRIs in SSG1 + * 2. The target VRI draws its frame and marks its own active SSG as ready, but SSG1 is still + * waiting on other things in the SSG + * 3. Another SSG2 is created for the target VRI. The second frame renders and marks its own + * second SSG as complete. SSG2 has nothing else to wait on, so it will apply at this point, + * even though SSG1 has not finished. + * 4. Frame2 will get to SF first and Frame1 will later get to SF when SSG1 completes. + * <p> + * The code below ensures the SSGs that contains the VRI maintain an order. We create a new SSG + * that's a safeguard SSG. Its only job is to prevent the next active SSG from completing. + * The current active SSG for VRI will add a transaction committed callback and when that's + * invoked, it will mark the safeguard SSG as ready. If a new request to create a SSG comes + * in and the safeguard SSG is not null, it's added as part of the new active SSG. A new + * safeguard SSG is created to correspond to the new active SSG. This creates a chain to + * ensure the latter SSG always waits for the former SSG's transaction to get to SF. + */ + private void safeguardOverlappingSyncs(SurfaceSyncGroup activeSurfaceSyncGroup) { + SurfaceSyncGroup safeguardSsg = new SurfaceSyncGroup("VRI-Safeguard"); + // Always disable timeout on the safeguard sync + safeguardSsg.toggleTimeout(false /* enable */); + synchronized (mPreviousSyncSafeguardLock) { + if (mPreviousSyncSafeguard != null) { + activeSurfaceSyncGroup.add(mPreviousSyncSafeguard, null /* runnable */); + // Temporarily disable the timeout on the SSG that will contain the buffer. This + // is to ensure we don't timeout the active SSG before the previous one completes to + // ensure the order is maintained. The previous SSG has a timeout on its own SSG + // so it's guaranteed to complete. + activeSurfaceSyncGroup.toggleTimeout(false /* enable */); + mPreviousSyncSafeguard.addSyncCompleteCallback(mSimpleExecutor, () -> { + // Once we receive that the previous sync guard has been invoked, we can re-add + // the timeout on the active sync to ensure we eventually complete so it's not + // stuck permanently. + activeSurfaceSyncGroup.toggleTimeout(true /*enable */); + }); + } + mPreviousSyncSafeguard = safeguardSsg; + } + + Transaction t = new Transaction(); + t.addTransactionCommittedListener(mSimpleExecutor, () -> { + safeguardSsg.markSyncReady(); + synchronized (mPreviousSyncSafeguardLock) { + if (mPreviousSyncSafeguard == safeguardSsg) { + mPreviousSyncSafeguard = null; + } + } + }); + activeSurfaceSyncGroup.addTransaction(t); + } + @Override public SurfaceSyncGroup getOrCreateSurfaceSyncGroup() { boolean newSyncGroup = false; @@ -11355,6 +11420,7 @@ public final class ViewRootImpl implements ViewParent, mHandler.post(runnable); } }); + safeguardOverlappingSyncs(mActiveSurfaceSyncGroup); updateSyncInProgressCount(mActiveSurfaceSyncGroup); newSyncGroup = true; } diff --git a/core/java/android/window/SurfaceSyncGroup.java b/core/java/android/window/SurfaceSyncGroup.java index b3d512488239..18405676dbd8 100644 --- a/core/java/android/window/SurfaceSyncGroup.java +++ b/core/java/android/window/SurfaceSyncGroup.java @@ -38,6 +38,7 @@ import android.view.SurfaceView; import android.view.WindowManagerGlobal; import com.android.internal.annotations.GuardedBy; +import com.android.internal.annotations.VisibleForTesting; import java.util.concurrent.Executor; import java.util.concurrent.atomic.AtomicInteger; @@ -63,7 +64,12 @@ public final class SurfaceSyncGroup { private static final int MAX_COUNT = 100; private static final AtomicInteger sCounter = new AtomicInteger(0); - private static final int TRANSACTION_READY_TIMEOUT = 1000 * Build.HW_TIMEOUT_MULTIPLIER; + + /** + * @hide + */ + @VisibleForTesting + public static final int TRANSACTION_READY_TIMEOUT = 1000 * Build.HW_TIMEOUT_MULTIPLIER; private static Supplier<Transaction> sTransactionFactory = Transaction::new; @@ -123,6 +129,14 @@ public final class SurfaceSyncGroup { @GuardedBy("mLock") private boolean mTimeoutAdded; + /** + * Disable the timeout for this SSG so it will never be set until there's an explicit call to + * add a timeout. + */ + @GuardedBy("mLock") + private boolean mTimeoutDisabled; + + private static boolean isLocalBinder(IBinder binder) { return !(binder instanceof BinderProxy); } @@ -223,6 +237,10 @@ public final class SurfaceSyncGroup { */ public void addSyncCompleteCallback(Executor executor, Runnable runnable) { synchronized (mLock) { + if (mFinished) { + executor.execute(runnable); + return; + } mSyncCompleteCallbacks.add(new Pair<>(executor, runnable)); } } @@ -768,6 +786,21 @@ public final class SurfaceSyncGroup { } } + /** + * @hide + */ + public void toggleTimeout(boolean enable) { + synchronized (mLock) { + mTimeoutDisabled = !enable; + if (mTimeoutAdded && !enable) { + mHandler.removeCallbacksAndMessages(this); + mTimeoutAdded = false; + } else if (!mTimeoutAdded && enable) { + addTimeout(); + } + } + } + private void addTimeout() { synchronized (sHandlerThreadLock) { if (sHandlerThread == null) { @@ -777,7 +810,7 @@ public final class SurfaceSyncGroup { } synchronized (mLock) { - if (mTimeoutAdded) { + if (mTimeoutAdded || mTimeoutDisabled) { // We only need one timeout for the entire SurfaceSyncGroup since we just want to // ensure it doesn't stay stuck forever. return; diff --git a/services/core/java/com/android/server/wm/BLASTSyncEngine.java b/services/core/java/com/android/server/wm/BLASTSyncEngine.java index 45c24c406a30..7ecc0839fe53 100644 --- a/services/core/java/com/android/server/wm/BLASTSyncEngine.java +++ b/services/core/java/com/android/server/wm/BLASTSyncEngine.java @@ -168,14 +168,13 @@ class BLASTSyncEngine { class CommitCallback implements Runnable { // Can run a second time if the action completes after the timeout. boolean ran = false; - public void onCommitted() { + public void onCommitted(SurfaceControl.Transaction t) { synchronized (mWm.mGlobalLock) { if (ran) { return; } mHandler.removeCallbacks(this); ran = true; - SurfaceControl.Transaction t = new SurfaceControl.Transaction(); for (WindowContainer wc : wcAwaitingCommit) { wc.onSyncTransactionCommitted(t); } @@ -194,12 +193,12 @@ class BLASTSyncEngine { Slog.e(TAG, "WM sent Transaction to organized, but never received" + " commit callback. Application ANR likely to follow."); Trace.traceEnd(TRACE_TAG_WINDOW_MANAGER); - onCommitted(); - + onCommitted(merged); } }; CommitCallback callback = new CommitCallback(); - merged.addTransactionCommittedListener((r) -> { r.run(); }, callback::onCommitted); + merged.addTransactionCommittedListener(Runnable::run, + () -> callback.onCommitted(new SurfaceControl.Transaction())); mHandler.postDelayed(callback, BLAST_TIMEOUT_DURATION); Trace.traceBegin(TRACE_TAG_WINDOW_MANAGER, "onTransactionReady"); diff --git a/services/tests/wmtests/AndroidManifest.xml b/services/tests/wmtests/AndroidManifest.xml index f12b53acd06b..fe7cd4a5edd9 100644 --- a/services/tests/wmtests/AndroidManifest.xml +++ b/services/tests/wmtests/AndroidManifest.xml @@ -89,6 +89,12 @@ <activity android:name="com.android.server.wm.SurfaceControlViewHostTests$TestActivity" /> + <activity android:name="android.server.wm.scvh.SurfaceSyncGroupActivity" + android:screenOrientation="locked" + android:turnScreenOn="true" + android:theme="@style/WhiteBackgroundTheme" + android:exported="true"/> + <service android:name="android.view.cts.surfacevalidator.LocalMediaProjectionService" android:foregroundServiceType="mediaProjection" android:enabled="true"> diff --git a/services/tests/wmtests/src/com/android/server/wm/SurfaceSyncGroupTests.java b/services/tests/wmtests/src/com/android/server/wm/SurfaceSyncGroupTests.java new file mode 100644 index 000000000000..9db647acebb7 --- /dev/null +++ b/services/tests/wmtests/src/com/android/server/wm/SurfaceSyncGroupTests.java @@ -0,0 +1,187 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.wm; + +import static android.window.SurfaceSyncGroup.TRANSACTION_READY_TIMEOUT; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +import android.app.Instrumentation; +import android.graphics.Bitmap; +import android.graphics.Color; +import android.graphics.PixelFormat; +import android.graphics.Rect; +import android.os.Handler; +import android.os.HandlerThread; +import android.platform.test.annotations.Presubmit; +import android.server.wm.scvh.SurfaceSyncGroupActivity; +import android.view.SurfaceControl; +import android.view.View; +import android.view.ViewTreeObserver; +import android.view.WindowManager; +import android.view.cts.surfacevalidator.BitmapPixelChecker; +import android.window.SurfaceSyncGroup; + +import androidx.test.InstrumentationRegistry; +import androidx.test.rule.ActivityTestRule; + +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +@Presubmit +public class SurfaceSyncGroupTests { + private static final String TAG = "SurfaceSyncGroupTests"; + + @Rule + public ActivityTestRule<SurfaceSyncGroupActivity> mActivityRule = new ActivityTestRule<>( + SurfaceSyncGroupActivity.class); + + private SurfaceSyncGroupActivity mActivity; + + Instrumentation mInstrumentation; + + private final HandlerThread mHandlerThread = new HandlerThread("applyTransaction"); + private Handler mHandler; + + @Before + public void setup() { + mActivity = mActivityRule.getActivity(); + mInstrumentation = InstrumentationRegistry.getInstrumentation(); + mHandlerThread.start(); + mHandler = mHandlerThread.getThreadHandler(); + } + + @Test + public void testOverlappingSyncsEnsureOrder_WhenTimeout() throws InterruptedException { + WindowManager.LayoutParams params = new WindowManager.LayoutParams(); + params.format = PixelFormat.TRANSLUCENT; + + CountDownLatch secondDrawCompleteLatch = new CountDownLatch(1); + CountDownLatch bothSyncGroupsComplete = new CountDownLatch(2); + final SurfaceSyncGroup firstSsg = new SurfaceSyncGroup(TAG + "-first"); + final SurfaceSyncGroup secondSsg = new SurfaceSyncGroup(TAG + "-second"); + final SurfaceSyncGroup infiniteSsg = new SurfaceSyncGroup(TAG + "-infinite"); + + SurfaceControl.Transaction t = new SurfaceControl.Transaction(); + t.addTransactionCommittedListener(Runnable::run, bothSyncGroupsComplete::countDown); + firstSsg.addTransaction(t); + + View backgroundView = mActivity.getBackgroundView(); + firstSsg.add(backgroundView.getRootSurfaceControl(), + () -> mActivity.runOnUiThread(() -> backgroundView.setBackgroundColor(Color.RED))); + + addSecondSyncGroup(secondSsg, secondDrawCompleteLatch, bothSyncGroupsComplete); + + assertTrue("Failed to draw two frames", + secondDrawCompleteLatch.await(5, TimeUnit.SECONDS)); + + mHandler.postDelayed(() -> { + // Don't add a markSyncReady for the first sync group until after it's added to another + // SSG to ensure the timeout is longer than the second frame's timeout. The infinite SSG + // will never complete to ensure it reaches the timeout, but only after the second SSG + // had a chance to reach its timeout. + infiniteSsg.add(firstSsg, null /* runnable */); + firstSsg.markSyncReady(); + }, 200); + + assertTrue("Failed to wait for both SurfaceSyncGroups to apply", + bothSyncGroupsComplete.await(5, TimeUnit.SECONDS)); + + validateScreenshot(); + } + + @Test + public void testOverlappingSyncsEnsureOrder_WhileHoldingTransaction() + throws InterruptedException { + WindowManager.LayoutParams params = new WindowManager.LayoutParams(); + params.format = PixelFormat.TRANSLUCENT; + + CountDownLatch secondDrawCompleteLatch = new CountDownLatch(1); + CountDownLatch bothSyncGroupsComplete = new CountDownLatch(2); + + final SurfaceSyncGroup firstSsg = new SurfaceSyncGroup(TAG + "-first", + transaction -> mHandler.postDelayed(() -> { + try { + assertTrue("Failed to draw two frames", + secondDrawCompleteLatch.await(5, TimeUnit.SECONDS)); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + transaction.apply(); + }, TRANSACTION_READY_TIMEOUT + 200)); + final SurfaceSyncGroup secondSsg = new SurfaceSyncGroup(TAG + "-second"); + + SurfaceControl.Transaction t = new SurfaceControl.Transaction(); + t.addTransactionCommittedListener(Runnable::run, bothSyncGroupsComplete::countDown); + firstSsg.addTransaction(t); + + View backgroundView = mActivity.getBackgroundView(); + firstSsg.add(backgroundView.getRootSurfaceControl(), + () -> mActivity.runOnUiThread(() -> backgroundView.setBackgroundColor(Color.RED))); + firstSsg.markSyncReady(); + + addSecondSyncGroup(secondSsg, secondDrawCompleteLatch, bothSyncGroupsComplete); + + assertTrue("Failed to wait for both SurfaceSyncGroups to apply", + bothSyncGroupsComplete.await(5, TimeUnit.SECONDS)); + + validateScreenshot(); + } + + private void addSecondSyncGroup(SurfaceSyncGroup surfaceSyncGroup, + CountDownLatch waitForSecondDraw, CountDownLatch bothSyncGroupsComplete) { + View backgroundView = mActivity.getBackgroundView(); + ViewTreeObserver viewTreeObserver = backgroundView.getViewTreeObserver(); + viewTreeObserver.registerFrameCommitCallback(() -> mHandler.post(() -> { + surfaceSyncGroup.add(backgroundView.getRootSurfaceControl(), + () -> mActivity.runOnUiThread( + () -> backgroundView.setBackgroundColor(Color.BLUE))); + + SurfaceControl.Transaction t = new SurfaceControl.Transaction(); + t.addTransactionCommittedListener(Runnable::run, bothSyncGroupsComplete::countDown); + surfaceSyncGroup.addTransaction(t); + surfaceSyncGroup.markSyncReady(); + viewTreeObserver.registerFrameCommitCallback(waitForSecondDraw::countDown); + })); + } + + private void validateScreenshot() { + Bitmap screenshot = mInstrumentation.getUiAutomation().takeScreenshot( + mActivity.getWindow()); + assertNotNull("Failed to generate a screenshot", screenshot); + Bitmap swBitmap = screenshot.copy(Bitmap.Config.ARGB_8888, false); + screenshot.recycle(); + + BitmapPixelChecker pixelChecker = new BitmapPixelChecker(Color.BLUE); + int halfWidth = swBitmap.getWidth() / 2; + int halfHeight = swBitmap.getHeight() / 2; + // We don't need to check all the pixels since we only care that at least some of them are + // blue. If the buffers were submitted out of order, all the pixels will be red. + Rect bounds = new Rect(halfWidth, halfHeight, halfWidth + 10, halfHeight + 10); + int numMatchingPixels = pixelChecker.getNumMatchingPixels(swBitmap, bounds); + assertEquals("Expected 100 received " + numMatchingPixels + " matching pixels", 100, + numMatchingPixels); + + swBitmap.recycle(); + } +} |