SurfaceView: Synchronize position updates with fixed size changes

This cl solves a couple of issues:
1. Render thread workers and UI thread share the surface size. They work
on different frames (UI thread sets up the next frame while render
thread is working on the current frame. Accessing the surface size is
unsafe and can result in flickers.

2. UI thread is changing the geometry on size change which might
conflict with render thread causing flickers. This is because of
unsafe accesses as the one mentioned above and the UI thread may
not have the up-to-date position of the view.

This cl fixes the issues by only applying geometry changes in the
UI thread when creating the surface, otherwise the render thread
workers are responsible for updating the geometry. The RenderNode
position update listeners are updated whenever the surface size
changes in order to capture the new size and accompanying changes
which must be applied with the new geometry changes.

Note: updating the position update listeners will trigger a
position update callback so we are guaranteed to apply the
changes in scenarios where the fixed size changes but the view
size does not change.

Test: atest SurfaceViewSyncTest#testSurfaceViewChangeFixedSize
Test: atest SurfaceViewSyncTest#testSurfaceViewChangeFixedSizeWithViewSizeChanges
Test: go/wm-smoke
Test: repro steps from b/190449942
Fixes: 190449942
Change-Id: I076321c853f9a0f6cbf169637e3f3ede60361d60

1 files changed, 95 insertions, 56 deletions

diff --git a/core/java/android/view/SurfaceView.java b/core/java/android/view/SurfaceView.java
index 6c2d6a1c3f2e..c1956e45653b 100644
--- a/core/java/android/view/SurfaceView.java
+++ b/core/java/android/view/SurfaceView.java
@@ -33,6 +33,7 @@ import android.graphics.Color;
 import android.graphics.Matrix;
 import android.graphics.Paint;
 import android.graphics.PixelFormat;
+import android.graphics.Point;
 import android.graphics.Rect;
 import android.graphics.Region;
 import android.graphics.RenderNode;
@@ -237,15 +238,6 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
             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.
      */
@@ -295,7 +287,6 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
             int defStyleRes, boolean disableBackgroundLayer) {
         super(context, attrs, defStyleAttr, defStyleRes);
         mUseBlastAdapter = useBlastAdapter(context);
-        mRenderNode.addPositionUpdateListener(mPositionListener);
 
         setWillNotDraw(true);
         mDisableBackgroundLayer = disableBackgroundLayer;
@@ -943,6 +934,24 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
         }
     }
 
