From 72a8241ff73e9373c23128f7481d29217465bc0a Mon Sep 17 00:00:00 2001 From: Evan Rosky Date: Thu, 18 Nov 2021 10:59:08 -0800 Subject: [PATCH 1/2] Queue startTransition calls made while already collecting Shell Transitions only supports 1 transition collecting at a time. This limitation comes from BLASTSyncEngine. However, because Shell can call startTransition at any time (and doesn't have a way to know if WM is already "collecting" for a transition), we can't just crash in this case. We also can't just use the existing transition because it has a different "client" expecting results. So, the best we can do is queue them up. Fortunately, the startTransition API Shell uses is designed such that all the relevant operations are in one WCT, so we can maintain server-side correctness. This CL adds a queueing mechanism for these situations. In practice, this should actually never be needed; however, it is *technically* possible and automated tests are more likely to hit it. Bug: 183993924 Test: pass existing Change-Id: I255fb336cb30c7d1e1b82188a31f9c693b9e138c --- data/etc/services.core.protolog.json | 48 ++++------- .../com/android/server/wm/Transition.java | 27 ++++-- .../server/wm/TransitionController.java | 82 +++++++++++++++++-- .../server/wm/WindowOrganizerController.java | 42 +++++++--- .../android/server/wm/TransitionTests.java | 7 +- 5 files changed, 150 insertions(+), 56 deletions(-) diff --git a/data/etc/services.core.protolog.json b/data/etc/services.core.protolog.json index 535d656462f45..89aa662db2f98 100644 --- a/data/etc/services.core.protolog.json +++ b/data/etc/services.core.protolog.json @@ -13,6 +13,12 @@ "group": "WM_DEBUG_STARTING_WINDOW", "at": "com\/android\/server\/wm\/ActivityRecord.java" }, + "-2125618712": { + "message": "PendingStartTransition found", + "level": "VERBOSE", + "group": "WM_DEBUG_WINDOW_TRANSITIONS", + "at": "com\/android\/server\/wm\/TransitionController.java" + }, "-2121056984": { "message": "%s", "level": "WARN", @@ -103,18 +109,6 @@ "group": "WM_DEBUG_STATES", "at": "com\/android\/server\/wm\/TaskFragment.java" }, - "-2002500255": { - "message": "Defer removing snapshot surface in %dms", - "level": "VERBOSE", - "group": "WM_DEBUG_STARTING_WINDOW", - "at": "com\/android\/server\/wm\/TaskSnapshotSurface.java" - }, - "-1991255017": { - "message": "Drawing snapshot surface sizeMismatch=%b", - "level": "VERBOSE", - "group": "WM_DEBUG_STARTING_WINDOW", - "at": "com\/android\/server\/wm\/TaskSnapshotSurface.java" - }, "-1980468143": { "message": "DisplayArea appeared name=%s", "level": "VERBOSE", @@ -331,6 +325,12 @@ "group": "WM_DEBUG_RECENTS_ANIMATIONS", "at": "com\/android\/server\/wm\/RecentsAnimationController.java" }, + "-1764792832": { + "message": "Start collecting in Transition: %s", + "level": "VERBOSE", + "group": "WM_DEBUG_WINDOW_TRANSITIONS", + "at": "com\/android\/server\/wm\/TransitionController.java" + }, "-1750384749": { "message": "Launch on display check: allow launch on public display", "level": "DEBUG", @@ -1597,12 +1597,6 @@ "group": "WM_DEBUG_FOCUS_LIGHT", "at": "com\/android\/server\/wm\/DisplayContent.java" }, - "-405536909": { - "message": "Removing snapshot surface", - "level": "VERBOSE", - "group": "WM_DEBUG_STARTING_WINDOW", - "at": "com\/android\/server\/wm\/TaskSnapshotSurface.java" - }, "-401282500": { "message": "destroyIfPossible: r=%s destroy returned removed=%s", "level": "DEBUG", @@ -2005,12 +1999,6 @@ "group": "WM_ERROR", "at": "com\/android\/server\/wm\/WindowManagerService.java" }, - "44438983": { - "message": "performLayout: Activity exiting now removed %s", - "level": "VERBOSE", - "group": "WM_DEBUG_ADD_REMOVE", - "at": "com\/android\/server\/wm\/DisplayContent.java" - }, "45285419": { "message": "startingWindow was set but startingSurface==null, couldn't remove", "level": "VERBOSE", @@ -3211,6 +3199,12 @@ "group": "WM_ERROR", "at": "com\/android\/server\/wm\/WindowManagerService.java" }, + "1333520287": { + "message": "Creating PendingTransition: %s", + "level": "VERBOSE", + "group": "WM_DEBUG_WINDOW_TRANSITIONS", + "at": "com\/android\/server\/wm\/TransitionController.java" + }, "1337596507": { "message": "Sending to proc %s new compat %s", "level": "VERBOSE", @@ -3271,12 +3265,6 @@ "group": "WM_DEBUG_LAYER_MIRRORING", "at": "com\/android\/server\/wm\/DisplayContent.java" }, - "1417601133": { - "message": "Enqueueing ADD_STARTING", - "level": "VERBOSE", - "group": "WM_DEBUG_STARTING_WINDOW", - "at": "com\/android\/server\/wm\/ActivityRecord.java" - }, "1422781269": { "message": "Resuming rotation after re-position", "level": "DEBUG", diff --git a/services/core/java/com/android/server/wm/Transition.java b/services/core/java/com/android/server/wm/Transition.java index b13c9a9e3e14e..18df316f54766 100644 --- a/services/core/java/com/android/server/wm/Transition.java +++ b/services/core/java/com/android/server/wm/Transition.java @@ -101,6 +101,9 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe /** The default package for resources */ private static final String DEFAULT_PACKAGE = "android"; + /** The transition has been created but isn't collecting yet. */ + private static final int STATE_PENDING = -1; + /** The transition has been created and is collecting, but hasn't formally started. */ private static final int STATE_COLLECTING = 0; @@ -122,6 +125,7 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe private static final int STATE_ABORT = 3; @IntDef(prefix = { "STATE_" }, value = { + STATE_PENDING, STATE_COLLECTING, STATE_STARTED, STATE_PLAYING, @@ -131,7 +135,7 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe @interface TransitionState {} final @TransitionType int mType; - private int mSyncId; + private int mSyncId = -1; private @TransitionFlags int mFlags; private final TransitionController mController; private final BLASTSyncEngine mSyncEngine; @@ -171,7 +175,7 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe private IRemoteCallback mClientAnimationStartCallback = null; private IRemoteCallback mClientAnimationFinishCallback = null; - private @TransitionState int mState = STATE_COLLECTING; + private @TransitionState int mState = STATE_PENDING; private final ReadyTracker mReadyTracker = new ReadyTracker(); // TODO(b/188595497): remove when not needed. @@ -179,13 +183,12 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe private boolean mNavBarAttachedToApp = false; private int mRecentsDisplayId = INVALID_DISPLAY; - Transition(@TransitionType int type, @TransitionFlags int flags, long timeoutMs, + Transition(@TransitionType int type, @TransitionFlags int flags, TransitionController controller, BLASTSyncEngine syncEngine) { mType = type; mFlags = flags; mController = controller; mSyncEngine = syncEngine; - mSyncId = mSyncEngine.startSyncSet(this, timeoutMs, TAG); } void addFlag(int flag) { @@ -216,13 +219,24 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe return mFlags; } + /** Starts collecting phase. Once this starts, all relevant surface operations are sync. */ + void startCollecting(long timeoutMs) { + if (mState != STATE_PENDING) { + throw new IllegalStateException("Attempting to re-use a transition"); + } + mState = STATE_COLLECTING; + mSyncId = mSyncEngine.startSyncSet(this, timeoutMs, TAG); + } + /** * Formally starts the transition. Participants can be collected before this is started, * but this won't consider itself ready until started -- even if all the participants have * drawn. */ void start() { - if (mState >= STATE_STARTED) { + if (mState < STATE_COLLECTING) { + throw new IllegalStateException("Can't start Transition which isn't collecting."); + } else if (mState >= STATE_STARTED) { Slog.w(TAG, "Transition already started: " + mSyncId); } mState = STATE_STARTED; @@ -235,6 +249,9 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe * Adds wc to set of WindowContainers participating in this transition. */ void collect(@NonNull WindowContainer wc) { + if (mState < STATE_COLLECTING) { + throw new IllegalStateException("Transition hasn't started collecting."); + } if (mSyncId < 0) return; ProtoLog.v(ProtoLogGroup.WM_DEBUG_WINDOW_TRANSITIONS, "Collecting in transition %d: %s", mSyncId, wc); diff --git a/services/core/java/com/android/server/wm/TransitionController.java b/services/core/java/com/android/server/wm/TransitionController.java index fe968ec2e57b2..b482cfa0d4822 100644 --- a/services/core/java/com/android/server/wm/TransitionController.java +++ b/services/core/java/com/android/server/wm/TransitionController.java @@ -30,6 +30,7 @@ import android.annotation.NonNull; import android.annotation.Nullable; import android.app.ActivityManager; import android.app.IApplicationThread; +import android.os.Handler; import android.os.IBinder; import android.os.IRemoteCallback; import android.os.RemoteException; @@ -91,6 +92,20 @@ class TransitionController { /** The transition currently being constructed (collecting participants). */ private Transition mCollectingTransition = null; + /** + * A queue of transitions waiting for their turn to collect. We can only collect 1 transition + * at a time because BLASTSyncEngine only supports 1 sync at a time, so we have to queue them. + * + * WMCore has enough information to ensure that it won't end up collecting multiple transitions + * in parallel by itself; however, Shell can start transitions at arbitrary times via + * {@link WindowOrganizerController#startTransition}, so we have to support those coming in + * at any time (even while already collecting). + * + * This is really just a back-up for unrealistic situations (eg. during tests). In practice, + * this shouldn't ever happen. + */ + private final ArrayList mPendingStartTransitions = new ArrayList<>(); + // TODO(b/188595497): remove when not needed. final StatusBarManagerInternal mStatusBar; @@ -128,16 +143,42 @@ class TransitionController { throw new IllegalStateException("Shell Transitions not enabled"); } if (mCollectingTransition != null) { - throw new IllegalStateException("Simultaneous transitions not supported yet."); + throw new IllegalStateException("Simultaneous transition collection not supported" + + " yet. Use {@link #createPendingTransition} for explicit queueing."); } + Transition transit = new Transition(type, flags, this, mAtm.mWindowManager.mSyncEngine); + ProtoLog.v(ProtoLogGroup.WM_DEBUG_WINDOW_TRANSITIONS, "Creating Transition: %s", transit); + moveToCollecting(transit); + return transit; + } + + /** Starts Collecting */ + private void moveToCollecting(@NonNull Transition transition) { + if (mCollectingTransition != null) { + throw new IllegalStateException("Simultaneous transition collection not supported."); + } + mCollectingTransition = transition; // Distinguish change type because the response time is usually expected to be not too long. - final long timeoutMs = type == TRANSIT_CHANGE ? CHANGE_TIMEOUT_MS : DEFAULT_TIMEOUT_MS; - mCollectingTransition = new Transition(type, flags, timeoutMs, this, - mAtm.mWindowManager.mSyncEngine); - ProtoLog.v(ProtoLogGroup.WM_DEBUG_WINDOW_TRANSITIONS, "Creating Transition: %s", + final long timeoutMs = + transition.mType == TRANSIT_CHANGE ? CHANGE_TIMEOUT_MS : DEFAULT_TIMEOUT_MS; + mCollectingTransition.startCollecting(timeoutMs); + ProtoLog.v(ProtoLogGroup.WM_DEBUG_WINDOW_TRANSITIONS, "Start collecting in Transition: %s", mCollectingTransition); dispatchLegacyAppTransitionPending(); - return mCollectingTransition; + } + + /** Creates a transition representation but doesn't start collecting. */ + @NonNull + PendingStartTransition createPendingTransition(@WindowManager.TransitionType int type) { + if (mTransitionPlayer == null) { + throw new IllegalStateException("Shell Transitions not enabled"); + } + final PendingStartTransition out = new PendingStartTransition(new Transition(type, + 0 /* flags */, this, mAtm.mWindowManager.mSyncEngine)); + ProtoLog.v(ProtoLogGroup.WM_DEBUG_WINDOW_TRANSITIONS, "Creating PendingTransition: %s", + out.mTransition); + mPendingStartTransitions.add(out); + return out; } void registerTransitionPlayer(@Nullable ITransitionPlayer player, @@ -410,6 +451,15 @@ class TransitionController { throw new IllegalStateException("Trying to move non-collecting transition to playing"); } mCollectingTransition = null; + if (!mPendingStartTransitions.isEmpty()) { + ProtoLog.v(ProtoLogGroup.WM_DEBUG_WINDOW_TRANSITIONS, "PendingStartTransition found"); + final PendingStartTransition p = mPendingStartTransitions.remove(0); + moveToCollecting(p.mTransition); + if (p.mOnStartCollecting != null) { + // Post this so that the now-playing transition setup isn't interrupted. + p.mHandler.post(p.mOnStartCollecting); + } + } if (mPlayingTransitions.isEmpty()) { setAnimationRunning(true /* running */); } @@ -507,6 +557,26 @@ class TransitionController { proto.end(token); } + /** Represents a startTransition call made while a transition was already collecting. */ + static class PendingStartTransition { + final Transition mTransition; + + /** Handler to post the collecting callback onto. */ + private Handler mHandler; + + /** Runnable to post to `mHandler` once this pending transition starts collecting. */ + private Runnable mOnStartCollecting; + + PendingStartTransition(Transition transition) { + mTransition = transition; + } + + void setOnStartCollecting(@NonNull Handler handler, @NonNull Runnable callback) { + mHandler = handler; + mOnStartCollecting = callback; + } + } + static class TransitionMetricsReporter extends ITransitionMetricsReporter.Stub { private final ArrayMap mMetricConsumers = new ArrayMap<>(); diff --git a/services/core/java/com/android/server/wm/WindowOrganizerController.java b/services/core/java/com/android/server/wm/WindowOrganizerController.java index 455856ce22cd0..d0f0220a796b7 100644 --- a/services/core/java/com/android/server/wm/WindowOrganizerController.java +++ b/services/core/java/com/android/server/wm/WindowOrganizerController.java @@ -220,31 +220,47 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub try { synchronized (mGlobalLock) { Transition transition = Transition.fromBinder(transitionToken); + if (mTransitionController.getTransitionPlayer() == null && transition == null) { + Slog.w(TAG, "Using shell transitions API for legacy transitions."); + if (t == null) { + throw new IllegalArgumentException("Can't use legacy transitions in" + + " compatibility mode with no WCT."); + } + applyTransaction(t, -1 /* syncId */, null, caller); + return null; + } // In cases where transition is already provided, the "readiness lifecycle" of the // transition is determined outside of this transaction. However, if this is a // direct call from shell, the entire transition lifecycle is contained in the // provided transaction and thus we can setReady immediately after apply. - boolean needsSetReady = transition == null && t != null; + final boolean needsSetReady = transition == null && t != null; + final WindowContainerTransaction wct = + t != null ? t : new WindowContainerTransaction(); if (transition == null) { if (type < 0) { throw new IllegalArgumentException("Can't create transition with no type"); } - if (mTransitionController.getTransitionPlayer() == null) { - Slog.w(TAG, "Using shell transitions API for legacy transitions."); - if (t == null) { - throw new IllegalArgumentException("Can't use legacy transitions in" - + " compatibility mode with no WCT."); - } - applyTransaction(t, -1 /* syncId */, null, caller); - return null; + // If there is already a collecting transition, queue up a new transition and + // return that. The actual start and apply will then be deferred until that + // transition starts collecting. This should almost never happen except during + // tests. + if (mTransitionController.isCollecting()) { + Slog.e(TAG, "startTransition() while one is already collecting."); + final TransitionController.PendingStartTransition pt = + mTransitionController.createPendingTransition(type); + pt.setOnStartCollecting(mService.mH, () -> { + pt.mTransition.start(); + applyTransaction(wct, -1 /*syncId*/, pt.mTransition, caller); + if (needsSetReady) { + pt.mTransition.setAllReady(); + } + }); + return pt.mTransition; } transition = mTransitionController.createTransition(type); } transition.start(); - if (t == null) { - t = new WindowContainerTransaction(); - } - applyTransaction(t, -1 /*syncId*/, transition, caller); + applyTransaction(wct, -1 /*syncId*/, transition, caller); if (needsSetReady) { transition.setAllReady(); } diff --git a/services/tests/wmtests/src/com/android/server/wm/TransitionTests.java b/services/tests/wmtests/src/com/android/server/wm/TransitionTests.java index 141588a87585b..ae3ce04df7619 100644 --- a/services/tests/wmtests/src/com/android/server/wm/TransitionTests.java +++ b/services/tests/wmtests/src/com/android/server/wm/TransitionTests.java @@ -75,7 +75,9 @@ public class TransitionTests extends WindowTestsBase { private Transition createTestTransition(int transitType) { TransitionController controller = mock(TransitionController.class); final BLASTSyncEngine sync = createTestBLASTSyncEngine(); - return new Transition(transitType, 0 /* flags */, 0 /* timeoutMs */, controller, sync); + final Transition t = new Transition(transitType, 0 /* flags */, controller, sync); + t.startCollecting(0 /* timeoutMs */); + return t; } @Test @@ -466,12 +468,13 @@ public class TransitionTests extends WindowTestsBase { final BLASTSyncEngine sync = new BLASTSyncEngine(mWm); final CountDownLatch latch = new CountDownLatch(1); // When the timeout is reached, it will finish the sync-group and notify transaction ready. - new Transition(TRANSIT_OPEN, 0 /* flags */, 10 /* timeoutMs */, controller, sync) { + final Transition t = new Transition(TRANSIT_OPEN, 0 /* flags */, controller, sync) { @Override public void onTransactionReady(int syncId, SurfaceControl.Transaction transaction) { latch.countDown(); } }; + t.startCollecting(10 /* timeoutMs */); assertTrue(awaitInWmLock(() -> latch.await(3, TimeUnit.SECONDS))); } From 1b97f8262c1a3f2b7244522e57198c382f6a923c Mon Sep 17 00:00:00 2001 From: Chris Li Date: Thu, 20 Jan 2022 18:32:52 +0800 Subject: [PATCH 2/2] Queue applySyncTransaction calls when there is active BLAST sync We currently queue applySyncTransaction in WM Shell, but it doesn't sync with Shell transition collection. However, when both happen, it may cause issue because the BLASTSyncEngine currently only support 1 sync at a time. Move the Shell transition queue to WindowOrganizerController as we only queue organizer sync. Unfortunately, unlike organizer sync that is in one WCT, we can't prevent any core-initiated transition. We don't expect it to happen, but if it does, we need to handle them case by case. Fix: 213866869 Test: atest WMShellFlickerTests:ExpandBubbleScreen Change-Id: Ida331585b018e848e2f801da55cb6a4b999950f4 --- data/etc/services.core.protolog.json | 24 ++-- .../android/server/wm/BLASTSyncEngine.java | 66 ++++++++-- .../server/wm/TransitionController.java | 45 +------ .../android/server/wm/WindowContainer.java | 7 +- .../server/wm/WindowOrganizerController.java | 120 ++++++++++++++++-- 5 files changed, 189 insertions(+), 73 deletions(-) diff --git a/data/etc/services.core.protolog.json b/data/etc/services.core.protolog.json index 89aa662db2f98..5d42c89240a24 100644 --- a/data/etc/services.core.protolog.json +++ b/data/etc/services.core.protolog.json @@ -13,12 +13,6 @@ "group": "WM_DEBUG_STARTING_WINDOW", "at": "com\/android\/server\/wm\/ActivityRecord.java" }, - "-2125618712": { - "message": "PendingStartTransition found", - "level": "VERBOSE", - "group": "WM_DEBUG_WINDOW_TRANSITIONS", - "at": "com\/android\/server\/wm\/TransitionController.java" - }, "-2121056984": { "message": "%s", "level": "WARN", @@ -2785,6 +2779,12 @@ "group": "WM_DEBUG_APP_TRANSITIONS", "at": "com\/android\/server\/wm\/AppTransitionController.java" }, + "800698875": { + "message": "SyncGroup %d: Started when there is other active SyncGroup", + "level": "WARN", + "group": "WM_DEBUG_SYNC_ENGINE", + "at": "com\/android\/server\/wm\/BLASTSyncEngine.java" + }, "806891543": { "message": "Setting mOrientationChangeComplete=true because wtoken %s numInteresting=%d numDrawn=%d", "level": "INFO", @@ -3685,12 +3685,6 @@ "group": "WM_DEBUG_WINDOW_ORGANIZER", "at": "com\/android\/server\/wm\/DisplayAreaPolicyBuilder.java" }, - "1884961873": { - "message": "Sleep still need to stop %d activities", - "level": "VERBOSE", - "group": "WM_DEBUG_STATES", - "at": "com\/android\/server\/wm\/Task.java" - }, "1891501279": { "message": "cancelAnimation(): reason=%s", "level": "DEBUG", @@ -3823,6 +3817,12 @@ "group": "WM_DEBUG_BOOT", "at": "com\/android\/server\/wm\/WindowManagerService.java" }, + "2034988903": { + "message": "PendingStartTransaction found", + "level": "VERBOSE", + "group": "WM_DEBUG_WINDOW_TRANSITIONS", + "at": "com\/android\/server\/wm\/WindowOrganizerController.java" + }, "2039056415": { "message": "Found matching affinity candidate!", "level": "DEBUG", diff --git a/services/core/java/com/android/server/wm/BLASTSyncEngine.java b/services/core/java/com/android/server/wm/BLASTSyncEngine.java index bb4519cfc6790..5c1ddd9643253 100644 --- a/services/core/java/com/android/server/wm/BLASTSyncEngine.java +++ b/services/core/java/com/android/server/wm/BLASTSyncEngine.java @@ -64,6 +64,11 @@ class BLASTSyncEngine { void onTransactionReady(int mSyncId, SurfaceControl.Transaction transaction); } + interface SyncEngineListener { + /** Called when there is no more active sync set. */ + void onSyncEngineFree(); + } + /** * Holds state associated with a single synchronous set of operations. */ @@ -137,6 +142,9 @@ class BLASTSyncEngine { Trace.traceEnd(TRACE_TAG_WINDOW_MANAGER); mActiveSyncs.remove(mSyncId); mWm.mH.removeCallbacks(mOnTimeout); + if (mSyncEngineListener != null && mActiveSyncs.size() == 0) { + mSyncEngineListener.onSyncEngineFree(); + } } private void setReady(boolean ready) { @@ -175,22 +183,54 @@ class BLASTSyncEngine { private final WindowManagerService mWm; private int mNextSyncId = 0; private final SparseArray mActiveSyncs = new SparseArray<>(); + private SyncEngineListener mSyncEngineListener; BLASTSyncEngine(WindowManagerService wms) { mWm = wms; } + /** Sets listener listening to whether the sync engine is free. */ + void setSyncEngineListener(SyncEngineListener listener) { + mSyncEngineListener = listener; + } + + /** + * Prepares a {@link SyncGroup} that is not active yet. Caller must call {@link #startSyncSet} + * before calling {@link #addToSyncSet(int, WindowContainer)} on any {@link WindowContainer}. + */ + SyncGroup prepareSyncSet(TransactionReadyListener listener, String name) { + return new SyncGroup(listener, mNextSyncId++, name); + } + int startSyncSet(TransactionReadyListener listener) { return startSyncSet(listener, WindowState.BLAST_TIMEOUT_DURATION, ""); } int startSyncSet(TransactionReadyListener listener, long timeoutMs, String name) { - final int id = mNextSyncId++; - final SyncGroup s = new SyncGroup(listener, id, name); - mActiveSyncs.put(id, s); - ProtoLog.v(WM_DEBUG_SYNC_ENGINE, "SyncGroup %d: Started for listener: %s", id, listener); + final SyncGroup s = prepareSyncSet(listener, name); + startSyncSet(s, timeoutMs); + return s.mSyncId; + } + + void startSyncSet(SyncGroup s) { + startSyncSet(s, WindowState.BLAST_TIMEOUT_DURATION); + } + + void startSyncSet(SyncGroup s, long timeoutMs) { + if (mActiveSyncs.size() != 0) { + // We currently only support one sync at a time, so start a new SyncGroup when there is + // another may cause issue. + ProtoLog.w(WM_DEBUG_SYNC_ENGINE, + "SyncGroup %d: Started when there is other active SyncGroup", s.mSyncId); + } + mActiveSyncs.put(s.mSyncId, s); + ProtoLog.v(WM_DEBUG_SYNC_ENGINE, "SyncGroup %d: Started for listener: %s", + s.mSyncId, s.mListener); scheduleTimeout(s, timeoutMs); - return id; + } + + boolean hasActiveSync() { + return mActiveSyncs.size() != 0; } @VisibleForTesting @@ -199,11 +239,11 @@ class BLASTSyncEngine { } void addToSyncSet(int id, WindowContainer wc) { - mActiveSyncs.get(id).addToSync(wc); + getSyncGroup(id).addToSync(wc); } void setReady(int id, boolean ready) { - mActiveSyncs.get(id).setReady(ready); + getSyncGroup(id).setReady(ready); } void setReady(int id) { @@ -211,14 +251,22 @@ class BLASTSyncEngine { } boolean isReady(int id) { - return mActiveSyncs.get(id).mReady; + return getSyncGroup(id).mReady; } /** * Aborts the sync (ie. it doesn't wait for ready or anything to finish) */ void abort(int id) { - mActiveSyncs.get(id).finishNow(); + getSyncGroup(id).finishNow(); + } + + private SyncGroup getSyncGroup(int id) { + final SyncGroup syncGroup = mActiveSyncs.get(id); + if (syncGroup == null) { + throw new IllegalStateException("SyncGroup is not started yet id=" + id); + } + return syncGroup; } void onSurfacePlacement() { diff --git a/services/core/java/com/android/server/wm/TransitionController.java b/services/core/java/com/android/server/wm/TransitionController.java index b482cfa0d4822..7a031db3a226f 100644 --- a/services/core/java/com/android/server/wm/TransitionController.java +++ b/services/core/java/com/android/server/wm/TransitionController.java @@ -30,7 +30,6 @@ import android.annotation.NonNull; import android.annotation.Nullable; import android.app.ActivityManager; import android.app.IApplicationThread; -import android.os.Handler; import android.os.IBinder; import android.os.IRemoteCallback; import android.os.RemoteException; @@ -92,20 +91,6 @@ class TransitionController { /** The transition currently being constructed (collecting participants). */ private Transition mCollectingTransition = null; - /** - * A queue of transitions waiting for their turn to collect. We can only collect 1 transition - * at a time because BLASTSyncEngine only supports 1 sync at a time, so we have to queue them. - * - * WMCore has enough information to ensure that it won't end up collecting multiple transitions - * in parallel by itself; however, Shell can start transitions at arbitrary times via - * {@link WindowOrganizerController#startTransition}, so we have to support those coming in - * at any time (even while already collecting). - * - * This is really just a back-up for unrealistic situations (eg. during tests). In practice, - * this shouldn't ever happen. - */ - private final ArrayList mPendingStartTransitions = new ArrayList<>(); - // TODO(b/188595497): remove when not needed. final StatusBarManagerInternal mStatusBar; @@ -175,9 +160,13 @@ class TransitionController { } final PendingStartTransition out = new PendingStartTransition(new Transition(type, 0 /* flags */, this, mAtm.mWindowManager.mSyncEngine)); + // We want to start collecting immediately when the engine is free, otherwise it may + // be busy again. + out.setStartSync(() -> { + moveToCollecting(out.mTransition); + }); ProtoLog.v(ProtoLogGroup.WM_DEBUG_WINDOW_TRANSITIONS, "Creating PendingTransition: %s", out.mTransition); - mPendingStartTransitions.add(out); return out; } @@ -451,15 +440,6 @@ class TransitionController { throw new IllegalStateException("Trying to move non-collecting transition to playing"); } mCollectingTransition = null; - if (!mPendingStartTransitions.isEmpty()) { - ProtoLog.v(ProtoLogGroup.WM_DEBUG_WINDOW_TRANSITIONS, "PendingStartTransition found"); - final PendingStartTransition p = mPendingStartTransitions.remove(0); - moveToCollecting(p.mTransition); - if (p.mOnStartCollecting != null) { - // Post this so that the now-playing transition setup isn't interrupted. - p.mHandler.post(p.mOnStartCollecting); - } - } if (mPlayingTransitions.isEmpty()) { setAnimationRunning(true /* running */); } @@ -557,24 +537,13 @@ class TransitionController { proto.end(token); } - /** Represents a startTransition call made while a transition was already collecting. */ - static class PendingStartTransition { + /** Represents a startTransition call made while there is other active BLAST SyncGroup. */ + class PendingStartTransition extends WindowOrganizerController.PendingTransaction { final Transition mTransition; - /** Handler to post the collecting callback onto. */ - private Handler mHandler; - - /** Runnable to post to `mHandler` once this pending transition starts collecting. */ - private Runnable mOnStartCollecting; - PendingStartTransition(Transition transition) { mTransition = transition; } - - void setOnStartCollecting(@NonNull Handler handler, @NonNull Runnable callback) { - mHandler = handler; - mOnStartCollecting = callback; - } } static class TransitionMetricsReporter extends ITransitionMetricsReporter.Stub { diff --git a/services/core/java/com/android/server/wm/WindowContainer.java b/services/core/java/com/android/server/wm/WindowContainer.java index 400684840467a..1205dee595b90 100644 --- a/services/core/java/com/android/server/wm/WindowContainer.java +++ b/services/core/java/com/android/server/wm/WindowContainer.java @@ -85,8 +85,8 @@ import android.view.MagnificationSpec; import android.view.RemoteAnimationDefinition; import android.view.RemoteAnimationTarget; import android.view.SurfaceControl; -import android.view.SurfaceControlViewHost; import android.view.SurfaceControl.Builder; +import android.view.SurfaceControlViewHost; import android.view.SurfaceSession; import android.view.TaskTransitionSpec; import android.view.WindowManager; @@ -3331,7 +3331,10 @@ class WindowContainer extends ConfigurationContainer< ProtoLog.v(WM_DEBUG_SYNC_ENGINE, "setSyncGroup #%d on %s", group.mSyncId, this); if (group != null) { if (mSyncGroup != null && mSyncGroup != group) { - throw new IllegalStateException("Can't sync on 2 engines simultaneously"); + // This can still happen if WMCore starts a new transition when there is ongoing + // sync transaction from Shell. Please file a bug if it happens. + throw new IllegalStateException("Can't sync on 2 engines simultaneously" + + " currentSyncId=" + mSyncGroup.mSyncId + " newSyncId=" + group.mSyncId); } } mSyncGroup = group; diff --git a/services/core/java/com/android/server/wm/WindowOrganizerController.java b/services/core/java/com/android/server/wm/WindowOrganizerController.java index d0f0220a796b7..27024ce77bd2b 100644 --- a/services/core/java/com/android/server/wm/WindowOrganizerController.java +++ b/services/core/java/com/android/server/wm/WindowOrganizerController.java @@ -76,6 +76,7 @@ import android.window.TaskFragmentCreationParams; import android.window.WindowContainerTransaction; import com.android.internal.annotations.VisibleForTesting; +import com.android.internal.protolog.ProtoLogGroup; import com.android.internal.protolog.common.ProtoLog; import com.android.internal.util.ArrayUtils; import com.android.internal.util.function.pooled.PooledConsumer; @@ -94,7 +95,7 @@ import java.util.Map; * @see android.window.WindowOrganizer */ class WindowOrganizerController extends IWindowOrganizerController.Stub - implements BLASTSyncEngine.TransactionReadyListener { + implements BLASTSyncEngine.TransactionReadyListener, BLASTSyncEngine.SyncEngineListener { private static final String TAG = "WindowOrganizerController"; @@ -117,6 +118,21 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub private final HashMap mTransactionCallbacksByPendingSyncId = new HashMap(); + /** + * A queue of transaction waiting for their turn to sync. Currently {@link BLASTSyncEngine} only + * supports 1 sync at a time, so we have to queue them. + * + * WMCore has enough information to ensure that it won't end up collecting multiple transitions + * in parallel by itself; however, Shell can start transitions/apply sync transaction at + * arbitrary times via {@link WindowOrganizerController#startTransition} and + * {@link WindowOrganizerController#applySyncTransaction}, so we have to support those coming in + * at any time (even while already syncing). + * + * This is really just a back-up for unrealistic situations (eg. during tests). In practice, + * this shouldn't ever happen. + */ + private final ArrayList mPendingTransactions = new ArrayList<>(); + final TaskOrganizerController mTaskOrganizerController; final DisplayAreaOrganizerController mDisplayAreaOrganizerController; final TaskFragmentOrganizerController mTaskFragmentOrganizerController; @@ -140,6 +156,7 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub void setWindowManager(WindowManagerService wms) { mTransitionController = new TransitionController(mService, wms.mTaskSnapshotController); mTransitionController.registerLegacyListener(wms.mActivityManagerAppTransitionNotifier); + wms.mSyncEngine.setSyncEngineListener(this); } TransitionController getTransitionController() { @@ -184,6 +201,11 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub final long ident = Binder.clearCallingIdentity(); try { synchronized (mGlobalLock) { + if (callback == null) { + applyTransaction(t, -1 /* syncId*/, null /*transition*/, caller); + return -1; + } + /** * If callback is non-null we are looking to synchronize this transaction by * collecting all the results in to a SurfaceFlinger transaction and then delivering @@ -196,13 +218,25 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub * all the WindowContainers will eventually finish applying their changes and notify * the BLASTSyncEngine which will deliver the Transaction to the callback. */ - int syncId = -1; - if (callback != null) { - syncId = startSyncWithOrganizer(callback); - } - applyTransaction(t, syncId, null /*transition*/, caller); - if (syncId >= 0) { + final BLASTSyncEngine.SyncGroup syncGroup = prepareSyncWithOrganizer(callback); + final int syncId = syncGroup.mSyncId; + if (!mService.mWindowManager.mSyncEngine.hasActiveSync()) { + mService.mWindowManager.mSyncEngine.startSyncSet(syncGroup); + applyTransaction(t, syncId, null /*transition*/, caller); setSyncReady(syncId); + } else { + // Because the BLAST engine only supports one sync at a time, queue the + // transaction. + final PendingTransaction pt = new PendingTransaction(); + // Start sync group immediately when the SyncEngine is free. + pt.setStartSync(() -> + mService.mWindowManager.mSyncEngine.startSyncSet(syncGroup)); + // Those will be post so that it won't interrupt ongoing transition. + pt.setStartTransaction(() -> { + applyTransaction(t, syncId, null /*transition*/, caller); + setSyncReady(syncId); + }); + mPendingTransactions.add(pt); } return syncId; } @@ -244,17 +278,19 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub // return that. The actual start and apply will then be deferred until that // transition starts collecting. This should almost never happen except during // tests. - if (mTransitionController.isCollecting()) { + if (mService.mWindowManager.mSyncEngine.hasActiveSync()) { Slog.e(TAG, "startTransition() while one is already collecting."); final TransitionController.PendingStartTransition pt = mTransitionController.createPendingTransition(type); - pt.setOnStartCollecting(mService.mH, () -> { + // Those will be post so that it won't interrupt ongoing transition. + pt.setStartTransaction(() -> { pt.mTransition.start(); applyTransaction(wct, -1 /*syncId*/, pt.mTransition, caller); if (needsSetReady) { pt.mTransition.setAllReady(); } }); + mPendingTransactions.add(pt); return pt.mTransition; } transition = mTransitionController.createTransition(type); @@ -1052,11 +1088,24 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub return mTaskFragmentOrganizerController; } + /** + * This will prepare a {@link BLASTSyncEngine.SyncGroup} for the organizer to track, but the + * {@link BLASTSyncEngine.SyncGroup} may not be active until the {@link BLASTSyncEngine} is + * free. + */ + private BLASTSyncEngine.SyncGroup prepareSyncWithOrganizer( + IWindowContainerTransactionCallback callback) { + final BLASTSyncEngine.SyncGroup s = mService.mWindowManager.mSyncEngine + .prepareSyncSet(this, ""); + mTransactionCallbacksByPendingSyncId.put(s.mSyncId, callback); + return s; + } + @VisibleForTesting int startSyncWithOrganizer(IWindowContainerTransactionCallback callback) { - int id = mService.mWindowManager.mSyncEngine.startSyncSet(this); - mTransactionCallbacksByPendingSyncId.put(id, callback); - return id; + final BLASTSyncEngine.SyncGroup s = prepareSyncWithOrganizer(callback); + mService.mWindowManager.mSyncEngine.startSyncSet(s); + return s.mSyncId; } @VisibleForTesting @@ -1087,6 +1136,19 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub mTransactionCallbacksByPendingSyncId.remove(syncId); } + @Override + public void onSyncEngineFree() { + if (mPendingTransactions.isEmpty()) { + return; + } + + ProtoLog.v(ProtoLogGroup.WM_DEBUG_WINDOW_TRANSITIONS, "PendingStartTransaction found"); + final PendingTransaction pt = mPendingTransactions.remove(0); + pt.startSync(); + // Post this so that the now-playing transition setup isn't interrupted. + mService.mH.post(pt::startTransaction); + } + @Override public void registerTransitionPlayer(ITransitionPlayer player) { enforceTaskPermission("registerTransitionPlayer()"); @@ -1345,4 +1407,38 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub + result + " when starting " + intent); } } + + /** + * Represents a sync {@link WindowContainerTransaction} call made while there is other active + * {@link BLASTSyncEngine.SyncGroup}. + */ + static class PendingTransaction { + private Runnable mStartSync; + private Runnable mStartTransaction; + + /** + * The callback will be called immediately when the {@link BLASTSyncEngine} is free. One + * should call {@link BLASTSyncEngine#startSyncSet(BLASTSyncEngine.SyncGroup)} here to + * reserve the {@link BLASTSyncEngine}. + */ + void setStartSync(@NonNull Runnable callback) { + mStartSync = callback; + } + + /** + * The callback will be post to the main handler after the {@link BLASTSyncEngine} is free + * to apply the pending {@link WindowContainerTransaction}. + */ + void setStartTransaction(@NonNull Runnable callback) { + mStartTransaction = callback; + } + + private void startSync() { + mStartSync.run(); + } + + private void startTransaction() { + mStartTransaction.run(); + } + } }