Merge "Revert^2 "Drive SurfaceView visibility on renderthread"" into udc-dev am: ebdb8ed17a

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/22216471

Change-Id: I68fcb0912f4ed3526a1128cf7306aebb9be0cdb6
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

2 files changed, 37 insertions, 48 deletions

diff --git a/core/java/android/view/SurfaceView.java b/core/java/android/view/SurfaceView.java
index b46a68c1d5fd..cdea97ce9b7f 100644
--- a/core/java/android/view/SurfaceView.java
+++ b/core/java/android/view/SurfaceView.java
@@ -33,7 +33,6 @@ 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;
@@ -851,10 +850,14 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
             }
             mParentSurfaceSequenceId = viewRoot.getSurfaceSequenceId();
 
-            if (mViewVisibility) {
-                surfaceUpdateTransaction.show(mSurfaceControl);
-            } else {
-                surfaceUpdateTransaction.hide(mSurfaceControl);
+            // Only control visibility if we're not hardware-accelerated. Otherwise we'll
+            // let renderthread drive since offscreen SurfaceControls should not be visible.
+            if (!isHardwareAccelerated()) {
+                if (mViewVisibility) {
+                    surfaceUpdateTransaction.show(mSurfaceControl);
+                } else {
+                    surfaceUpdateTransaction.hide(mSurfaceControl);
+                }
             }
 
             updateBackgroundVisibility(surfaceUpdateTransaction);
@@ -1417,12 +1420,10 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
     }
 
     private final Rect mRTLastReportedPosition = new Rect();
