From 5f6772539d5277ae3e55f3969c196746f1b930a2 Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Tue, 9 Feb 2021 12:54:16 -0800 Subject: [PATCH 1/2] Fix SurfaceView background layer leaks The background layer was parented to the root sv layer but in blast when recreating the sv surface, we were only cleaning up the blast surface. This would leak the root container layer and the background color layer that was parented to it. Instead with blast enabled, we keep both the root and the blast layer alive. If the surface needs to be recreated, we only recreate the BlastBufferQueue adapter. This also simplifies the preserve old surface logic because we no longer need to track the old surface control. The blast layer will continue presenting the buffer from the old BBQ. Test: -1 screen with video, check we are not leaking background layers Test: go/wm-smoke Fixes: 175743319 Change-Id: I0f5d57e512bfae4fff9cbcd52f6939103ad3a4b0 --- core/java/android/view/SurfaceView.java | 89 ++++++++++++++++--------- 1 file changed, 59 insertions(+), 30 deletions(-) diff --git a/core/java/android/view/SurfaceView.java b/core/java/android/view/SurfaceView.java index 70ec2d42b59ba..6eba83fee48c9 100644 --- a/core/java/android/view/SurfaceView.java +++ b/core/java/android/view/SurfaceView.java @@ -1083,7 +1083,12 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall if (creating) { updateOpaqueFlag(); - mDeferredDestroySurfaceControl = createSurfaceControls(viewRoot); + final String name = "SurfaceView[" + viewRoot.getTitle().toString() + "]"; + if (mUseBlastAdapter) { + createBlastSurfaceControls(viewRoot, name); + } else { + mDeferredDestroySurfaceControl = createSurfaceControls(viewRoot, name); + } } else if (mSurfaceControl == null) { return; } @@ -1220,53 +1225,77 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall * out, the old surface can be persevered until the new one has drawn by keeping the reference * of the old SurfaceControl alive. */ - private SurfaceControl createSurfaceControls(ViewRootImpl viewRoot) { - final String name = "SurfaceView[" + viewRoot.getTitle().toString() + "]"; - - SurfaceControl.Builder builder = new SurfaceControl.Builder(mSurfaceSession) + private SurfaceControl createSurfaceControls(ViewRootImpl viewRoot, String name) { + final SurfaceControl previousSurfaceControl = mSurfaceControl; + mSurfaceControl = new SurfaceControl.Builder(mSurfaceSession) .setName(name) .setLocalOwnerView(this) .setParent(viewRoot.getBoundsLayer()) - .setCallsite("SurfaceView.updateSurface"); + .setCallsite("SurfaceView.updateSurface") + .setBufferSize(mSurfaceWidth, mSurfaceHeight) + .setFlags(mSurfaceFlags) + .setFormat(mFormat) + .build(); + mBackgroundControl = createBackgroundControl(name); + return previousSurfaceControl; + } - final SurfaceControl previousSurfaceControl; - if (mUseBlastAdapter) { - mSurfaceControl = builder + private SurfaceControl createBackgroundControl(String name) { + return new SurfaceControl.Builder(mSurfaceSession) + .setName("Background for " + name) + .setLocalOwnerView(this) + .setOpaque(true) + .setColorLayer() + .setParent(mSurfaceControl) + .setCallsite("SurfaceView.updateSurface") + .build(); + } + + // We don't recreate the surface controls but only recreate the adapter. Since the blast layer + // is still alive, the old buffers will continue to be presented until replaced by buffers from + // the new adapter. This means we do not need to track the old surface control and destroy it + // after the client has drawn to avoid any flickers. + private void createBlastSurfaceControls(ViewRootImpl viewRoot, String name) { + if (mSurfaceControl == null) { + mSurfaceControl = new SurfaceControl.Builder(mSurfaceSession) + .setName(name) + .setLocalOwnerView(this) + .setParent(viewRoot.getBoundsLayer()) + .setCallsite("SurfaceView.updateSurface") .setContainerLayer() .build(); - previousSurfaceControl = mBlastSurfaceControl; + } + + if (mBlastSurfaceControl == null) { mBlastSurfaceControl = new SurfaceControl.Builder(mSurfaceSession) .setName(name + "(BLAST)") .setLocalOwnerView(this) - .setBufferSize(mSurfaceWidth, mSurfaceHeight) .setParent(mSurfaceControl) .setFlags(mSurfaceFlags) .setHidden(false) .setBLASTLayer() .setCallsite("SurfaceView.updateSurface") .build(); - mBlastBufferQueue = new BLASTBufferQueue(name, mBlastSurfaceControl, mSurfaceWidth, - mSurfaceHeight, mFormat, true /* TODO */); } else { - previousSurfaceControl = mSurfaceControl; - mSurfaceControl = builder - .setBufferSize(mSurfaceWidth, mSurfaceHeight) - .setFlags(mSurfaceFlags) - .setFormat(mFormat) - .build(); - mBlastSurfaceControl = null; - mBlastBufferQueue = null; + // update blast layer + mTmpTransaction + .setOpaque(mBlastSurfaceControl, (mSurfaceFlags & SurfaceControl.OPAQUE) != 0) + .setSecure(mBlastSurfaceControl, (mSurfaceFlags & SurfaceControl.SECURE) != 0) + .show(mBlastSurfaceControl) + .apply(); } - mBackgroundControl = new SurfaceControl.Builder(mSurfaceSession) - .setName("Background for " + name) - .setLocalOwnerView(this) - .setOpaque(true) - .setColorLayer() - .setParent(mSurfaceControl) - .setCallsite("SurfaceView.updateSurface") - .build(); - return previousSurfaceControl; + if (mBackgroundControl == null) { + mBackgroundControl = createBackgroundControl(name); + } + + // Always recreate the IGBP for compatibility. This can be optimized in the future but + // the behavior change will need to be gated by SDK version. + if (mBlastBufferQueue != null) { + mBlastBufferQueue.destroy(); + } + mBlastBufferQueue = new BLASTBufferQueue(name, mBlastSurfaceControl, mSurfaceWidth, + mSurfaceHeight, mFormat, true /* TODO */); } private void onDrawFinished() { From 127b0e6f1e5371761e5dd2c22087fc07010a0380 Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Wed, 10 Feb 2021 10:40:04 -0800 Subject: [PATCH 2/2] Use surface size instead of frame size when adjusting layer bounds When exiting splitscreen we can end up cropping the SurfaceView incorrectly. This is because when drag drag resizing, the surface is resized to full screen and the window frame is updated in a subsequent relayout call. To fix this we use the surface size after adjusting for surface insets. Test: exit chrome from splitscreen to fullscreen, check that SV resizes correctly Test: atest SurfaceViewSyncTest Test: go/wm-smoke Fixes: 179911825 Change-Id: I0ccbdb5afa507b7a67e6aa7071ab5fb71f8a8cb8 --- core/java/android/view/ViewRootImpl.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java index 036a703f178c8..3e1451ffd3a09 100644 --- a/core/java/android/view/ViewRootImpl.java +++ b/core/java/android/view/ViewRootImpl.java @@ -1897,11 +1897,12 @@ public final class ViewRootImpl implements ViewParent, } private void setBoundsLayerCrop(Transaction t) { - // mWinFrame is already adjusted for surface insets. So offset it and use it as - // the cropping bounds. - mTempBoundsRect.set(mWinFrame); - mTempBoundsRect.offsetTo(mWindowAttributes.surfaceInsets.left, - mWindowAttributes.surfaceInsets.top); + // Adjust of insets and update the bounds layer so child surfaces do not draw into + // the surface inset region. + mTempBoundsRect.set(0, 0, mSurfaceSize.x, mSurfaceSize.y); + mTempBoundsRect.inset(mWindowAttributes.surfaceInsets.left, + mWindowAttributes.surfaceInsets.top, + mWindowAttributes.surfaceInsets.right, mWindowAttributes.surfaceInsets.bottom); t.setWindowCrop(mBoundsLayer, mTempBoundsRect); }