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

1 files changed, 46 insertions, 21 deletions

diff --git a/core/java/android/view/SurfaceView.java b/core/java/android/view/SurfaceView.java
index 2b96a14b04d4..7bdf5cf879f3 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;
             }
         }