From 046ec8336e8c7a997f8a85e564ed3dd363d44045 Mon Sep 17 00:00:00 2001 From: Evan Rosky Date: Thu, 12 May 2022 22:35:56 -0700 Subject: [PATCH] Fix pip bundling Not all pips should be their own transition. This CL changes the rules so that any pip originally requested by the system (even if done via the app -- eg legacy pause+userLeaving) will not get their own transition and instead be bundled into the current one. This also fixes an issue where transient launch committal was only entering pip for the new auto-enter case but not the legacy userLeaving case. Bug: 220196913 Bug: 231150615 Test: launch a legacy userLeave-enters-pip app and then go home (swipe or button). Change-Id: I3c20341971ab4512509daa70314742f84c1ccd6b --- data/etc/services.core.protolog.json | 18 ++-- .../server/wm/ActivityClientController.java | 5 +- .../server/wm/ActivityTaskManagerService.java | 23 +++-- .../com/android/server/wm/TaskFragment.java | 2 +- .../com/android/server/wm/Transition.java | 85 +++++++++++++------ 5 files changed, 85 insertions(+), 48 deletions(-) diff --git a/data/etc/services.core.protolog.json b/data/etc/services.core.protolog.json index 6c3c303c122cf..0d6a7967ed665 100644 --- a/data/etc/services.core.protolog.json +++ b/data/etc/services.core.protolog.json @@ -763,6 +763,12 @@ "group": "WM_ERROR", "at": "com\/android\/server\/wm\/WindowManagerService.java" }, + "-1383884640": { + "message": " allReady query: used=%b override=%b defer=%d states=[%s]", + "level": "VERBOSE", + "group": "WM_DEBUG_WINDOW_TRANSITIONS", + "at": "com\/android\/server\/wm\/Transition.java" + }, "-1376035390": { "message": "No task found", "level": "DEBUG", @@ -2545,12 +2551,6 @@ "group": "WM_DEBUG_ADD_REMOVE", "at": "com\/android\/server\/wm\/ActivityRecord.java" }, - "352982444": { - "message": " allReady query: used=%b override=%b states=[%s]", - "level": "VERBOSE", - "group": "WM_DEBUG_WINDOW_TRANSITIONS", - "at": "com\/android\/server\/wm\/Transition.java" - }, "355720268": { "message": "stopFreezingDisplayLocked: Unfreezing now", "level": "DEBUG", @@ -2929,12 +2929,6 @@ "group": "WM_DEBUG_LOCKTASK", "at": "com\/android\/server\/wm\/ActivityTaskManagerService.java" }, - "716528224": { - "message": "Focused window found using wmService.getFocusedWindowLocked()", - "level": "DEBUG", - "group": "WM_DEBUG_BACK_PREVIEW", - "at": "com\/android\/server\/wm\/BackNavigationController.java" - }, "726205185": { "message": "Moving to DESTROYED: %s (destroy skipped)", "level": "VERBOSE", diff --git a/services/core/java/com/android/server/wm/ActivityClientController.java b/services/core/java/com/android/server/wm/ActivityClientController.java index d7a0ca0231d14..0f23d161393af 100644 --- a/services/core/java/com/android/server/wm/ActivityClientController.java +++ b/services/core/java/com/android/server/wm/ActivityClientController.java @@ -725,7 +725,7 @@ class ActivityClientController extends IActivityClientController.Stub { synchronized (mGlobalLock) { final ActivityRecord r = ensureValidPictureInPictureActivityParams( "enterPictureInPictureMode", token, params); - return mService.enterPictureInPictureMode(r, params); + return mService.enterPictureInPictureMode(r, params, true /* fromClient */); } } finally { Binder.restoreCallingIdentity(origId); @@ -856,7 +856,8 @@ class ActivityClientController extends IActivityClientController.Stub { } if (r.pictureInPictureArgs.isAutoEnterEnabled()) { - return mService.enterPictureInPictureMode(r, r.pictureInPictureArgs); + return mService.enterPictureInPictureMode(r, r.pictureInPictureArgs, + false /* fromClient */); } try { diff --git a/services/core/java/com/android/server/wm/ActivityTaskManagerService.java b/services/core/java/com/android/server/wm/ActivityTaskManagerService.java index 7524825a998ac..06242b391d4bc 100644 --- a/services/core/java/com/android/server/wm/ActivityTaskManagerService.java +++ b/services/core/java/com/android/server/wm/ActivityTaskManagerService.java @@ -95,6 +95,7 @@ import static com.android.server.am.EventLogTags.writeBootProgressEnableScreen; import static com.android.server.am.EventLogTags.writeConfigurationChanged; import static com.android.server.wm.ActivityInterceptorCallback.FIRST_ORDERED_ID; import static com.android.server.wm.ActivityInterceptorCallback.LAST_ORDERED_ID; +import static com.android.server.wm.ActivityRecord.State.PAUSING; import static com.android.server.wm.ActivityTaskManagerDebugConfig.DEBUG_ALL; import static com.android.server.wm.ActivityTaskManagerDebugConfig.POSTFIX_ROOT_TASK; import static com.android.server.wm.ActivityTaskManagerDebugConfig.POSTFIX_SWITCH; @@ -3449,10 +3450,12 @@ public class ActivityTaskManagerService extends IActivityTaskManager.Stub { /** * Puts the given activity in picture in picture mode if possible. * + * @param fromClient true if this comes from a client call (eg. Activity.enterPip). * @return true if the activity is now in picture-in-picture mode, or false if it could not * enter picture-in-picture mode. */ - boolean enterPictureInPictureMode(@NonNull ActivityRecord r, PictureInPictureParams params) { + boolean enterPictureInPictureMode(@NonNull ActivityRecord r, + @NonNull PictureInPictureParams params, boolean fromClient) { // If the activity is already in picture in picture mode, then just return early if (r.inPinnedWindowingMode()) { return true; @@ -3465,9 +3468,17 @@ public class ActivityTaskManagerService extends IActivityTaskManager.Stub { return false; } - // Create a transition for this pip entry. We guarantee that this gets its own transition - // by queueing this transition on SyncEngine. This is shared by all the entry paths. - final Transition transition = getTransitionController().isShellTransitionsEnabled() + // If the app is using legacy-entry (not auto-enter), then we will get a client-request + // that was actually a server-request (via pause(userLeaving=true)). This happens when + // the app is PAUSING, so detect that case here. + boolean originallyFromClient = fromClient + && (!r.isState(PAUSING) || params.isAutoEnterEnabled()); + + // Create a transition only for this pip entry if it is coming from the app without the + // system requesting that the app enter-pip. If the system requested it, that means it + // should be part of that transition if possible. + final Transition transition = + (getTransitionController().isShellTransitionsEnabled() && originallyFromClient) ? new Transition(TRANSIT_PIP, 0 /* flags */, getTransitionController(), mWindowManager.mSyncEngine) : null; @@ -3498,7 +3509,7 @@ public class ActivityTaskManagerService extends IActivityTaskManager.Stub { mActivityClientController.dismissKeyguard(r.token, new KeyguardDismissCallback() { @Override public void onDismissSucceeded() { - if (mWindowManager.mSyncEngine.hasActiveSync()) { + if (transition != null && mWindowManager.mSyncEngine.hasActiveSync()) { ProtoLog.v(ProtoLogGroup.WM_DEBUG_WINDOW_TRANSITIONS, "Creating Pending Pip-Enter: %s", transition); mWindowManager.mSyncEngine.queueSyncSet( @@ -3516,7 +3527,7 @@ public class ActivityTaskManagerService extends IActivityTaskManager.Stub { }, null /* message */); } else { // Enter picture in picture immediately otherwise - if (mWindowManager.mSyncEngine.hasActiveSync()) { + if (transition != null && mWindowManager.mSyncEngine.hasActiveSync()) { ProtoLog.v(ProtoLogGroup.WM_DEBUG_WINDOW_TRANSITIONS, "Creating Pending Pip-Enter: %s", transition); mWindowManager.mSyncEngine.queueSyncSet( diff --git a/services/core/java/com/android/server/wm/TaskFragment.java b/services/core/java/com/android/server/wm/TaskFragment.java index 9c7a27c1bbdeb..e4294340af922 100644 --- a/services/core/java/com/android/server/wm/TaskFragment.java +++ b/services/core/java/com/android/server/wm/TaskFragment.java @@ -1518,7 +1518,7 @@ class TaskFragment extends WindowContainer { if (prev.attachedToProcess()) { if (shouldAutoPip) { boolean didAutoPip = mAtmService.enterPictureInPictureMode( - prev, prev.pictureInPictureArgs); + prev, prev.pictureInPictureArgs, false /* fromClient */); ProtoLog.d(WM_DEBUG_STATES, "Auto-PIP allowed, entering PIP mode " + "directly: %s, didAutoPip: %b", prev, didAutoPip); } else { diff --git a/services/core/java/com/android/server/wm/Transition.java b/services/core/java/com/android/server/wm/Transition.java index d5acf4fed330b..3fcaa5c21ccca 100644 --- a/services/core/java/com/android/server/wm/Transition.java +++ b/services/core/java/com/android/server/wm/Transition.java @@ -54,6 +54,7 @@ import static android.window.TransitionInfo.FLAG_SHOW_WALLPAPER; import static android.window.TransitionInfo.FLAG_STARTING_WINDOW_TRANSFER_RECIPIENT; import static android.window.TransitionInfo.FLAG_TRANSLUCENT; +import static com.android.server.wm.ActivityRecord.State.RESUMED; import static com.android.server.wm.ActivityTaskManagerInternal.APP_TRANSITION_RECENTS_ANIM; import static com.android.server.wm.ActivityTaskManagerInternal.APP_TRANSITION_SPLASH_SCREEN; import static com.android.server.wm.ActivityTaskManagerInternal.APP_TRANSITION_WINDOWS_DRAWN; @@ -477,6 +478,58 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe mCanPipOnFinish = canPipOnFinish; } + private boolean didCommitTransientLaunch() { + if (mTransientLaunches == null) return false; + for (int j = 0; j < mTransientLaunches.size(); ++j) { + if (mTransientLaunches.keyAt(j).isVisibleRequested()) { + return true; + } + } + return false; + } + + /** + * Check if pip-entry is possible after finishing and enter-pip if it is. + * + * @return true if we are *guaranteed* to enter-pip. This means we return false if there's + * a chance we won't thus legacy-entry (via pause+userLeaving) will return false. + */ + private boolean checkEnterPipOnFinish(@NonNull ActivityRecord ar) { + if (!mCanPipOnFinish || !ar.isVisible() || ar.getTask() == null) return false; + + if (ar.pictureInPictureArgs != null && ar.pictureInPictureArgs.isAutoEnterEnabled()) { + if (didCommitTransientLaunch()) { + // force enable pip-on-task-switch now that we've committed to actually launching + // to the transient activity. + ar.supportsEnterPipOnTaskSwitch = true; + } + return mController.mAtm.enterPictureInPictureMode(ar, ar.pictureInPictureArgs, + false /* fromClient */); + } + + // Legacy pip-entry (not via isAutoEnterEnabled). + boolean canPip = ar.getDeferHidingClient(); + if (!canPip && didCommitTransientLaunch()) { + // force enable pip-on-task-switch now that we've committed to actually launching to the + // transient activity, and then recalculate whether we can attempt pip. + ar.supportsEnterPipOnTaskSwitch = true; + canPip = ar.checkEnterPictureInPictureState( + "finishTransition", true /* beforeStopping */) + && ar.isState(RESUMED); + } + if (!canPip) return false; + try { + // Legacy PIP-enter requires pause event with user-leaving. + mController.mAtm.mTaskSupervisor.mUserLeaving = true; + ar.getTaskFragment().startPausing(false /* uiSleeping */, + null /* resuming */, "finishTransition"); + } finally { + mController.mAtm.mTaskSupervisor.mUserLeaving = false; + } + // Return false anyway because there's no guarantee that the app will enter pip. + return false; + } + /** * The transition has finished animating and is ready to finalize WM state. This should not * be called directly; use {@link TransitionController#finishTransition} instead. @@ -503,32 +556,9 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe // then doing commitVisibility here would actually be out-of-order and leave the // activity in a bad state. if (!visibleAtTransitionEnd && !ar.isVisibleRequested()) { - boolean commitVisibility = true; - if (mCanPipOnFinish && ar.isVisible() && ar.getTask() != null) { - if (ar.pictureInPictureArgs != null - && ar.pictureInPictureArgs.isAutoEnterEnabled()) { - if (mTransientLaunches != null) { - for (int j = 0; j < mTransientLaunches.size(); ++j) { - if (mTransientLaunches.keyAt(j).isVisibleRequested()) { - // force enable pip-on-task-switch now that we've committed - // to actually launching to the transient activity. - ar.supportsEnterPipOnTaskSwitch = true; - break; - } - } - } - mController.mAtm.enterPictureInPictureMode(ar, ar.pictureInPictureArgs); - // Avoid commit visibility to false here, or else we will get a sudden - // "flash" / surface going invisible for a split second. - commitVisibility = false; - } else if (ar.getDeferHidingClient()) { - // Legacy PIP-enter requires pause event with user-leaving. - mController.mAtm.mTaskSupervisor.mUserLeaving = true; - ar.getTaskFragment().startPausing(false /* uiSleeping */, - null /* resuming */, "finishTransition"); - mController.mAtm.mTaskSupervisor.mUserLeaving = false; - } - } + final boolean commitVisibility = !checkEnterPipOnFinish(ar); + // Avoid commit visibility if entering pip or else we will get a sudden + // "flash" / surface going invisible for a split second. if (commitVisibility) { ProtoLog.v(ProtoLogGroup.WM_DEBUG_WINDOW_TRANSITIONS, " Commit activity becoming invisible: %s", ar); @@ -1720,7 +1750,8 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe /** @return true if all tracked subtrees are ready. */ boolean allReady() { ProtoLog.v(ProtoLogGroup.WM_DEBUG_WINDOW_TRANSITIONS, " allReady query: used=%b " - + "override=%b states=[%s]", mUsed, mReadyOverride, groupsToString()); + + "override=%b defer=%d states=[%s]", mUsed, mReadyOverride, mDeferReadyDepth, + groupsToString()); // If the readiness has never been touched, mUsed will be false. We never want to // consider a transition ready if nothing has been reported on it. if (!mUsed) return false;