Replace SyncTarget with SurfaceSyncGroup

Remove SyncTarget and make all uses of SyncTarget use SurfaceSyncGroup.
This makes sense moving forward because we want to add SurfaceSyncGroups
to other SurfaceSyncGroups.

The flow will be the following:
1. Create a SurfaceSyncGroup
2. Add that SurfaceSyncGroup to another SurfaceSyncGroup if it needs to
   be combined with other things
3. When no more things need to be added to the SurfaceSyncGroup and it's
   ready to finish, call onTransactionReady. This will mark the
   SurfaceSyncGroup as ready and allow it to apply or merge if it has no
   pending SurfaceSyncGroup children

This works well in VRI since we can just create a single
SurfaceSyncGroup to represent the VRI itself. SV can add their own
SurfaceSyncGroups to the VRI's one. VRI's SurfaceSyncGroup can be added
to any other SurfaceSyncGroups that want to synchronize the VRI. VRI
just calls onTransactionReady directly on its SurfaceSyncGroup when it
has drawn its buffer. If it has a parent, it will be notified and the
transaction merged into the parent. If no parent, it will be applied
directly.

Test: SurfaceSyncGroupTest
Test: SurfaceSyncGroupContinuousTest
Bug: 237804605
Change-Id: If691412505e8ba2503dd093399227327a0b6fdb1

6 files changed, 424 insertions, 316 deletions

diff --git a/core/java/android/view/AttachedSurfaceControl.java b/core/java/android/view/AttachedSurfaceControl.java
index c69298192109..3b082bcfcfb2 100644
--- a/core/java/android/view/AttachedSurfaceControl.java
+++ b/core/java/android/view/AttachedSurfaceControl.java
@@ -140,13 +140,13 @@ public interface AttachedSurfaceControl {
     }
 
     /**
-     * Returns a SyncTarget that can be used to sync {@link AttachedSurfaceControl} in a
+     * Returns a SurfaceSyncGroup that can be used to sync {@link AttachedSurfaceControl} in a
      * {@link SurfaceSyncGroup}
      *
      * @hide
      */
     @Nullable
-    default SurfaceSyncGroup.SyncTarget getSyncTarget() {
+    default SurfaceSyncGroup getOrCreateSurfaceSyncGroup() {
         return null;
     }
 }
diff --git a/core/java/android/view/SurfaceControlViewHost.java b/core/java/android/view/SurfaceControlViewHost.java
index 18897725e98f..0a134be9ca84 100644
--- a/core/java/android/view/SurfaceControlViewHost.java
+++ b/core/java/android/view/SurfaceControlViewHost.java
@@ -404,14 +404,6 @@ public class SurfaceControlViewHost {
     }
 
     /**
-     * @hide
-     */
-    @TestApi
-    public void relayout(WindowManager.LayoutParams attrs) {
-        relayout(attrs, SurfaceControl.Transaction::apply);
-    }
-
-    /**
      * Forces relayout and draw and allows to set a custom callback when it is finished
      * @hide
      */
@@ -423,6 +415,14 @@ public class SurfaceControlViewHost {
     }
 
     /**
+     * @hide
+     */
+    @TestApi
+    public void relayout(WindowManager.LayoutParams attrs) {
+        mViewRoot.setLayoutParams(attrs, false);
+    }
+
+    /**
      * Modify the size of the root view.
      *
      * @param width Width in pixels
diff --git a/core/java/android/view/SurfaceView.java b/core/java/android/view/SurfaceView.java
index 33ea92de68b4..9db084e01598 100644
--- a/core/java/android/view/SurfaceView.java
+++ b/core/java/android/view/SurfaceView.java
@@ -982,8 +982,8 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
 
                 final boolean redrawNeeded = sizeChanged || creating || hintChanged
                         || (mVisible && !mDrawFinished) || alphaChanged || relativeZChanged;
-                boolean shouldSyncBuffer =
-                        redrawNeeded && viewRoot.wasRelayoutRequested() && viewRoot.isInLocalSync();
+                boolean shouldSyncBuffer = redrawNeeded && viewRoot.wasRelayoutRequested()
+                        && viewRoot.isInWMSRequestedSync();
                 SyncBufferTransactionCallback syncBufferTransactionCallback = null;
                 if (shouldSyncBuffer) {
                     syncBufferTransactionCallback = new SyncBufferTransactionCallback();
@@ -1073,35 +1073,34 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
     private void handleSyncBufferCallback(SurfaceHolder.Callback[] callbacks,
             SyncBufferTransactionCallback syncBufferTransactionCallback) {
 
-        getViewRootImpl().addToSync((parentSyncGroup, syncBufferCallback) ->
-                redrawNeededAsync(callbacks, () -> {
-                    Transaction t = null;
-                    if (mBlastBufferQueue != null) {
-                        mBlastBufferQueue.stopContinuousSyncTransaction();
-                        t = syncBufferTransactionCallback.waitForTransaction();
-                    }
+        final SurfaceSyncGroup surfaceSyncGroup = new SurfaceSyncGroup();
+        getViewRootImpl().addToSync(surfaceSyncGroup);
+        redrawNeededAsync(callbacks, () -> {
+            Transaction t = null;
+            if (mBlastBufferQueue != null) {
+                mBlastBufferQueue.stopContinuousSyncTransaction();
+                t = syncBufferTransactionCallback.waitForTransaction();
+            }
 
-                    syncBufferCallback.onTransactionReady(t);
-                    onDrawFinished();
-                }));
+            surfaceSyncGroup.onTransactionReady(t);
+            onDrawFinished();
+        });
     }
 
     private void handleSyncNoBuffer(SurfaceHolder.Callback[] callbacks) {
-        final SurfaceSyncGroup syncGroup = new SurfaceSyncGroup();
+        final SurfaceSyncGroup surfaceSyncGroup = new SurfaceSyncGroup();
         synchronized (mSyncGroups) {
-            mSyncGroups.add(syncGroup);
+            mSyncGroups.add(surfaceSyncGroup);
         }
 
-        syncGroup.addToSync((parentSyncGroup, syncBufferCallback) ->
-                redrawNeededAsync(callbacks, () -> {
-                    syncBufferCallback.onTransactionReady(null);
-                    onDrawFinished();
-                    synchronized (mSyncGroups) {
-                        mSyncGroups.remove(syncGroup);
-                    }
-                }));
+        redrawNeededAsync(callbacks, () -> {
+            synchronized (mSyncGroups) {
+                mSyncGroups.remove(surfaceSyncGroup);
+            }
+            surfaceSyncGroup.onTransactionReady(null);
+            onDrawFinished();
+        });
 
-        syncGroup.markSyncReady();
     }
 
     private void redrawNeededAsync(SurfaceHolder.Callback[] callbacks,
@@ -1119,7 +1118,7 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
         if (viewRoot != null) {
             synchronized (mSyncGroups) {
                 for (SurfaceSyncGroup syncGroup : mSyncGroups) {
-                    viewRoot.mergeSync(syncGroup);
+                    viewRoot.addToSync(syncGroup);
                 }
             }
         }
diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java
index 09a9d46ba257..c1e71dc77cda 100644
--- a/core/java/android/view/ViewRootImpl.java
+++ b/core/java/android/view/ViewRootImpl.java
@@ -597,19 +597,21 @@ public final class ViewRootImpl implements ViewParent,
     String mLastPerformDrawSkippedReason;
     /** The reason the last call to performTraversals() returned without drawing */
     String mLastPerformTraversalsSkipDrawReason;
