From ed6794ece553f42e9f17099e9eaf7f62966f3ac6 Mon Sep 17 00:00:00 2001 From: Tiger Huang Date: Tue, 7 May 2019 20:07:59 +0800 Subject: Fix a bug about the z-order of layers caused by merging transactions Each WindowContainer has its own pending transaction. These transactions may be merged to the global one within WindowContainer.prepareSurfaces() in a hierarchy-based order, not in a time-based order. It may cause that a later surface operation overwritten by an earlier surface operation. For example, atokenA.setLayer(t1, 0) was called earlier, and then atokenA.setLayer(t2, 1) was called later. However, the layer of atokenA might eventually be 0 because t1 could be merged into the global transaction later. This CL uses a single transaction per display to solve this problem. Fix: 120282809 Test: atest SurfaceAnimatorTest Test: Manual test with the steps in the issue. Change-Id: Idca57d01d8be884369510642c2d9345b6ee4e3b1 --- core/java/android/view/SurfaceControl.java | 3 ++ .../java/com/android/server/wm/AppWindowToken.java | 12 +++---- .../java/com/android/server/wm/DisplayContent.java | 18 +++++++---- services/core/java/com/android/server/wm/Task.java | 1 + .../server/wm/TaskScreenshotAnimatable.java | 2 +- .../core/java/com/android/server/wm/TaskStack.java | 13 +------- .../com/android/server/wm/WindowContainer.java | 37 +++++++++++++++------- .../java/com/android/server/wm/WindowState.java | 3 +- .../java/com/android/server/wm/WindowToken.java | 8 ----- 9 files changed, 52 insertions(+), 45 deletions(-) diff --git a/core/java/android/view/SurfaceControl.java b/core/java/android/view/SurfaceControl.java index d67c8847f3bc..63e14853b51d 100644 --- a/core/java/android/view/SurfaceControl.java +++ b/core/java/android/view/SurfaceControl.java @@ -2660,6 +2660,9 @@ public final class SurfaceControl implements Parcelable { */ @NonNull public Transaction merge(@NonNull Transaction other) { + if (this == other) { + return this; + } mResizedSurfaces.putAll(other.mResizedSurfaces); other.mResizedSurfaces.clear(); nativeMergeTransaction(mNativeObject, other.mNativeObject); diff --git a/services/core/java/com/android/server/wm/AppWindowToken.java b/services/core/java/com/android/server/wm/AppWindowToken.java index fd90f0339e63..ecbecbafd3d5 100644 --- a/services/core/java/com/android/server/wm/AppWindowToken.java +++ b/services/core/java/com/android/server/wm/AppWindowToken.java @@ -2005,7 +2005,7 @@ class AppWindowToken extends WindowToken implements WindowManagerService.AppFree } layoutLetterbox(winHint); if (mLetterbox != null && mLetterbox.needsApplySurfaceChanges()) { - mLetterbox.applySurfaceChanges(mPendingTransaction); + mLetterbox.applySurfaceChanges(getPendingTransaction()); } } @@ -3059,13 +3059,13 @@ class AppWindowToken extends WindowToken implements WindowManagerService.AppFree if (mSurfaceControl != null) { if (show && !mLastSurfaceShowing) { - mPendingTransaction.show(mSurfaceControl); + getPendingTransaction().show(mSurfaceControl); } else if (!show && mLastSurfaceShowing) { - mPendingTransaction.hide(mSurfaceControl); + getPendingTransaction().hide(mSurfaceControl); } } if (mThumbnail != null) { - mThumbnail.setShowing(mPendingTransaction, show); + mThumbnail.setShowing(getPendingTransaction(), show); } mLastSurfaceShowing = show; super.prepareSurfaces(); @@ -3225,8 +3225,8 @@ class AppWindowToken extends WindowToken implements WindowManagerService.AppFree private void updateColorTransform() { if (mSurfaceControl != null && mLastAppSaturationInfo != null) { - mPendingTransaction.setColorTransform(mSurfaceControl, mLastAppSaturationInfo.mMatrix, - mLastAppSaturationInfo.mTranslation); + getPendingTransaction().setColorTransform(mSurfaceControl, + mLastAppSaturationInfo.mMatrix, mLastAppSaturationInfo.mTranslation); mWmService.scheduleAnimationLocked(); } } diff --git a/services/core/java/com/android/server/wm/DisplayContent.java b/services/core/java/com/android/server/wm/DisplayContent.java index 58a03b26b959..1e826ee96837 100644 --- a/services/core/java/com/android/server/wm/DisplayContent.java +++ b/services/core/java/com/android/server/wm/DisplayContent.java @@ -3340,7 +3340,7 @@ class DisplayContent extends WindowContainer implements ConfigurationConta setOrientation(SCREEN_ORIENTATION_UNSET); } + @Override DisplayContent getDisplayContent() { return mStack != null ? mStack.getDisplayContent() : null; } diff --git a/services/core/java/com/android/server/wm/TaskScreenshotAnimatable.java b/services/core/java/com/android/server/wm/TaskScreenshotAnimatable.java index 17e4b897c9da..ee4e462cb85e 100644 --- a/services/core/java/com/android/server/wm/TaskScreenshotAnimatable.java +++ b/services/core/java/com/android/server/wm/TaskScreenshotAnimatable.java @@ -77,7 +77,7 @@ class TaskScreenshotAnimatable implements SurfaceAnimator.Animatable { @Override public SurfaceControl.Transaction getPendingTransaction() { - return mTask.mPendingTransaction; + return mTask.getPendingTransaction(); } @Override diff --git a/services/core/java/com/android/server/wm/TaskStack.java b/services/core/java/com/android/server/wm/TaskStack.java index 757f6a1c2f94..ab5e071f572a 100644 --- a/services/core/java/com/android/server/wm/TaskStack.java +++ b/services/core/java/com/android/server/wm/TaskStack.java @@ -93,10 +93,6 @@ public class TaskStack extends WindowContainer implements /** Unique identifier */ final int mStackId; - /** The display this stack sits under. */ - // TODO: Track parent marks like this in WindowContainer. - private DisplayContent mDisplayContent; - /** For comparison with DisplayContent bounds. */ private Rect mTmpRect = new Rect(); private Rect mTmpRect2 = new Rect(); @@ -177,10 +173,6 @@ public class TaskStack extends WindowContainer implements EventLog.writeEvent(EventLogTags.WM_STACK_CREATED, stackId); } - DisplayContent getDisplayContent() { - return mDisplayContent; - } - Task findHomeTask() { if (!isActivityTypeHome() || mChildren.isEmpty()) { return null; @@ -825,8 +817,7 @@ public class TaskStack extends WindowContainer implements throw new IllegalStateException("onDisplayChanged: Already attached"); } - final boolean movedToNewDisplay = mDisplayContent == null; - mDisplayContent = dc; + super.onDisplayChanged(dc); updateSurfaceBounds(); if (mAnimationBackgroundSurface == null) { @@ -834,8 +825,6 @@ public class TaskStack extends WindowContainer implements .setName("animation background stackId=" + mStackId) .build(); } - - super.onDisplayChanged(dc); } /** diff --git a/services/core/java/com/android/server/wm/WindowContainer.java b/services/core/java/com/android/server/wm/WindowContainer.java index d5c3e4f34c26..bbef261d17bb 100644 --- a/services/core/java/com/android/server/wm/WindowContainer.java +++ b/services/core/java/com/android/server/wm/WindowContainer.java @@ -109,14 +109,19 @@ class WindowContainer extends ConfigurationContainer< // The owner/creator for this container. No controller if null. WindowContainerController mController; + // The display this window container is on. + protected DisplayContent mDisplayContent; + protected SurfaceControl mSurfaceControl; private int mLastLayer = 0; private SurfaceControl mLastRelativeToLayer = null; + // TODO(b/132320879): Remove this from WindowContainers except DisplayContent. + private final Transaction mPendingTransaction; + /** * Applied as part of the animation pass in "prepareSurfaces". */ - protected final Transaction mPendingTransaction; protected final SurfaceAnimator mSurfaceAnimator; protected final WindowManagerService mWmService; @@ -320,12 +325,12 @@ class WindowContainer extends ConfigurationContainer< } if (mSurfaceControl != null) { - mPendingTransaction.remove(mSurfaceControl); + getPendingTransaction().remove(mSurfaceControl); // Merge to parent transaction to ensure the transactions on this WindowContainer are // applied in native even if WindowContainer is removed. if (mParent != null) { - mParent.getPendingTransaction().merge(mPendingTransaction); + mParent.getPendingTransaction().merge(getPendingTransaction()); } mSurfaceControl = null; @@ -508,12 +513,20 @@ class WindowContainer extends ConfigurationContainer< * @param dc The display this container is on after changes. */ void onDisplayChanged(DisplayContent dc) { + mDisplayContent = dc; + if (dc != null && dc != this) { + dc.getPendingTransaction().merge(mPendingTransaction); + } for (int i = mChildren.size() - 1; i >= 0; --i) { final WindowContainer child = mChildren.get(i); child.onDisplayChanged(dc); } } + DisplayContent getDisplayContent() { + return mDisplayContent; + } + void setWaitingForDrawnIfResizingChanged() { for (int i = mChildren.size() - 1; i >= 0; --i) { final WindowContainer wc = mChildren.get(i); @@ -1180,13 +1193,7 @@ class WindowContainer extends ConfigurationContainer< } } - /** - * TODO: Once we totally eliminate global transaction we will pass transaction in here - * rather than merging to global. - */ void prepareSurfaces() { - SurfaceControl.mergeToGlobalTransaction(getPendingTransaction()); - // If a leash has been set when the transaction was committed, then the leash reparent has // been committed. mCommittedReparentToAnimationLeash = mSurfaceAnimator.hasLeash(); @@ -1204,8 +1211,8 @@ class WindowContainer extends ConfigurationContainer< } /** - * Trigger a call to prepareSurfaces from the animation thread, such that - * mPendingTransaction will be applied. + * Trigger a call to prepareSurfaces from the animation thread, such that pending transactions + * will be applied. */ void scheduleAnimation() { if (mParent != null) { @@ -1224,6 +1231,14 @@ class WindowContainer extends ConfigurationContainer< @Override public Transaction getPendingTransaction() { + final DisplayContent displayContent = getDisplayContent(); + if (displayContent != null && displayContent != this) { + return displayContent.getPendingTransaction(); + } + // This WindowContainer has not attached to a display yet or this is a DisplayContent, so we + // let the caller to save the surface operations within the local mPendingTransaction. + // If this is not a DisplayContent, we will merge it to the pending transaction of its + // display once it attaches to it. return mPendingTransaction; } diff --git a/services/core/java/com/android/server/wm/WindowState.java b/services/core/java/com/android/server/wm/WindowState.java index dd3c6004dcad..5ef184adc52f 100644 --- a/services/core/java/com/android/server/wm/WindowState.java +++ b/services/core/java/com/android/server/wm/WindowState.java @@ -1313,6 +1313,7 @@ class WindowState extends WindowContainer implements WindowManagerP mOrientationChangeTimedOut = true; } + @Override DisplayContent getDisplayContent() { return mToken.getDisplayContent(); } @@ -4602,7 +4603,7 @@ class WindowState extends WindowContainer implements WindowManagerP new WindowAnimationSpec(anim, mSurfacePosition, false /* canSkipFirstFrame */, 0 /* windowCornerRadius */), mWmService.mSurfaceAnimationRunner); - startAnimation(mPendingTransaction, adapter); + startAnimation(getPendingTransaction(), adapter); commitPendingTransaction(); } diff --git a/services/core/java/com/android/server/wm/WindowToken.java b/services/core/java/com/android/server/wm/WindowToken.java index f0b9c62f2843..8aee0f2a8308 100644 --- a/services/core/java/com/android/server/wm/WindowToken.java +++ b/services/core/java/com/android/server/wm/WindowToken.java @@ -82,9 +82,6 @@ class WindowToken extends WindowContainer { // windows will be put to the bottom of the list. boolean sendingToBottom; - // The display this token is on. - protected DisplayContent mDisplayContent; - /** The owner has {@link android.Manifest.permission#MANAGE_APP_TOKENS} */ final boolean mOwnerCanManageAppTokens; @@ -249,10 +246,6 @@ class WindowToken extends WindowContainer { return null; } - DisplayContent getDisplayContent() { - return mDisplayContent; - } - @Override void removeImmediately() { if (mDisplayContent != null) { @@ -266,7 +259,6 @@ class WindowToken extends WindowContainer { @Override void onDisplayChanged(DisplayContent dc) { dc.reParentWindowToken(this); - mDisplayContent = dc; // TODO(b/36740756): One day this should perhaps be hooked // up with goodToGo, so we don't move a window -- cgit v1.2.3-59-g8ed1b