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
This commit is contained in:
Robert Carr
@@ -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) {
|
||||
|
||||
Reference in New Issue
Block a user