-    /** The state of the local sync, if one is in progress. Can be one of the states below. */
-    int mLocalSyncState;
+    /** The state of the WMS requested sync, if one is in progress. Can be one of the states
+     * below. */
+    int mWmsRequestSyncGroupState;
 
-    // The possible states of the local sync, see createSyncIfNeeded()
-    private final int LOCAL_SYNC_NONE = 0;
-    private final int LOCAL_SYNC_PENDING = 1;
-    private final int LOCAL_SYNC_RETURNED = 2;
-    private final int LOCAL_SYNC_MERGED = 3;
+    // The possible states of the WMS requested sync, see createSyncIfNeeded()
+    private static final int WMS_SYNC_NONE = 0;
+    private static final int WMS_SYNC_PENDING = 1;
+    private static final int WMS_SYNC_RETURNED = 2;
+    private static final int WMS_SYNC_MERGED = 3;
 
     /**
-     * Set whether the draw should send the buffer to system server. When set to true, VRI will
-     * create a sync transaction with BBQ and send the resulting buffer to system server. If false,
-     * VRI will not try to sync a buffer in BBQ, but still report when a draw occurred.
+     * Set whether the requested SurfaceSyncGroup should sync the buffer. When set to true, VRI will
+     * create a sync transaction with BBQ and send the resulting buffer back to the
+     * SurfaceSyncGroup. If false, VRI will not try to sync a buffer in BBQ, but still report when a
+     * draw occurred.
      */
     private boolean mSyncBuffer = false;
 
@@ -851,8 +853,19 @@ public final class ViewRootImpl implements ViewParent,
         return mHandwritingInitiator;
     }
 
-    private SurfaceSyncGroup mSyncGroup;
-    private SurfaceSyncGroup.TransactionReadyCallback mTransactionReadyCallback;
+    /**
+     * A SurfaceSyncGroup that is created when WMS requested to sync the buffer
+     */
+    private SurfaceSyncGroup mWmsRequestSyncGroup;
+
+    /**
+     * The SurfaceSyncGroup that represents the active VRI SurfaceSyncGroup. This is non null if
+     * anyone requested the SurfaceSyncGroup for this VRI to ensure that anyone trying to sync with
+     * this VRI are collected together. The SurfaceSyncGroup is cleared when the VRI draws since
+     * that is the stop point where all changes are have been applied. A new SurfaceSyncGroup is
+     * created after that point when something wants to sync VRI again.
+     */
+    private SurfaceSyncGroup mActiveSurfaceSyncGroup;
 
     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
@@ -3613,6 +3626,12 @@ public final class ViewRootImpl implements ViewParent,
         boolean cancelAndRedraw = cancelDueToPreDrawListener
                  || (cancelDraw && mDrewOnceForSync);
         if (!cancelAndRedraw) {
+            // A sync was already requested before the WMS requested sync. This means we need to
+            // sync the buffer, regardless if WMS wants to sync the buffer.
+            if (mActiveSurfaceSyncGroup != null) {
+                mSyncBuffer = true;
+            }
+
             createSyncIfNeeded();
             mDrewOnceForSync = true;
         }
@@ -3626,8 +3645,8 @@ public final class ViewRootImpl implements ViewParent,
                 mPendingTransitions.clear();
             }
 
-            if (mTransactionReadyCallback != null) {
-                mTransactionReadyCallback.onTransactionReady(null);
+            if (mActiveSurfaceSyncGroup != null) {
+                mActiveSurfaceSyncGroup.onTransactionReady(null);
             }
         } else if (cancelAndRedraw) {
             mLastPerformTraversalsSkipDrawReason = cancelDueToPreDrawListener
@@ -3642,8 +3661,8 @@ public final class ViewRootImpl implements ViewParent,
                 }
                 mPendingTransitions.clear();
             }
-            if (!performDraw() && mTransactionReadyCallback != null) {
-                mTransactionReadyCallback.onTransactionReady(null);
+            if (!performDraw() && mActiveSurfaceSyncGroup != null) {
+                mActiveSurfaceSyncGroup.onTransactionReady(null);
             }
         }
 
