Ensure overlapping syncs don't submit buffers out of order

This change 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:

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.

The code ensures the SSGs that contains the VRI maintain an order. What
happens here is we create a new SSG that's a placeholder. Its only job
is to prevent a SSG from completing. The active SSG for VRI will add a
transaction committed callback and when that's invoked, it will mark the
placeholder SSG as ready. If a new request to create a SSG comes in and
the placeholder SSG is not null, it's added as part of the new active SSG.
A new placeholder 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.

Test: SurfaceSyncGroupTests#testOverlappingSyncsEnsureOrder
Test: WmTests:com.android.server.wm.SurfaceSyncGroupTests
Bug: 272189296
Change-Id: I921d78e347ecfb9786ebe4643308b347c5436332

5 files changed, 298 insertions, 7 deletions

diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java
index 86e7fb09c5ea..182f5c089e83 100644
--- a/core/java/android/view/ViewRootImpl.java
+++ b/core/java/android/view/ViewRootImpl.java
@@ -882,6 +882,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
@@ -11312,6 +11322,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;
@@ -11338,6 +11403,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 d916a1be1a03..ff664a1671b0 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();
+    }
+}