-    private final Point mRTLastReportedSurfaceSize = new Point();
 
     private class SurfaceViewPositionUpdateListener implements RenderNode.PositionUpdateListener {
         private final int mRtSurfaceWidth;
         private final int mRtSurfaceHeight;
-        private boolean mRtFirst = true;
         private final SurfaceControl.Transaction mPositionChangedTransaction =
                 new SurfaceControl.Transaction();
 
@@ -1433,15 +1434,6 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
 
         @Override
         public void positionChanged(long frameNumber, int left, int top, int right, int bottom) {
-            if (!mRtFirst && (mRTLastReportedPosition.left == left
-                    && mRTLastReportedPosition.top == top
-                    && mRTLastReportedPosition.right == right
-                    && mRTLastReportedPosition.bottom == bottom
-                    && mRTLastReportedSurfaceSize.x == mRtSurfaceWidth
-                    && mRTLastReportedSurfaceSize.y == mRtSurfaceHeight)) {
-                return;
-            }
-            mRtFirst = false;
             try {
                 if (DEBUG_POSITION) {
                     Log.d(TAG, String.format(
@@ -1452,8 +1444,8 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
                 }
                 synchronized (mSurfaceControlLock) {
                     if (mSurfaceControl == null) return;
+
                     mRTLastReportedPosition.set(left, top, right, bottom);
-                    mRTLastReportedSurfaceSize.set(mRtSurfaceWidth, mRtSurfaceHeight);
                     onSetSurfacePositionAndScale(mPositionChangedTransaction, mSurfaceControl,
                             mRTLastReportedPosition.left /*positionLeft*/,
                             mRTLastReportedPosition.top /*positionTop*/,
@@ -1461,10 +1453,8 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
                                     / (float) mRtSurfaceWidth /*postScaleX*/,
                             mRTLastReportedPosition.height()
                                     / (float) mRtSurfaceHeight /*postScaleY*/);
-                    if (mViewVisibility) {
-                        // b/131239825
-                        mPositionChangedTransaction.show(mSurfaceControl);
-                    }
+
+                    mPositionChangedTransaction.show(mSurfaceControl);
                 }
                 applyOrMergeTransaction(mPositionChangedTransaction, frameNumber);
             } catch (Exception ex) {
@@ -1490,7 +1480,6 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
                         System.identityHashCode(this), frameNumber));
             }
             mRTLastReportedPosition.setEmpty();
-            mRTLastReportedSurfaceSize.set(-1, -1);
 
             // positionLost can be called while UI thread is un-paused.
             synchronized (mSurfaceControlLock) {
diff --git a/libs/hwui/jni/android_graphics_RenderNode.cpp b/libs/hwui/jni/android_graphics_RenderNode.cpp
index db7639029187..ac1f92dee507 100644
--- a/libs/hwui/jni/android_graphics_RenderNode.cpp
+++ b/libs/hwui/jni/android_graphics_RenderNode.cpp
@@ -605,15 +605,25 @@ static void android_view_RenderNode_requestPositionUpdates(JNIEnv* env, jobject,
             }
             mPreviousPosition = bounds;
 
+            ATRACE_NAME("Update SurfaceView position");
+
 #ifdef __ANDROID__ // Layoutlib does not support CanvasContext
-            incStrong(0);
-            auto functor = std::bind(
-                std::mem_fn(&PositionListenerTrampoline::doUpdatePositionAsync), this,
-                (jlong) info.canvasContext.getFrameNumber(),
-                (jint) bounds.left, (jint) bounds.top,
-                (jint) bounds.right, (jint) bounds.bottom);
-
-            info.canvasContext.enqueueFrameWork(std::move(functor));
+            JNIEnv* env = jnienv();
+            // Update the new position synchronously. We cannot defer this to
+            // a worker pool to process asynchronously because the UI thread
+            // may be unblocked by the time a worker thread can process this,
+            // In particular if the app removes a view from the view tree before
+            // this callback is dispatched, then we lose the position
+            // information for this frame.
+            jboolean keepListening = env->CallStaticBooleanMethod(
+                    gPositionListener.clazz, gPositionListener.callPositionChanged, mListener,
+                    static_cast<jlong>(info.canvasContext.getFrameNumber()),
+                    static_cast<jint>(bounds.left), static_cast<jint>(bounds.top),
+                    static_cast<jint>(bounds.right), static_cast<jint>(bounds.bottom));
+            if (!keepListening) {
+                env->DeleteGlobalRef(mListener);
+                mListener = nullptr;
+            }
 #endif
         }
 
@@ -628,7 +638,14 @@ static void android_view_RenderNode_requestPositionUpdates(JNIEnv* env, jobject,
             ATRACE_NAME("SurfaceView position lost");
             JNIEnv* env = jnienv();
 #ifdef __ANDROID__ // Layoutlib does not support CanvasContext
-            // TODO: Remember why this is synchronous and then make a comment
+            // Update the lost position synchronously. We cannot defer this to
+            // a worker pool to process asynchronously because the UI thread
+            // may be unblocked by the time a worker thread can process this,
+            // In particular if a view's rendernode is readded to the scene
+            // before this callback is dispatched, then we report that we lost
+            // position information on the wrong frame, which can be problematic
+            // for views like SurfaceView which rely on RenderNode callbacks
+            // for driving visibility.
             jboolean keepListening = env->CallStaticBooleanMethod(
                     gPositionListener.clazz, gPositionListener.callPositionLost, mListener,
                     info ? info->canvasContext.getFrameNumber() : 0);
@@ -708,23 +725,6 @@ static void android_view_RenderNode_requestPositionUpdates(JNIEnv* env, jobject,
             }
         }
 
-        void doUpdatePositionAsync(jlong frameNumber, jint left, jint top,
-                jint right, jint bottom) {
-            ATRACE_NAME("Update SurfaceView position");
-
-            JNIEnv* env = jnienv();
-            jboolean keepListening = env->CallStaticBooleanMethod(
-                    gPositionListener.clazz, gPositionListener.callPositionChanged, mListener,
-                    frameNumber, left, top, right, bottom);
-            if (!keepListening) {
-                env->DeleteGlobalRef(mListener);
-                mListener = nullptr;
-            }
-
-            // We need to release ourselves here
-            decStrong(0);
-        }
-
         JavaVM* mVm;
         jobject mListener;
         uirenderer::Rect mPreviousPosition;