Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 783ae3c7 authored by chaviw's avatar chaviw
Browse files

Make sure VRI waits for SV to draw before report 

VRI is not currently waiting for SV to draw since SV may start its draw
before VRI opened the SyncSet. If that happens, VRI will not include SV
in its SyncSet and report draw finished even if SV is not complete.

This fix has SV create its own SyncSet when redrawNeeded. Then if VRI
creates a sync, SV will merge its own SyncSet into that one. If SV
already finished drawing, then nothing will get merged, but it means SV
is ready already. If SV is not finished, VRI will now also wait for SV's
draw to finish before calling finishDraw.

Test: Long delay in surfaceRedrawNeededAsync
Test: SurfaceSyncerTest
Bug: 230998394

Change-Id: I44331b8f54951e6dc633a4845bbf690abde0f95e
parent b2c0a717

core/java/android/view/SurfaceView.java

+37 −1
Original line number Diff line number Diff line
@@ -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

+15 −0
Original line number Diff line number Diff line
@@ -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

+99 −12
Original line number Diff line number Diff line
@@ -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);

            mSyncSets.put(mIdCounter, syncSet);

            return mIdCounter;

            SyncSet syncSet = new SyncSet(syncId, transaction -> {

                synchronized (mSyncSetLock) {

                    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 true;

        return syncSet.addSyncableSurface(syncTarget);

    }



    /**
@@ -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

+86 −24
Original line number Diff line number Diff line
@@ -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();

        mSurfaceSyncer.addToSync(startSyncId, syncable);

        SyncTarget syncTarget = new SyncTarget();

        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();

        Syncable syncable2 = new Syncable();

        Syncable syncable3 = new Syncable();

        SyncTarget syncTarget1 = new SyncTarget();

        SyncTarget syncTarget2 = new SyncTarget();

        SyncTarget syncTarget3 = new SyncTarget();



        mSurfaceSyncer.addToSync(startSyncId, syncable1);

        mSurfaceSyncer.addToSync(startSyncId, syncable2);

        mSurfaceSyncer.addToSync(startSyncId, syncable3);

        mSurfaceSyncer.addToSync(startSyncId, syncTarget1);

        mSurfaceSyncer.addToSync(startSyncId, syncTarget2);

        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();

        Syncable syncable2 = new Syncable();

        SyncTarget syncTarget1 = new SyncTarget();

        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();

        Syncable syncable2 = new Syncable();

        SyncTarget syncTarget1 = new SyncTarget();

        SyncTarget syncTarget2 = new SyncTarget();



        assertTrue(mSurfaceSyncer.addToSync(startSyncId1, syncable1));

        assertTrue(mSurfaceSyncer.addToSync(startSyncId2, syncable2));

        assertTrue(mSurfaceSyncer.addToSync(startSyncId1, syncTarget1));

        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