From 8c1629929b617e8d03696dabfb9c5688d8d0e2ee Mon Sep 17 00:00:00 2001 From: Chavi Weingarten Date: Wed, 21 Jun 2023 17:30:34 +0000 Subject: [PATCH] Register frame callbacks for sync right before draw The drawListener is invoked in VRI right before calling into native draw, but after the frame callback is registered to handle sync. This can cause issues if something is trying to ensure a sync via the drawListener callback. Instead, set up the frame callback and sync right before calling draw so all sync requests are picked up in that frame Test: SurfaceSyncGroupContinuousTest Test: SurfaceSyncGroupTests Test: AttachedSurfaceControlSyncTest Bug: 287729934 Change-Id: I6cd583d66457729ecc0b229a1e486db5dca69b3f --- core/java/android/view/ViewRootImpl.java | 58 ++++++++++++++---------- 1 file changed, 33 insertions(+), 25 deletions(-) diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java index d2c6a765d9be8..9316dbf8a4582 100644 --- a/core/java/android/view/ViewRootImpl.java +++ b/core/java/android/view/ViewRootImpl.java @@ -3820,7 +3820,7 @@ public final class ViewRootImpl implements ViewParent, } mPendingTransitions.clear(); } - if (!performDraw() && mActiveSurfaceSyncGroup != null) { + if (!performDraw(mActiveSurfaceSyncGroup) && mActiveSurfaceSyncGroup != null) { mActiveSurfaceSyncGroup.markSyncReady(); } } @@ -4583,6 +4583,10 @@ public final class ViewRootImpl implements ViewParent, }); } + /** + * These callbacks check if the draw failed for any reason and apply + * those transactions directly so they don't get stuck forever. + */ private void registerCallbackForPendingTransactions() { Transaction t = new Transaction(); t.merge(mPendingTransaction); @@ -4611,7 +4615,7 @@ public final class ViewRootImpl implements ViewParent, }); } - private boolean performDraw() { + private boolean performDraw(@Nullable SurfaceSyncGroup surfaceSyncGroup) { mLastPerformDrawSkippedReason = null; if (mAttachInfo.mDisplayState == Display.STATE_OFF && !mReportNextDraw) { mLastPerformDrawSkippedReason = "screen_off"; @@ -4621,7 +4625,7 @@ public final class ViewRootImpl implements ViewParent, return false; } - final boolean fullRedrawNeeded = mFullRedrawNeeded || mActiveSurfaceSyncGroup != null; + final boolean fullRedrawNeeded = mFullRedrawNeeded || surfaceSyncGroup != null; mFullRedrawNeeded = false; mIsDrawing = true; @@ -4629,22 +4633,12 @@ public final class ViewRootImpl implements ViewParent, addFrameCommitCallbackIfNeeded(); - boolean usingAsyncReport = isHardwareEnabled() && mActiveSurfaceSyncGroup != null; - if (usingAsyncReport) { - 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 - // apply those transactions directly so they don't get stuck forever. - registerCallbackForPendingTransactions(); - } - mHasPendingTransactions = false; + boolean usingAsyncReport; try { - boolean canUseAsync = draw(fullRedrawNeeded, usingAsyncReport && mSyncBuffer); - if (usingAsyncReport && !canUseAsync) { + usingAsyncReport = draw(fullRedrawNeeded, surfaceSyncGroup, mSyncBuffer); + if (mAttachInfo.mThreadedRenderer != null && !usingAsyncReport) { mAttachInfo.mThreadedRenderer.setFrameCallback(null); - usingAsyncReport = false; } } finally { mIsDrawing = false; @@ -4682,10 +4676,12 @@ public final class ViewRootImpl implements ViewParent, } if (mSurfaceHolder != null && mSurface.isValid()) { - final SurfaceSyncGroup surfaceSyncGroup = mActiveSurfaceSyncGroup; - SurfaceCallbackHelper sch = new SurfaceCallbackHelper(() -> - mHandler.post(() -> surfaceSyncGroup.markSyncReady())); - mActiveSurfaceSyncGroup = null; + usingAsyncReport = true; + SurfaceCallbackHelper sch = new SurfaceCallbackHelper(() -> { + if (surfaceSyncGroup != null) { + surfaceSyncGroup.markSyncReady(); + } + }); SurfaceHolder.Callback callbacks[] = mSurfaceHolder.getCallbacks(); @@ -4696,8 +4692,9 @@ public final class ViewRootImpl implements ViewParent, } } } - if (mActiveSurfaceSyncGroup != null && !usingAsyncReport) { - mActiveSurfaceSyncGroup.markSyncReady(); + + if (surfaceSyncGroup != null && !usingAsyncReport) { + surfaceSyncGroup.markSyncReady(); } if (mPerformContentCapture) { performContentCaptureInitialReport(); @@ -4790,7 +4787,8 @@ public final class ViewRootImpl implements ViewParent, } } - private boolean draw(boolean fullRedrawNeeded, boolean forceDraw) { + private boolean draw(boolean fullRedrawNeeded, + @Nullable SurfaceSyncGroup activeSyncGroup, boolean syncBuffer) { Surface surface = mSurface; if (!surface.isValid()) { return false; @@ -4934,9 +4932,19 @@ public final class ViewRootImpl implements ViewParent, mAttachInfo.mThreadedRenderer.setTargetHdrSdrRatio(mRenderHdrSdrRatio); } - if (forceDraw) { - mAttachInfo.mThreadedRenderer.forceDrawNextFrame(); + if (activeSyncGroup != null) { + registerCallbacksForSync(syncBuffer, activeSyncGroup); + if (syncBuffer) { + mAttachInfo.mThreadedRenderer.forceDrawNextFrame(); + } + } else if (mHasPendingTransactions) { + // Register a calback if there's no sync involved but there were calls to + // applyTransactionOnDraw. If there is a sync involved, the sync callback will + // handle merging the pending transaction. + registerCallbackForPendingTransactions(); } + mHasPendingTransactions = false; + mAttachInfo.mThreadedRenderer.draw(mView, mAttachInfo, this); } else { // If we get here with a disabled & requested hardware renderer, something went