From f541d037f59dc178db81babf6f7fa84c9c3072d5 Mon Sep 17 00:00:00 2001 From: chaviw Date: Wed, 7 Apr 2021 17:35:12 -0500 Subject: [PATCH] Use separate Transaction object in SV Currently, setAlpha was using mRtTransaction to send the SC transaction. However, that's also used by positionChanged. This could cause synchronization issues because both callbacks are invoked from a thread pool with no synchronization ensured. Instead, create a Transaction that's used just for FrameCallbacks. FrameCallbacks will all occur on the same thread so it's safe to use the same object. Additionally, created a separate transaction that should be used for positionChanged. This is because it's invoked from a thread pool can could interfere with the main render thread transactions. Finally, use the original mRtTransaction only for places that are called synchronously from renderThread. Test: Hard to repro original issue. Bubbles works Bug: 184559328 Change-Id: Ia5a36ad063c19e8a481a57170e47029f7c2c7ee7 --- core/java/android/view/SurfaceView.java | 67 +++++++++++++++++-------- 1 file changed, 46 insertions(+), 21 deletions(-) diff --git a/core/java/android/view/SurfaceView.java b/core/java/android/view/SurfaceView.java index 2b96a14b04d48..7bdf5cf879f32 100644 --- a/core/java/android/view/SurfaceView.java +++ b/core/java/android/view/SurfaceView.java @@ -30,6 +30,7 @@ import android.graphics.BLASTBufferQueue; import android.graphics.BlendMode; import android.graphics.Canvas; import android.graphics.Color; +import android.graphics.HardwareRenderer; import android.graphics.Matrix; import android.graphics.Paint; import android.graphics.PixelFormat; @@ -221,8 +222,35 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall private int mPendingReportDraws; - private SurfaceControl.Transaction mRtTransaction = new SurfaceControl.Transaction(); - private SurfaceControl.Transaction mTmpTransaction = new SurfaceControl.Transaction(); + /** + * Transaction that should be used from the render thread. This transaction is only thread safe + * with other calls directly from the render thread. + */ + private final SurfaceControl.Transaction mRtTransaction = new SurfaceControl.Transaction(); + + /** + * Transaction that should be used whe + * {@link HardwareRenderer.FrameDrawingCallback#onFrameDraw} is invoked. All + * frame callbacks can use the same transaction since they will be thread safe + */ + private final SurfaceControl.Transaction mFrameCallbackTransaction = + new SurfaceControl.Transaction(); + + /** + * Transaction that should be used for + * {@link RenderNode.PositionUpdateListener#positionChanged(long, int, int, int, int)} + * The callback is invoked from a thread pool so it's not thread safe with other render thread + * transactions. Keep the transactions for position changed callbacks on its own transaction. + */ + private final SurfaceControl.Transaction mPositionChangedTransaction = + new SurfaceControl.Transaction(); + + /** + * A temporary transaction holder that should only be used when applying right away. There + * should be no assumption about thread safety for this transaction. + */ + private final SurfaceControl.Transaction mTmpTransaction = new SurfaceControl.Transaction(); + private int mParentSurfaceSequenceId; private RemoteAccessibilityController mRemoteAccessibilityController = @@ -432,7 +460,6 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall * This gets called on a RenderThread worker thread, so members accessed here must * be protected by a lock. */ - final boolean useBLAST = useBLASTSync(viewRoot); viewRoot.registerRtFrameCallback(frame -> { try { synchronized (mSurfaceControlLock) { @@ -456,8 +483,9 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall Log.d(TAG, System.identityHashCode(this) + " updateSurfaceAlpha RT: set alpha=" + alpha); } - mRtTransaction.setAlpha(mSurfaceControl, alpha); - applyRtTransaction(frame); + + mFrameCallbackTransaction.setAlpha(mSurfaceControl, alpha); + applyOrMergeTransaction(mFrameCallbackTransaction, frame); } // It's possible that mSurfaceControl is released in the UI thread before // the transaction completes. If that happens, an exception is thrown, which @@ -806,7 +834,6 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall * This gets called on a RenderThread worker thread, so members accessed here must * be protected by a lock. */ - final boolean useBLAST = useBLASTSync(viewRoot); viewRoot.registerRtFrameCallback(frame -> { try { synchronized (mSurfaceControlLock) { @@ -814,8 +841,8 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall return; } - updateRelativeZ(mRtTransaction); - applyRtTransaction(frame); + updateRelativeZ(mFrameCallbackTransaction); + applyOrMergeTransaction(mFrameCallbackTransaction, frame); } // It's possible that mSurfaceControl is released in the UI thread before // the transaction completes. If that happens, an exception is thrown, which @@ -1380,22 +1407,21 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall return mRTLastReportedPosition; } - private void setParentSpaceRectangle(Rect position, long frameNumber) { + private void setParentSpaceRectangle(Rect position, long frameNumber, Transaction t) { final ViewRootImpl viewRoot = getViewRootImpl(); - applySurfaceTransforms(mSurfaceControl, mRtTransaction, position); - applyChildSurfaceTransaction_renderWorker(mRtTransaction, viewRoot.mSurface, frameNumber); - applyRtTransaction(frameNumber); + applySurfaceTransforms(mSurfaceControl, t, position); + applyChildSurfaceTransaction_renderWorker(t, viewRoot.mSurface, frameNumber); + applyOrMergeTransaction(t, frameNumber); } - private void applyRtTransaction(long frameNumber) { + private void applyOrMergeTransaction(Transaction t, long frameNumber) { final ViewRootImpl viewRoot = getViewRootImpl(); boolean useBLAST = viewRoot != null && useBLASTSync(viewRoot); if (useBLAST) { // If we are using BLAST, merge the transaction with the viewroot buffer transaction. - viewRoot.mergeWithNextTransaction(mRtTransaction, frameNumber); - return; + viewRoot.mergeWithNextTransaction(t, frameNumber); } else { - mRtTransaction.apply(); + t.apply(); } } @@ -1436,7 +1462,8 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall left, top, right, bottom)); } mRTLastReportedPosition.set(left, top, right, bottom); - setParentSpaceRectangle(mRTLastReportedPosition, frameNumber); + setParentSpaceRectangle(mRTLastReportedPosition, frameNumber, + mPositionChangedTransaction); // Now overwrite mRTLastReportedPosition with our values } catch (Exception ex) { Log.e(TAG, "Exception from repositionChild", ex); @@ -1448,7 +1475,7 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall float bottom, float vecX, float vecY, float maxStretch) { mRtTransaction.setStretchEffect(mSurfaceControl, left, top, right, bottom, vecX, vecY, maxStretch); - applyRtTransaction(frameNumber); + applyOrMergeTransaction(mRtTransaction, frameNumber); } @Override @@ -1468,14 +1495,12 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall * need to hold the lock here. */ synchronized (mSurfaceControlLock) { - final ViewRootImpl viewRoot = getViewRootImpl(); - mRtTransaction.hide(mSurfaceControl); if (mRtReleaseSurfaces) { mRtReleaseSurfaces = false; releaseSurfaces(mRtTransaction); } - applyRtTransaction(frameNumber); + applyOrMergeTransaction(mRtTransaction, frameNumber); mRtHandlingPositionUpdates = false; } }