From 65425b4db735656d98f66074594fb31596351207 Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Thu, 23 Mar 2023 03:45:38 +0000 Subject: [PATCH] Revert^2 "Drive SurfaceView visibility on renderthread" This includes a change to RenderNode which ensures that RenderNode callbacks are dispatched synchronously in prepareTree(). This is because the UI thread may be unblocked while replaying drawing commands, and we don't wait on worker threads for a given frame until after we're done drawing. 13be0e2c409209f5a9aa453edbbce32f67d690e0 Bug: 274519506 Bug: 269113414 Test: CtsSurfaceControlTests Change-Id: Iae5c7d096930ec76fd54d9c41a1a479c86d18870 --- core/java/android/view/SurfaceView.java | 33 ++++-------- libs/hwui/jni/android_graphics_RenderNode.cpp | 52 +++++++++---------- 2 files changed, 37 insertions(+), 48 deletions(-) diff --git a/core/java/android/view/SurfaceView.java b/core/java/android/view/SurfaceView.java index b46a68c1d5fde..cdea97ce9b7ff 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 db7639029187f..ac1f92dee5076 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; -#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); + ATRACE_NAME("Update SurfaceView position"); - info.canvasContext.enqueueFrameWork(std::move(functor)); +#ifdef __ANDROID__ // Layoutlib does not support CanvasContext + 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(info.canvasContext.getFrameNumber()), + static_cast(bounds.left), static_cast(bounds.top), + static_cast(bounds.right), static_cast(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;