+
+    // The position update listener is used to safely share the surface size between render thread
+    // workers and the UI thread. Both threads need to know the surface size to determine the scale.
+    // The parent layer scales the surface size to view size. The child (BBQ) layer scales
+    // the buffer to the surface size. Both scales along with the window crop must be applied
+    // 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,
+            @Nullable Transaction geometryTransaction) {
+        if (mPositionListener != null) {
+            mRenderNode.removePositionUpdateListener(mPositionListener);
+        }
+        mPositionListener = new SurfaceViewPositionUpdateListener(surfaceWidth, surfaceHeight,
+                geometryTransaction);
+        mRenderNode.addPositionUpdateListener(mPositionListener);
+    }
+
     private boolean performSurfaceTransaction(ViewRootImpl viewRoot, Translator translator,
             boolean creating, boolean sizeChanged, boolean hintChanged) {
         boolean realSizeChanged = false;
@@ -985,13 +994,13 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
             // While creating the surface, we will set it's initial
             // geometry. Outside of that though, we should generally
             // leave it to the RenderThread.
-            //
-            // There is one more case when the buffer size changes we aren't yet
-            // prepared to sync (as even following the transaction applying
-            // we still need to latch a buffer).
-            // b/28866173
-            if (sizeChanged || creating || !mRtHandlingPositionUpdates) {
-                onSetSurfacePositionAndScaleRT(mTmpTransaction, mSurfaceControl,
+            Transaction geometryTransaction = new Transaction();
+            geometryTransaction.setCornerRadius(mSurfaceControl, mCornerRadius);
+            if ((sizeChanged || hintChanged) && !creating) {
+                setBufferSize(geometryTransaction);
+            }
+            if (sizeChanged || creating || !isHardwareAccelerated()) {
+                onSetSurfacePositionAndScaleRT(geometryTransaction, mSurfaceControl,
                         mScreenRect.left, /*positionLeft*/
                         mScreenRect.top /*positionTop*/ ,
                         mScreenRect.width() / (float) mSurfaceWidth /*postScaleX*/,
@@ -1002,17 +1011,31 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
                 // use SCALING_MODE_SCALE and submit a larger size than the surface
                 // size.
                 if (mClipSurfaceToBounds && mClipBounds != null) {
-                    mTmpTransaction.setWindowCrop(mSurfaceControl, mClipBounds);
+                    geometryTransaction.setWindowCrop(mSurfaceControl, mClipBounds);
                 } else {
-                    mTmpTransaction.setWindowCrop(mSurfaceControl, mSurfaceWidth,
+                    geometryTransaction.setWindowCrop(mSurfaceControl, mSurfaceWidth,
                             mSurfaceHeight);
                 }
-            }
-            mTmpTransaction.setCornerRadius(mSurfaceControl, mCornerRadius);
-            if ((sizeChanged || hintChanged) && !creating) {
-                setBufferSize(mTmpTransaction);
-            }
 
+                boolean applyChangesOnRenderThread =
+                        sizeChanged && !creating && isHardwareAccelerated();
+                if (isHardwareAccelerated()) {
+                    // This will consume the passed in transaction and the transaction will be
+                    // applied on a render worker thread.
+                    replacePositionUpdateListener(mSurfaceWidth, mSurfaceHeight,
+                            applyChangesOnRenderThread ? geometryTransaction : null);
+                }
+                if (DEBUG_POSITION) {
+                    Log.d(TAG, String.format(
+                            "%d updateSurfacePosition %s"
+                                + "position = [%d, %d, %d, %d] surfaceSize = %dx%d",
+                            System.identityHashCode(this),
+                            applyChangesOnRenderThread ? "RenderWorker" : "UiThread",
+                            mScreenRect.left, mScreenRect.top, mScreenRect.right,
+                            mScreenRect.bottom, mSurfaceWidth, mSurfaceHeight));
+                }
+            }
+            mTmpTransaction.merge(geometryTransaction);
             mTmpTransaction.apply();
             updateEmbeddedAccessibilityMatrix();
 
@@ -1399,19 +1422,6 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
         mTmpTransaction.apply();
     }
 
-    private void applySurfaceTransforms(SurfaceControl surface, SurfaceControl.Transaction t,
-            Rect position) {
-        onSetSurfacePositionAndScaleRT(t, surface,
-                position.left /*positionLeft*/,
-                position.top /*positionTop*/,
-                position.width() / (float) mSurfaceWidth /*postScaleX*/,
-                position.height() / (float) mSurfaceHeight /*postScaleY*/);
-
-        if (mViewVisibility) {
-            t.show(surface);
-        }
-    }
-
     /**
      * @return The last render position of the backing surface or an empty rect.
      *
@@ -1421,13 +1431,6 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
         return mRTLastReportedPosition;
     }
 
-    private void setParentSpaceRectangle(Rect position, long frameNumber, Transaction t) {
-        final ViewRootImpl viewRoot = getViewRootImpl();
-        applySurfaceTransforms(mSurfaceControl, t, position);
-        applyChildSurfaceTransaction_renderWorker(t, viewRoot.mSurface, frameNumber);
-        applyOrMergeTransaction(t, frameNumber);
-    }
-
     private void applyOrMergeTransaction(Transaction t, long frameNumber) {
         final ViewRootImpl viewRoot = getViewRootImpl();
         boolean useBLAST = viewRoot != null && useBLASTSync(viewRoot);
@@ -1440,9 +1443,24 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
     }
 
     private Rect mRTLastReportedPosition = new Rect();
-
-    private RenderNode.PositionUpdateListener mPositionListener =
-            new RenderNode.PositionUpdateListener() {
+    private Point mRTLastReportedSurfaceSize = new Point();
+
+    private class SurfaceViewPositionUpdateListener implements RenderNode.PositionUpdateListener {
+        int mRtSurfaceWidth = -1;
+        int mRtSurfaceHeight = -1;
+        private final SurfaceControl.Transaction mPositionChangedTransaction =
+                new SurfaceControl.Transaction();
+        boolean mPendingTransaction = false;
+
+        SurfaceViewPositionUpdateListener(int surfaceWidth, int surfaceHeight,
+                @Nullable Transaction t) {
+            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) {
@@ -1464,21 +1482,34 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
             if (mRTLastReportedPosition.left == left
                     && mRTLastReportedPosition.top == top
                     && mRTLastReportedPosition.right == right
-                    && mRTLastReportedPosition.bottom == bottom) {
+                    && mRTLastReportedPosition.bottom == bottom
+                    && mRTLastReportedSurfaceSize.x == mRtSurfaceWidth
+                    && mRTLastReportedSurfaceSize.y == mRtSurfaceHeight
+                    && !mPendingTransaction) {
                 return;
             }
             try {
                 if (DEBUG_POSITION) {
                     Log.d(TAG, String.format(
                             "%d updateSurfacePosition RenderWorker, frameNr = %d, "
-                                    + "position = [%d, %d, %d, %d]",
-                            System.identityHashCode(this), frameNumber,
-                            left, top, right, bottom));
+                                    + "position = [%d, %d, %d, %d] surfaceSize = %dx%d",
+                            System.identityHashCode(SurfaceView.this), frameNumber,
+                            left, top, right, bottom, mRtSurfaceWidth, mRtSurfaceHeight));
                 }
                 mRTLastReportedPosition.set(left, top, right, bottom);
-                setParentSpaceRectangle(mRTLastReportedPosition, frameNumber,
-                        mPositionChangedTransaction);
-                // Now overwrite mRTLastReportedPosition with our values
+                mRTLastReportedSurfaceSize.set(mRtSurfaceWidth, mRtSurfaceHeight);
+                onSetSurfacePositionAndScaleRT(mPositionChangedTransaction, mSurfaceControl,
+                        mRTLastReportedPosition.left /*positionLeft*/,
+                        mRTLastReportedPosition.top /*positionTop*/,
+                        mRTLastReportedPosition.width() / (float) mRtSurfaceWidth /*postScaleX*/,
+                        mRTLastReportedPosition.height() / (float) mRtSurfaceHeight /*postScaleY*/);
+                if (mViewVisibility) {
+                    mPositionChangedTransaction.show(mSurfaceControl);
+                }
+                applyChildSurfaceTransaction_renderWorker(mPositionChangedTransaction,
+                        getViewRootImpl().mSurface, frameNumber);
+                applyOrMergeTransaction(mPositionChangedTransaction, frameNumber);
+                mPendingTransaction = false;
             } catch (Exception ex) {
                 Log.e(TAG, "Exception from repositionChild", ex);
             }
@@ -1502,7 +1533,13 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
                         System.identityHashCode(this), frameNumber));
             }
             mRTLastReportedPosition.setEmpty();
-
+            mRTLastReportedSurfaceSize.set(-1, -1);
+            if (mPendingTransaction) {
+                Log.w(TAG, System.identityHashCode(SurfaceView.this)
+                        + "Pending transaction cleared.");
+                mPositionChangedTransaction.clear();
+                mPendingTransaction = false;
+            }
             if (mSurfaceControl == null) {
                 return;
             }
@@ -1521,7 +1558,9 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
                 mRtHandlingPositionUpdates = false;
             }
         }
-    };
+    }
+
+    private SurfaceViewPositionUpdateListener mPositionListener = null;
 
     private SurfaceHolder.Callback[] getSurfaceCallbacks() {
         SurfaceHolder.Callback[] callbacks;