Android/frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

SurfaceView: Revert SurfaceLock locking changes

Browse Source 
This reverts commit 9e3cd0533f
and commit 74c760394a.

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
This commit is contained in:
Vishnu Nair
2022-08-11 23:43:44 +00:00
parent 2701e8074c
commit e0b684f7d0
1 changed files with 103 additions and 106 deletions
Download Patch File Download Diff File Expand all files Collapse all files

209
core/java/android/view/SurfaceView.java
View File

@@ -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) {