From c3ebae4ca884a1b56930b322d171d06b675fbe4d Mon Sep 17 00:00:00 2001 From: Galia Peycheva Date: Fri, 23 Sep 2022 12:41:00 +0200 Subject: [PATCH] Fix pip update transaction out of order When an animation is playing, PipAnimationController#onAnimationUpdate is called with the fraction of the animation progress. This fraction is used to calculate a scale value (in a matrix), which is applied in a transaction to the pip surface. This scaling is invisible to WM core, so the bounds of the pip surface are still the bounds before the animation began. In SF, those bounds are scaled and drawn. When the animation is finished, onAnimationEnd is called. onAnimationEnd is special, because it finishes the resizing and sends the updated pip task bounds to WM core in a WindowContainerTransaction. The WindowContainerTransaction is applied separately by WM core. Additionally, the transactions forged by onAnimationUpdate are not applied immediately. In each onAnimationUpdate, the pip surface transaction is applied in a SyncRtSurfaceTransactionApplier (together with the transaction for the pip menu), which waits until the next draw is complete and then merges the pip surface transaction into the global frame transaction. In onAnimationEnd, however, the pip update is not applied in a SurfaceControl.Transaction, but a WindowContainerTransaction is sent to WM core, which applies it at some later point. If the pip surface transaction from the last onAnimationUpdate is merged+applied after the WindowContainerTransaction is applied in WM core, then the two transactions are committed out of order and the pip surface ends up in this weird state, where an extra scale was applied to it. To address this, we delay the onAnimationEnd to the next frame, which adds enough time to let the last onAnimationUpdate transaction be applied. We do this only when the PiP transaction has to be synced with the PiP menu drawing, because the synchronization between the two is what causes the last onAnimationUpdate transaction to be applied with a delay Bug: 228317396 Test: manual - change pip aspect ratio several times Change-Id: Iaf4005b34b63ab7a123e508ace80f966014a0805 --- .../wm/shell/pip/PipMenuController.java | 6 +++ .../wm/shell/pip/PipTaskOrganizer.java | 40 ++++++++++++++++--- .../pip/phone/PhonePipMenuController.java | 12 ++++++ .../wm/shell/pip/tv/TvPipMenuController.java | 12 ++++++ 4 files changed, 65 insertions(+), 5 deletions(-) diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/pip/PipMenuController.java b/libs/WindowManager/Shell/src/com/android/wm/shell/pip/PipMenuController.java index 16f1d1c2944c3..f81c9f80830ae 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/pip/PipMenuController.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/pip/PipMenuController.java @@ -23,6 +23,7 @@ import static android.view.WindowManager.LayoutParams.FLAG_WATCH_OUTSIDE_TOUCH; import static android.view.WindowManager.LayoutParams.PRIVATE_FLAG_TRUSTED_OVERLAY; import static android.view.WindowManager.LayoutParams.TYPE_APPLICATION_OVERLAY; +import android.annotation.NonNull; import android.annotation.Nullable; import android.app.ActivityManager.RunningTaskInfo; import android.app.RemoteAction; @@ -69,6 +70,11 @@ public interface PipMenuController { */ void setAppActions(List appActions, RemoteAction closeAction); + /** + * Wait until the next frame to run the given Runnable. + */ + void runWithNextFrame(@NonNull Runnable runnable); + /** * Resize the PiP menu with the given bounds. The PiP SurfaceControl is given if there is a * need to synchronize the movements on the same frame as PiP. diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/pip/PipTaskOrganizer.java b/libs/WindowManager/Shell/src/com/android/wm/shell/pip/PipTaskOrganizer.java index f170e774739fc..2d7c5ce6feb5d 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/pip/PipTaskOrganizer.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/pip/PipTaskOrganizer.java @@ -179,8 +179,10 @@ public class PipTaskOrganizer implements ShellTaskOrganizer.TaskListener, // This is necessary in case there was a resize animation ongoing when exit PIP // started, in which case the first resize will be skipped to let the exit // operation handle the final resize out of PIP mode. See b/185306679. - finishResize(tx, destinationBounds, direction, animationType); - sendOnPipTransitionFinished(direction); + finishResizeDelayedIfNeeded(() -> { + finishResize(tx, destinationBounds, direction, animationType); + sendOnPipTransitionFinished(direction); + }); } } @@ -196,6 +198,34 @@ public class PipTaskOrganizer implements ShellTaskOrganizer.TaskListener, } }; + /** + * Finishes resizing the PiP, delaying the operation if it has to be synced with the PiP menu. + * + * This is done to avoid a race condition between the last transaction applied in + * onAnimationUpdate and the finishResize in onAnimationEnd. finishResize creates a + * WindowContainerTransaction, which is to be applied by WmCore later. It may happen that it + * gets applied before the transaction created by the last onAnimationUpdate. As a result of + * this, the PiP surface may get scaled after the new bounds are applied by WmCore, which + * makes the PiP surface have unexpected bounds. To avoid this, we delay the finishResize + * operation until the next frame. This aligns the last onAnimationUpdate transaction with the + * WCT application. + * + * The race only happens when the PiP surface transaction has to be synced with the PiP menu + * due to the necessity for a delay when syncing the PiP surface, the PiP menu surface and + * the PiP menu contents. + */ + private void finishResizeDelayedIfNeeded(Runnable finishResizeRunnable) { + if (!shouldSyncPipTransactionWithMenu()) { + finishResizeRunnable.run(); + return; + } + mPipMenuController.runWithNextFrame(finishResizeRunnable); + } + + private boolean shouldSyncPipTransactionWithMenu() { + return mPipMenuController.isMenuVisible(); + } + @VisibleForTesting final PipTransitionController.PipTransitionCallback mPipTransitionCallback = new PipTransitionController.PipTransitionCallback() { @@ -221,7 +251,7 @@ public class PipTaskOrganizer implements ShellTaskOrganizer.TaskListener, @Override public boolean handlePipTransaction(SurfaceControl leash, SurfaceControl.Transaction tx, Rect destinationBounds) { - if (mPipMenuController.isMenuVisible()) { + if (shouldSyncPipTransactionWithMenu()) { mPipMenuController.movePipMenu(leash, tx, destinationBounds); return true; } @@ -1223,7 +1253,7 @@ public class PipTaskOrganizer implements ShellTaskOrganizer.TaskListener, mSurfaceTransactionHelper .crop(tx, mLeash, toBounds) .round(tx, mLeash, mPipTransitionState.isInPip()); - if (mPipMenuController.isMenuVisible()) { + if (shouldSyncPipTransactionWithMenu()) { mPipMenuController.resizePipMenu(mLeash, tx, toBounds); } else { tx.apply(); @@ -1265,7 +1295,7 @@ public class PipTaskOrganizer implements ShellTaskOrganizer.TaskListener, mSurfaceTransactionHelper .scale(tx, mLeash, startBounds, toBounds, degrees) .round(tx, mLeash, startBounds, toBounds); - if (mPipMenuController.isMenuVisible()) { + if (shouldSyncPipTransactionWithMenu()) { mPipMenuController.movePipMenu(mLeash, tx, toBounds); } else { tx.apply(); diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/pip/phone/PhonePipMenuController.java b/libs/WindowManager/Shell/src/com/android/wm/shell/pip/phone/PhonePipMenuController.java index 281ea530e9e13..27902b2231ba7 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/pip/phone/PhonePipMenuController.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/pip/phone/PhonePipMenuController.java @@ -305,6 +305,18 @@ public class PhonePipMenuController implements PipMenuController { showResizeHandle); } + @Override + public void runWithNextFrame(Runnable runnable) { + if (mPipMenuView == null || mPipMenuView.getViewRootImpl() == null) { + runnable.run(); + } + + mPipMenuView.getViewRootImpl().registerRtFrameCallback(frame -> { + mMainHandler.post(runnable); + }); + mPipMenuView.invalidate(); + } + /** * Move the PiP menu, which does a translation and possibly a scale transformation. */ diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/pip/tv/TvPipMenuController.java b/libs/WindowManager/Shell/src/com/android/wm/shell/pip/tv/TvPipMenuController.java index 4ce45e142c643..7d4b43be4f731 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/pip/tv/TvPipMenuController.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/pip/tv/TvPipMenuController.java @@ -465,6 +465,18 @@ public class TvPipMenuController implements PipMenuController, TvPipMenuView.Lis return mSystemWindows.getViewSurface(v); } + @Override + public void runWithNextFrame(Runnable runnable) { + if (mPipMenuView == null || mPipMenuView.getViewRootImpl() == null) { + runnable.run(); + } + + mPipMenuView.getViewRootImpl().registerRtFrameCallback(frame -> { + mMainHandler.post(runnable); + }); + mPipMenuView.invalidate(); + } + @Override public void movePipMenu(SurfaceControl pipLeash, SurfaceControl.Transaction transaction, Rect pipDestBounds) {