@@ -3657,39 +3676,40 @@ public final class ViewRootImpl implements ViewParent,
         if (!cancelAndRedraw) {
             mReportNextDraw = false;
             mLastReportNextDrawReason = null;
-            mTransactionReadyCallback = null;
+            mActiveSurfaceSyncGroup = null;
             mSyncBuffer = false;
-            if (isInLocalSync()) {
-                mSyncGroup.markSyncReady();
-                mSyncGroup = null;
-                mLocalSyncState = LOCAL_SYNC_NONE;
+            if (isInWMSRequestedSync()) {
+                mWmsRequestSyncGroup.onTransactionReady(null);
+                mWmsRequestSyncGroup = null;
+                mWmsRequestSyncGroupState = WMS_SYNC_NONE;
             }
         }
     }
 
     private void createSyncIfNeeded() {
-        // Started a sync already or there's nothing needing to sync
-        if (isInLocalSync() || !mReportNextDraw) {
+        // WMS requested sync already started or there's nothing needing to sync
+        if (isInWMSRequestedSync() || !mReportNextDraw) {
             return;
         }
 
         final int seqId = mSyncSeqId;
-        mLocalSyncState = LOCAL_SYNC_PENDING;
-        mSyncGroup = new SurfaceSyncGroup(transaction -> {
-            mLocalSyncState = LOCAL_SYNC_RETURNED;
+        mWmsRequestSyncGroupState = WMS_SYNC_PENDING;
+        mWmsRequestSyncGroup = new SurfaceSyncGroup(t -> {
+            mWmsRequestSyncGroupState = WMS_SYNC_RETURNED;
             // Callback will be invoked on executor thread so post to main thread.
             mHandler.postAtFrontOfQueue(() -> {
-                if (transaction != null) {
-                    mSurfaceChangedTransaction.merge(transaction);
+                if (t != null) {
+                    mSurfaceChangedTransaction.merge(t);
                 }
-                mLocalSyncState = LOCAL_SYNC_MERGED;
+                mWmsRequestSyncGroupState = WMS_SYNC_MERGED;
                 reportDrawFinished(seqId);
             });
         });
         if (DEBUG_BLAST) {
-            Log.d(mTag, "Setup new sync id=" + mSyncGroup);
+            Log.d(mTag, "Setup new sync id=" + mWmsRequestSyncGroup);
         }
-        mSyncGroup.addToSync(mSyncTarget);
+
+        mWmsRequestSyncGroup.addToSync(this);
         notifySurfaceSyncStarted();
     }
 
@@ -4325,19 +4345,11 @@ public final class ViewRootImpl implements ViewParent,
         return mAttachInfo.mThreadedRenderer != null && mAttachInfo.mThreadedRenderer.isEnabled();
     }
 
-    void addToSync(SurfaceSyncGroup.SyncTarget syncable) {
-        if (!isInLocalSync()) {
-            return;
-        }
-        mSyncGroup.addToSync(syncable);
-    }
-
     /**
-     * This VRI is currently in the middle of a sync request, but specifically one initiated from
-     * within VRI.
+     * This VRI is currently in the middle of a sync request that was initiated by WMS.
      */
-    public boolean isInLocalSync() {
-        return mSyncGroup != null;
+    public boolean isInWMSRequestedSync() {
+        return mWmsRequestSyncGroup != null;
     }
 
     private void addFrameCommitCallbackIfNeeded() {
@@ -4404,7 +4416,7 @@ public final class ViewRootImpl implements ViewParent,
             return false;
         }
 
-        final boolean fullRedrawNeeded = mFullRedrawNeeded || mTransactionReadyCallback != null;
+        final boolean fullRedrawNeeded = mFullRedrawNeeded || mActiveSurfaceSyncGroup != null;
         mFullRedrawNeeded = false;
 
         mIsDrawing = true;
@@ -4412,9 +4424,9 @@ public final class ViewRootImpl implements ViewParent,
 
         addFrameCommitCallbackIfNeeded();
 
-        boolean usingAsyncReport = isHardwareEnabled() && mTransactionReadyCallback != null;
+        boolean usingAsyncReport = isHardwareEnabled() && mActiveSurfaceSyncGroup != null;
         if (usingAsyncReport) {
-            registerCallbacksForSync(mSyncBuffer, mTransactionReadyCallback);
+            registerCallbacksForSync(mSyncBuffer, mActiveSurfaceSyncGroup);
         } else if (mHasPendingTransactions) {
             // These callbacks are only needed if there's no sync involved and there were calls to
             // applyTransactionOnDraw. These callbacks check if the draw failed for any reason and
@@ -4465,11 +4477,10 @@ public final class ViewRootImpl implements ViewParent,
             }
 
             if (mSurfaceHolder != null && mSurface.isValid()) {
-                final SurfaceSyncGroup.TransactionReadyCallback transactionReadyCallback =
-                        mTransactionReadyCallback;
+                final SurfaceSyncGroup surfaceSyncGroup = mActiveSurfaceSyncGroup;
                 SurfaceCallbackHelper sch = new SurfaceCallbackHelper(() ->
-                        mHandler.post(() -> transactionReadyCallback.onTransactionReady(null)));
-                mTransactionReadyCallback = null;
+                        mHandler.post(() -> surfaceSyncGroup.onTransactionReady(null)));
+                mActiveSurfaceSyncGroup = null;
 
                 SurfaceHolder.Callback callbacks[] = mSurfaceHolder.getCallbacks();
 
@@ -4480,8 +4491,8 @@ public final class ViewRootImpl implements ViewParent,
                 }
             }
         }
-        if (mTransactionReadyCallback != null && !usingAsyncReport) {
-            mTransactionReadyCallback.onTransactionReady(null);
+        if (mActiveSurfaceSyncGroup != null && !usingAsyncReport) {
+            mActiveSurfaceSyncGroup.onTransactionReady(null);
         }
         if (mPerformContentCapture) {
             performContentCaptureInitialReport();
@@ -8571,8 +8582,8 @@ public final class ViewRootImpl implements ViewParent,
             writer.println(innerPrefix + "mLastPerformDrawFailedReason="
                 + mLastPerformDrawSkippedReason);
         }
-        if (mLocalSyncState != LOCAL_SYNC_NONE) {
-            writer.println(innerPrefix + "mLocalSyncState=" + mLocalSyncState);
+        if (mWmsRequestSyncGroupState != WMS_SYNC_NONE) {
+            writer.println(innerPrefix + "mWmsRequestSyncGroupState=" + mWmsRequestSyncGroupState);
         }
         writer.println(innerPrefix + "mLastReportedMergedConfiguration="
                 + mLastReportedMergedConfiguration);
@@ -11160,7 +11171,7 @@ public final class ViewRootImpl implements ViewParent,
     }
 
     private void registerCallbacksForSync(boolean syncBuffer,
-            final SurfaceSyncGroup.TransactionReadyCallback transactionReadyCallback) {
+            final SurfaceSyncGroup surfaceSyncGroup) {
         if (!isHardwareEnabled()) {
             return;
         }
@@ -11187,7 +11198,7 @@ public final class ViewRootImpl implements ViewParent,
                 // pendingDrawFinished.
                 if ((syncResult
                         & (SYNC_LOST_SURFACE_REWARD_IF_FOUND | SYNC_CONTEXT_IS_STOPPED)) != 0) {
-                    transactionReadyCallback.onTransactionReady(
+                    surfaceSyncGroup.onTransactionReady(
                             mBlastBufferQueue.gatherPendingTransactions(frame));
                     return null;
                 }
@@ -11198,7 +11209,7 @@ public final class ViewRootImpl implements ViewParent,
 
                 if (syncBuffer) {
                     mBlastBufferQueue.syncNextTransaction(
-                            transactionReadyCallback::onTransactionReady);
+                            surfaceSyncGroup::onTransactionReady);
                 }
 
                 return didProduceBuffer -> {
@@ -11218,7 +11229,7 @@ public final class ViewRootImpl implements ViewParent,
                         // since the frame didn't draw on this vsync. It's possible the frame will
                         // draw later, but it's better to not be sync than to block on a frame that
                         // may never come.
-                        transactionReadyCallback.onTransactionReady(
+                        surfaceSyncGroup.onTransactionReady(
                                 mBlastBufferQueue.gatherPendingTransactions(frame));
                         return;
                     }
@@ -11227,35 +11238,23 @@ public final class ViewRootImpl implements ViewParent,
                     // syncNextTransaction callback. Instead, just report back to the Syncer so it
                     // knows that this sync request is complete.
                     if (!syncBuffer) {
-                        transactionReadyCallback.onTransactionReady(null);
+                        surfaceSyncGroup.onTransactionReady(null);
                     }
                 };
             }
         });
     }
 
-    public final SurfaceSyncGroup.SyncTarget mSyncTarget = new SurfaceSyncGroup.SyncTarget() {
-        @Override
-        public void onAddedToSyncGroup(SurfaceSyncGroup parentSyncGroup,
-                SurfaceSyncGroup.TransactionReadyCallback transactionReadyCallback) {
-            updateSyncInProgressCount(parentSyncGroup);
-            if (!isInLocalSync()) {
-                // Always sync the buffer if the sync request did not come from VRI.
-                mSyncBuffer = true;
-            }
-
-            if (mTransactionReadyCallback != null) {
-                Log.d(mTag, "Already set sync for the next draw.");
-                mTransactionReadyCallback.onTransactionReady(null);
-            }
-            if (DEBUG_BLAST) {
-                Log.d(mTag, "Setting syncFrameCallback");
-            }
-            mTransactionReadyCallback = transactionReadyCallback;
+    @Override
+    public SurfaceSyncGroup getOrCreateSurfaceSyncGroup() {
+        if (mActiveSurfaceSyncGroup == null) {
+            mActiveSurfaceSyncGroup = new SurfaceSyncGroup();
+            updateSyncInProgressCount(mActiveSurfaceSyncGroup);
             if (!mIsInTraversal && !mTraversalScheduled) {
                 scheduleTraversals();
             }
         }
+        return mActiveSurfaceSyncGroup;
     };
 
     private final Executor mSimpleExecutor = Runnable::run;
@@ -11280,15 +11279,10 @@ public final class ViewRootImpl implements ViewParent,
         });
     }
 
