SurfaceView: Synchronize all surface view changes with VRI draw

Re landing with the following changes:
We initially cloned the SurfaceControl handle to avoid locking when
accessing the SurfaceControl from the position listener callbacks
running from RT workers. But this meant the last reference to the
layer handle would only be released by GC. If SurfaceViews are
created and destroyed rapidly, we would be at the mercy of GC to
release buffers.

Original change:

There are three transaction queues that can submit SurfaceView
changes.

1. Buffer updates via BBQ apply token
2. SCC apply token
3. ViewRootImpl BBQ apply token

It makes sense for most SurfaceView changes to be synchronized with
ViewRootImpl draws since the caller can optionally synchronize the
change with main window content.

This change eliminates the tmp transaction that is applied directly
via the SCC apply token and instead applies them with the ViewRootImpl
draw transaction.

Also take the opportunity to scope down mSurfaceControlLock usage.

Test: atest SurfaceViewSyncTest
Test: go/wm-smoke
Test: run mem tests via forrest
Bug: b/217973491, b/221631942
Change-Id: Idba712d146e62d7346920dc4f060cba92d47fada

2 files changed, 78 insertions, 109 deletions

diff --git a/core/java/android/view/SurfaceView.java b/core/java/android/view/SurfaceView.java
index 9aa243b241f7..6d94de7e1f90 100644
--- a/core/java/android/view/SurfaceView.java
+++ b/core/java/android/view/SurfaceView.java
@@ -133,9 +133,8 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
     private boolean mDisableBackgroundLayer = false;
 
     /**
-     * We use this lock to protect access to mSurfaceControl and
-     * SurfaceViewPositionUpdateListener#mPositionChangedTransaction. Both are accessed on the UI
-     * thread and the render thread.
+     * We use this lock to protect access to mSurfaceControl. Both are accessed on the UI
+     * thread and the render thread via RenderNode.PositionUpdateListener#positionLost.
      */
     final Object mSurfaceControlLock = new Object();
     final Rect mTmpRect = new Rect();
@@ -224,12 +223,6 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
     private final SurfaceControl.Transaction mFrameCallbackTransaction =
             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 =
@@ -760,7 +753,7 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
                 mBlastBufferQueue = null;
             }
 
