From e8e32863f43315f801e7b336a1d4dc6f687bec54 Mon Sep 17 00:00:00 2001 From: Hongwei Wang Date: Wed, 8 Apr 2020 13:23:45 -0700 Subject: [PATCH] Ensure TaskOrganizer works for newly created pinned stack Steps to reproduce the bug: - Restart the device - Open Duo app and make a call - When in call, try entering PiP Duo application contains multiple activities and we create a new stack from its top activity when moving it to pinned mode. This newly created stack does not carry on the existing PictureInPictureParams, nor it is marked as visible. Therefore, no onTaskAppeared would be sent to the task organizer. Also in this change: - Removed aspectRatio and sourceHintBounds used to be passed into RootWindowContainer#moveActivityToPinnedStack, they are used in moveActivityToPinnedStack and one should refer to the PictureInPictureParams set on ActivityRecord - Added a null check for existing token when onTaskInfoChanged is invoked in PipTaskOrganizer, it should be a fatal error that onTaskInfoChanged is called ahead of onTaskAppeared Bug: 152933995 Test: manually enter/exit PiP mode for Duo app Test: atest RootActivityContainerTests Test: atest ActivityRecordTests Change-Id: Ifa9ad8768ba47ce043b8dd86cadc729931edcb14 --- .../com/android/systemui/pip/PipTaskOrganizer.java | 13 +++++++++---- .../java/com/android/server/wm/ActivityRecord.java | 13 +++++++++++++ .../server/wm/ActivityTaskManagerService.java | 5 +---- .../com/android/server/wm/RootWindowContainer.java | 14 +++++++------- .../server/wm/RootActivityContainerTests.java | 8 ++------ 5 files changed, 32 insertions(+), 21 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/pip/PipTaskOrganizer.java b/packages/SystemUI/src/com/android/systemui/pip/PipTaskOrganizer.java index a95d6b7a73cdc..f89a01e0c73b9 100644 --- a/packages/SystemUI/src/com/android/systemui/pip/PipTaskOrganizer.java +++ b/packages/SystemUI/src/com/android/systemui/pip/PipTaskOrganizer.java @@ -126,7 +126,7 @@ public class PipTaskOrganizer extends TaskOrganizer { }; @SuppressWarnings("unchecked") - private Handler.Callback mUpdateCallbacks = (msg) -> { + private final Handler.Callback mUpdateCallbacks = (msg) -> { SomeArgs args = (SomeArgs) msg.obj; Consumer updateBoundsCallback = (Consumer) args.arg1; switch (msg.what) { @@ -282,7 +282,7 @@ public class PipTaskOrganizer extends TaskOrganizer { */ @Override public void onTaskVanished(ActivityManager.RunningTaskInfo info) { - WindowContainerToken token = info.token; + final WindowContainerToken token = info.token; Objects.requireNonNull(token, "Requires valid WindowContainerToken"); if (token.asBinder() != mToken.asBinder()) { Log.wtf(TAG, "Unrecognized token: " + token); @@ -297,6 +297,7 @@ public class PipTaskOrganizer extends TaskOrganizer { @Override public void onTaskInfoChanged(ActivityManager.RunningTaskInfo info) { + Objects.requireNonNull(mToken, "onTaskInfoChanged requires valid existing mToken"); final PictureInPictureParams newParams = info.pictureInPictureParams; if (!shouldUpdateDestinationBounds(newParams)) { Log.d(TAG, "Ignored onTaskInfoChanged with PiP param: " + newParams); @@ -375,7 +376,7 @@ public class PipTaskOrganizer extends TaskOrganizer { @PipAnimationController.TransitionDirection int direction, int durationMs, Consumer updateBoundsCallback) { if (!mInPip) { - // Ignore animation when we are no longer in PIP + // can be initiated in other component, ignore if we are no longer in PIP return; } SomeArgs args = SomeArgs.obtain(); @@ -427,6 +428,10 @@ public class PipTaskOrganizer extends TaskOrganizer { private void scheduleFinishResizePip(SurfaceControl.Transaction tx, Rect destinationBounds, @PipAnimationController.TransitionDirection int direction, Consumer updateBoundsCallback) { + if (!mInPip) { + // can be initiated in other component, ignore if we are no longer in PIP + return; + } SomeArgs args = SomeArgs.obtain(); args.arg1 = updateBoundsCallback; args.arg2 = tx; @@ -441,7 +446,7 @@ public class PipTaskOrganizer extends TaskOrganizer { public void scheduleOffsetPip(Rect originalBounds, int offset, int duration, Consumer updateBoundsCallback) { if (!mInPip) { - // Ignore offsets when we are no longer in PIP + // can be initiated in other component, ignore if we are no longer in PIP return; } SomeArgs args = SomeArgs.obtain(); diff --git a/services/core/java/com/android/server/wm/ActivityRecord.java b/services/core/java/com/android/server/wm/ActivityRecord.java index ed9b019166575..0b33cbdd69aeb 100644 --- a/services/core/java/com/android/server/wm/ActivityRecord.java +++ b/services/core/java/com/android/server/wm/ActivityRecord.java @@ -1298,6 +1298,19 @@ final class ActivityRecord extends WindowToken implements WindowManagerService.A if (newTask != null && isState(RESUMED)) { newTask.setResumedActivity(this, "onParentChanged"); } + + if (stack != null && stack.topRunningActivity() == this) { + // carry over the PictureInPictureParams to the parent stack without calling + // TaskOrganizerController#dispatchTaskInfoChanged. + // this is to ensure the stack holding up-to-dated pinned stack information + // when activity is re-parented to enter pip mode, see also + // RootWindowContainer#moveActivityToPinnedStack + stack.mPictureInPictureParams.copyOnlySet(pictureInPictureArgs); + // make ensure the TaskOrganizer still works after re-parenting + if (firstWindowDrawn) { + stack.setHasBeenVisible(true); + } + } } private void updateColorTransform() { diff --git a/services/core/java/com/android/server/wm/ActivityTaskManagerService.java b/services/core/java/com/android/server/wm/ActivityTaskManagerService.java index c253cd2c32970..08683caa2ac09 100644 --- a/services/core/java/com/android/server/wm/ActivityTaskManagerService.java +++ b/services/core/java/com/android/server/wm/ActivityTaskManagerService.java @@ -4095,11 +4095,8 @@ public class ActivityTaskManagerService extends IActivityTaskManager.Stub { r.setPictureInPictureParams(params); final float aspectRatio = r.pictureInPictureArgs.getAspectRatio(); final List actions = r.pictureInPictureArgs.getActions(); - // Adjust the source bounds by the insets for the transition down - final Rect sourceBounds = new Rect( - r.pictureInPictureArgs.getSourceRectHint()); mRootWindowContainer.moveActivityToPinnedStack( - r, sourceBounds, aspectRatio, "enterPictureInPictureMode"); + r, "enterPictureInPictureMode"); final ActivityStack stack = r.getRootTask(); stack.setPictureInPictureAspectRatio(aspectRatio); stack.setPictureInPictureActions(actions); diff --git a/services/core/java/com/android/server/wm/RootWindowContainer.java b/services/core/java/com/android/server/wm/RootWindowContainer.java index 1a70de71df4f0..e8f7ba550bd8d 100644 --- a/services/core/java/com/android/server/wm/RootWindowContainer.java +++ b/services/core/java/com/android/server/wm/RootWindowContainer.java @@ -2147,13 +2147,11 @@ class RootWindowContainer extends WindowContainer return false; } - moveActivityToPinnedStack(r, null /* sourceBounds */, 0f /* aspectRatio */, - "moveTopActivityToPinnedStack"); + moveActivityToPinnedStack(r, "moveTopActivityToPinnedStack"); return true; } - void moveActivityToPinnedStack(ActivityRecord r, Rect sourceHintBounds, float aspectRatio, - String reason) { + void moveActivityToPinnedStack(ActivityRecord r, String reason) { mService.deferWindowLayout(); final TaskDisplayArea taskDisplayArea = r.getDisplayArea(); @@ -2176,17 +2174,19 @@ class RootWindowContainer extends WindowContainer final ActivityStack stack; if (singleActivity) { stack = r.getRootTask(); - stack.setWindowingMode(WINDOWING_MODE_PINNED); } else { // In the case of multiple activities, we will create a new task for it and then // move the PIP activity into the task. - stack = taskDisplayArea.createStack(WINDOWING_MODE_PINNED, r.getActivityType(), + stack = taskDisplayArea.createStack(WINDOWING_MODE_UNDEFINED, r.getActivityType(), ON_TOP, r.info, r.intent, false /* createdByOrganizer */); // There are multiple activities in the task and moving the top activity should // reveal/leave the other activities in their original task. - r.reparent(stack, MAX_VALUE, "moveActivityToStack"); + // On the other hand, ActivityRecord#onParentChanged takes care of setting the + // up-to-dated pinned stack information on this newly created stack. + r.reparent(stack, MAX_VALUE, reason); } + stack.setWindowingMode(WINDOWING_MODE_PINNED); // Reset the state that indicates it can enter PiP while pausing after we've moved it // to the pinned stack diff --git a/services/tests/wmtests/src/com/android/server/wm/RootActivityContainerTests.java b/services/tests/wmtests/src/com/android/server/wm/RootActivityContainerTests.java index 7613655e9f35d..3c9051547eed1 100644 --- a/services/tests/wmtests/src/com/android/server/wm/RootActivityContainerTests.java +++ b/services/tests/wmtests/src/com/android/server/wm/RootActivityContainerTests.java @@ -58,7 +58,6 @@ import android.content.pm.ActivityInfo; import android.content.pm.ApplicationInfo; import android.content.pm.ResolveInfo; import android.content.res.Resources; -import android.graphics.Rect; import android.platform.test.annotations.Presubmit; import android.util.Pair; @@ -126,9 +125,7 @@ public class RootActivityContainerTests extends ActivityTestsBase { ensureStackPlacement(mFullscreenStack, firstActivity, secondActivity); // Move first activity to pinned stack. - final Rect sourceBounds = new Rect(); - mRootWindowContainer.moveActivityToPinnedStack(firstActivity, sourceBounds, - 0f /*aspectRatio*/, "initialMove"); + mRootWindowContainer.moveActivityToPinnedStack(firstActivity, "initialMove"); final TaskDisplayArea taskDisplayArea = mFullscreenStack.getDisplayArea(); ActivityStack pinnedStack = taskDisplayArea.getRootPinnedTask(); @@ -137,8 +134,7 @@ public class RootActivityContainerTests extends ActivityTestsBase { ensureStackPlacement(mFullscreenStack, secondActivity); // Move second activity to pinned stack. - mRootWindowContainer.moveActivityToPinnedStack(secondActivity, sourceBounds, - 0f /*aspectRatio*/, "secondMove"); + mRootWindowContainer.moveActivityToPinnedStack(secondActivity, "secondMove"); // Need to get stacks again as a new instance might have been created. pinnedStack = taskDisplayArea.getRootPinnedTask();