From 74c760394aacd5a57ba0acade8cb8151ba14678c Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Thu, 21 Apr 2022 08:55:51 -0700 Subject: [PATCH] SurfaceView: Fix overlocking of mSurfaceLock mSurfaceLock is used to ensure the Surface object stays valid over the span of the calls to lockCanvas and unlockCanvasAndPost which could come from any thread. In API Level 29 and below, updateSurface was split in to two paths, a first path (which would acquire mSurfaceLock) for when the Surface could be destroyed, and second path for geometry updates only. The consolidation of these two paths in updateSurface created an overlocking of mSurfaceLock, which is now acquired even when mSurface won't be modified. In fact it's acquired whenever the geometry changes. This means an application rendering thread which creates delays between lock and unlock canvas could create undesirable and needless delays on the main UI thread, if the SurfaceView geometry updates. We also have some underlocking, where we aren't actually locking when destroying the SurfaceView. Test: Manual with test app Bug: 206846658 Change-Id: Idc17d111eee7a7365af4e94a156e7c46c7ea8171 --- core/java/android/view/SurfaceView.java | 230 ++++++++++++------------ 1 file changed, 119 insertions(+), 111 deletions(-) diff --git a/core/java/android/view/SurfaceView.java b/core/java/android/view/SurfaceView.java index fd557e79c9c17..71e457dcd700d 100644 --- a/core/java/android/view/SurfaceView.java +++ b/core/java/android/view/SurfaceView.java @@ -719,9 +719,14 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall private void releaseSurfaces(boolean releaseSurfacePackage) { mSurfaceAlpha = 1f; - - synchronized (mSurfaceControlLock) { + + mSurfaceLock.lock(); + try { mSurface.destroy(); + } finally { + mSurfaceLock.unlock(); + } + synchronized (mSurfaceControlLock) { if (mBlastBufferQueue != null) { mBlastBufferQueue.destroy(); mBlastBufferQueue = null; @@ -770,105 +775,99 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall Transaction surfaceUpdateTransaction) { boolean realSizeChanged = false; - mSurfaceLock.lock(); - try { - mDrawingStopped = !mVisible; + 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(); - - 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); - } 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 { - 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(); } + 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); + } 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 { + 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; } @@ -1103,21 +1102,30 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall * Surface for compatibility reasons. */ private void copySurface(boolean surfaceControlCreated, boolean bufferSizeChanged) { - if (surfaceControlCreated) { - mSurface.copyFrom(mBlastBufferQueue); - } + // 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 (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()); - } - } + if (needsWorkaround) { + if (mBlastBufferQueue != null) { + mSurface.transferFrom(mBlastBufferQueue.createSurfaceWithHandle()); + } + } + } finally { + mSurfaceLock.unlock(); + } } private void setBufferSize(Transaction transaction) {