From e0b684f7d00d0a33d5498e548dfa03a81330e826 Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Thu, 11 Aug 2022 23:43:44 +0000 Subject: [PATCH] SurfaceView: Revert SurfaceLock locking changes This reverts commit 9e3cd0533fa4dbca75d65750f9ffcd9817779c3c and commit 74c760394aacd5a57ba0acade8cb8151ba14678c. Initially we made a change to address a performance issue in SurfaceView caused by holding the mSurfaceLock whenever we went though the updateSurfaces loop. This had a side effects of causing an ANR since the locking order changed. The followup fix removed the lock but broke some API contracts around lock and unlock canvas. Subsequent fixes, which never went into QPR, introduced new issues. Changing the locking order is too risky QPR so lets revert and try to rework the locking mechanism in U. Test: app in b/234006724 does not ANR Test: app in b/235188096 does not crash Test: app in b/239895124 does not ANR Test: app in b/239142077 does not crash Test: atest SurfaceViewTest Bug: 235188096 Change-Id: I99c5bb707ecad7c8d3c1fbd8b4105a77d58c145d --- core/java/android/view/SurfaceView.java | 209 ++++++++++++------------ 1 file changed, 103 insertions(+), 106 deletions(-) diff --git a/core/java/android/view/SurfaceView.java b/core/java/android/view/SurfaceView.java index 536a0ac6403b6..0927bc7b94270 100644 --- a/core/java/android/view/SurfaceView.java +++ b/core/java/android/view/SurfaceView.java @@ -724,9 +724,9 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall private void releaseSurfaces(boolean releaseSurfacePackage) { mSurfaceAlpha = 1f; - mSurface.destroy(); synchronized (mSurfaceControlLock) { + mSurface.destroy(); if (mBlastBufferQueue != null) { mBlastBufferQueue.destroy(); mBlastBufferQueue = null; @@ -775,99 +775,105 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall Transaction surfaceUpdateTransaction) { boolean realSizeChanged = false; - mDrawingStopped = !mVisible; + mSurfaceLock.lock(); + try { + mDrawingStopped = !mVisible; - if (DEBUG) Log.i(TAG, System.identityHashCode(this) + " " - + "Cur surface: " + mSurface); + if (DEBUG) Log.i(TAG, System.identityHashCode(this) + " " + + "Cur surface: " + mSurface); - // If we are creating the surface control or the parent surface has not - // changed, then set relative z. Otherwise allow the parent - // 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) { - updateRelativeZ(surfaceUpdateTransaction); - if (mSurfacePackage != null) { - reparentSurfacePackage(surfaceUpdateTransaction, mSurfacePackage); + // If we are creating the surface control or the parent surface has not + // changed, then set relative z. Otherwise allow the parent + // 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) { + updateRelativeZ(surfaceUpdateTransaction); + if (mSurfacePackage != null) { + reparentSurfacePackage(surfaceUpdateTransaction, mSurfacePackage); + } } - } - mParentSurfaceSequenceId = viewRoot.getSurfaceSequenceId(); + mParentSurfaceSequenceId = viewRoot.getSurfaceSequenceId(); - if (mViewVisibility) { - surfaceUpdateTransaction.show(mSurfaceControl); - } else { - surfaceUpdateTransaction.hide(mSurfaceControl); - } - - - - updateBackgroundVisibility(surfaceUpdateTransaction); - updateBackgroundColor(surfaceUpdateTransaction); - if (mUseAlpha) { - float alpha = getFixedAlpha(); - surfaceUpdateTransaction.setAlpha(mSurfaceControl, alpha); - mSurfaceAlpha = alpha; - } - - surfaceUpdateTransaction.setCornerRadius(mSurfaceControl, mCornerRadius); - if ((sizeChanged || hintChanged) && !creating) { - setBufferSize(surfaceUpdateTransaction); - } - if (sizeChanged || creating || !isHardwareAccelerated()) { - // 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) { - surfaceUpdateTransaction.setWindowCrop(mSurfaceControl, mClipBounds); + if (mViewVisibility) { + surfaceUpdateTransaction.show(mSurfaceControl); } else { - surfaceUpdateTransaction.setWindowCrop(mSurfaceControl, mSurfaceWidth, - mSurfaceHeight); + surfaceUpdateTransaction.hide(mSurfaceControl); } - 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); + + updateBackgroundVisibility(surfaceUpdateTransaction); + updateBackgroundColor(surfaceUpdateTransaction); + if (mUseAlpha) { + float alpha = getFixedAlpha(); + surfaceUpdateTransaction.setAlpha(mSurfaceControl, alpha); + mSurfaceAlpha = alpha; + } + + surfaceUpdateTransaction.setCornerRadius(mSurfaceControl, mCornerRadius); + if ((sizeChanged || hintChanged) && !creating) { + setBufferSize(surfaceUpdateTransaction); + } + if (sizeChanged || creating || !isHardwareAccelerated()) { + + // 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) { + surfaceUpdateTransaction.setWindowCrop(mSurfaceControl, mClipBounds); + } else { + surfaceUpdateTransaction.setWindowCrop(mSurfaceControl, mSurfaceWidth, + mSurfaceHeight); + } + + 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); + } 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( + "%d performSurfaceTransaction %s " + + "position = [%d, %d, %d, %d] surfaceSize = %dx%d", + System.identityHashCode(this), + isHardwareAccelerated() ? "RenderWorker" : "UI Thread", + mScreenRect.left, mScreenRect.top, mScreenRect.right, + mScreenRect.bottom, mSurfaceWidth, mSurfaceHeight)); + } + } + applyTransactionOnVriDraw(surfaceUpdateTransaction); + updateEmbeddedAccessibilityMatrix(false); + + mSurfaceFrame.left = 0; + mSurfaceFrame.top = 0; + if (translator == null) { + mSurfaceFrame.right = mSurfaceWidth; + mSurfaceFrame.bottom = 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( - "%d performSurfaceTransaction %s " - + "position = [%d, %d, %d, %d] surfaceSize = %dx%d", - System.identityHashCode(this), - isHardwareAccelerated() ? "RenderWorker" : "UI Thread", - mScreenRect.left, mScreenRect.top, mScreenRect.right, - mScreenRect.bottom, mSurfaceWidth, mSurfaceHeight)); + float appInvertedScale = translator.applicationInvertedScale; + mSurfaceFrame.right = (int) (mSurfaceWidth * appInvertedScale + 0.5f); + mSurfaceFrame.bottom = (int) (mSurfaceHeight * appInvertedScale + 0.5f); } + final int surfaceWidth = mSurfaceFrame.right; + final int surfaceHeight = mSurfaceFrame.bottom; + realSizeChanged = mLastSurfaceWidth != surfaceWidth + || mLastSurfaceHeight != surfaceHeight; + mLastSurfaceWidth = surfaceWidth; + mLastSurfaceHeight = surfaceHeight; + } finally { + mSurfaceLock.unlock(); } - applyTransactionOnVriDraw(surfaceUpdateTransaction); - updateEmbeddedAccessibilityMatrix(false); - mSurfaceFrame.left = 0; - mSurfaceFrame.top = 0; - if (translator == null) { - mSurfaceFrame.right = mSurfaceWidth; - mSurfaceFrame.bottom = mSurfaceHeight; - } else { - float appInvertedScale = translator.applicationInvertedScale; - mSurfaceFrame.right = (int) (mSurfaceWidth * appInvertedScale + 0.5f); - mSurfaceFrame.bottom = (int) (mSurfaceHeight * appInvertedScale + 0.5f); - } - final int surfaceWidth = mSurfaceFrame.right; - final int surfaceHeight = mSurfaceFrame.bottom; - realSizeChanged = mLastSurfaceWidth != surfaceWidth - || mLastSurfaceHeight != surfaceHeight; - mLastSurfaceWidth = surfaceWidth; - mLastSurfaceHeight = surfaceHeight; - return realSizeChanged; } @@ -1133,30 +1139,21 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall * Surface for compatibility reasons. */ private void copySurface(boolean surfaceControlCreated, boolean bufferSizeChanged) { - // Some legacy applications use the underlying native {@link Surface} object - // as a key to whether anything has changed. In these cases, updates to the - // existing {@link Surface} will be ignored when the size changes. - // Therefore, we must explicitly recreate the {@link Surface} in these - // cases. - boolean needsWorkaround = bufferSizeChanged && - getContext().getApplicationInfo().targetSdkVersion < Build.VERSION_CODES.O; - if (!surfaceControlCreated && !needsWorkaround) { - return; - } - mSurfaceLock.lock(); - try { - if (surfaceControlCreated) { - mSurface.copyFrom(mBlastBufferQueue); - } + if (surfaceControlCreated) { + mSurface.copyFrom(mBlastBufferQueue); + } - if (needsWorkaround) { - if (mBlastBufferQueue != null) { - mSurface.transferFrom(mBlastBufferQueue.createSurfaceWithHandle()); - } - } - } finally { - mSurfaceLock.unlock(); - } + if (bufferSizeChanged && getContext().getApplicationInfo().targetSdkVersion + < Build.VERSION_CODES.O) { + // Some legacy applications use the underlying native {@link Surface} object + // as a key to whether anything has changed. In these cases, updates to the + // existing {@link Surface} will be ignored when the size changes. + // Therefore, we must explicitly recreate the {@link Surface} in these + // cases. + if (mBlastBufferQueue != null) { + mSurface.transferFrom(mBlastBufferQueue.createSurfaceWithHandle()); + } + } } private void setBufferSize(Transaction transaction) {