From 5b00d1426d7f74e33129e9f2e47f27a18c838abd Mon Sep 17 00:00:00 2001 From: Chris Li Date: Sat, 2 Oct 2021 13:33:34 +0800 Subject: [PATCH] Fix unfreeze on unstarted freezer Before, when trigger change transition and screen freeze before the previous transition is finished, it will cancel the animation and call to unfreeze. However, it will then unfreeze the new snapshot instead of the old one, which cause a client crash because of missing leash. The fix contains two changes: 1. Let SurfaceAnimator to take the snapshot from the freezer as it does for animation leash. 2. When an animation leash needs to be removed, check if it is the current animation leash (parent of the window surface) before reparenting the window surface back to the parent window surface. Bug: 201622511 Bug: 196173550 Test: atest WmTests:WindowContainerTests #testStartChangeTransitionWhenPreviousIsNotFinished Test: click demo app launch spit before the previous one is finished Change-Id: I58fb05a971b49981d168d00c9fb42014b4d3cd4d --- .../android/server/wm/SurfaceAnimator.java | 47 +++++++++++-- .../com/android/server/wm/SurfaceFreezer.java | 43 ++++++------ .../android/server/wm/WindowContainer.java | 30 ++++++--- .../server/wm/WindowContainerTests.java | 67 +++++++++++++++++++ 4 files changed, 146 insertions(+), 41 deletions(-) diff --git a/services/core/java/com/android/server/wm/SurfaceAnimator.java b/services/core/java/com/android/server/wm/SurfaceAnimator.java index c7bf8ecfe949c..d712bbf0fdef1 100644 --- a/services/core/java/com/android/server/wm/SurfaceAnimator.java +++ b/services/core/java/com/android/server/wm/SurfaceAnimator.java @@ -57,8 +57,11 @@ class SurfaceAnimator { @VisibleForTesting SurfaceControl mLeash; @VisibleForTesting + SurfaceFreezer.Snapshot mSnapshot; + @VisibleForTesting final Animatable mAnimatable; - private final OnAnimationFinishedCallback mInnerAnimationFinishedCallback; + @VisibleForTesting + final OnAnimationFinishedCallback mInnerAnimationFinishedCallback; /** * Static callback to run on all animations started through this SurfaceAnimator @@ -151,12 +154,14 @@ class SurfaceAnimator { * @param animationFinishedCallback The callback being triggered when the animation finishes. * @param animationCancelledCallback The callback is triggered after the SurfaceAnimator sends a * cancel call to the underlying AnimationAdapter. + * @param snapshotAnim The animation to run for the snapshot. {@code null} if there is no + * snapshot. */ void startAnimation(Transaction t, AnimationAdapter anim, boolean hidden, @AnimationType int type, @Nullable OnAnimationFinishedCallback animationFinishedCallback, @Nullable Runnable animationCancelledCallback, - @Nullable SurfaceFreezer freezer) { + @Nullable AnimationAdapter snapshotAnim, @Nullable SurfaceFreezer freezer) { cancelAnimation(t, true /* restarting */, true /* forwardCancel */); mAnimation = anim; mAnimationType = type; @@ -181,12 +186,16 @@ class SurfaceAnimator { return; } mAnimation.startAnimation(mLeash, t, type, mInnerAnimationFinishedCallback); + if (snapshotAnim != null) { + mSnapshot = freezer.takeSnapshotForAnimation(); + mSnapshot.startAnimation(t, snapshotAnim, type); + } } void startAnimation(Transaction t, AnimationAdapter anim, boolean hidden, @AnimationType int type) { startAnimation(t, anim, hidden, type, null /* animationFinishedCallback */, - null /* animationCancelledCallback */, null /* freezer */); + null /* animationCancelledCallback */, null /* snapshotAnim */, null /* freezer */); } /** @@ -328,6 +337,7 @@ class SurfaceAnimator { final OnAnimationFinishedCallback animationFinishedCallback = mSurfaceAnimationFinishedCallback; final Runnable animationCancelledCallback = mAnimationCancelledCallback; + final SurfaceFreezer.Snapshot snapshot = mSnapshot; reset(t, false); if (animation != null) { if (!mAnimationStartDelayed && forwardCancel) { @@ -346,9 +356,14 @@ class SurfaceAnimator { } } - if (forwardCancel && leash != null) { - t.remove(leash); - mService.scheduleAnimationLocked(); + if (forwardCancel) { + if (snapshot != null) { + snapshot.cancelAnimation(t, false /* restarting */); + } + if (leash != null) { + t.remove(leash); + mService.scheduleAnimationLocked(); + } } if (!restarting) { @@ -361,6 +376,12 @@ class SurfaceAnimator { mAnimation = null; mSurfaceAnimationFinishedCallback = null; mAnimationType = ANIMATION_TYPE_NONE; + final SurfaceFreezer.Snapshot snapshot = mSnapshot; + mSnapshot = null; + if (snapshot != null) { + // Reset the mSnapshot reference before calling the callback to prevent circular reset. + snapshot.cancelAnimation(t, !destroyLeash); + } if (mLeash == null) { return; } @@ -377,11 +398,15 @@ class SurfaceAnimator { boolean scheduleAnim = false; final SurfaceControl surface = animatable.getSurfaceControl(); final SurfaceControl parent = animatable.getParentSurfaceControl(); + final SurfaceControl curAnimationLeash = animatable.getAnimationLeash(); // If the surface was destroyed or the leash is invalid, we don't care to reparent it back. // Note that we also set this variable to true even if the parent isn't valid anymore, in // order to ensure onAnimationLeashLost still gets called in this case. - final boolean reparent = surface != null; + // If the animation leash is set, and it is different from the removing leash, it means the + // surface now has a new animation surface. We don't want to reparent for that. + final boolean reparent = surface != null && (curAnimationLeash == null + || curAnimationLeash.equals(leash)); if (reparent) { if (DEBUG_ANIM) Slog.i(TAG, "Reparenting to original parent: " + parent); // We shouldn't really need these isValid checks but we do @@ -607,6 +632,14 @@ class SurfaceAnimator { */ void onAnimationLeashLost(Transaction t); + /** + * Gets the last created animation leash that has not lost yet. + */ + @Nullable + default SurfaceControl getAnimationLeash() { + return null; + } + /** * @return A new surface to be used for the animation leash, inserted at the correct * position in the hierarchy. diff --git a/services/core/java/com/android/server/wm/SurfaceFreezer.java b/services/core/java/com/android/server/wm/SurfaceFreezer.java index 89986cefb207c..9a0cabebbdc67 100644 --- a/services/core/java/com/android/server/wm/SurfaceFreezer.java +++ b/services/core/java/com/android/server/wm/SurfaceFreezer.java @@ -17,7 +17,6 @@ package com.android.server.wm; import static com.android.internal.protolog.ProtoLogGroup.WM_SHOW_TRANSACTIONS; -import static com.android.server.wm.SurfaceAnimator.ANIMATION_TYPE_APP_TRANSITION; import static com.android.server.wm.SurfaceAnimator.ANIMATION_TYPE_SCREEN_ROTATION; import android.annotation.NonNull; @@ -27,13 +26,11 @@ import android.graphics.PixelFormat; import android.graphics.Point; import android.graphics.Rect; import android.hardware.HardwareBuffer; -import android.view.Surface; import android.view.SurfaceControl; +import com.android.internal.annotations.VisibleForTesting; import com.android.internal.protolog.common.ProtoLog; -import java.util.function.Supplier; - /** * This class handles "freezing" of an Animatable. The Animatable in question should implement * Freezable. @@ -54,7 +51,8 @@ class SurfaceFreezer { private final Freezable mAnimatable; private final WindowManagerService mWmService; - private SurfaceControl mLeash; + @VisibleForTesting + SurfaceControl mLeash; Snapshot mSnapshot = null; final Rect mFreezeBounds = new Rect(); @@ -94,7 +92,7 @@ class SurfaceFreezer { if (buffer == null || buffer.getWidth() <= 1 || buffer.getHeight() <= 1) { return; } - mSnapshot = new Snapshot(mWmService.mSurfaceFactory, t, screenshotBuffer, mLeash); + mSnapshot = new Snapshot(t, screenshotBuffer, mLeash); } } @@ -108,6 +106,18 @@ class SurfaceFreezer { return out; } + /** + * Used by {@link SurfaceAnimator}. This "transfers" the snapshot leash to be used for + * animation. By transferring the leash, this will no longer try to clean-up the leash when + * finished. + */ + @Nullable + Snapshot takeSnapshotForAnimation() { + final Snapshot out = mSnapshot; + mSnapshot = null; + return out; + } + /** * Clean-up the snapshot and remove leash. If the leash was taken, this just cleans-up the * snapshot. @@ -115,6 +125,7 @@ class SurfaceFreezer { void unfreeze(SurfaceControl.Transaction t) { if (mSnapshot != null) { mSnapshot.cancelAnimation(t, false /* restarting */); + mSnapshot = null; } if (mLeash == null) { return; @@ -163,13 +174,12 @@ class SurfaceFreezer { class Snapshot { private SurfaceControl mSurfaceControl; private AnimationAdapter mAnimation; - private SurfaceAnimator.OnAnimationFinishedCallback mFinishedCallback; /** * @param t Transaction to create the thumbnail in. * @param screenshotBuffer A thumbnail or placeholder for thumbnail to initialize with. */ - Snapshot(Supplier surfaceFactory, SurfaceControl.Transaction t, + Snapshot(SurfaceControl.Transaction t, SurfaceControl.ScreenshotHardwareBuffer screenshotBuffer, SurfaceControl parent) { // We can't use a delegating constructor since we need to // reference this::onAnimationFinished @@ -211,19 +221,15 @@ class SurfaceFreezer { * component responsible for running the animation. It runs the animation with * {@link AnimationAdapter#startAnimation} once the hierarchy with * the Leash has been set up. - * @param animationFinishedCallback The callback being triggered when the animation - * finishes. */ - void startAnimation(SurfaceControl.Transaction t, AnimationAdapter anim, int type, - @Nullable SurfaceAnimator.OnAnimationFinishedCallback animationFinishedCallback) { + void startAnimation(SurfaceControl.Transaction t, AnimationAdapter anim, int type) { cancelAnimation(t, true /* restarting */); mAnimation = anim; - mFinishedCallback = animationFinishedCallback; if (mSurfaceControl == null) { cancelAnimation(t, false /* restarting */); return; } - mAnimation.startAnimation(mSurfaceControl, t, type, animationFinishedCallback); + mAnimation.startAnimation(mSurfaceControl, t, type, null /* finishCallback */); } /** @@ -235,18 +241,9 @@ class SurfaceFreezer { void cancelAnimation(SurfaceControl.Transaction t, boolean restarting) { final SurfaceControl leash = mSurfaceControl; final AnimationAdapter animation = mAnimation; - final SurfaceAnimator.OnAnimationFinishedCallback animationFinishedCallback = - mFinishedCallback; mAnimation = null; - mFinishedCallback = null; if (animation != null) { animation.onAnimationCancelled(leash); - if (!restarting) { - if (animationFinishedCallback != null) { - animationFinishedCallback.onAnimationFinished( - ANIMATION_TYPE_APP_TRANSITION, animation); - } - } } if (!restarting) { destroy(t); diff --git a/services/core/java/com/android/server/wm/WindowContainer.java b/services/core/java/com/android/server/wm/WindowContainer.java index 9a4bf6318249a..e90290383a175 100644 --- a/services/core/java/com/android/server/wm/WindowContainer.java +++ b/services/core/java/com/android/server/wm/WindowContainer.java @@ -178,6 +178,10 @@ class WindowContainer extends ConfigurationContainer< */ protected final SurfaceAnimator mSurfaceAnimator; + /** The parent leash added for animation. */ + @Nullable + private SurfaceControl mAnimationLeash; + final SurfaceFreezer mSurfaceFreezer; protected final WindowManagerService mWmService; final TransitionController mTransitionController; @@ -2561,11 +2565,14 @@ class WindowContainer extends ConfigurationContainer< * @param animationFinishedCallback The callback being triggered when the animation finishes. * @param animationCancelledCallback The callback is triggered after the SurfaceAnimator sends a * cancel call to the underlying AnimationAdapter. + * @param snapshotAnim The animation to run for the snapshot. {@code null} if there is no + * snapshot. */ void startAnimation(Transaction t, AnimationAdapter anim, boolean hidden, @AnimationType int type, @Nullable OnAnimationFinishedCallback animationFinishedCallback, - @Nullable Runnable animationCancelledCallback) { + @Nullable Runnable animationCancelledCallback, + @Nullable AnimationAdapter snapshotAnim) { if (DEBUG_ANIM) { Slog.v(TAG, "Starting animation on " + this + ": type=" + type + ", anim=" + anim); } @@ -2573,14 +2580,14 @@ class WindowContainer extends ConfigurationContainer< // TODO: This should use isVisible() but because isVisible has a really weird meaning at // the moment this doesn't work for all animatable window containers. mSurfaceAnimator.startAnimation(t, anim, hidden, type, animationFinishedCallback, - animationCancelledCallback, mSurfaceFreezer); + animationCancelledCallback, snapshotAnim, mSurfaceFreezer); } void startAnimation(Transaction t, AnimationAdapter anim, boolean hidden, @AnimationType int type, @Nullable OnAnimationFinishedCallback animationFinishedCallback) { startAnimation(t, anim, hidden, type, animationFinishedCallback, - null /* adapterAnimationCancelledCallback */); + null /* adapterAnimationCancelledCallback */, null /* snapshotAnim */); } void startAnimation(Transaction t, AnimationAdapter anim, boolean hidden, @@ -2832,17 +2839,12 @@ class WindowContainer extends ConfigurationContainer< ? taskDisplayArea::clearBackgroundColor : () -> {}; startAnimation(getPendingTransaction(), adapter, !isVisible(), - ANIMATION_TYPE_APP_TRANSITION, - (type, anim) -> cleanUpCallback.run(), - cleanUpCallback); + ANIMATION_TYPE_APP_TRANSITION, (type, anim) -> cleanUpCallback.run(), + cleanUpCallback, thumbnailAdapter); if (adapter.getShowWallpaper()) { getDisplayContent().pendingLayoutChanges |= FINISH_LAYOUT_REDO_WALLPAPER; } - if (thumbnailAdapter != null) { - mSurfaceFreezer.mSnapshot.startAnimation(getPendingTransaction(), - thumbnailAdapter, ANIMATION_TYPE_APP_TRANSITION, (type, anim) -> { }); - } } } @@ -2972,6 +2974,7 @@ class WindowContainer extends ConfigurationContainer< @Override public void onAnimationLeashCreated(Transaction t, SurfaceControl leash) { mLastLayer = -1; + mAnimationLeash = leash; reassignLayer(t); // Leash is now responsible for position, so set our position to 0. @@ -2981,11 +2984,16 @@ class WindowContainer extends ConfigurationContainer< @Override public void onAnimationLeashLost(Transaction t) { mLastLayer = -1; - mSurfaceFreezer.unfreeze(t); + mAnimationLeash = null; reassignLayer(t); updateSurfacePosition(t); } + @Override + public SurfaceControl getAnimationLeash() { + return mAnimationLeash; + } + private void doAnimationFinished(@AnimationType int type, AnimationAdapter anim) { for (int i = 0; i < mSurfaceAnimationSources.size(); ++i) { mSurfaceAnimationSources.valueAt(i).onAnimationFinished(type, anim); diff --git a/services/tests/wmtests/src/com/android/server/wm/WindowContainerTests.java b/services/tests/wmtests/src/com/android/server/wm/WindowContainerTests.java index 68053eb324b2f..bbeb980353edf 100644 --- a/services/tests/wmtests/src/com/android/server/wm/WindowContainerTests.java +++ b/services/tests/wmtests/src/com/android/server/wm/WindowContainerTests.java @@ -25,6 +25,7 @@ import static android.content.pm.ActivityInfo.SCREEN_ORIENTATION_UNSPECIFIED; import static android.view.WindowManager.LayoutParams.TYPE_BASE_APPLICATION; import static android.view.WindowManager.TRANSIT_CLOSE; import static android.view.WindowManager.TRANSIT_OLD_TASK_CLOSE; +import static android.view.WindowManager.TRANSIT_OLD_TASK_FRAGMENT_CHANGE; import static android.view.WindowManager.TRANSIT_OLD_TASK_OPEN; import static android.view.WindowManager.TRANSIT_OPEN; import static android.window.DisplayAreaOrganizer.FEATURE_DEFAULT_TASK_CONTAINER; @@ -52,6 +53,7 @@ import static com.android.server.wm.WindowContainer.POSITION_TOP; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; @@ -1106,6 +1108,71 @@ public class WindowContainerTests extends WindowTestsBase { verify(surfaceAnimator, never()).setRelativeLayer(any(), any(), anyInt()); } + @Test + public void testStartChangeTransitionWhenPreviousIsNotFinished() { + final WindowContainer container = createTaskFragmentWithParentTask( + createTask(mDisplayContent), false); + container.mSurfaceControl = mock(SurfaceControl.class); + final SurfaceAnimator surfaceAnimator = container.mSurfaceAnimator; + final SurfaceFreezer surfaceFreezer = container.mSurfaceFreezer; + final SurfaceControl.Transaction t = mock(SurfaceControl.Transaction.class); + spyOn(container); + spyOn(surfaceAnimator); + spyOn(surfaceFreezer); + doReturn(t).when(container).getPendingTransaction(); + doReturn(t).when(container).getSyncTransaction(); + + // Leash and snapshot created for change transition. + container.initializeChangeTransition(new Rect(0, 0, 1000, 2000)); + // Can't really take a snapshot, manually set one. + surfaceFreezer.mSnapshot = mock(SurfaceFreezer.Snapshot.class); + + assertNotNull(surfaceFreezer.mLeash); + assertEquals(surfaceFreezer.mLeash, container.getAnimationLeash()); + + // Start animation: surfaceAnimator take over the leash and snapshot from surfaceFreezer. + container.applyAnimationUnchecked(null /* lp */, true /* enter */, + TRANSIT_OLD_TASK_FRAGMENT_CHANGE, false /* isVoiceInteraction */, + null /* sources */); + + assertNull(surfaceFreezer.mLeash); + assertNull(surfaceFreezer.mSnapshot); + assertNotNull(surfaceAnimator.mLeash); + assertNotNull(surfaceAnimator.mSnapshot); + final SurfaceControl prevLeash = surfaceAnimator.mLeash; + final SurfaceFreezer.Snapshot prevSnapshot = surfaceAnimator.mSnapshot; + + // Prepare another change transition. + container.initializeChangeTransition(new Rect(0, 0, 1000, 2000)); + surfaceFreezer.mSnapshot = mock(SurfaceFreezer.Snapshot.class); + + assertNotNull(surfaceFreezer.mLeash); + assertEquals(surfaceFreezer.mLeash, container.getAnimationLeash()); + assertNotEquals(prevLeash, container.getAnimationLeash()); + + // Start another animation before the previous one is finished, it should reset the previous + // one, but not change the current one. + container.applyAnimationUnchecked(null /* lp */, true /* enter */, + TRANSIT_OLD_TASK_FRAGMENT_CHANGE, false /* isVoiceInteraction */, + null /* sources */); + + verify(container, never()).onAnimationLeashLost(any()); + verify(surfaceFreezer, never()).unfreeze(any()); + assertNotNull(surfaceAnimator.mLeash); + assertNotNull(surfaceAnimator.mSnapshot); + assertEquals(surfaceAnimator.mLeash, container.getAnimationLeash()); + assertNotEquals(prevLeash, surfaceAnimator.mLeash); + assertNotEquals(prevSnapshot, surfaceAnimator.mSnapshot); + + // Clean up after animation finished. + surfaceAnimator.mInnerAnimationFinishedCallback.onAnimationFinished( + ANIMATION_TYPE_APP_TRANSITION, surfaceAnimator.getAnimation()); + + verify(container).onAnimationLeashLost(any()); + assertNull(surfaceAnimator.mLeash); + assertNull(surfaceAnimator.mSnapshot); + } + /* Used so we can gain access to some protected members of the {@link WindowContainer} class */ private static class TestWindowContainer extends WindowContainer { private final int mLayer;