Merge "Make sure VRI waits for SV to draw before report" into tm-dev

core/java/android/view/SurfaceView.java

@@ -41,11 +41,13 @@ import android.os.Handler;
 import android.os.IBinder;
 import android.os.Looper;
 import android.os.SystemClock;
+import android.util.ArraySet;
 import android.util.AttributeSet;
 import android.util.Log;
 import android.view.SurfaceControl.Transaction;
 import android.view.accessibility.AccessibilityNodeInfo;
 import android.view.accessibility.IAccessibilityEmbeddedConnection;
+import android.window.SurfaceSyncer;
 
 import com.android.internal.view.SurfaceCallbackHelper;
 
@@ -204,6 +206,9 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
 
     private int mSurfaceFlags = SurfaceControl.HIDDEN;
 
+    private final SurfaceSyncer mSurfaceSyncer = new SurfaceSyncer();
+    private final ArraySet<Integer> mSyncIds = new ArraySet<>();
+
     /**
      * Transaction that should be used from the render thread. This transaction is only thread safe
      * with other calls directly from the render thread.
@@ -1018,7 +1023,7 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
                             if (shouldSyncBuffer) {
                                 handleSyncBufferCallback(callbacks, syncBufferTransactionCallback);
                             } else {
-                                redrawNeededAsync(callbacks, this::onDrawFinished);
+                                handleSyncNoBuffer(callbacks);
                             }
                         }
                     }
@@ -1061,7 +1066,23 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
                     syncBufferCallback.onBufferReady(t);
                     onDrawFinished();
                 }));
+    }
 
+    private void handleSyncNoBuffer(SurfaceHolder.Callback[] callbacks) {
+        final int syncId = mSurfaceSyncer.setupSync(this::onDrawFinished);
+
+        mSurfaceSyncer.addToSync(syncId, syncBufferCallback -> redrawNeededAsync(callbacks,
+                () -> {
+                    syncBufferCallback.onBufferReady(null);
+                    synchronized (mSyncIds) {
+                        mSyncIds.remove(syncId);
+                    }
+                }));
+
+        mSurfaceSyncer.markSyncReady(syncId);
+        synchronized (mSyncIds) {
+            mSyncIds.add(syncId);
+        }
     }
 
     private void redrawNeededAsync(SurfaceHolder.Callback[] callbacks,
@@ -1070,6 +1091,21 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
         sch.dispatchSurfaceRedrawNeededAsync(mSurfaceHolder, callbacks);
     }
 
+    /**
+     * @hide
+     */
+    @Override
+    public void surfaceSyncStarted() {
+        ViewRootImpl viewRoot = getViewRootImpl();
+        if (viewRoot != null) {
+            synchronized (mSyncIds) {
+                for (int syncId : mSyncIds) {
+                    viewRoot.mergeSync(syncId, mSurfaceSyncer);
+                }
+            }
+        }
+    }
+
     private static class SyncBufferTransactionCallback {
         private final CountDownLatch mCountDownLatch = new CountDownLatch(1);
         private Transaction mTransaction;

core/java/android/view/ViewRootImpl.java

@@ -2048,6 +2048,7 @@ public final class ViewRootImpl implements ViewParent,
         void surfaceCreated(Transaction t);
         void surfaceReplaced(Transaction t);
         void surfaceDestroyed();
+        default void surfaceSyncStarted() {};
     }
 
     private final ArrayList<SurfaceChangedCallback> mSurfaceChangedCallbacks = new ArrayList<>();
@@ -2082,6 +2083,12 @@ public final class ViewRootImpl implements ViewParent,
         }
     }
 
