From e1e26f5c4020b4e2a561de0dafb0d35d155191cb Mon Sep 17 00:00:00 2001 From: Chavi Weingarten Date: Wed, 26 Oct 2022 16:26:49 +0000 Subject: [PATCH] Remove onSyncComplete from SurfaceSyncGroup There was already a way to register for an entire SurfaceSyncGroup completing. There just wasn't a way to tell what SurfaceSyncGroup something was added to in order to register in VRI. Therefore, add the addedToSync argument in the readyToSync callback, allowing VRI to register a complete callback instead of exposing a new API. Also renamed onReadyToSync to onAddedToSyncGroup to reflect what is happening. Renamed SyncBufferCallback to TransactionReadyCallback since there isn't a guarantee the transaction contains a buffer. Test: SurfaceSyncGroupTest Test: NotificationShade Sync Bug: 237804605 Change-Id: Ib58735cd86593a976e3f4559997316275bd82cd9 --- core/java/android/view/SurfaceView.java | 10 +- core/java/android/view/ViewRootImpl.java | 101 ++++++++++-------- .../java/android/window/SurfaceSyncGroup.java | 69 ++++++------ .../server/wm/SurfaceSyncGroupTest.java | 9 +- 4 files changed, 94 insertions(+), 95 deletions(-) diff --git a/core/java/android/view/SurfaceView.java b/core/java/android/view/SurfaceView.java index b24303b41abd3..720813ad81ef7 100644 --- a/core/java/android/view/SurfaceView.java +++ b/core/java/android/view/SurfaceView.java @@ -1073,7 +1073,7 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall private void handleSyncBufferCallback(SurfaceHolder.Callback[] callbacks, SyncBufferTransactionCallback syncBufferTransactionCallback) { - getViewRootImpl().addToSync(syncBufferCallback -> + getViewRootImpl().addToSync((parentSyncGroup, syncBufferCallback) -> redrawNeededAsync(callbacks, () -> { Transaction t = null; if (mBlastBufferQueue != null) { @@ -1081,7 +1081,7 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall t = syncBufferTransactionCallback.waitForTransaction(); } - syncBufferCallback.onBufferReady(t); + syncBufferCallback.onTransactionReady(t); onDrawFinished(); })); } @@ -1092,9 +1092,9 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall mSyncGroups.add(syncGroup); } - syncGroup.addToSync(syncBufferCallback -> redrawNeededAsync(callbacks, - () -> { - syncBufferCallback.onBufferReady(null); + syncGroup.addToSync((parentSyncGroup, syncBufferCallback) -> + redrawNeededAsync(callbacks, () -> { + syncBufferCallback.onTransactionReady(null); onDrawFinished(); synchronized (mSyncGroups) { mSyncGroups.remove(syncGroup); diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java index e664ebf4c4849..093ba8972378c 100644 --- a/core/java/android/view/ViewRootImpl.java +++ b/core/java/android/view/ViewRootImpl.java @@ -228,6 +228,7 @@ import java.util.List; import java.util.Objects; import java.util.Queue; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.Executor; /** * The top of a view hierarchy, implementing the needed protocol between View @@ -848,7 +849,7 @@ public final class ViewRootImpl implements ViewParent, } private SurfaceSyncGroup mSyncGroup; - private SurfaceSyncGroup.SyncBufferCallback mSyncBufferCallback; + private SurfaceSyncGroup.TransactionReadyCallback mTransactionReadyCallback; private int mNumSyncsInProgress = 0; private HashSet mRootScrollCaptureCallbacks; @@ -3607,8 +3608,8 @@ public final class ViewRootImpl implements ViewParent, mPendingTransitions.clear(); } - if (mSyncBufferCallback != null) { - mSyncBufferCallback.onBufferReady(null); + if (mTransactionReadyCallback != null) { + mTransactionReadyCallback.onTransactionReady(null); } } else if (cancelAndRedraw) { mLastPerformTraversalsSkipDrawReason = cancelDueToPreDrawListener @@ -3623,8 +3624,8 @@ public final class ViewRootImpl implements ViewParent, } mPendingTransitions.clear(); } - if (!performDraw() && mSyncBufferCallback != null) { - mSyncBufferCallback.onBufferReady(null); + if (!performDraw() && mTransactionReadyCallback != null) { + mTransactionReadyCallback.onTransactionReady(null); } } @@ -3638,7 +3639,7 @@ public final class ViewRootImpl implements ViewParent, if (!cancelAndRedraw) { mReportNextDraw = false; mLastReportNextDrawReason = null; - mSyncBufferCallback = null; + mTransactionReadyCallback = null; mSyncBuffer = false; if (isInLocalSync()) { mSyncGroup.markSyncReady(); @@ -4385,7 +4386,7 @@ public final class ViewRootImpl implements ViewParent, return false; } - final boolean fullRedrawNeeded = mFullRedrawNeeded || mSyncBufferCallback != null; + final boolean fullRedrawNeeded = mFullRedrawNeeded || mTransactionReadyCallback != null; mFullRedrawNeeded = false; mIsDrawing = true; @@ -4393,9 +4394,9 @@ public final class ViewRootImpl implements ViewParent, addFrameCommitCallbackIfNeeded(); - boolean usingAsyncReport = isHardwareEnabled() && mSyncBufferCallback != null; + boolean usingAsyncReport = isHardwareEnabled() && mTransactionReadyCallback != null; if (usingAsyncReport) { - registerCallbacksForSync(mSyncBuffer, mSyncBufferCallback); + registerCallbacksForSync(mSyncBuffer, mTransactionReadyCallback); } 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 @@ -4446,10 +4447,11 @@ public final class ViewRootImpl implements ViewParent, } if (mSurfaceHolder != null && mSurface.isValid()) { - final SurfaceSyncGroup.SyncBufferCallback syncBufferCallback = mSyncBufferCallback; + final SurfaceSyncGroup.TransactionReadyCallback transactionReadyCallback = + mTransactionReadyCallback; SurfaceCallbackHelper sch = new SurfaceCallbackHelper(() -> - mHandler.post(() -> syncBufferCallback.onBufferReady(null))); - mSyncBufferCallback = null; + mHandler.post(() -> transactionReadyCallback.onTransactionReady(null))); + mTransactionReadyCallback = null; SurfaceHolder.Callback callbacks[] = mSurfaceHolder.getCallbacks(); @@ -4460,8 +4462,8 @@ public final class ViewRootImpl implements ViewParent, } } } - if (mSyncBufferCallback != null && !usingAsyncReport) { - mSyncBufferCallback.onBufferReady(null); + if (mTransactionReadyCallback != null && !usingAsyncReport) { + mTransactionReadyCallback.onTransactionReady(null); } if (mPerformContentCapture) { performContentCaptureInitialReport(); @@ -11097,7 +11099,7 @@ public final class ViewRootImpl implements ViewParent, } private void registerCallbacksForSync(boolean syncBuffer, - final SurfaceSyncGroup.SyncBufferCallback syncBufferCallback) { + final SurfaceSyncGroup.TransactionReadyCallback transactionReadyCallback) { if (!isHardwareEnabled()) { return; } @@ -11124,7 +11126,7 @@ public final class ViewRootImpl implements ViewParent, // pendingDrawFinished. if ((syncResult & (SYNC_LOST_SURFACE_REWARD_IF_FOUND | SYNC_CONTEXT_IS_STOPPED)) != 0) { - syncBufferCallback.onBufferReady( + transactionReadyCallback.onTransactionReady( mBlastBufferQueue.gatherPendingTransactions(frame)); return null; } @@ -11134,7 +11136,8 @@ public final class ViewRootImpl implements ViewParent, } if (syncBuffer) { - mBlastBufferQueue.syncNextTransaction(syncBufferCallback::onBufferReady); + mBlastBufferQueue.syncNextTransaction( + transactionReadyCallback::onTransactionReady); } return didProduceBuffer -> { @@ -11154,7 +11157,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. - syncBufferCallback.onBufferReady( + transactionReadyCallback.onTransactionReady( mBlastBufferQueue.gatherPendingTransactions(frame)); return; } @@ -11163,22 +11166,49 @@ 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) { - syncBufferCallback.onBufferReady(null); + transactionReadyCallback.onTransactionReady(null); } }; } }); } + private final Executor mPostAtFrontExecutor = new Executor() { + @Override + public void execute(Runnable command) { + mHandler.postAtFrontOfQueue(command); + } + }; + public final SurfaceSyncGroup.SyncTarget mSyncTarget = new SurfaceSyncGroup.SyncTarget() { @Override - public void onReadyToSync(SurfaceSyncGroup.SyncBufferCallback syncBufferCallback) { - readyToSync(syncBufferCallback); + 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 (mAttachInfo.mThreadedRenderer != null) { + HardwareRenderer.setRtAnimationsEnabled(false); + } + + 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; + if (!mIsInTraversal && !mTraversalScheduled) { + scheduleTraversals(); + } } - @Override - public void onSyncComplete() { - mHandler.postAtFrontOfQueue(() -> { + private void updateSyncInProgressCount(SurfaceSyncGroup parentSyncGroup) { + mNumSyncsInProgress++; + parentSyncGroup.addSyncCompleteCallback(mPostAtFrontExecutor, () -> { if (--mNumSyncsInProgress == 0 && mAttachInfo.mThreadedRenderer != null) { HardwareRenderer.setRtAnimationsEnabled(true); } @@ -11191,29 +11221,6 @@ public final class ViewRootImpl implements ViewParent, return mSyncTarget; } - private void readyToSync(SurfaceSyncGroup.SyncBufferCallback syncBufferCallback) { - mNumSyncsInProgress++; - if (!isInLocalSync()) { - // Always sync the buffer if the sync request did not come from VRI. - mSyncBuffer = true; - } - if (mAttachInfo.mThreadedRenderer != null) { - HardwareRenderer.setRtAnimationsEnabled(false); - } - - if (mSyncBufferCallback != null) { - Log.d(mTag, "Already set sync for the next draw."); - mSyncBufferCallback.onBufferReady(null); - } - if (DEBUG_BLAST) { - Log.d(mTag, "Setting syncFrameCallback"); - } - mSyncBufferCallback = syncBufferCallback; - if (!mIsInTraversal && !mTraversalScheduled) { - scheduleTraversals(); - } - } - void mergeSync(SurfaceSyncGroup otherSyncGroup) { if (!isInLocalSync()) { return; diff --git a/core/java/android/window/SurfaceSyncGroup.java b/core/java/android/window/SurfaceSyncGroup.java index 4248096f307d0..3950739419307 100644 --- a/core/java/android/window/SurfaceSyncGroup.java +++ b/core/java/android/window/SurfaceSyncGroup.java @@ -57,12 +57,13 @@ import java.util.function.Supplier; * option is provided. * * The following is what happens within the {@link SurfaceSyncGroup} - * 1. Each SyncTarget will get a {@link SyncTarget#onReadyToSync} callback that contains a - * {@link SyncBufferCallback}. - * 2. Each {@link SyncTarget} needs to invoke {@link SyncBufferCallback#onBufferReady(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 SyncBufferCallback finishes for the SurfaceSyncGroup, in most cases the + * 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. * @@ -86,8 +87,6 @@ public final class SurfaceSyncGroup { private final Transaction mTransaction = sTransactionFactory.get(); @GuardedBy("mLock") private boolean mSyncReady; - @GuardedBy("mLock") - private final Set mSyncTargets = new ArraySet<>(); @GuardedBy("mLock") private Consumer mSyncRequestCompleteCallback; @@ -197,14 +196,13 @@ public final class SurfaceSyncGroup { * Add a {@link SyncTarget} to a sync set. The sync set will wait for all * SyncableSurfaces to complete before notifying. * - * @param syncTarget A SyncableSurface that implements how to handle syncing - * buffers. + * @param syncTarget A SyncTarget that implements how to handle syncing transactions. * @return true if the SyncTarget was successfully added to the SyncGroup, false otherwise. */ public boolean addToSync(SyncTarget syncTarget) { - SyncBufferCallback syncBufferCallback = new SyncBufferCallback() { + TransactionReadyCallback transactionReadyCallback = new TransactionReadyCallback() { @Override - public void onBufferReady(Transaction t) { + public void onTransactionReady(Transaction t) { synchronized (mLock) { if (t != null) { mTransaction.merge(t); @@ -221,10 +219,9 @@ public final class SurfaceSyncGroup { + "SyncTargets can be added."); return false; } - mPendingSyncs.add(syncBufferCallback.hashCode()); - mSyncTargets.add(syncTarget); + mPendingSyncs.add(transactionReadyCallback.hashCode()); } - syncTarget.onReadyToSync(syncBufferCallback); + syncTarget.onAddedToSyncGroup(this, transactionReadyCallback); return true; } @@ -256,17 +253,13 @@ public final class SurfaceSyncGroup { Log.d(TAG, "Successfully finished sync id=" + this); } - for (SyncTarget syncTarget : mSyncTargets) { - syncTarget.onSyncComplete(); - } - mSyncTargets.clear(); mSyncRequestCompleteCallback.accept(mTransaction); mFinished = true; } /** * Add a Transaction to this sync set. This allows the caller to provide other info that - * should be synced with the buffers. + * should be synced with the transactions. */ public void addTransactionToSync(Transaction t) { synchronized (mLock) { @@ -334,9 +327,10 @@ public final class SurfaceSyncGroup { } @Override - public void onReadyToSync(SyncBufferCallback syncBufferCallback) { + public void onAddedToSyncGroup(SurfaceSyncGroup parentSyncGroup, + TransactionReadyCallback transactionReadyCallback) { mFrameCallbackConsumer.accept( - () -> mSurfaceView.syncNextFrame(syncBufferCallback::onBufferReady)); + () -> mSurfaceView.syncNextFrame(transactionReadyCallback::onTransactionReady)); } } @@ -345,22 +339,19 @@ public final class SurfaceSyncGroup { */ public interface SyncTarget { /** - * Called when the Syncable is ready to begin handing a sync request. When invoked, the - * implementor is required to call {@link SyncBufferCallback#onBufferReady(Transaction)} - * and {@link SyncBufferCallback#onBufferReady(Transaction)} in order for this Syncable - * to be marked as complete. + * 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 syncBufferCallback A SyncBufferCallback that the caller must invoke onBufferReady + * @param parentSyncGroup The sync group this target has been added to. + * @param transactionReadyCallback A TransactionReadyCallback that the caller must invoke + * onTransactionReady */ - void onReadyToSync(SyncBufferCallback syncBufferCallback); - - /** - * There's no guarantee about the thread this callback is invoked on. - */ - default void onSyncComplete() { - } + void onAddedToSyncGroup(SurfaceSyncGroup parentSyncGroup, + TransactionReadyCallback transactionReadyCallback); } /** @@ -368,14 +359,14 @@ public final class SurfaceSyncGroup { * completed. The caller should invoke the calls when the rendering has started and finished a * frame. */ - public interface SyncBufferCallback { + public interface TransactionReadyCallback { /** - * Invoked when the transaction contains the buffer and is ready to sync. + * Invoked when the transaction is ready to sync. * - * @param t The transaction that contains the buffer to be synced. This can be null if - * there's nothing to sync + * @param t The transaction that contains the anything to be included in the synced. This + * can be null if there's nothing to sync */ - void onBufferReady(@Nullable Transaction t); + void onTransactionReady(@Nullable Transaction t); } /** 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 846a5066e036c..5e1fae095db87 100644 --- a/services/tests/wmtests/src/com/android/server/wm/SurfaceSyncGroupTest.java +++ b/services/tests/wmtests/src/com/android/server/wm/SurfaceSyncGroupTest.java @@ -192,16 +192,17 @@ public class SurfaceSyncGroupTest { } private static class SyncTarget implements SurfaceSyncGroup.SyncTarget { - private SurfaceSyncGroup.SyncBufferCallback mSyncBufferCallback; + private SurfaceSyncGroup.TransactionReadyCallback mTransactionReadyCallback; @Override - public void onReadyToSync(SurfaceSyncGroup.SyncBufferCallback syncBufferCallback) { - mSyncBufferCallback = syncBufferCallback; + public void onAddedToSyncGroup(SurfaceSyncGroup parentSyncGroup, + SurfaceSyncGroup.TransactionReadyCallback transactionReadyCallback) { + mTransactionReadyCallback = transactionReadyCallback; } void onBufferReady() { SurfaceControl.Transaction t = new StubTransaction(); - mSyncBufferCallback.onBufferReady(t); + mTransactionReadyCallback.onTransactionReady(t); } } }