-    @Override
-    public SurfaceSyncGroup.SyncTarget getSyncTarget() {
-        return mSyncTarget;
-    }
-
-    void mergeSync(SurfaceSyncGroup otherSyncGroup) {
-        if (!isInLocalSync()) {
+    void addToSync(SurfaceSyncGroup syncable) {
+        if (mActiveSurfaceSyncGroup == null) {
             return;
         }
-        mSyncGroup.merge(otherSyncGroup);
+        mActiveSurfaceSyncGroup.addToSync(syncable, false /* parentSyncGroupMerge */);
     }
 }
diff --git a/core/java/android/window/SurfaceSyncGroup.java b/core/java/android/window/SurfaceSyncGroup.java
index 395073941930..250652a53677 100644
--- a/core/java/android/window/SurfaceSyncGroup.java
+++ b/core/java/android/window/SurfaceSyncGroup.java
@@ -40,62 +40,63 @@ import java.util.function.Supplier;
  * mechanism so each sync implementation doesn't need to handle it themselves. The SurfaceSyncGroup
  * class is used the following way.
  *
- * 1. {@link #SurfaceSyncGroup()} constructor is called
- * 2. {@link #addToSync(SyncTarget)} is called for every SyncTarget object that wants to be
- * included in the sync. If the addSync is called for an {@link AttachedSurfaceControl} or
- * {@link SurfaceView} it needs to be called on the UI thread. When addToSync is called, it's
+ * 1. {@link #addToSync(SurfaceSyncGroup, boolean)} is called for every SurfaceSyncGroup object that
+ * wants to be included in the sync. If the addSync is called for an {@link AttachedSurfaceControl}
+ * or {@link SurfaceView} it needs to be called on the UI thread. When addToSync is called, it's
  * guaranteed that any UI updates that were requested before addToSync but after the last frame
  * drew, will be included in the sync.
- * 3. {@link #markSyncReady()} should be called when all the {@link SyncTarget}s have been added
- * to the SurfaceSyncGroup. At this point, the SurfaceSyncGroup is closed and no more SyncTargets
- * can be added to it.
- * 4. The SurfaceSyncGroup will gather the data for each SyncTarget using the steps described below.
- * When all the SyncTargets have finished, the syncRequestComplete will be invoked and the
- * transaction will either be applied or sent to the caller. In most cases, only the
- * SurfaceSyncGroup  should be handling the Transaction object directly. However, there are some
+ * 2. {@link #markSyncReady()} should be called when all the {@link SurfaceSyncGroup}s have been
+ * added to the SurfaceSyncGroup. At this point, the SurfaceSyncGroup is closed and no more
+ * SurfaceSyncGroups can be added to it.
+ * 3. The SurfaceSyncGroup will gather the data for each SurfaceSyncGroup using the steps described
+ * below. When all the SurfaceSyncGroups have finished, the syncRequestComplete will be invoked and
+ * the transaction will either be applied or sent to the caller. In most cases, only the
+ * SurfaceSyncGroup should be handling the Transaction object directly. However, there are some
  * cases where the framework needs to send the Transaction elsewhere, like in ViewRootImpl, so that
  * option is provided.
  *
- * The following is what happens within the {@link SurfaceSyncGroup}
- * 1. Each SyncTarget will get a {@link SyncTarget#onAddedToSyncGroup} callback that contains a
- * {@link TransactionReadyCallback}.
- * 2. Each {@link SyncTarget} needs to invoke
- * {@link TransactionReadyCallback#onTransactionReady(Transaction)}. This makes sure the
- * SurfaceSyncGroup knows when the SyncTarget is complete, allowing the SurfaceSyncGroup to get the
- * Transaction that contains the buffer.
- * 3. When the final TransactionReadyCallback finishes for the SurfaceSyncGroup, in most cases the
- * transaction is applied and then the sync complete callbacks are invoked, letting the callers know
- * the sync is now complete.
+ * The following is what happens within the {@link android.window.SurfaceSyncGroup}
+ * 1. Each SurfaceSyncGroup will get a
+ * {@link SurfaceSyncGroup#onAddedToSyncGroup(SurfaceSyncGroup, TransactionReadyCallback)} callback
+ * that contains a  {@link TransactionReadyCallback}.
+ * 2. Each {@link SurfaceSyncGroup} needs to invoke
+ * {@link SurfaceSyncGroup#onTransactionReady(Transaction)}.
+ * This makes sure the parent SurfaceSyncGroup knows when the SurfaceSyncGroup is complete, allowing
+ * the parent SurfaceSyncGroup to get the Transaction that contains the changes for the child
+ * SurfaceSyncGroup
+ * 3. When the final TransactionReadyCallback finishes for the child SurfaceSyncGroups, the
+ * transaction is either applied if it's the top most parent or the final merged transaction is sent
+ * up to its parent SurfaceSyncGroup.
  *
  * @hide
  */
-public final class SurfaceSyncGroup {
+public class SurfaceSyncGroup {
     private static final String TAG = "SurfaceSyncGroup";
     private static final boolean DEBUG = false;
 
     private static Supplier<Transaction> sTransactionFactory = Transaction::new;
 
     /**
-     * Class that collects the {@link SyncTarget}s and notifies when all the surfaces have
+     * Class that collects the {@link SurfaceSyncGroup}s and notifies when all the surfaces have
      * a frame ready.
      */
     private final Object mLock = new Object();
 
     @GuardedBy("mLock")
-    private final Set<Integer> mPendingSyncs = new ArraySet<>();
+    private final Set<TransactionReadyCallback> mPendingSyncs = new ArraySet<>();
     @GuardedBy("mLock")
     private final Transaction mTransaction = sTransactionFactory.get();
     @GuardedBy("mLock")
     private boolean mSyncReady;
 
     @GuardedBy("mLock")
-    private Consumer<Transaction> mSyncRequestCompleteCallback;
+    private boolean mFinished;
 
     @GuardedBy("mLock")
-    private final Set<SurfaceSyncGroup> mMergedSyncGroups = new ArraySet<>();
+    private TransactionReadyCallback mTransactionReadyCallback;
 
     @GuardedBy("mLock")
-    private boolean mFinished;
+    private SurfaceSyncGroup mParentSyncGroup;
 
     @GuardedBy("mLock")
     private final ArraySet<Pair<Executor, Runnable>> mSyncCompleteCallbacks = new ArraySet<>();
@@ -122,16 +123,16 @@ public final class SurfaceSyncGroup {
     /**
      * Creates a sync.
      *
-     * @param syncRequestComplete The complete callback that contains the syncId and transaction
-     *                            with all the sync data merged. The Transaction passed back can be
-     *                            null.
+     * @param transactionReadyCallback The complete callback that contains the syncId and
+     *                                 transaction with all the sync data merged. The Transaction
+     *                                 passed back can be null.
      *
      * NOTE: Only should be used by ViewRootImpl
      * @hide
      */
-    public SurfaceSyncGroup(Consumer<Transaction> syncRequestComplete) {
-        mSyncRequestCompleteCallback = transaction -> {
-            syncRequestComplete.accept(transaction);
+    public SurfaceSyncGroup(Consumer<Transaction> transactionReadyCallback) {
+        mTransactionReadyCallback = transaction -> {
+            transactionReadyCallback.accept(transaction);
             synchronized (mLock) {
                 for (Pair<Executor, Runnable> callback : mSyncCompleteCallbacks) {
                     callback.first.execute(callback.second);
@@ -157,6 +158,31 @@ public final class SurfaceSyncGroup {
     }
 
     /**
+     * Mark the sync set as ready to complete. No more data can be added to the specified
+     * syncId.
+     * Once the sync set is marked as ready, it will be able to complete once all Syncables in the
+     * set have completed their sync
+     */
+    public void markSyncReady() {
+        onTransactionReady(null);
+    }
+
+    /**
+     * Similar to {@link #markSyncReady()}, but a transaction is passed in to merge with the
+     * SurfaceSyncGroup.
+     * @param t The transaction that merges into the main Transaction for the SurfaceSyncGroup.
+     */
+    public void onTransactionReady(@Nullable Transaction t) {
+        synchronized (mLock) {
+            mSyncReady = true;
+            if (t != null) {
+                mTransaction.merge(t);
+            }
+            checkIfSyncIsComplete();
+        }
+    }
+
+    /**
      * Add a SurfaceView to a sync set. This is different than
      * {@link #addToSync(AttachedSurfaceControl)} because it requires the caller to notify the start
      * and finish drawing in order to sync.
@@ -171,7 +197,13 @@ public final class SurfaceSyncGroup {
     @UiThread
     public boolean addToSync(SurfaceView surfaceView,
             Consumer<SurfaceViewFrameCallback> frameCallbackConsumer) {
-        return addToSync(new SurfaceViewSyncTarget(surfaceView, frameCallbackConsumer));
+        SurfaceSyncGroup surfaceSyncGroup = new SurfaceSyncGroup();
+        if (addToSync(surfaceSyncGroup, false /* parentSyncGroupMerge */)) {
+            frameCallbackConsumer.accept(
+                    () -> surfaceView.syncNextFrame(surfaceSyncGroup::onTransactionReady));
+            return true;
+        }
+        return false;
     }
 
     /**
@@ -185,29 +217,38 @@ public final class SurfaceSyncGroup {
         if (viewRoot == null) {
             return false;
         }
-        SyncTarget syncTarget = viewRoot.getSyncTarget();
-        if (syncTarget == null) {
+        SurfaceSyncGroup surfaceSyncGroup = viewRoot.getOrCreateSurfaceSyncGroup();
+        if (surfaceSyncGroup == null) {
             return false;
         }
-        return addToSync(syncTarget);
+        return addToSync(surfaceSyncGroup, false /* parentSyncGroupMerge */);
     }
 
     /**
-     * Add a {@link SyncTarget} to a sync set. The sync set will wait for all
+     * Add a {@link SurfaceSyncGroup} to a sync set. The sync set will wait for all
      * SyncableSurfaces to complete before notifying.
      *
-     * @param syncTarget A SyncTarget that implements how to handle syncing transactions.
-     * @return true if the SyncTarget was successfully added to the SyncGroup, false otherwise.
+     * @param surfaceSyncGroup A SyncableSurface that implements how to handle syncing
+     *                         buffers.
+     * @return true if the SyncGroup was successfully added to the current SyncGroup, false
+     * otherwise.
      */
-    public boolean addToSync(SyncTarget syncTarget) {
+    public boolean addToSync(SurfaceSyncGroup surfaceSyncGroup, boolean parentSyncGroupMerge) {
         TransactionReadyCallback transactionReadyCallback = new TransactionReadyCallback() {
             @Override
             public void onTransactionReady(Transaction t) {
                 synchronized (mLock) {
                     if (t != null) {
+                        // When an older parent sync group is added due to a child syncGroup getting
+                        // added to multiple groups, we need to maintain merge order so the older
+                        // parentSyncGroup transactions are overwritten by anything in the newer
+                        // parentSyncGroup.
+                        if (parentSyncGroupMerge) {
+                            t.merge(mTransaction);
+                        }
                         mTransaction.merge(t);
                     }
-                    mPendingSyncs.remove(hashCode());
+                    mPendingSyncs.remove(this);
                     checkIfSyncIsComplete();
                 }
             }
@@ -216,35 +257,38 @@ public final class SurfaceSyncGroup {
         synchronized (mLock) {
             if (mSyncReady) {
                 Log.e(TAG, "Sync " + this + " was already marked as ready. No more "
-                        + "SyncTargets can be added.");
+                        + "SurfaceSyncGroups can be added.");
                 return false;
             }
-            mPendingSyncs.add(transactionReadyCallback.hashCode());
+            mPendingSyncs.add(transactionReadyCallback);
         }
-        syncTarget.onAddedToSyncGroup(this, transactionReadyCallback);
+        surfaceSyncGroup.onAddedToSyncGroup(this, transactionReadyCallback);
         return true;
     }
 
     /**
-     * Mark the sync set as ready to complete. No more data can be added to the specified
-     * syncId.
-     * Once the sync set is marked as ready, it will be able to complete once all Syncables in the
-     * set have completed their sync
+     * Add a Transaction to this sync set. This allows the caller to provide other info that
+     * should be synced with the transactions.
      */
-    public void markSyncReady() {
+    public void addTransactionToSync(Transaction t) {
         synchronized (mLock) {
-            mSyncReady = true;
-            checkIfSyncIsComplete();
+            mTransaction.merge(t);
         }
     }
 
     @GuardedBy("mLock")
     private void checkIfSyncIsComplete() {
-        if (!mSyncReady || !mPendingSyncs.isEmpty() || !mMergedSyncGroups.isEmpty()) {
+        if (mFinished) {
             if (DEBUG) {
-                Log.d(TAG, "Syncable is not complete. mSyncReady=" + mSyncReady
-                        + " mPendingSyncs=" + mPendingSyncs.size() + " mergedSyncs="
-                        + mMergedSyncGroups.size());
+                Log.d(TAG, "SurfaceSyncGroup=" + this + " is already complete");
+            }
+            return;
+        }
+
+        if (!mSyncReady || !mPendingSyncs.isEmpty()) {
+            if (DEBUG) {
+                Log.d(TAG, "SurfaceSyncGroup=" + this + " is not complete. mSyncReady="
+                        + mSyncReady + " mPendingSyncs=" + mPendingSyncs.size());
             }
             return;
         }
@@ -252,114 +296,48 @@ public final class SurfaceSyncGroup {
         if (DEBUG) {
             Log.d(TAG, "Successfully finished sync id=" + this);
         }
-
-        mSyncRequestCompleteCallback.accept(mTransaction);
+        mTransactionReadyCallback.onTransactionReady(mTransaction);
         mFinished = true;
     }
 
-    /**
-     * Add a Transaction to this sync set. This allows the caller to provide other info that
-     * should be synced with the transactions.
-     */
-    public void addTransactionToSync(Transaction t) {
-        synchronized (mLock) {
-            mTransaction.merge(t);
-        }
-    }
-
-    private void updateCallback(Consumer<Transaction> transactionConsumer) {
+    private void onAddedToSyncGroup(SurfaceSyncGroup parentSyncGroup,
+            TransactionReadyCallback transactionReadyCallback) {
+        boolean finished = false;
         synchronized (mLock) {
             if (mFinished) {
-                Log.e(TAG, "Attempting to merge SyncGroup " + this + " when sync is"
-                        + " already complete");
-                transactionConsumer.accept(null);
-            }
-
-            final Consumer<Transaction> oldCallback = mSyncRequestCompleteCallback;
-            mSyncRequestCompleteCallback = transaction -> {
-                oldCallback.accept(null);
-                transactionConsumer.accept(transaction);
-            };
-        }
-    }
-
-    /**
-     * Merge a SyncGroup into this SyncGroup. Since SyncGroups could still have pending SyncTargets,
-     * we need to make sure those can still complete before the mergeTo SyncGroup is considered
-     * complete.
-     *
-     * We keep track of all the merged SyncGroups until they are marked as done, and then they
-     * are removed from the set. This SyncGroup is not considered done until all the merged
-     * SyncGroups are done.
-     *
-     * When the merged SyncGroup is complete, it will invoke the original syncRequestComplete
-     * callback but send an empty transaction to ensure the changes are applied early. This
-     * is needed in case the original sync is relying on the callback to continue processing.
-     *
-     * @param otherSyncGroup The other SyncGroup to merge into this one.
-     */
-    public void merge(SurfaceSyncGroup otherSyncGroup) {
-        synchronized (mLock) {
-            mMergedSyncGroups.add(otherSyncGroup);
-        }
-        otherSyncGroup.updateCallback(transaction -> {
-            synchronized (mLock) {
-                mMergedSyncGroups.remove(otherSyncGroup);
-                if (transaction != null) {
-                    mTransaction.merge(transaction);
+                finished = true;
+            } else {
+                // If this SurfaceSyncGroup was already added to a different SurfaceSyncGroup, we
+                // need to combine everything. We can add the old SurfaceSyncGroup parent to the new
+                // parent so the new parent doesn't complete until the old parent does.
+                // Additionally, the old parent will not get the final transaction object and
+                // instead will send it to the new parent, ensuring that any other SurfaceSyncGroups
+                // from the original parent are also combined with the new parent SurfaceSyncGroup.
+                if (mParentSyncGroup != null) {
+                    Log.d(TAG, "Already part of sync group " + mParentSyncGroup + " " + this);
+                    parentSyncGroup.addToSync(mParentSyncGroup, true /* parentSyncGroupMerge */);
                 }
-                checkIfSyncIsComplete();
+                mParentSyncGroup = parentSyncGroup;
+                final TransactionReadyCallback lastCallback = mTransactionReadyCallback;
+                mTransactionReadyCallback = t -> {
+                    lastCallback.onTransactionReady(null);
+                    transactionReadyCallback.onTransactionReady(t);
+                };
             }
-        });
-    }
-
-    /**
-     * Wrapper class to help synchronize SurfaceViews
-     */
-    private static class SurfaceViewSyncTarget implements SyncTarget {
-        private final SurfaceView mSurfaceView;
-        private final Consumer<SurfaceViewFrameCallback> mFrameCallbackConsumer;
-
-        SurfaceViewSyncTarget(SurfaceView surfaceView,
-                Consumer<SurfaceViewFrameCallback> frameCallbackConsumer) {
-            mSurfaceView = surfaceView;
-            mFrameCallbackConsumer = frameCallbackConsumer;
         }
 
-        @Override
-        public void onAddedToSyncGroup(SurfaceSyncGroup parentSyncGroup,
-                TransactionReadyCallback transactionReadyCallback) {
-            mFrameCallbackConsumer.accept(
-                    () -> mSurfaceView.syncNextFrame(transactionReadyCallback::onTransactionReady));
+        // Invoke the callback outside of the lock when the SurfaceSyncGroup being added was already
+        // complete.
+        if (finished) {
+            transactionReadyCallback.onTransactionReady(null);
         }
     }
-
-    /**
-     * A SyncTarget that can be added to a sync set.
-     */
-    public interface SyncTarget {
-        /**
-         * Called when the SyncTarget has been added to a SyncGroup as is ready to begin handing a
-         * sync request. When invoked, the implementor is required to call
-         * {@link TransactionReadyCallback#onTransactionReady(Transaction)} in order for this
-         * SurfaceSyncGroup to fully complete.
-         *
-         * Always invoked on the thread that initiated the call to {@link #addToSync(SyncTarget)}
-         *
-         * @param parentSyncGroup The sync group this target has been added to.
-         * @param transactionReadyCallback A TransactionReadyCallback that the caller must invoke
-         *                                 onTransactionReady
-         */
-        void onAddedToSyncGroup(SurfaceSyncGroup parentSyncGroup,
-                TransactionReadyCallback transactionReadyCallback);
-    }
-
     /**
      * Interface so the SurfaceSyncer can know when it's safe to start and when everything has been
      * completed. The caller should invoke the calls when the rendering has started and finished a
      * frame.
      */
-    public interface TransactionReadyCallback {
+    private interface TransactionReadyCallback {
         /**
          * Invoked when the transaction is ready to sync.
          *
diff --git a/services/tests/wmtests/src/com/android/server/wm/SurfaceSyncGroupTest.java b/services/tests/wmtests/src/com/android/server/wm/SurfaceSyncGroupTest.java
index 5e1fae095db8..7d9f29c0a63d 100644
--- a/services/tests/wmtests/src/com/android/server/wm/SurfaceSyncGroupTest.java
+++ b/services/tests/wmtests/src/com/android/server/wm/SurfaceSyncGroupTest.java
@@ -20,6 +20,9 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
 
 import android.platform.test.annotations.Presubmit;
 import android.view.SurfaceControl;
@@ -31,12 +34,15 @@ import org.junit.Before;
 import org.junit.Test;
 
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.Executor;
 import java.util.concurrent.TimeUnit;
 
 @SmallTest
 @Presubmit
 public class SurfaceSyncGroupTest {
 
+    private final Executor mExecutor = Runnable::run;
+
     @Before
     public void setup() {
         SurfaceSyncGroup.setTransactionFactory(StubTransaction::new);
@@ -45,10 +51,11 @@ public class SurfaceSyncGroupTest {
     @Test
     public void testSyncOne() throws InterruptedException {
         final CountDownLatch finishedLatch = new CountDownLatch(1);
-        SurfaceSyncGroup syncGroup = new SurfaceSyncGroup(transaction -> finishedLatch.countDown());
+        SurfaceSyncGroup syncGroup = new SurfaceSyncGroup();
+        syncGroup.addSyncCompleteCallback(mExecutor, finishedLatch::countDown);
         SyncTarget syncTarget = new SyncTarget();
-        syncGroup.addToSync(syncTarget);
-        syncGroup.markSyncReady();
+        syncGroup.addToSync(syncTarget, false /* parentSyncGroupMerge */);
+        syncGroup.onTransactionReady(null);
 
         syncTarget.onBufferReady();
 
@@ -59,15 +66,16 @@ public class SurfaceSyncGroupTest {
     @Test
     public void testSyncMultiple() throws InterruptedException {
         final CountDownLatch finishedLatch = new CountDownLatch(1);
-        SurfaceSyncGroup syncGroup = new SurfaceSyncGroup(transaction -> finishedLatch.countDown());
+        SurfaceSyncGroup syncGroup = new SurfaceSyncGroup();
+        syncGroup.addSyncCompleteCallback(mExecutor, finishedLatch::countDown);
         SyncTarget syncTarget1 = new SyncTarget();
         SyncTarget syncTarget2 = new SyncTarget();
         SyncTarget syncTarget3 = new SyncTarget();
 
-        syncGroup.addToSync(syncTarget1);
-        syncGroup.addToSync(syncTarget2);
-        syncGroup.addToSync(syncTarget3);
-        syncGroup.markSyncReady();
+        syncGroup.addToSync(syncTarget1, false /* parentSyncGroupMerge */);
+        syncGroup.addToSync(syncTarget2, false /* parentSyncGroupMerge */);
+        syncGroup.addToSync(syncTarget3, false /* parentSyncGroupMerge */);
+        syncGroup.onTransactionReady(null);
 
         syncTarget1.onBufferReady();
         assertNotEquals(0, finishedLatch.getCount());
@@ -83,35 +91,35 @@ public class SurfaceSyncGroupTest {
 
     @Test
     public void testAddSyncWhenSyncComplete() {
-        final CountDownLatch finishedLatch = new CountDownLatch(1);
-        SurfaceSyncGroup syncGroup = new SurfaceSyncGroup(transaction -> finishedLatch.countDown());
+        SurfaceSyncGroup syncGroup = new SurfaceSyncGroup();
 
         SyncTarget syncTarget1 = new SyncTarget();
         SyncTarget syncTarget2 = new SyncTarget();
 
-        assertTrue(syncGroup.addToSync(syncTarget1));
-        syncGroup.markSyncReady();
+        assertTrue(syncGroup.addToSync(syncTarget1, false /* parentSyncGroupMerge */));
+        syncGroup.onTransactionReady(null);
         // Adding to a sync that has been completed is also invalid since the sync id has been
         // cleared.
-        assertFalse(syncGroup.addToSync(syncTarget2));
+        assertFalse(syncGroup.addToSync(syncTarget2, false /* parentSyncGroupMerge */));
     }
 
     @Test
-    public void testMultiplesyncGroups() throws InterruptedException {
+    public void testMultipleSyncGroups() throws InterruptedException {
         final CountDownLatch finishedLatch1 = new CountDownLatch(1);
         final CountDownLatch finishedLatch2 = new CountDownLatch(1);
-        SurfaceSyncGroup syncGroup1 = new SurfaceSyncGroup(
-                transaction -> finishedLatch1.countDown());
-        SurfaceSyncGroup syncGroup2 = new SurfaceSyncGroup(
-                transaction -> finishedLatch2.countDown());
+        SurfaceSyncGroup syncGroup1 = new SurfaceSyncGroup();
+        SurfaceSyncGroup syncGroup2 = new SurfaceSyncGroup();
+
+        syncGroup1.addSyncCompleteCallback(mExecutor, finishedLatch1::countDown);
+        syncGroup2.addSyncCompleteCallback(mExecutor, finishedLatch2::countDown);
 
         SyncTarget syncTarget1 = new SyncTarget();
         SyncTarget syncTarget2 = new SyncTarget();
 
-        assertTrue(syncGroup1.addToSync(syncTarget1));
-        assertTrue(syncGroup2.addToSync(syncTarget2));
-        syncGroup1.markSyncReady();
-        syncGroup2.markSyncReady();
+        assertTrue(syncGroup1.addToSync(syncTarget1, false /* parentSyncGroupMerge */));
+        assertTrue(syncGroup2.addToSync(syncTarget2, false /* parentSyncGroupMerge */));
+        syncGroup1.onTransactionReady(null);
+        syncGroup2.onTransactionReady(null);
 
         syncTarget1.onBufferReady();
 
@@ -126,22 +134,23 @@ public class SurfaceSyncGroupTest {
     }
 
     @Test
-    public void testMergeSync() throws InterruptedException {
+    public void testAddSyncGroup() throws InterruptedException {
         final CountDownLatch finishedLatch1 = new CountDownLatch(1);
         final CountDownLatch finishedLatch2 = new CountDownLatch(1);
-        SurfaceSyncGroup syncGroup1 = new SurfaceSyncGroup(
-                transaction -> finishedLatch1.countDown());
-        SurfaceSyncGroup syncGroup2 = new SurfaceSyncGroup(
-                transaction -> finishedLatch2.countDown());
+        SurfaceSyncGroup syncGroup1 = new SurfaceSyncGroup();
+        SurfaceSyncGroup syncGroup2 = new SurfaceSyncGroup();
+
+        syncGroup1.addSyncCompleteCallback(mExecutor, finishedLatch1::countDown);
+        syncGroup2.addSyncCompleteCallback(mExecutor, finishedLatch2::countDown);
 
         SyncTarget syncTarget1 = new SyncTarget();
         SyncTarget syncTarget2 = new SyncTarget();
 
-        assertTrue(syncGroup1.addToSync(syncTarget1));
-        assertTrue(syncGroup2.addToSync(syncTarget2));
-        syncGroup1.markSyncReady();
-        syncGroup2.merge(syncGroup1);
-        syncGroup2.markSyncReady();
+        assertTrue(syncGroup1.addToSync(syncTarget1, false /* parentSyncGroupMerge */));
+        assertTrue(syncGroup2.addToSync(syncTarget2, false /* parentSyncGroupMerge */));
+        syncGroup1.onTransactionReady(null);
+        syncGroup2.addToSync(syncGroup1, false /* parentSyncGroupMerge */);
+        syncGroup2.onTransactionReady(null);
 
         // Finish syncTarget2 first to test that the syncGroup is not complete until the merged sync
         // is also done.
@@ -161,28 +170,29 @@ public class SurfaceSyncGroupTest {
     }
 
     @Test
-    public void testMergeSyncAlreadyComplete() throws InterruptedException {
+    public void testAddSyncAlreadyComplete() throws InterruptedException {
         final CountDownLatch finishedLatch1 = new CountDownLatch(1);
         final CountDownLatch finishedLatch2 = new CountDownLatch(1);
-        SurfaceSyncGroup syncGroup1 = new SurfaceSyncGroup(
-                transaction -> finishedLatch1.countDown());
-        SurfaceSyncGroup syncGroup2 = new SurfaceSyncGroup(
-                transaction -> finishedLatch2.countDown());
+        SurfaceSyncGroup syncGroup1 = new SurfaceSyncGroup();
+        SurfaceSyncGroup syncGroup2 = new SurfaceSyncGroup();
+
+        syncGroup1.addSyncCompleteCallback(mExecutor, finishedLatch1::countDown);
+        syncGroup2.addSyncCompleteCallback(mExecutor, finishedLatch2::countDown);
 
         SyncTarget syncTarget1 = new SyncTarget();
         SyncTarget syncTarget2 = new SyncTarget();
 
-        assertTrue(syncGroup1.addToSync(syncTarget1));
-        assertTrue(syncGroup2.addToSync(syncTarget2));
-        syncGroup1.markSyncReady();
+        assertTrue(syncGroup1.addToSync(syncTarget1, false /* parentSyncGroupMerge */));
+        assertTrue(syncGroup2.addToSync(syncTarget2, false /* parentSyncGroupMerge */));
+        syncGroup1.onTransactionReady(null);
         syncTarget1.onBufferReady();
 
         // The first sync will still get a callback when it's sync requirements are done.
         finishedLatch1.await(5, TimeUnit.SECONDS);
         assertEquals(0, finishedLatch1.getCount());
 
-        syncGroup2.merge(syncGroup1);
-        syncGroup2.markSyncReady();
+        syncGroup2.addToSync(syncGroup1, false /* parentSyncGroupMerge */);
+        syncGroup2.onTransactionReady(null);
         syncTarget2.onBufferReady();
 
         // Verify that the second sync will receive complete since the merged sync was already
@@ -191,18 +201,145 @@ public class SurfaceSyncGroupTest {
         assertEquals(0, finishedLatch2.getCount());
     }
 
-    private static class SyncTarget implements SurfaceSyncGroup.SyncTarget {
-        private SurfaceSyncGroup.TransactionReadyCallback mTransactionReadyCallback;
+    @Test
+    public void testAddSyncAlreadyInASync_NewSyncReadyFirst() throws InterruptedException {
+        final CountDownLatch finishedLatch1 = new CountDownLatch(1);
+        final CountDownLatch finishedLatch2 = new CountDownLatch(1);
+        SurfaceSyncGroup syncGroup1 = new SurfaceSyncGroup();
+        SurfaceSyncGroup syncGroup2 = new SurfaceSyncGroup();
 
-        @Override
-        public void onAddedToSyncGroup(SurfaceSyncGroup parentSyncGroup,
-                SurfaceSyncGroup.TransactionReadyCallback transactionReadyCallback) {
-            mTransactionReadyCallback = transactionReadyCallback;
-        }
+        syncGroup1.addSyncCompleteCallback(mExecutor, finishedLatch1::countDown);
+        syncGroup2.addSyncCompleteCallback(mExecutor, finishedLatch2::countDown);
+
+        SyncTarget syncTarget1 = new SyncTarget();
+        SyncTarget syncTarget2 = new SyncTarget();
+        SyncTarget syncTarget3 = new SyncTarget();
+
+        assertTrue(syncGroup1.addToSync(syncTarget1, false /* parentSyncGroupMerge */));
+        assertTrue(syncGroup1.addToSync(syncTarget2, false /* parentSyncGroupMerge */));
+
+        // Add syncTarget1 to syncGroup2 so it forces syncGroup1 into syncGroup2
+        assertTrue(syncGroup2.addToSync(syncTarget1, false /* parentSyncGroupMerge */));
+        assertTrue(syncGroup2.addToSync(syncTarget3, false /* parentSyncGroupMerge */));
+
+        syncGroup1.onTransactionReady(null);
+        syncGroup2.onTransactionReady(null);
+
+        // Make target1 and target3 ready, but not target2. SyncGroup2 should not be ready since
+        // SyncGroup2 also waits for all of SyncGroup1 to finish, which includes target2
+        syncTarget1.onBufferReady();
+        syncTarget3.onBufferReady();
+
+        // Neither SyncGroup will be ready.
+        finishedLatch1.await(1, TimeUnit.SECONDS);
+        finishedLatch2.await(1, TimeUnit.SECONDS);
+
+        assertEquals(1, finishedLatch1.getCount());
+        assertEquals(1, finishedLatch2.getCount());
+
+        syncTarget2.onBufferReady();
+
+        // Both sync groups should be ready after target2 completed.
+        finishedLatch1.await(5, TimeUnit.SECONDS);
+        finishedLatch2.await(5, TimeUnit.SECONDS);
+        assertEquals(0, finishedLatch1.getCount());
+        assertEquals(0, finishedLatch2.getCount());
+    }
+
+    @Test
+    public void testAddSyncAlreadyInASync_OldSyncFinishesFirst() throws InterruptedException {
+        final CountDownLatch finishedLatch1 = new CountDownLatch(1);
+        final CountDownLatch finishedLatch2 = new CountDownLatch(1);
+        SurfaceSyncGroup syncGroup1 = new SurfaceSyncGroup();
+        SurfaceSyncGroup syncGroup2 = new SurfaceSyncGroup();
+
+        syncGroup1.addSyncCompleteCallback(mExecutor, finishedLatch1::countDown);
+        syncGroup2.addSyncCompleteCallback(mExecutor, finishedLatch2::countDown);
+
+        SyncTarget syncTarget1 = new SyncTarget();
+        SyncTarget syncTarget2 = new SyncTarget();
+        SyncTarget syncTarget3 = new SyncTarget();
+
+        assertTrue(syncGroup1.addToSync(syncTarget1, false /* parentSyncGroupMerge */));
+        assertTrue(syncGroup1.addToSync(syncTarget2, false /* parentSyncGroupMerge */));
+        syncTarget2.onBufferReady();
+
+        // Add syncTarget1 to syncGroup2 so it forces syncGroup1 into syncGroup2
+        assertTrue(syncGroup2.addToSync(syncTarget1, false /* parentSyncGroupMerge */));
+        assertTrue(syncGroup2.addToSync(syncTarget3, false /* parentSyncGroupMerge */));
+
+        syncGroup1.onTransactionReady(null);
+        syncGroup2.onTransactionReady(null);
+
+        syncTarget1.onBufferReady();
+
+        // Only SyncGroup1 will be ready, but SyncGroup2 still needs its own targets to be ready.
+        finishedLatch1.await(1, TimeUnit.SECONDS);
+        finishedLatch2.await(1, TimeUnit.SECONDS);
+
+        assertEquals(0, finishedLatch1.getCount());
+        assertEquals(1, finishedLatch2.getCount());
+
+        syncTarget3.onBufferReady();
+
+        // SyncGroup2 is finished after target3 completed.
+        finishedLatch2.await(1, TimeUnit.SECONDS);
+        assertEquals(0, finishedLatch2.getCount());
+    }
+
+    @Test
+    public void testParentSyncGroupMerge_true() {
+        // Temporarily set a new transaction factory so it will return the stub transaction for
+        // the sync group.
+        SurfaceControl.Transaction parentTransaction = spy(new StubTransaction());
+        SurfaceSyncGroup.setTransactionFactory(() -> parentTransaction);
+
+        final CountDownLatch finishedLatch = new CountDownLatch(1);
+        SurfaceSyncGroup syncGroup = new SurfaceSyncGroup();
+        syncGroup.addSyncCompleteCallback(mExecutor, finishedLatch::countDown);
+
+        SurfaceControl.Transaction targetTransaction = spy(new StubTransaction());
+        SurfaceSyncGroup.setTransactionFactory(() -> targetTransaction);
+
+        SyncTarget syncTarget = new SyncTarget();
+        assertTrue(syncGroup.addToSync(syncTarget, true /* parentSyncGroupMerge */));
+        syncTarget.onTransactionReady(null);
+
+        // When parentSyncGroupMerge is true, the transaction passed in merges the main SyncGroup
+        // transaction first because it knows the previous parentSyncGroup is older so it should
+        // be overwritten by anything newer.
+        verify(targetTransaction).merge(parentTransaction);
+        verify(parentTransaction).merge(targetTransaction);
+    }
+
+    @Test
+    public void testParentSyncGroupMerge_false() {
+        // Temporarily set a new transaction factory so it will return the stub transaction for
+        // the sync group.
+        SurfaceControl.Transaction parentTransaction = spy(new StubTransaction());
+        SurfaceSyncGroup.setTransactionFactory(() -> parentTransaction);
+
+        final CountDownLatch finishedLatch = new CountDownLatch(1);
+        SurfaceSyncGroup syncGroup = new SurfaceSyncGroup();
+        syncGroup.addSyncCompleteCallback(mExecutor, finishedLatch::countDown);
+
+        SurfaceControl.Transaction targetTransaction = spy(new StubTransaction());
+        SurfaceSyncGroup.setTransactionFactory(() -> targetTransaction);
+
+        SyncTarget syncTarget = new SyncTarget();
+        assertTrue(syncGroup.addToSync(syncTarget, false /* parentSyncGroupMerge */));
+        syncTarget.onTransactionReady(null);
+
+        // When parentSyncGroupMerge is false, the transaction passed in should not merge
+        // the main SyncGroup since we don't need to change the transaction order
+        verify(targetTransaction, never()).merge(parentTransaction);
+        verify(parentTransaction).merge(targetTransaction);
+    }
 
+    private static class SyncTarget extends SurfaceSyncGroup {
         void onBufferReady() {
             SurfaceControl.Transaction t = new StubTransaction();
-            mTransactionReadyCallback.onTransactionReady(t);
+            onTransactionReady(t);
         }
     }
 }