+    private void notifySurfaceSyncStarted() {
+        for (int i = 0; i < mSurfaceChangedCallbacks.size(); i++) {
+            mSurfaceChangedCallbacks.get(i).surfaceSyncStarted();
+        }
+    }
+
     /**
      * @return child layer with the same bounds as its parent {@code mSurface} and cropped to the
      * surface insets. If the layer does not exist, it is created.
@@ -3541,6 +3548,7 @@ public final class ViewRootImpl implements ViewParent,
             Log.d(mTag, "Setup new sync id=" + mSyncId);
         }
         mSurfaceSyncer.addToSync(mSyncId, mSyncTarget);
+        notifySurfaceSyncStarted();
     }
 
     private void notifyContentCatpureEvents() {
@@ -10965,4 +10973,11 @@ public final class ViewRootImpl implements ViewParent,
             scheduleTraversals();
         }
     }
+
+    void mergeSync(int syncId, SurfaceSyncer otherSyncer) {
+        if (!isInLocalSync()) {
+            return;
+        }
+        mSurfaceSyncer.merge(mSyncId, syncId, otherSyncer);
+    }
 }

core/java/android/window/SurfaceSyncer.java

@@ -117,13 +117,18 @@ public class SurfaceSyncer {
      */
     public int setupSync(@NonNull Consumer<Transaction> syncRequestComplete) {
         synchronized (mSyncSetLock) {
-            mIdCounter++;
+            final int syncId = mIdCounter++;
             if (DEBUG) {
-                Log.d(TAG, "setupSync " + mIdCounter);
+                Log.d(TAG, "setupSync " + syncId);
             }
-            SyncSet syncSet = new SyncSet(mIdCounter, syncRequestComplete);
+            SyncSet syncSet = new SyncSet(syncId, transaction -> {
-            mSyncSets.put(mIdCounter, syncSet);
+                synchronized (mSyncSetLock) {
-            return mIdCounter;
+                    mSyncSets.remove(syncId);
+                }
+                syncRequestComplete.accept(transaction);
+            });
+            mSyncSets.put(syncId, syncSet);
+            return syncId;
         }
     }
 
