From ab76bbc21160b3030df7532884ffc52672d48219 Mon Sep 17 00:00:00 2001 From: Winson Chung Date: Mon, 14 Aug 2017 13:33:51 -0700 Subject: [PATCH] Ensure that PiP mode changed callback if animation is interrupted - If a PiP enter animation callback is interrupted, the activity is never actually put into PiP mode, and will never receive onPictureInPictureModeChanged(false) even if enterPictureInPictureMode() returns true due to the change being deduped (it was never in that mode). In this specific case, force a callback to be made to the app so that it has a signal that it is no longer in PiP mode. Bug: 63749396 Test: bit FrameworksServicesTests:com.android.server.wm.BoundsAnimationControllerTests Test: android.server.cts.ActivityManagerPinnedStackTests Test: #testEnterPipInterruptedCallbacks Change-Id: I301c70e4fb0f2175dd6d7b5feae065b41df2878d --- .../com/android/server/am/ActivityRecord.java | 7 ++- .../server/am/ActivityStackSupervisor.java | 43 +++++++++---------- .../server/am/PinnedActivityStack.java | 7 +-- .../server/wm/BoundsAnimationController.java | 41 +++++++++++------- .../server/wm/BoundsAnimationTarget.java | 2 +- .../wm/PinnedStackWindowController.java | 6 ++- .../server/wm/PinnedStackWindowListener.java | 3 +- .../java/com/android/server/wm/TaskStack.java | 12 +++--- .../wm/BoundsAnimationControllerTests.java | 32 ++++++++++---- 9 files changed, 90 insertions(+), 63 deletions(-) diff --git a/services/core/java/com/android/server/am/ActivityRecord.java b/services/core/java/com/android/server/am/ActivityRecord.java index 43e26506f202a..4fd27196309e1 100644 --- a/services/core/java/com/android/server/am/ActivityRecord.java +++ b/services/core/java/com/android/server/am/ActivityRecord.java @@ -659,14 +659,14 @@ final class ActivityRecord extends ConfigurationContainer implements AppWindowCo } } - void updatePictureInPictureMode(Rect targetStackBounds) { + void updatePictureInPictureMode(Rect targetStackBounds, boolean forceUpdate) { if (task == null || task.getStack() == null || app == null || app.thread == null) { return; } final boolean inPictureInPictureMode = (task.getStackId() == PINNED_STACK_ID) && (targetStackBounds != null); - if (inPictureInPictureMode != mLastReportedPictureInPictureMode) { + if (inPictureInPictureMode != mLastReportedPictureInPictureMode || forceUpdate) { // Picture-in-picture mode changes also trigger a multi-window mode change as well, so // update that here in order mLastReportedPictureInPictureMode = inPictureInPictureMode; @@ -681,8 +681,7 @@ final class ActivityRecord extends ConfigurationContainer implements AppWindowCo private void schedulePictureInPictureModeChanged(Configuration overrideConfig) { try { app.thread.schedulePictureInPictureModeChanged(appToken, - mLastReportedPictureInPictureMode, - overrideConfig); + mLastReportedPictureInPictureMode, overrideConfig); } catch (Exception e) { // If process died, no one cares. } diff --git a/services/core/java/com/android/server/am/ActivityStackSupervisor.java b/services/core/java/com/android/server/am/ActivityStackSupervisor.java index 54bb526b745bf..a5d21fa3124f1 100644 --- a/services/core/java/com/android/server/am/ActivityStackSupervisor.java +++ b/services/core/java/com/android/server/am/ActivityStackSupervisor.java @@ -4411,31 +4411,29 @@ public class ActivityStackSupervisor extends ConfigurationContainer implements D return; } - scheduleUpdatePictureInPictureModeIfNeeded(task, stack.mBounds, false /* immediate */); + scheduleUpdatePictureInPictureModeIfNeeded(task, stack.mBounds); } - void scheduleUpdatePictureInPictureModeIfNeeded(TaskRecord task, Rect targetStackBounds, - boolean immediate) { - - if (immediate) { - mHandler.removeMessages(REPORT_PIP_MODE_CHANGED_MSG); - for (int i = task.mActivities.size() - 1; i >= 0; i--) { - final ActivityRecord r = task.mActivities.get(i); - if (r.app != null && r.app.thread != null) { - r.updatePictureInPictureMode(targetStackBounds); - } + void scheduleUpdatePictureInPictureModeIfNeeded(TaskRecord task, Rect targetStackBounds) { + for (int i = task.mActivities.size() - 1; i >= 0; i--) { + final ActivityRecord r = task.mActivities.get(i); + if (r.app != null && r.app.thread != null) { + mPipModeChangedActivities.add(r); } - } else { - for (int i = task.mActivities.size() - 1; i >= 0; i--) { - final ActivityRecord r = task.mActivities.get(i); - if (r.app != null && r.app.thread != null) { - mPipModeChangedActivities.add(r); - } - } - mPipModeChangedTargetStackBounds = targetStackBounds; + } + mPipModeChangedTargetStackBounds = targetStackBounds; - if (!mHandler.hasMessages(REPORT_PIP_MODE_CHANGED_MSG)) { - mHandler.sendEmptyMessage(REPORT_PIP_MODE_CHANGED_MSG); + if (!mHandler.hasMessages(REPORT_PIP_MODE_CHANGED_MSG)) { + mHandler.sendEmptyMessage(REPORT_PIP_MODE_CHANGED_MSG); + } + } + + void updatePictureInPictureMode(TaskRecord task, Rect targetStackBounds, boolean forceUpdate) { + mHandler.removeMessages(REPORT_PIP_MODE_CHANGED_MSG); + for (int i = task.mActivities.size() - 1; i >= 0; i--) { + final ActivityRecord r = task.mActivities.get(i); + if (r.app != null && r.app.thread != null) { + r.updatePictureInPictureMode(targetStackBounds, forceUpdate); } } } @@ -4497,7 +4495,8 @@ public class ActivityStackSupervisor extends ConfigurationContainer implements D synchronized (mService) { for (int i = mPipModeChangedActivities.size() - 1; i >= 0; i--) { final ActivityRecord r = mPipModeChangedActivities.remove(i); - r.updatePictureInPictureMode(mPipModeChangedTargetStackBounds); + r.updatePictureInPictureMode(mPipModeChangedTargetStackBounds, + false /* forceUpdate */); } } } break; diff --git a/services/core/java/com/android/server/am/PinnedActivityStack.java b/services/core/java/com/android/server/am/PinnedActivityStack.java index 392fbb29d0913..a601ee1c0c89c 100644 --- a/services/core/java/com/android/server/am/PinnedActivityStack.java +++ b/services/core/java/com/android/server/am/PinnedActivityStack.java @@ -91,15 +91,16 @@ class PinnedActivityStack extends ActivityStack return mWindowContainerController.deferScheduleMultiWindowModeChanged(); } - public void updatePictureInPictureModeForPinnedStackAnimation(Rect targetStackBounds) { + public void updatePictureInPictureModeForPinnedStackAnimation(Rect targetStackBounds, + boolean forceUpdate) { // It is guaranteed that the activities requiring the update will be in the pinned stack at // this point (either reparented before the animation into PiP, or before reparenting after // the animation out of PiP) synchronized(this) { ArrayList tasks = getAllTasks(); for (int i = 0; i < tasks.size(); i++ ) { - mStackSupervisor.scheduleUpdatePictureInPictureModeIfNeeded(tasks.get(i), - targetStackBounds, true /* immediate */); + mStackSupervisor.updatePictureInPictureMode(tasks.get(i), targetStackBounds, + forceUpdate); } } } diff --git a/services/core/java/com/android/server/wm/BoundsAnimationController.java b/services/core/java/com/android/server/wm/BoundsAnimationController.java index cff2fadd76491..7953ee4309349 100644 --- a/services/core/java/com/android/server/wm/BoundsAnimationController.java +++ b/services/core/java/com/android/server/wm/BoundsAnimationController.java @@ -21,7 +21,6 @@ import static com.android.server.wm.WindowManagerDebugConfig.TAG_WITH_CLASS_NAME import static com.android.server.wm.WindowManagerDebugConfig.TAG_WM; import android.animation.AnimationHandler; -import android.animation.AnimationHandler.AnimationFrameCallbackProvider; import android.animation.Animator; import android.animation.ValueAnimator; import android.annotation.IntDef; @@ -32,13 +31,11 @@ import android.os.IBinder; import android.os.Debug; import android.util.ArrayMap; import android.util.Slog; -import android.view.Choreographer; import android.view.animation.AnimationUtils; import android.view.animation.Interpolator; import android.view.WindowManagerInternal; import com.android.internal.annotations.VisibleForTesting; -import com.android.internal.graphics.SfVsyncFrameCallbackProvider; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; @@ -142,9 +139,6 @@ public class BoundsAnimationController { // True if this this animation was canceled and will be replaced the another animation from // the same {@link #BoundsAnimationTarget} target. private boolean mSkipFinalResize; - // True if this animation replaced a previous animation of the same - // {@link #BoundsAnimationTarget} target. - private final boolean mSkipAnimationStart; // True if this animation was canceled by the user, not as a part of a replacing animation private boolean mSkipAnimationEnd; @@ -159,6 +153,7 @@ public class BoundsAnimationController { // Whether to schedule PiP mode changes on animation start/end private @SchedulePipModeChangedState int mSchedulePipModeChangedState; + private @SchedulePipModeChangedState int mPrevSchedulePipModeChangedState; // Depending on whether we are animating from // a smaller to a larger size @@ -171,14 +166,14 @@ public class BoundsAnimationController { BoundsAnimator(BoundsAnimationTarget target, Rect from, Rect to, @SchedulePipModeChangedState int schedulePipModeChangedState, - boolean moveFromFullscreen, boolean moveToFullscreen, - boolean replacingExistingAnimation) { + @SchedulePipModeChangedState int prevShedulePipModeChangedState, + boolean moveFromFullscreen, boolean moveToFullscreen) { super(); mTarget = target; mFrom.set(from); mTo.set(to); - mSkipAnimationStart = replacingExistingAnimation; mSchedulePipModeChangedState = schedulePipModeChangedState; + mPrevSchedulePipModeChangedState = prevShedulePipModeChangedState; mMoveFromFullscreen = moveFromFullscreen; mMoveToFullscreen = moveToFullscreen; addUpdateListener(this); @@ -200,7 +195,7 @@ public class BoundsAnimationController { @Override public void onAnimationStart(Animator animation) { if (DEBUG) Slog.d(TAG, "onAnimationStart: mTarget=" + mTarget - + " mSkipAnimationStart=" + mSkipAnimationStart + + " mPrevSchedulePipModeChangedState=" + mPrevSchedulePipModeChangedState + " mSchedulePipModeChangedState=" + mSchedulePipModeChangedState); mFinishAnimationAfterTransition = false; mTmpRect.set(mFrom.left, mFrom.top, mFrom.left + mFrozenTaskWidth, @@ -210,18 +205,26 @@ public class BoundsAnimationController { // running updateBooster(); - // Ensure that we have prepared the target for animation before - // we trigger any size changes, so it can swap surfaces - // in to appropriate modes, or do as it wishes otherwise. - if (!mSkipAnimationStart) { + // Ensure that we have prepared the target for animation before we trigger any size + // changes, so it can swap surfaces in to appropriate modes, or do as it wishes + // otherwise. + if (mPrevSchedulePipModeChangedState == NO_PIP_MODE_CHANGED_CALLBACKS) { mTarget.onAnimationStart(mSchedulePipModeChangedState == - SCHEDULE_PIP_MODE_CHANGED_ON_START); + SCHEDULE_PIP_MODE_CHANGED_ON_START, false /* forceUpdate */); // When starting an animation from fullscreen, pause here and wait for the // windows-drawn signal before we start the rest of the transition down into PiP. if (mMoveFromFullscreen) { pause(); } + } else if (mPrevSchedulePipModeChangedState == SCHEDULE_PIP_MODE_CHANGED_ON_END && + mSchedulePipModeChangedState == SCHEDULE_PIP_MODE_CHANGED_ON_START) { + // We are replacing a running animation into PiP, but since it hasn't completed, the + // client will not currently receive any picture-in-picture mode change callbacks. + // However, we still need to report to them that they are leaving PiP, so this will + // force an update via a mode changed callback. + mTarget.onAnimationStart(true /* schedulePipModeChangedCallback */, + true /* forceUpdate */); } // Immediately update the task bounds if they have to become larger, but preserve @@ -388,6 +391,8 @@ public class BoundsAnimationController { boolean moveFromFullscreen, boolean moveToFullscreen) { final BoundsAnimator existing = mRunningAnimations.get(target); final boolean replacing = existing != null; + @SchedulePipModeChangedState int prevSchedulePipModeChangedState = + NO_PIP_MODE_CHANGED_CALLBACKS; if (DEBUG) Slog.d(TAG, "animateBounds: target=" + target + " from=" + from + " to=" + to + " schedulePipModeChangedState=" + schedulePipModeChangedState @@ -403,6 +408,9 @@ public class BoundsAnimationController { return existing; } + // Save the previous state + prevSchedulePipModeChangedState = existing.mSchedulePipModeChangedState; + // Update the PiP callback states if we are replacing the animation if (existing.mSchedulePipModeChangedState == SCHEDULE_PIP_MODE_CHANGED_ON_START) { if (schedulePipModeChangedState == SCHEDULE_PIP_MODE_CHANGED_ON_START) { @@ -428,7 +436,8 @@ public class BoundsAnimationController { existing.cancel(); } final BoundsAnimator animator = new BoundsAnimator(target, from, to, - schedulePipModeChangedState, moveFromFullscreen, moveToFullscreen, replacing); + schedulePipModeChangedState, prevSchedulePipModeChangedState, + moveFromFullscreen, moveToFullscreen); mRunningAnimations.put(target, animator); animator.setFloatValues(0f, 1f); animator.setDuration((animationDuration != -1 ? animationDuration diff --git a/services/core/java/com/android/server/wm/BoundsAnimationTarget.java b/services/core/java/com/android/server/wm/BoundsAnimationTarget.java index 8b1bf7bf77dc2..647a2d6deac62 100644 --- a/services/core/java/com/android/server/wm/BoundsAnimationTarget.java +++ b/services/core/java/com/android/server/wm/BoundsAnimationTarget.java @@ -31,7 +31,7 @@ interface BoundsAnimationTarget { * @param schedulePipModeChangedCallback whether or not to schedule the PiP mode changed * callbacks */ - void onAnimationStart(boolean schedulePipModeChangedCallback); + void onAnimationStart(boolean schedulePipModeChangedCallback, boolean forceUpdate); /** * Sets the size of the target (without any intermediate steps, like scheduling animation) diff --git a/services/core/java/com/android/server/wm/PinnedStackWindowController.java b/services/core/java/com/android/server/wm/PinnedStackWindowController.java index 989e8f23693a6..fc0baaded1e5d 100644 --- a/services/core/java/com/android/server/wm/PinnedStackWindowController.java +++ b/services/core/java/com/android/server/wm/PinnedStackWindowController.java @@ -202,10 +202,12 @@ public class PinnedStackWindowController extends StackWindowController { */ /** Calls directly into activity manager so window manager lock shouldn't held. */ - public void updatePictureInPictureModeForPinnedStackAnimation(Rect targetStackBounds) { + public void updatePictureInPictureModeForPinnedStackAnimation(Rect targetStackBounds, + boolean forceUpdate) { if (mListener != null) { PinnedStackWindowListener listener = (PinnedStackWindowListener) mListener; - listener.updatePictureInPictureModeForPinnedStackAnimation(targetStackBounds); + listener.updatePictureInPictureModeForPinnedStackAnimation(targetStackBounds, + forceUpdate); } } } diff --git a/services/core/java/com/android/server/wm/PinnedStackWindowListener.java b/services/core/java/com/android/server/wm/PinnedStackWindowListener.java index 12b9c1f0c5521..33e8a60329bf6 100644 --- a/services/core/java/com/android/server/wm/PinnedStackWindowListener.java +++ b/services/core/java/com/android/server/wm/PinnedStackWindowListener.java @@ -28,5 +28,6 @@ public interface PinnedStackWindowListener extends StackWindowListener { * Called when the stack container pinned stack animation will change the picture-in-picture * mode. This is a direct call into ActivityManager. */ - default void updatePictureInPictureModeForPinnedStackAnimation(Rect targetStackBounds) {} + default void updatePictureInPictureModeForPinnedStackAnimation(Rect targetStackBounds, + boolean forceUpdate) {} } diff --git a/services/core/java/com/android/server/wm/TaskStack.java b/services/core/java/com/android/server/wm/TaskStack.java index 39878ccb07ddf..55151a192fbff 100644 --- a/services/core/java/com/android/server/wm/TaskStack.java +++ b/services/core/java/com/android/server/wm/TaskStack.java @@ -1498,7 +1498,7 @@ public class TaskStack extends WindowContainer implements DimLayer.DimLaye } @Override // AnimatesBounds - public void onAnimationStart(boolean schedulePipModeChangedCallback) { + public void onAnimationStart(boolean schedulePipModeChangedCallback, boolean forceUpdate) { // Hold the lock since this is called from the BoundsAnimator running on the UiThread synchronized (mService.mWindowMap) { mBoundsAnimatingRequested = false; @@ -1523,9 +1523,11 @@ public class TaskStack extends WindowContainer implements DimLayer.DimLaye final PinnedStackWindowController controller = (PinnedStackWindowController) getController(); if (schedulePipModeChangedCallback && controller != null) { - // We need to schedule the PiP mode change after the animation down, so use the - // final bounds - controller.updatePictureInPictureModeForPinnedStackAnimation(null); + // We need to schedule the PiP mode change before the animation up. It is possible + // in this case for the animation down to not have been completed, so always + // force-schedule and update to the client to ensure that it is notified that it + // is no longer in picture-in-picture mode + controller.updatePictureInPictureModeForPinnedStackAnimation(null, forceUpdate); } } } @@ -1553,7 +1555,7 @@ public class TaskStack extends WindowContainer implements DimLayer.DimLaye // We need to schedule the PiP mode change after the animation down, so use the // final bounds controller.updatePictureInPictureModeForPinnedStackAnimation( - mBoundsAnimationTarget); + mBoundsAnimationTarget, false /* forceUpdate */); } if (finalStackSize != null) { diff --git a/services/tests/servicestests/src/com/android/server/wm/BoundsAnimationControllerTests.java b/services/tests/servicestests/src/com/android/server/wm/BoundsAnimationControllerTests.java index 9d32496c78176..0081214a24da3 100644 --- a/services/tests/servicestests/src/com/android/server/wm/BoundsAnimationControllerTests.java +++ b/services/tests/servicestests/src/com/android/server/wm/BoundsAnimationControllerTests.java @@ -126,6 +126,7 @@ public class BoundsAnimationControllerTests extends WindowTestsBase { boolean mMovedToFullscreen; boolean mAnimationStarted; boolean mSchedulePipModeChangedOnStart; + boolean mForcePipModeChangedCallback; boolean mAnimationEnded; Rect mAnimationEndFinalStackBounds; boolean mSchedulePipModeChangedOnEnd; @@ -140,6 +141,7 @@ public class BoundsAnimationControllerTests extends WindowTestsBase { mAnimationStarted = false; mAnimationEnded = false; mAnimationEndFinalStackBounds = null; + mForcePipModeChangedCallback = false; mSchedulePipModeChangedOnStart = false; mSchedulePipModeChangedOnEnd = false; mStackBounds = from; @@ -148,10 +150,11 @@ public class BoundsAnimationControllerTests extends WindowTestsBase { } @Override - public void onAnimationStart(boolean schedulePipModeChangedCallback) { + public void onAnimationStart(boolean schedulePipModeChangedCallback, boolean forceUpdate) { mAwaitingAnimationStart = false; mAnimationStarted = true; mSchedulePipModeChangedOnStart = schedulePipModeChangedCallback; + mForcePipModeChangedCallback = forceUpdate; } @Override @@ -232,7 +235,7 @@ public class BoundsAnimationControllerTests extends WindowTestsBase { return this; } - BoundsAnimationDriver restart(Rect to) { + BoundsAnimationDriver restart(Rect to, boolean expectStartedAndPipModeChangedCallback) { if (mAnimator == null) { throw new IllegalArgumentException("Call start() to start a new animation"); } @@ -251,8 +254,15 @@ public class BoundsAnimationControllerTests extends WindowTestsBase { assertSame(oldAnimator, mAnimator); } - // No animation start for replacing animation - assertTrue(!mTarget.mAnimationStarted); + if (expectStartedAndPipModeChangedCallback) { + // Replacing animation with pending pip mode changed callback, ensure we update + assertTrue(mTarget.mAnimationStarted); + assertTrue(mTarget.mSchedulePipModeChangedOnStart); + assertTrue(mTarget.mForcePipModeChangedCallback); + } else { + // No animation start for replacing animation + assertTrue(!mTarget.mAnimationStarted); + } mTarget.mAnimationStarted = true; return this; } @@ -467,7 +477,7 @@ public class BoundsAnimationControllerTests extends WindowTestsBase { mDriver.start(BOUNDS_FULL, BOUNDS_FLOATING) .expectStarted(!SCHEDULE_PIP_MODE_CHANGED) .update(0.25f) - .restart(BOUNDS_FLOATING) + .restart(BOUNDS_FLOATING, false /* expectStartedAndPipModeChangedCallback */) .end() .expectEnded(SCHEDULE_PIP_MODE_CHANGED, !MOVE_TO_FULLSCREEN); } @@ -478,7 +488,8 @@ public class BoundsAnimationControllerTests extends WindowTestsBase { mDriver.start(BOUNDS_FULL, BOUNDS_FLOATING) .expectStarted(!SCHEDULE_PIP_MODE_CHANGED) .update(0.25f) - .restart(BOUNDS_SMALLER_FLOATING) + .restart(BOUNDS_SMALLER_FLOATING, + false /* expectStartedAndPipModeChangedCallback */) .end() .expectEnded(SCHEDULE_PIP_MODE_CHANGED, !MOVE_TO_FULLSCREEN); } @@ -486,10 +497,12 @@ public class BoundsAnimationControllerTests extends WindowTestsBase { @UiThreadTest @Test public void testFullscreenToFloatingCancelFromAnimationToFullscreenBounds() throws Exception { + // When animating from fullscreen and the animation is interruped, we expect the animation + // start callback to be made, with a forced pip mode change callback mDriver.start(BOUNDS_FULL, BOUNDS_FLOATING) .expectStarted(!SCHEDULE_PIP_MODE_CHANGED) .update(0.25f) - .restart(BOUNDS_FULL) + .restart(BOUNDS_FULL, true /* expectStartedAndPipModeChangedCallback */) .end() .expectEnded(!SCHEDULE_PIP_MODE_CHANGED, MOVE_TO_FULLSCREEN); } @@ -512,7 +525,7 @@ public class BoundsAnimationControllerTests extends WindowTestsBase { mDriver.start(BOUNDS_FLOATING, BOUNDS_FULL) .expectStarted(SCHEDULE_PIP_MODE_CHANGED) .update(0.25f) - .restart(BOUNDS_FULL) + .restart(BOUNDS_FULL, false /* expectStartedAndPipModeChangedCallback */) .end() .expectEnded(!SCHEDULE_PIP_MODE_CHANGED, MOVE_TO_FULLSCREEN); } @@ -523,7 +536,8 @@ public class BoundsAnimationControllerTests extends WindowTestsBase { mDriver.start(BOUNDS_FLOATING, BOUNDS_FULL) .expectStarted(SCHEDULE_PIP_MODE_CHANGED) .update(0.25f) - .restart(BOUNDS_SMALLER_FLOATING) + .restart(BOUNDS_SMALLER_FLOATING, + false /* expectStartedAndPipModeChangedCallback */) .end() .expectEnded(SCHEDULE_PIP_MODE_CHANGED, !MOVE_TO_FULLSCREEN); }