From 5d49a4be93f64150fb5aee5cd4cdda41ffc4676f Mon Sep 17 00:00:00 2001 From: Pablo Gamito Date: Tue, 29 Mar 2022 13:43:20 +0000 Subject: [PATCH 1/2] Remove edge extension surface onAnimationLeashLost This ensures that we don't run into any problems with race conditions and concurrent access to the transaction like we would if we were to clean up the the animation finished callback. The problem was that we were removing the extensionSurfaces using the mFrameTransaction in the animation finish callback which is which is executed in the AnimationThread but mFrameTransaction is also used for applying the animations frame by frame for all animations going through the SurfaceAnimationRunner which run on the choreographer's looper thread. This means we can get in a case where two different threads try and access the mFrameTransaction which isn't supported (b/226317621). To address this we make sure that the extension clean up, which removes the edge extension surface, runs through the onAnimationLeashLost callback so that it is applied on the same transaction and in the same thread that the window being extended in the animation is reparented out of the animation leash avoiding any concurrent access to the transaction. Bug: 226553448 Test: atest CtsWindowManagerDeviceTestCases:AnimationEdgeExtensionTests Change-Id: I7e65277a2ccf316f3b81d5a5aaebf0c892592aa9 --- .../server/wm/SurfaceAnimationRunner.java | 88 ++++++++++++------- .../android/server/wm/WindowContainer.java | 1 + 2 files changed, 57 insertions(+), 32 deletions(-) diff --git a/services/core/java/com/android/server/wm/SurfaceAnimationRunner.java b/services/core/java/com/android/server/wm/SurfaceAnimationRunner.java index b57670914c118..65dca86d02599 100644 --- a/services/core/java/com/android/server/wm/SurfaceAnimationRunner.java +++ b/services/core/java/com/android/server/wm/SurfaceAnimationRunner.java @@ -50,7 +50,6 @@ import com.android.server.AnimationThread; import com.android.server.wm.LocalAnimationAdapter.AnimationSpec; import java.util.ArrayList; -import java.util.List; import java.util.function.Supplier; /** @@ -66,6 +65,12 @@ class SurfaceAnimationRunner { */ private final Object mCancelLock = new Object(); + /** + * Lock for synchronizing {@link #mEdgeExtensions} to prevent race conditions when managing + * created edge extension surfaces. + */ + private final Object mEdgeExtensionLock = new Object(); + @VisibleForTesting Choreographer mChoreographer; @@ -93,6 +98,11 @@ class SurfaceAnimationRunner { @GuardedBy("mLock") private boolean mAnimationStartDeferred; + // Mapping animation leashes to a list of edge extension surfaces associated with them + @GuardedBy("mEdgeExtensionLock") + private final ArrayMap> mEdgeExtensions = + new ArrayMap<>(); + /** * There should only ever be one instance of this class. Usual spot for it is with * {@link WindowManagerService} @@ -154,21 +164,22 @@ class SurfaceAnimationRunner { boolean requiresEdgeExtension = requiresEdgeExtension(a); if (requiresEdgeExtension) { + final ArrayList extensionSurfaces = new ArrayList<>(); + synchronized (mEdgeExtensionLock) { + mEdgeExtensions.put(animationLeash, extensionSurfaces); + } + mPreProcessingAnimations.put(animationLeash, runningAnim); // We must wait for t to be committed since otherwise the leash doesn't have the // windows we want to screenshot and extend as children. t.addTransactionCommittedListener(Runnable::run, () -> { final WindowAnimationSpec animationSpec = a.asWindowAnimationSpec(); - final Runnable cleanUpEdgeExtension = edgeExtendWindow(animationLeash, + + edgeExtendWindow(animationLeash, animationSpec.getRootTaskBounds(), animationSpec.getAnimation(), mFrameTransaction); - runningAnim.mFinishCallback = () -> { - cleanUpEdgeExtension.run(); - finishCallback.run(); - }; - synchronized (mLock) { // only run if animation is not yet canceled by this point if (mPreProcessingAnimations.get(animationLeash) == runningAnim) { @@ -320,7 +331,7 @@ class SurfaceAnimationRunner { mApplyScheduled = false; } - private Runnable edgeExtendWindow(SurfaceControl leash, Rect bounds, Animation a, + private void edgeExtendWindow(SurfaceControl leash, Rect bounds, Animation a, Transaction transaction) { final Transformation transformationAtStart = new Transformation(); a.getTransformationAt(0, transformationAtStart); @@ -335,17 +346,14 @@ class SurfaceAnimationRunner { final int targetSurfaceHeight = bounds.height(); final int targetSurfaceWidth = bounds.width(); - final List extensionSurfaces = new ArrayList<>(); - if (maxExtensionInsets.left < 0) { final Rect edgeBounds = new Rect(0, 0, 1, targetSurfaceHeight); final Rect extensionRect = new Rect(0, 0, -maxExtensionInsets.left, targetSurfaceHeight); final int xPos = maxExtensionInsets.left; final int yPos = 0; - final SurfaceControl extensionSurface = createExtensionSurface(leash, edgeBounds, + createExtensionSurface(leash, edgeBounds, extensionRect, xPos, yPos, "Left Edge Extension", transaction); - extensionSurfaces.add(extensionSurface); } if (maxExtensionInsets.top < 0) { @@ -354,9 +362,8 @@ class SurfaceAnimationRunner { targetSurfaceWidth, -maxExtensionInsets.top); final int xPos = 0; final int yPos = maxExtensionInsets.top; - final SurfaceControl extensionSurface = createExtensionSurface(leash, edgeBounds, + createExtensionSurface(leash, edgeBounds, extensionRect, xPos, yPos, "Top Edge Extension", transaction); - extensionSurfaces.add(extensionSurface); } if (maxExtensionInsets.right < 0) { @@ -366,9 +373,8 @@ class SurfaceAnimationRunner { -maxExtensionInsets.right, targetSurfaceHeight); final int xPos = targetSurfaceWidth; final int yPos = 0; - final SurfaceControl extensionSurface = createExtensionSurface(leash, edgeBounds, + createExtensionSurface(leash, edgeBounds, extensionRect, xPos, yPos, "Right Edge Extension", transaction); - extensionSurfaces.add(extensionSurface); } if (maxExtensionInsets.bottom < 0) { @@ -378,23 +384,25 @@ class SurfaceAnimationRunner { targetSurfaceWidth, -maxExtensionInsets.bottom); final int xPos = maxExtensionInsets.left; final int yPos = targetSurfaceHeight; - final SurfaceControl extensionSurface = createExtensionSurface(leash, edgeBounds, + createExtensionSurface(leash, edgeBounds, extensionRect, xPos, yPos, "Bottom Edge Extension", transaction); - extensionSurfaces.add(extensionSurface); } - - Runnable cleanUp = () -> { - for (final SurfaceControl extensionSurface : extensionSurfaces) { - if (extensionSurface != null) { - transaction.remove(extensionSurface); - } - } - }; - - return cleanUp; } - private SurfaceControl createExtensionSurface(SurfaceControl surfaceToExtend, Rect edgeBounds, + private void createExtensionSurface(SurfaceControl leash, Rect edgeBounds, + Rect extensionRect, int xPos, int yPos, String layerName, + Transaction startTransaction) { + synchronized (mEdgeExtensionLock) { + if (!mEdgeExtensions.containsKey(leash)) { + // Animation leash has already been removed so we shouldn't perform any extension + return; + } + createExtensionSurfaceLocked(leash, edgeBounds, extensionRect, xPos, yPos, layerName, + startTransaction); + } + } + + private void createExtensionSurfaceLocked(SurfaceControl surfaceToExtend, Rect edgeBounds, Rect extensionRect, int xPos, int yPos, String layerName, Transaction startTransaction) { final SurfaceControl edgeExtensionLayer = new SurfaceControl.Builder() @@ -420,7 +428,7 @@ class SurfaceAnimationRunner { if (edgeBuffer == null) { Log.e("SurfaceAnimationRunner", "Failed to create edge extension - " + "edge buffer is null"); - return null; + return; } android.graphics.BitmapShader shader = @@ -440,13 +448,13 @@ class SurfaceAnimationRunner { startTransaction.setPosition(edgeExtensionLayer, xPos, yPos); startTransaction.setVisibility(edgeExtensionLayer, true); - return edgeExtensionLayer; + mEdgeExtensions.get(surfaceToExtend).add(edgeExtensionLayer); } private static final class RunningAnimation { final AnimationSpec mAnimSpec; final SurfaceControl mLeash; - Runnable mFinishCallback; + final Runnable mFinishCallback; ValueAnimator mAnim; @GuardedBy("mCancelLock") @@ -459,6 +467,22 @@ class SurfaceAnimationRunner { } } + protected void onAnimationLeashLost(SurfaceControl animationLeash, + Transaction t) { + synchronized (mEdgeExtensionLock) { + if (!mEdgeExtensions.containsKey(animationLeash)) { + return; + } + + final ArrayList edgeExtensions = mEdgeExtensions.get(animationLeash); + for (int i = 0; i < edgeExtensions.size(); i++) { + final SurfaceControl extension = edgeExtensions.get(i); + t.remove(extension); + } + mEdgeExtensions.remove(animationLeash); + } + } + @VisibleForTesting interface AnimatorFactory { ValueAnimator makeAnimator(); diff --git a/services/core/java/com/android/server/wm/WindowContainer.java b/services/core/java/com/android/server/wm/WindowContainer.java index 99e39f1969e1e..214524c2f42cc 100644 --- a/services/core/java/com/android/server/wm/WindowContainer.java +++ b/services/core/java/com/android/server/wm/WindowContainer.java @@ -3179,6 +3179,7 @@ class WindowContainer extends ConfigurationContainer< @Override public void onAnimationLeashLost(Transaction t) { mLastLayer = -1; + mWmService.mSurfaceAnimationRunner.onAnimationLeashLost(mAnimationLeash, t); mAnimationLeash = null; reassignLayer(t); updateSurfacePosition(t); From 0c414c0740b78fde686fd35af7184e7232162998 Mon Sep 17 00:00:00 2001 From: Pablo Gamito Date: Tue, 29 Mar 2022 15:42:45 +0000 Subject: [PATCH 2/2] Revert "Revert "Use new T activity transitions in legacy"" This reverts commit 1250be73c8dd89fe5442df36f648d461606df4f0. Change-Id: Icb6902a2078feab30157ddaff8eb066133cb1384 --- .../internal/policy/TransitionAnimation.java | 25 ----------- .../res/anim/activity_close_enter_legacy.xml | 34 -------------- .../res/anim/activity_close_exit_legacy.xml | 44 ------------------ .../res/anim/activity_open_enter_legacy.xml | 42 ----------------- .../res/anim/activity_open_exit_legacy.xml | 45 ------------------- core/res/res/values/symbols.xml | 4 -- 6 files changed, 194 deletions(-) delete mode 100644 core/res/res/anim/activity_close_enter_legacy.xml delete mode 100644 core/res/res/anim/activity_close_exit_legacy.xml delete mode 100644 core/res/res/anim/activity_open_enter_legacy.xml delete mode 100644 core/res/res/anim/activity_open_exit_legacy.xml diff --git a/core/java/com/android/internal/policy/TransitionAnimation.java b/core/java/com/android/internal/policy/TransitionAnimation.java index e2d250589a8fb..fd8534d45b2b2 100644 --- a/core/java/com/android/internal/policy/TransitionAnimation.java +++ b/core/java/com/android/internal/policy/TransitionAnimation.java @@ -101,10 +101,6 @@ public class TransitionAnimation { private static final String DEFAULT_PACKAGE = "android"; - // TODO (b/215515255): remove once we full migrate to shell transitions - private static final boolean SHELL_TRANSITIONS_ENABLED = - SystemProperties.getBoolean("persist.wm.debug.shell_transit", false); - private final Context mContext; private final String mTag; @@ -259,9 +255,6 @@ public class TransitionAnimation { resId = ent.array.getResourceId(animAttr, 0); } } - if (!SHELL_TRANSITIONS_ENABLED) { - resId = updateToLegacyIfNeeded(resId); - } resId = updateToTranslucentAnimIfNeeded(resId, transit); if (ResourceId.isValid(resId)) { return loadAnimationSafely(context, resId, mTag); @@ -269,24 +262,6 @@ public class TransitionAnimation { return null; } - /** - * Replace animations that are not compatible with the legacy transition system with ones that - * are compatible with it. - * TODO (b/215515255): remove once we full migrate to shell transitions - */ - private int updateToLegacyIfNeeded(int anim) { - if (anim == R.anim.activity_open_enter) { - return R.anim.activity_open_enter_legacy; - } else if (anim == R.anim.activity_open_exit) { - return R.anim.activity_open_exit_legacy; - } else if (anim == R.anim.activity_close_enter) { - return R.anim.activity_close_enter_legacy; - } else if (anim == R.anim.activity_close_exit) { - return R.anim.activity_close_exit_legacy; - } - return anim; - } - /** Load animation by attribute Id from a specific AnimationStyle resource. */ @Nullable public Animation loadAnimationAttr(String packageName, int animStyleResId, int animAttr, diff --git a/core/res/res/anim/activity_close_enter_legacy.xml b/core/res/res/anim/activity_close_enter_legacy.xml deleted file mode 100644 index 9fa7c5498ea6c..0000000000000 --- a/core/res/res/anim/activity_close_enter_legacy.xml +++ /dev/null @@ -1,34 +0,0 @@ - - - - - - \ No newline at end of file diff --git a/core/res/res/anim/activity_close_exit_legacy.xml b/core/res/res/anim/activity_close_exit_legacy.xml deleted file mode 100644 index 1599ae8cb19fc..0000000000000 --- a/core/res/res/anim/activity_close_exit_legacy.xml +++ /dev/null @@ -1,44 +0,0 @@ - - - - - - - diff --git a/core/res/res/anim/activity_open_enter_legacy.xml b/core/res/res/anim/activity_open_enter_legacy.xml deleted file mode 100644 index 38d3e8ed06ce9..0000000000000 --- a/core/res/res/anim/activity_open_enter_legacy.xml +++ /dev/null @@ -1,42 +0,0 @@ - - - - - - diff --git a/core/res/res/anim/activity_open_exit_legacy.xml b/core/res/res/anim/activity_open_exit_legacy.xml deleted file mode 100644 index 3865d2149f42f..0000000000000 --- a/core/res/res/anim/activity_open_exit_legacy.xml +++ /dev/null @@ -1,45 +0,0 @@ - - - - - - - - - \ No newline at end of file diff --git a/core/res/res/values/symbols.xml b/core/res/res/values/symbols.xml index 94c9a122ee759..f946ba19b3c75 100644 --- a/core/res/res/values/symbols.xml +++ b/core/res/res/values/symbols.xml @@ -1715,10 +1715,6 @@ - - - -