From e47364890d5d5fd4bd8722c99e997df907f4739d Mon Sep 17 00:00:00 2001 From: Evan Rosky Date: Wed, 18 Jan 2023 20:49:02 -0800 Subject: [PATCH] Start animations in default handler after start transaction This was basically posting animation start onto anim executor which is a different thread. This caused a race where the animation could start (and apply some surface changes) before the transition startTransaction which could cause flicker in the corners (outside the rounded corner) since the animation background surface wasn't shown yet. Bug: 265007895 Test: flicker TaskTransitionTest Change-Id: I717a4a1e8be1ace1bbbed32b358290d2c59650b6 --- .../transition/DefaultTransitionHandler.java | 70 ++++++++----------- .../transition/ScreenRotationAnimation.java | 47 ++++++------- 2 files changed, 52 insertions(+), 65 deletions(-) diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/transition/DefaultTransitionHandler.java b/libs/WindowManager/Shell/src/com/android/wm/shell/transition/DefaultTransitionHandler.java index 618c4465db3ba..36ce0a69906aa 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/transition/DefaultTransitionHandler.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/transition/DefaultTransitionHandler.java @@ -445,7 +445,6 @@ public class DefaultTransitionHandler implements Transitions.TransitionHandler { backgroundColorForTransition = getTransitionBackgroundColorIfSet(info, change, a, backgroundColorForTransition); - boolean delayedEdgeExtension = false; if (!isTask && a.hasExtension()) { if (!Transitions.isOpeningType(change.getMode())) { // Can screenshot now (before startTransaction is applied) @@ -455,7 +454,6 @@ public class DefaultTransitionHandler implements Transitions.TransitionHandler { // may not be visible or ready yet. postStartTransactionCallbacks .add(t -> edgeExtendWindow(change, a, t, finishTransaction)); - delayedEdgeExtension = true; } } @@ -464,19 +462,9 @@ public class DefaultTransitionHandler implements Transitions.TransitionHandler { : new Rect(change.getEndAbsBounds()); clipRect.offsetTo(0, 0); - if (delayedEdgeExtension) { - // If the edge extension needs to happen after the startTransition has been - // applied, then we want to only start the animation after the edge extension - // postStartTransaction callback has been run - postStartTransactionCallbacks.add(t -> - startSurfaceAnimation(animations, a, change.getLeash(), onAnimFinish, - mTransactionPool, mMainExecutor, mAnimExecutor, - change.getEndRelOffset(), cornerRadius, clipRect)); - } else { - startSurfaceAnimation(animations, a, change.getLeash(), onAnimFinish, - mTransactionPool, mMainExecutor, mAnimExecutor, - change.getEndRelOffset(), cornerRadius, clipRect); - } + buildSurfaceAnimation(animations, a, change.getLeash(), onAnimFinish, + mTransactionPool, mMainExecutor, change.getEndRelOffset(), cornerRadius, + clipRect); if (info.getAnimationOptions() != null) { attachThumbnail(animations, onAnimFinish, change, info.getAnimationOptions(), @@ -490,19 +478,25 @@ public class DefaultTransitionHandler implements Transitions.TransitionHandler { startTransaction, finishTransaction); } - // postStartTransactionCallbacks require that the start transaction is already - // applied to run otherwise they may result in flickers and UI inconsistencies. - boolean waitForStartTransactionApply = postStartTransactionCallbacks.size() > 0; - startTransaction.apply(waitForStartTransactionApply); - - // Run tasks that require startTransaction to already be applied - for (Consumer postStartTransactionCallback : - postStartTransactionCallbacks) { - final SurfaceControl.Transaction t = mTransactionPool.acquire(); - postStartTransactionCallback.accept(t); - t.apply(); - mTransactionPool.release(t); + if (postStartTransactionCallbacks.size() > 0) { + // postStartTransactionCallbacks require that the start transaction is already + // applied to run otherwise they may result in flickers and UI inconsistencies. + startTransaction.apply(true /* sync */); + // startTransaction is empty now, so fill it with the edge-extension setup + for (Consumer postStartTransactionCallback : + postStartTransactionCallbacks) { + postStartTransactionCallback.accept(startTransaction); + } } + startTransaction.apply(); + + // now start animations. they are started on another thread, so we have to post them + // *after* applying the startTransaction + mAnimExecutor.execute(() -> { + for (int i = 0; i < animations.size(); ++i) { + animations.get(i).start(); + } + }); mRotator.cleanUp(finishTransaction); TransitionMetrics.getInstance().reportAnimationStart(transition); @@ -539,8 +533,8 @@ public class DefaultTransitionHandler implements Transitions.TransitionHandler { animations.removeAll(animGroupStore); onAnimFinish.run(); }; - anim.startAnimation(animGroup, finishCallback, mTransitionAnimationScaleSetting, - mMainExecutor, mAnimExecutor); + anim.buildAnimation(animGroup, finishCallback, mTransitionAnimationScaleSetting, + mMainExecutor); for (int i = animGroup.size() - 1; i >= 0; i--) { final Animator animator = animGroup.get(i); animGroupStore.add(animator); @@ -633,11 +627,12 @@ public class DefaultTransitionHandler implements Transitions.TransitionHandler { return a; } - static void startSurfaceAnimation(@NonNull ArrayList animations, + /** Builds an animator for the surface and adds it to the `animations` list. */ + static void buildSurfaceAnimation(@NonNull ArrayList animations, @NonNull Animation anim, @NonNull SurfaceControl leash, @NonNull Runnable finishCallback, @NonNull TransactionPool pool, - @NonNull ShellExecutor mainExecutor, @NonNull ShellExecutor animExecutor, - @Nullable Point position, float cornerRadius, @Nullable Rect clipRect) { + @NonNull ShellExecutor mainExecutor, @Nullable Point position, float cornerRadius, + @Nullable Rect clipRect) { final SurfaceControl.Transaction transaction = pool.acquire(); final ValueAnimator va = ValueAnimator.ofFloat(0f, 1f); final Transformation transformation = new Transformation(); @@ -691,7 +686,6 @@ public class DefaultTransitionHandler implements Transitions.TransitionHandler { } }); animations.add(va); - animExecutor.execute(va::start); } private void attachThumbnail(@NonNull ArrayList animations, @@ -745,9 +739,8 @@ public class DefaultTransitionHandler implements Transitions.TransitionHandler { }; a.restrictDuration(MAX_ANIMATION_DURATION); a.scaleCurrentDuration(mTransitionAnimationScaleSetting); - startSurfaceAnimation(animations, a, wt.getSurface(), finisher, mTransactionPool, - mMainExecutor, mAnimExecutor, change.getEndRelOffset(), - cornerRadius, change.getEndAbsBounds()); + buildSurfaceAnimation(animations, a, wt.getSurface(), finisher, mTransactionPool, + mMainExecutor, change.getEndRelOffset(), cornerRadius, change.getEndAbsBounds()); } private void attachThumbnailAnimation(@NonNull ArrayList animations, @@ -770,9 +763,8 @@ public class DefaultTransitionHandler implements Transitions.TransitionHandler { }; a.restrictDuration(MAX_ANIMATION_DURATION); a.scaleCurrentDuration(mTransitionAnimationScaleSetting); - startSurfaceAnimation(animations, a, wt.getSurface(), finisher, mTransactionPool, - mMainExecutor, mAnimExecutor, change.getEndRelOffset(), - cornerRadius, change.getEndAbsBounds()); + buildSurfaceAnimation(animations, a, wt.getSurface(), finisher, mTransactionPool, + mMainExecutor, change.getEndRelOffset(), cornerRadius, change.getEndAbsBounds()); } private static int getWallpaperTransitType(TransitionInfo info) { diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/transition/ScreenRotationAnimation.java b/libs/WindowManager/Shell/src/com/android/wm/shell/transition/ScreenRotationAnimation.java index 665267fde6ff9..e643170273dee 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/transition/ScreenRotationAnimation.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/transition/ScreenRotationAnimation.java @@ -21,7 +21,7 @@ import static android.view.WindowManager.LayoutParams.ROTATION_ANIMATION_CROSSFA import static android.view.WindowManager.LayoutParams.ROTATION_ANIMATION_JUMPCUT; import static android.view.WindowManagerPolicyConstants.SCREEN_FREEZE_LAYER_BASE; -import static com.android.wm.shell.transition.DefaultTransitionHandler.startSurfaceAnimation; +import static com.android.wm.shell.transition.DefaultTransitionHandler.buildSurfaceAnimation; import static com.android.wm.shell.transition.Transitions.TAG; import android.animation.Animator; @@ -244,11 +244,11 @@ class ScreenRotationAnimation { } /** - * Returns true if animating. + * Returns true if any animations were added to `animations`. */ - public boolean startAnimation(@NonNull ArrayList animations, + boolean buildAnimation(@NonNull ArrayList animations, @NonNull Runnable finishCallback, float animationScale, - @NonNull ShellExecutor mainExecutor, @NonNull ShellExecutor animExecutor) { + @NonNull ShellExecutor mainExecutor) { if (mScreenshotLayer == null) { // Can't do animation. return false; @@ -311,13 +311,11 @@ class ScreenRotationAnimation { mRotateAlphaAnimation.restrictDuration(MAX_ANIMATION_DURATION); mRotateAlphaAnimation.scaleCurrentDuration(animationScale); - startScreenshotAlphaAnimation(animations, finishCallback, mainExecutor, - animExecutor); - startDisplayRotation(animations, finishCallback, mainExecutor, animExecutor); + buildScreenshotAlphaAnimation(animations, finishCallback, mainExecutor); + startDisplayRotation(animations, finishCallback, mainExecutor); } else { - startDisplayRotation(animations, finishCallback, mainExecutor, animExecutor); - startScreenshotRotationAnimation(animations, finishCallback, mainExecutor, - animExecutor); + startDisplayRotation(animations, finishCallback, mainExecutor); + startScreenshotRotationAnimation(animations, finishCallback, mainExecutor); //startColorAnimation(mTransaction, animationScale); } @@ -325,27 +323,24 @@ class ScreenRotationAnimation { } private void startDisplayRotation(@NonNull ArrayList animations, - @NonNull Runnable finishCallback, @NonNull ShellExecutor mainExecutor, - @NonNull ShellExecutor animExecutor) { - startSurfaceAnimation(animations, mRotateEnterAnimation, mSurfaceControl, finishCallback, - mTransactionPool, mainExecutor, animExecutor, null /* position */, - 0 /* cornerRadius */, null /* clipRect */); + @NonNull Runnable finishCallback, @NonNull ShellExecutor mainExecutor) { + buildSurfaceAnimation(animations, mRotateEnterAnimation, mSurfaceControl, finishCallback, + mTransactionPool, mainExecutor, null /* position */, 0 /* cornerRadius */, + null /* clipRect */); } private void startScreenshotRotationAnimation(@NonNull ArrayList animations, - @NonNull Runnable finishCallback, @NonNull ShellExecutor mainExecutor, - @NonNull ShellExecutor animExecutor) { - startSurfaceAnimation(animations, mRotateExitAnimation, mAnimLeash, finishCallback, - mTransactionPool, mainExecutor, animExecutor, null /* position */, - 0 /* cornerRadius */, null /* clipRect */); + @NonNull Runnable finishCallback, @NonNull ShellExecutor mainExecutor) { + buildSurfaceAnimation(animations, mRotateExitAnimation, mAnimLeash, finishCallback, + mTransactionPool, mainExecutor, null /* position */, 0 /* cornerRadius */, + null /* clipRect */); } - private void startScreenshotAlphaAnimation(@NonNull ArrayList animations, - @NonNull Runnable finishCallback, @NonNull ShellExecutor mainExecutor, - @NonNull ShellExecutor animExecutor) { - startSurfaceAnimation(animations, mRotateAlphaAnimation, mAnimLeash, finishCallback, - mTransactionPool, mainExecutor, animExecutor, null /* position */, - 0 /* cornerRadius */, null /* clipRect */); + private void buildScreenshotAlphaAnimation(@NonNull ArrayList animations, + @NonNull Runnable finishCallback, @NonNull ShellExecutor mainExecutor) { + buildSurfaceAnimation(animations, mRotateAlphaAnimation, mAnimLeash, finishCallback, + mTransactionPool, mainExecutor, null /* position */, 0 /* cornerRadius */, + null /* clipRect */); } private void startColorAnimation(float animationScale, @NonNull ShellExecutor animExecutor) {