-            Transaction transaction = new Transaction();
+            final Transaction transaction = new Transaction();
             if (mSurfaceControl != null) {
                 transaction.remove(mSurfaceControl);
                 mSurfaceControl = null;
@@ -790,22 +783,17 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
     // synchronously otherwise we may see flickers.
     // When the listener is updated, we will get at least a single position update call so we can
     // guarantee any changes we post will be applied.
-    private void replacePositionUpdateListener(int surfaceWidth, int surfaceHeight,
-            Transaction geometryTransaction) {
+    private void replacePositionUpdateListener(int surfaceWidth, int surfaceHeight) {
         if (mPositionListener != null) {
             mRenderNode.removePositionUpdateListener(mPositionListener);
-            synchronized (mSurfaceControlLock) {
-                geometryTransaction = mPositionListener.getTransaction().merge(geometryTransaction);
-            }
         }
-        mPositionListener = new SurfaceViewPositionUpdateListener(surfaceWidth, surfaceHeight,
-                geometryTransaction);
+        mPositionListener = new SurfaceViewPositionUpdateListener(surfaceWidth, surfaceHeight);
         mRenderNode.addPositionUpdateListener(mPositionListener);
     }
 
     private boolean performSurfaceTransaction(ViewRootImpl viewRoot, Translator translator,
             boolean creating, boolean sizeChanged, boolean hintChanged,
-            Transaction geometryTransaction) {
+            Transaction surfaceUpdateTransaction) {
         boolean realSizeChanged = false;
 
         mSurfaceLock.lock();
@@ -820,59 +808,60 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
             // SurfaceChangedCallback to update the relative z. This is needed so that
             // we do not change the relative z before the server is ready to swap the
             // parent surface.
-            if (creating || (mParentSurfaceSequenceId == viewRoot.getSurfaceSequenceId())) {
-                updateRelativeZ(mTmpTransaction);
+            if (creating) {
+                updateRelativeZ(surfaceUpdateTransaction);
+                if (mSurfacePackage != null) {
+                    reparentSurfacePackage(surfaceUpdateTransaction, mSurfacePackage);
+                }
             }
             mParentSurfaceSequenceId = viewRoot.getSurfaceSequenceId();
 
             if (mViewVisibility) {
-                geometryTransaction.show(mSurfaceControl);
+                surfaceUpdateTransaction.show(mSurfaceControl);
             } else {
-                geometryTransaction.hide(mSurfaceControl);
+                surfaceUpdateTransaction.hide(mSurfaceControl);
             }
 
-            if (mSurfacePackage != null) {
-                reparentSurfacePackage(mTmpTransaction, mSurfacePackage);
-            }
 
-            updateBackgroundVisibility(mTmpTransaction);
-            updateBackgroundColor(mTmpTransaction);
+
+            updateBackgroundVisibility(surfaceUpdateTransaction);
+            updateBackgroundColor(surfaceUpdateTransaction);
             if (mUseAlpha) {
                 float alpha = getFixedAlpha();
-                mTmpTransaction.setAlpha(mSurfaceControl, alpha);
+                surfaceUpdateTransaction.setAlpha(mSurfaceControl, alpha);
                 mSurfaceAlpha = alpha;
             }
 
-            geometryTransaction.setCornerRadius(mSurfaceControl, mCornerRadius);
+            surfaceUpdateTransaction.setCornerRadius(mSurfaceControl, mCornerRadius);
             if ((sizeChanged || hintChanged) && !creating) {
-                setBufferSize(geometryTransaction);
+                setBufferSize(surfaceUpdateTransaction);
             }
             if (sizeChanged || creating || !isHardwareAccelerated()) {
-                onSetSurfacePositionAndScaleRT(geometryTransaction, mSurfaceControl,
-                        mScreenRect.left, /*positionLeft*/
-                        mScreenRect.top /*positionTop*/ ,
-                        mScreenRect.width() / (float) mSurfaceWidth /*postScaleX*/,
-                        mScreenRect.height() / (float) mSurfaceHeight /*postScaleY*/);
 
                 // Set a window crop when creating the surface or changing its size to
                 // crop the buffer to the surface size since the buffer producer may
                 // use SCALING_MODE_SCALE and submit a larger size than the surface
                 // size.
                 if (mClipSurfaceToBounds && mClipBounds != null) {
-                    geometryTransaction.setWindowCrop(mSurfaceControl, mClipBounds);
+                    surfaceUpdateTransaction.setWindowCrop(mSurfaceControl, mClipBounds);
                 } else {
-                    geometryTransaction.setWindowCrop(mSurfaceControl, mSurfaceWidth,
+                    surfaceUpdateTransaction.setWindowCrop(mSurfaceControl, mSurfaceWidth,
                             mSurfaceHeight);
                 }
 
-                geometryTransaction.setDesintationFrame(mBlastSurfaceControl, mSurfaceWidth,
+                surfaceUpdateTransaction.setDesintationFrame(mBlastSurfaceControl, mSurfaceWidth,
                             mSurfaceHeight);
 
                 if (isHardwareAccelerated()) {
                     // This will consume the passed in transaction and the transaction will be
                     // applied on a render worker thread.
-                    replacePositionUpdateListener(mSurfaceWidth, mSurfaceHeight,
-                            geometryTransaction);
+                    replacePositionUpdateListener(mSurfaceWidth, mSurfaceHeight);
+                } else {
+                    onSetSurfacePositionAndScale(surfaceUpdateTransaction, mSurfaceControl,
+                            mScreenRect.left /*positionLeft*/,
+                            mScreenRect.top /*positionTop*/,
+                            mScreenRect.width() / (float) mSurfaceWidth /*postScaleX*/,
+                            mScreenRect.height() / (float) mSurfaceHeight /*postScaleY*/);
                 }
                 if (DEBUG_POSITION) {
                     Log.d(TAG, String.format(
@@ -884,8 +873,7 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
                             mScreenRect.bottom, mSurfaceWidth, mSurfaceHeight));
                 }
             }
-            mTmpTransaction.merge(geometryTransaction);
-            mTmpTransaction.apply();
+            applyTransactionOnVriDraw(surfaceUpdateTransaction);
             updateEmbeddedAccessibilityMatrix();
 
             mSurfaceFrame.left = 0;
@@ -993,17 +981,17 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
                 mScreenRect.offset(surfaceInsets.left, surfaceInsets.top);
                 // Collect all geometry changes and apply these changes on the RenderThread worker
                 // via the RenderNode.PositionUpdateListener.
-                final Transaction geometryTransaction = new Transaction();
+                final Transaction surfaceUpdateTransaction = new Transaction();
                 if (creating) {
                     updateOpaqueFlag();
                     final String name = "SurfaceView[" + viewRoot.getTitle().toString() + "]";
-                    createBlastSurfaceControls(viewRoot, name, geometryTransaction);
+                    createBlastSurfaceControls(viewRoot, name, surfaceUpdateTransaction);
                 } else if (mSurfaceControl == null) {
                     return;
                 }
 
                 final boolean realSizeChanged = performSurfaceTransaction(viewRoot,
-                        translator, creating, sizeChanged, hintChanged, geometryTransaction);
+                        translator, creating, sizeChanged, hintChanged, surfaceUpdateTransaction);
                 final boolean redrawNeeded = sizeChanged || creating || hintChanged
                         || (mVisible && !mDrawFinished);
 
@@ -1139,7 +1127,7 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
      *
      */
     private void createBlastSurfaceControls(ViewRootImpl viewRoot, String name,
-            Transaction geometryTransaction) {
+            Transaction surfaceUpdateTransaction) {
         if (mSurfaceControl == null) {
             mSurfaceControl = new SurfaceControl.Builder(mSurfaceSession)
                     .setName(name)
@@ -1162,11 +1150,10 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
                     .build();
         } else {
             // update blast layer
-            mTmpTransaction
+            surfaceUpdateTransaction
                     .setOpaque(mBlastSurfaceControl, (mSurfaceFlags & SurfaceControl.OPAQUE) != 0)
                     .setSecure(mBlastSurfaceControl, (mSurfaceFlags & SurfaceControl.SECURE) != 0)
-                    .show(mBlastSurfaceControl)
-                    .apply();
+                    .show(mBlastSurfaceControl);
         }
 
         if (mBackgroundControl == null) {
@@ -1213,7 +1200,7 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
      *
      * @hide
      */
-    protected void onSetSurfacePositionAndScaleRT(@NonNull Transaction transaction,
+    protected void onSetSurfacePositionAndScale(@NonNull Transaction transaction,
             @NonNull SurfaceControl surface, int positionLeft, int positionTop,
             float postScaleX, float postScaleY) {
         transaction.setPosition(surface, positionLeft, positionTop);
@@ -1226,12 +1213,14 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
         if (mSurfaceControl == null) {
             return;
         }
-        onSetSurfacePositionAndScaleRT(mTmpTransaction, mSurfaceControl,
+        final Transaction transaction = new Transaction();
+        onSetSurfacePositionAndScale(transaction, mSurfaceControl,
                 mScreenRect.left, /*positionLeft*/
                 mScreenRect.top/*positionTop*/ ,
                 mScreenRect.width() / (float) mSurfaceWidth /*postScaleX*/,
                 mScreenRect.height() / (float) mSurfaceHeight /*postScaleY*/);
-        mTmpTransaction.apply();
+        applyTransactionOnVriDraw(transaction);
+        invalidate();
     }
 
     /**
@@ -1253,52 +1242,43 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
         }
     }
 
-    private Rect mRTLastReportedPosition = new Rect();
-    private Point mRTLastReportedSurfaceSize = new Point();
+    private final Rect mRTLastReportedPosition = new Rect();
+    private final Point mRTLastReportedSurfaceSize = new Point();
 
     private class SurfaceViewPositionUpdateListener implements RenderNode.PositionUpdateListener {
-        int mRtSurfaceWidth = -1;
-        int mRtSurfaceHeight = -1;
+        private final int mRtSurfaceWidth;
+        private final int mRtSurfaceHeight;
         private final SurfaceControl.Transaction mPositionChangedTransaction =
                 new SurfaceControl.Transaction();
-        boolean mPendingTransaction = false;
 
-        SurfaceViewPositionUpdateListener(int surfaceWidth, int surfaceHeight,
-                @Nullable Transaction t) {
+        SurfaceViewPositionUpdateListener(int surfaceWidth, int surfaceHeight) {
             mRtSurfaceWidth = surfaceWidth;
             mRtSurfaceHeight = surfaceHeight;
-            if (t != null) {
-                mPositionChangedTransaction.merge(t);
-                mPendingTransaction = true;
-            }
         }
 
         @Override
         public void positionChanged(long frameNumber, int left, int top, int right, int bottom) {
-            synchronized(mSurfaceControlLock) {
-                if (mSurfaceControl == null) {
-                    return;
-                }
-                if (mRTLastReportedPosition.left == left
-                        && mRTLastReportedPosition.top == top
-                        && mRTLastReportedPosition.right == right
-                        && mRTLastReportedPosition.bottom == bottom
-                        && mRTLastReportedSurfaceSize.x == mRtSurfaceWidth
-                        && mRTLastReportedSurfaceSize.y == mRtSurfaceHeight
-                        && !mPendingTransaction) {
-                    return;
+            if (mRTLastReportedPosition.left == left
+                    && mRTLastReportedPosition.top == top
+                    && mRTLastReportedPosition.right == right
+                    && mRTLastReportedPosition.bottom == bottom
+                    && mRTLastReportedSurfaceSize.x == mRtSurfaceWidth
+                    && mRTLastReportedSurfaceSize.y == mRtSurfaceHeight) {
+                return;
+            }
+            try {
+                if (DEBUG_POSITION) {
+                    Log.d(TAG, String.format(
+                            "%d updateSurfacePosition RenderWorker, frameNr = %d, "
+                                    + "position = [%d, %d, %d, %d] surfaceSize = %dx%d",
+                            System.identityHashCode(SurfaceView.this), frameNumber,
+                            left, top, right, bottom, mRtSurfaceWidth, mRtSurfaceHeight));
                 }
-                try {
-                    if (DEBUG_POSITION) {
-                        Log.d(TAG, String.format(
-                                "%d updateSurfacePosition RenderWorker, frameNr = %d, "
-                                        + "position = [%d, %d, %d, %d] surfaceSize = %dx%d",
-                                System.identityHashCode(SurfaceView.this), frameNumber,
-                                left, top, right, bottom, mRtSurfaceWidth, mRtSurfaceHeight));
-                    }
+                synchronized (mSurfaceControlLock) {
+                    if (mSurfaceControl == null) return;
                     mRTLastReportedPosition.set(left, top, right, bottom);
                     mRTLastReportedSurfaceSize.set(mRtSurfaceWidth, mRtSurfaceHeight);
-                    onSetSurfacePositionAndScaleRT(mPositionChangedTransaction, mSurfaceControl,
+                    onSetSurfacePositionAndScale(mPositionChangedTransaction, mSurfaceControl,
                             mRTLastReportedPosition.left /*positionLeft*/,
                             mRTLastReportedPosition.top /*positionTop*/,
                             mRTLastReportedPosition.width()
@@ -1306,13 +1286,13 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
                             mRTLastReportedPosition.height()
                                     / (float) mRtSurfaceHeight /*postScaleY*/);
                     if (mViewVisibility) {
+                        // b/131239825
                         mPositionChangedTransaction.show(mSurfaceControl);
                     }
-                    applyOrMergeTransaction(mPositionChangedTransaction, frameNumber);
-                    mPendingTransaction = false;
-                } catch (Exception ex) {
-                    Log.e(TAG, "Exception from repositionChild", ex);
                 }
+                applyOrMergeTransaction(mPositionChangedTransaction, frameNumber);
+            } catch (Exception ex) {
+                Log.e(TAG, "Exception from repositionChild", ex);
             }
         }
 
@@ -1336,28 +1316,14 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
             mRTLastReportedPosition.setEmpty();
             mRTLastReportedSurfaceSize.set(-1, -1);
 
-            /**
-             * positionLost can be called while UI thread is un-paused so we
-             * need to hold the lock here.
-             */
+            // positionLost can be called while UI thread is un-paused.
             synchronized (mSurfaceControlLock) {
-                if (mPendingTransaction) {
-                    Log.w(TAG, System.identityHashCode(SurfaceView.this)
-                            + "Pending transaction cleared.");
-                    mPositionChangedTransaction.clear();
-                    mPendingTransaction = false;
-                }
-                if (mSurfaceControl == null) {
-                    return;
-                }
+                if (mSurfaceControl == null) return;
+                // b/131239825
                 mRtTransaction.hide(mSurfaceControl);
                 applyOrMergeTransaction(mRtTransaction, frameNumber);
             }
         }
-
-        public Transaction getTransaction() {
-            return mPositionChangedTransaction;
-        }
     }
 
     private SurfaceViewPositionUpdateListener mPositionListener = null;
@@ -1404,8 +1370,10 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
      * @hide
      */
     public void setResizeBackgroundColor(int bgColor) {
-        setResizeBackgroundColor(mTmpTransaction, bgColor);
-        mTmpTransaction.apply();
+        final SurfaceControl.Transaction transaction = new SurfaceControl.Transaction();
+        setResizeBackgroundColor(transaction, bgColor);
+        applyTransactionOnVriDraw(transaction);
+        invalidate();
     }
 
     /**
diff --git a/core/java/android/widget/inline/InlineContentView.java b/core/java/android/widget/inline/InlineContentView.java
index 9712311aab7c..e4f483a29343 100644
--- a/core/java/android/widget/inline/InlineContentView.java
+++ b/core/java/android/widget/inline/InlineContentView.java
@@ -230,8 +230,9 @@ public class InlineContentView extends ViewGroup {
             int defStyleAttr, int defStyleRes) {
         super(context, attrs, defStyleAttr, defStyleRes);
         mSurfaceView = new SurfaceView(context, attrs, defStyleAttr, defStyleRes) {
+            // b/219807628
             @Override
-            protected void onSetSurfacePositionAndScaleRT(
+            protected void onSetSurfacePositionAndScale(
                     @NonNull SurfaceControl.Transaction transaction,
                     @NonNull SurfaceControl surface, int positionLeft, int positionTop,
                     float postScaleX, float postScaleY) {
@@ -248,7 +249,7 @@ public class InlineContentView extends ViewGroup {
                 postScaleX = InlineContentView.this.getScaleX();
                 postScaleY = InlineContentView.this.getScaleY();
 
-                super.onSetSurfacePositionAndScaleRT(transaction, surface, positionLeft,
+                super.onSetSurfacePositionAndScale(transaction, surface, positionLeft,
                         positionTop, postScaleX, postScaleY);
             }
         };