@@ -138,7 +143,6 @@ public class SurfaceSyncer {
         SyncSet syncSet;
         synchronized (mSyncSetLock) {
             syncSet = mSyncSets.get(syncId);
-            mSyncSets.remove(syncId);
         }
         if (syncSet == null) {
             Log.e(TAG, "Failed to find syncSet for syncId=" + syncId);
@@ -147,6 +151,31 @@ public class SurfaceSyncer {
         syncSet.markSyncReady();
     }
 
+    /**
+     * Merge another SyncSet into the specified syncId.
+     * @param syncId The current syncId to merge into
+     * @param otherSyncId The other syncId to be merged
+     * @param otherSurfaceSyncer The other SurfaceSyncer where the otherSyncId is from
+     */
+    public void merge(int syncId, int otherSyncId, SurfaceSyncer otherSurfaceSyncer) {
+        SyncSet syncSet;
+        synchronized (mSyncSetLock) {
+            syncSet = mSyncSets.get(syncId);
+        }
+
+        SyncSet otherSyncSet = otherSurfaceSyncer.getAndValidateSyncSet(otherSyncId);
+        if (otherSyncSet == null) {
+            return;
+        }
+
+        if (DEBUG) {
+            Log.d(TAG,
+                    "merge id=" + otherSyncId + " from=" + otherSurfaceSyncer + " into id=" + syncId
+                            + " from" + this);
+        }
+        syncSet.merge(otherSyncSet);
+    }
+
     /**
      * Add a SurfaceView to a sync set. This is different than {@link #addToSync(int, View)} because
      * it requires the caller to notify the start and finish drawing in order to sync.
@@ -199,8 +228,7 @@ public class SurfaceSyncer {
         if (DEBUG) {
             Log.d(TAG, "addToSync id=" + syncId);
         }
-        syncSet.addSyncableSurface(syncTarget);
+        return syncSet.addSyncableSurface(syncTarget);
-        return true;
     }
 
     /**
@@ -284,14 +312,21 @@ public class SurfaceSyncer {
         private final Set<SyncTarget> mSyncTargets = new ArraySet<>();
 
         private final int mSyncId;
-        private final Consumer<Transaction> mSyncRequestCompleteCallback;
+        @GuardedBy("mLock")
+        private Consumer<Transaction> mSyncRequestCompleteCallback;
+
+        @GuardedBy("mLock")
+        private final Set<SyncSet> mMergedSyncSets = new ArraySet<>();
+
+        @GuardedBy("mLock")
+        private boolean mFinished;
 
         private SyncSet(int syncId, Consumer<Transaction> syncRequestComplete) {
             mSyncId = syncId;
             mSyncRequestCompleteCallback = syncRequestComplete;
         }
 
-        void addSyncableSurface(SyncTarget syncTarget) {
+        boolean addSyncableSurface(SyncTarget syncTarget) {
             SyncBufferCallback syncBufferCallback = new SyncBufferCallback() {
                 @Override
                 public void onBufferReady(Transaction t) {
@@ -306,10 +341,16 @@ public class SurfaceSyncer {
             };
 
             synchronized (mLock) {
+                if (mSyncReady) {
+                    Log.e(TAG, "Sync " + mSyncId + " was already marked as ready. No more "
+                            + "SyncTargets can be added.");
+                    return false;
+                }
                 mPendingSyncs.add(syncBufferCallback.hashCode());
                 mSyncTargets.add(syncTarget);
             }
             syncTarget.onReadyToSync(syncBufferCallback);
+            return true;
         }
 
         void markSyncReady() {
@@ -321,10 +362,11 @@ public class SurfaceSyncer {
 
         @GuardedBy("mLock")
         private void checkIfSyncIsComplete() {
-            if (!mSyncReady || !mPendingSyncs.isEmpty()) {
+            if (!mSyncReady || !mPendingSyncs.isEmpty() || !mMergedSyncSets.isEmpty()) {
                 if (DEBUG) {
                     Log.d(TAG, "Syncable is not complete. mSyncReady=" + mSyncReady
-                            + " mPendingSyncs=" + mPendingSyncs.size());
+                            + " mPendingSyncs=" + mPendingSyncs.size() + " mergedSyncs="
+                            + mMergedSyncSets.size());
                 }
                 return;
             }
@@ -338,6 +380,7 @@ public class SurfaceSyncer {
             }
             mSyncTargets.clear();
             mSyncRequestCompleteCallback.accept(mTransaction);
+            mFinished = true;
         }
 
         /**
@@ -349,6 +392,50 @@ public class SurfaceSyncer {
                 mTransaction.merge(t);
             }
         }
+
+        public void updateCallback(Consumer<Transaction> transactionConsumer) {
+            synchronized (mLock) {
+                if (mFinished) {
+                    Log.e(TAG, "Attempting to merge SyncSet " + mSyncId + " when sync is"
+                            + " already complete");
+                    transactionConsumer.accept(new Transaction());
+                }
+
+                final Consumer<Transaction> oldCallback = mSyncRequestCompleteCallback;
+                mSyncRequestCompleteCallback = transaction -> {
+                    oldCallback.accept(new Transaction());
+                    transactionConsumer.accept(transaction);
+                };
+            }
+        }
+
+        /**
+         * Merge a SyncSet into this SyncSet. Since SyncSets could still have pending SyncTargets,
+         * we need to make sure those can still complete before the mergeTo syncSet is considered
+         * complete.
+         *
+         * We keep track of all the merged SyncSets until they are marked as done, and then they
+         * are removed from the set. This SyncSet is not considered done until all the merged
+         * SyncSets are done.
+         *
+         * When the merged SyncSet 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 otherSyncSet The other SyncSet to merge into this one.
+         */
+        public void merge(SyncSet otherSyncSet) {
+            synchronized (mLock) {
+                mMergedSyncSets.add(otherSyncSet);
+            }
+            otherSyncSet.updateCallback(transaction -> {
+                synchronized (mLock) {
+                    mMergedSyncSets.remove(otherSyncSet);
+                    mTransaction.merge(transaction);
+                    checkIfSyncIsComplete();
+                }
+            });
+        }
     }
 
     /**

services/tests/wmtests/src/com/android/server/wm/SurfaceSyncerTest.java

@@ -48,11 +48,11 @@ public class SurfaceSyncerTest {
     public void testSyncOne() throws InterruptedException {
         final CountDownLatch finishedLatch = new CountDownLatch(1);
         int startSyncId = mSurfaceSyncer.setupSync(transaction -> finishedLatch.countDown());
-        Syncable syncable = new Syncable();
+        SyncTarget syncTarget = new SyncTarget();
-        mSurfaceSyncer.addToSync(startSyncId, syncable);
+        mSurfaceSyncer.addToSync(startSyncId, syncTarget);
         mSurfaceSyncer.markSyncReady(startSyncId);
 
-        syncable.onBufferReady();
+        syncTarget.onBufferReady();
 
         finishedLatch.await(5, TimeUnit.SECONDS);
         assertEquals(0, finishedLatch.getCount());
@@ -62,22 +62,22 @@ public class SurfaceSyncerTest {
     public void testSyncMultiple() throws InterruptedException {
         final CountDownLatch finishedLatch = new CountDownLatch(1);
         int startSyncId = mSurfaceSyncer.setupSync(transaction -> finishedLatch.countDown());
-        Syncable syncable1 = new Syncable();
+        SyncTarget syncTarget1 = new SyncTarget();
-        Syncable syncable2 = new Syncable();
+        SyncTarget syncTarget2 = new SyncTarget();
-        Syncable syncable3 = new Syncable();
+        SyncTarget syncTarget3 = new SyncTarget();
 
-        mSurfaceSyncer.addToSync(startSyncId, syncable1);
+        mSurfaceSyncer.addToSync(startSyncId, syncTarget1);
-        mSurfaceSyncer.addToSync(startSyncId, syncable2);
+        mSurfaceSyncer.addToSync(startSyncId, syncTarget2);
-        mSurfaceSyncer.addToSync(startSyncId, syncable3);
+        mSurfaceSyncer.addToSync(startSyncId, syncTarget3);
         mSurfaceSyncer.markSyncReady(startSyncId);
 
-        syncable1.onBufferReady();
+        syncTarget1.onBufferReady();
         assertNotEquals(0, finishedLatch.getCount());
 
-        syncable3.onBufferReady();
+        syncTarget3.onBufferReady();
         assertNotEquals(0, finishedLatch.getCount());
 
-        syncable2.onBufferReady();
+        syncTarget2.onBufferReady();
 
         finishedLatch.await(5, TimeUnit.SECONDS);
         assertEquals(0, finishedLatch.getCount());
@@ -85,7 +85,7 @@ public class SurfaceSyncerTest {
 
     @Test
     public void testInvalidSyncId() {
-        assertFalse(mSurfaceSyncer.addToSync(0, new Syncable()));
+        assertFalse(mSurfaceSyncer.addToSync(0, new SyncTarget()));
     }
 
     @Test
@@ -93,14 +93,14 @@ public class SurfaceSyncerTest {
         final CountDownLatch finishedLatch = new CountDownLatch(1);
         int startSyncId = mSurfaceSyncer.setupSync(transaction -> finishedLatch.countDown());
 
-        Syncable syncable1 = new Syncable();
+        SyncTarget syncTarget1 = new SyncTarget();
-        Syncable syncable2 = new Syncable();
+        SyncTarget syncTarget2 = new SyncTarget();
 
-        assertTrue(mSurfaceSyncer.addToSync(startSyncId, syncable1));
+        assertTrue(mSurfaceSyncer.addToSync(startSyncId, syncTarget1));
         mSurfaceSyncer.markSyncReady(startSyncId);
         // Adding to a sync that has been completed is also invalid since the sync id has been
         // cleared.
-        assertFalse(mSurfaceSyncer.addToSync(startSyncId, syncable2));
+        assertFalse(mSurfaceSyncer.addToSync(startSyncId, syncTarget2));
     }
 
     @Test
@@ -110,27 +110,89 @@ public class SurfaceSyncerTest {
         int startSyncId1 = mSurfaceSyncer.setupSync(transaction -> finishedLatch1.countDown());
         int startSyncId2 = mSurfaceSyncer.setupSync(transaction -> finishedLatch2.countDown());
 
-        Syncable syncable1 = new Syncable();
+        SyncTarget syncTarget1 = new SyncTarget();
-        Syncable syncable2 = new Syncable();
+        SyncTarget syncTarget2 = new SyncTarget();
 
-        assertTrue(mSurfaceSyncer.addToSync(startSyncId1, syncable1));
+        assertTrue(mSurfaceSyncer.addToSync(startSyncId1, syncTarget1));
-        assertTrue(mSurfaceSyncer.addToSync(startSyncId2, syncable2));
+        assertTrue(mSurfaceSyncer.addToSync(startSyncId2, syncTarget2));
         mSurfaceSyncer.markSyncReady(startSyncId1);
         mSurfaceSyncer.markSyncReady(startSyncId2);
 
-        syncable1.onBufferReady();
+        syncTarget1.onBufferReady();
 
         finishedLatch1.await(5, TimeUnit.SECONDS);
         assertEquals(0, finishedLatch1.getCount());
         assertNotEquals(0, finishedLatch2.getCount());
 
-        syncable2.onBufferReady();
+        syncTarget2.onBufferReady();
 
         finishedLatch2.await(5, TimeUnit.SECONDS);
         assertEquals(0, finishedLatch2.getCount());
     }
 
-    private static class Syncable implements SurfaceSyncer.SyncTarget {
+    @Test
+    public void testMergeSync() throws InterruptedException {
+        final CountDownLatch finishedLatch1 = new CountDownLatch(1);
+        final CountDownLatch finishedLatch2 = new CountDownLatch(1);
+        int startSyncId1 = mSurfaceSyncer.setupSync(transaction -> finishedLatch1.countDown());
+        int startSyncId2 = mSurfaceSyncer.setupSync(transaction -> finishedLatch2.countDown());
+
+        SyncTarget syncTarget1 = new SyncTarget();
+        SyncTarget syncTarget2 = new SyncTarget();
+
+        assertTrue(mSurfaceSyncer.addToSync(startSyncId1, syncTarget1));
+        assertTrue(mSurfaceSyncer.addToSync(startSyncId2, syncTarget2));
+        mSurfaceSyncer.markSyncReady(startSyncId1);
+        mSurfaceSyncer.merge(startSyncId2, startSyncId1, mSurfaceSyncer);
+        mSurfaceSyncer.markSyncReady(startSyncId2);
+
+        // Finish syncTarget2 first to test that the syncSet is not complete until the merged sync
+        // is also done.
+        syncTarget2.onBufferReady();
+        finishedLatch2.await(1, TimeUnit.SECONDS);
+        // Sync did not complete yet
+        assertNotEquals(0, finishedLatch2.getCount());
+
+        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());
+
+        finishedLatch2.await(5, TimeUnit.SECONDS);
+        assertEquals(0, finishedLatch2.getCount());
+    }
+
+    @Test
+    public void testMergeSyncAlreadyComplete() throws InterruptedException {
+        final CountDownLatch finishedLatch1 = new CountDownLatch(1);
+        final CountDownLatch finishedLatch2 = new CountDownLatch(1);
+        int startSyncId1 = mSurfaceSyncer.setupSync(transaction -> finishedLatch1.countDown());
+        int startSyncId2 = mSurfaceSyncer.setupSync(transaction -> finishedLatch2.countDown());
+
+        SyncTarget syncTarget1 = new SyncTarget();
+        SyncTarget syncTarget2 = new SyncTarget();
+
+        assertTrue(mSurfaceSyncer.addToSync(startSyncId1, syncTarget1));
+        assertTrue(mSurfaceSyncer.addToSync(startSyncId2, syncTarget2));
+        mSurfaceSyncer.markSyncReady(startSyncId1);
+        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());
+
+        mSurfaceSyncer.merge(startSyncId2, startSyncId1, mSurfaceSyncer);
+        mSurfaceSyncer.markSyncReady(startSyncId2);
+        syncTarget2.onBufferReady();
+
+        // Verify that the second sync will receive complete since the merged sync was already
+        // completed before the merge.
+        finishedLatch2.await(5, TimeUnit.SECONDS);
+        assertEquals(0, finishedLatch2.getCount());
+    }
+
+    private static class SyncTarget implements SurfaceSyncer.SyncTarget {
         private SurfaceSyncer.SyncBufferCallback mSyncBufferCallback;
 
         @Override