From 61481b08c12eeab4a9ae0f4ae685e24ed91a9d47 Mon Sep 17 00:00:00 2001 From: Evan Rosky Date: Fri, 24 Jun 2022 15:57:29 -0700 Subject: [PATCH] Don't unset syncId in Transition. mSyncId was being used to check if "collecting" but then it would be -1 for logging and other purposes. This required mDebugId. This is a little silly though, so instead use mState to track "collecting" so that we can leave mSyncId unchanged -- this also allows logic based on the sync-id after it has started playing. Bug: 233625646 Test: no material change, tests still pass Change-Id: I00a0200c476f4f31c9b81c26d65cc4531d91e24d --- .../com/android/server/wm/Transition.java | 32 +++++++++---------- .../android/server/wm/TransitionTracer.java | 2 +- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/services/core/java/com/android/server/wm/Transition.java b/services/core/java/com/android/server/wm/Transition.java index 3c0cac0079e84..c27a3d34a02fd 100644 --- a/services/core/java/com/android/server/wm/Transition.java +++ b/services/core/java/com/android/server/wm/Transition.java @@ -149,9 +149,6 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe final @TransitionType int mType; private int mSyncId = -1; - // Used for tracking a Transition throughout a lifecycle (i.e. from STATE_COLLECTING to - // STATE_FINISHED or STATE_ABORT), and should only be used for testing and debugging. - private int mDebugId = -1; private @TransitionFlags int mFlags; private final TransitionController mController; private final BLASTSyncEngine mSyncEngine; @@ -295,11 +292,6 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe return mSyncId; } - @VisibleForTesting - int getDebugId() { - return mDebugId; - } - @TransitionFlags int getFlags() { return mFlags; @@ -315,6 +307,10 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe return mFinishTransaction; } + private boolean isCollecting() { + return mState == STATE_COLLECTING || mState == STATE_STARTED; + } + /** Starts collecting phase. Once this starts, all relevant surface operations are sync. */ void startCollecting(long timeoutMs) { if (mState != STATE_PENDING) { @@ -322,7 +318,6 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe } mState = STATE_COLLECTING; mSyncId = mSyncEngine.startSyncSet(this, timeoutMs, TAG); - mDebugId = mSyncId; mController.mTransitionTracer.logState(this); } @@ -353,7 +348,10 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe if (mState < STATE_COLLECTING) { throw new IllegalStateException("Transition hasn't started collecting."); } - if (mSyncId < 0) return; + if (!isCollecting()) { + // Too late, transition already started playing, so don't collect. + return; + } ProtoLog.v(ProtoLogGroup.WM_DEBUG_WINDOW_TRANSITIONS, "Collecting in transition %d: %s", mSyncId, wc); // "snapshot" all parents (as potential promotion targets). Do this before checking @@ -403,7 +401,10 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe * or waiting until after the animation to close). */ void collectExistenceChange(@NonNull WindowContainer wc) { - if (mSyncId < 0) return; + if (mState >= STATE_PLAYING) { + // Too late to collect. Don't check too-early here since `collect` will check that. + return; + } ProtoLog.v(ProtoLogGroup.WM_DEBUG_WINDOW_TRANSITIONS, "Existence Changed in transition %d:" + " %s", mSyncId, wc); collect(wc); @@ -437,7 +438,7 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe */ void setOverrideAnimation(TransitionInfo.AnimationOptions options, @Nullable IRemoteCallback startCallback, @Nullable IRemoteCallback finishCallback) { - if (mSyncId < 0) return; + if (!isCollecting()) return; mOverrideOptions = options; sendRemoteCallback(mClientAnimationStartCallback); mClientAnimationStartCallback = startCallback; @@ -455,7 +456,7 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe * The transition will wait for all groups to be ready. */ void setReady(WindowContainer wc, boolean ready) { - if (mSyncId < 0) return; + if (!isCollecting() || mSyncId < 0) return; mReadyTracker.setReadyFrom(wc, ready); applyReady(); } @@ -473,7 +474,7 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe * @see ReadyTracker#setAllReady. */ void setAllReady() { - if (mSyncId < 0) return; + if (!isCollecting() || mSyncId < 0) return; mReadyTracker.setAllReady(); applyReady(); } @@ -889,7 +890,6 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe // No player registered, so just finish/apply immediately cleanUpOnFailure(); } - mSyncId = -1; mOverrideOptions = null; reportStartReasonsToLogger(); @@ -1614,7 +1614,7 @@ class Transition extends Binder implements BLASTSyncEngine.TransactionReadyListe } boolean getLegacyIsReady() { - return (mState == STATE_STARTED || mState == STATE_COLLECTING) && mSyncId >= 0; + return isCollecting() && mSyncId >= 0; } static Transition fromBinder(IBinder binder) { diff --git a/services/core/java/com/android/server/wm/TransitionTracer.java b/services/core/java/com/android/server/wm/TransitionTracer.java index b1951e0381772..c1927d8643209 100644 --- a/services/core/java/com/android/server/wm/TransitionTracer.java +++ b/services/core/java/com/android/server/wm/TransitionTracer.java @@ -79,7 +79,7 @@ public class TransitionTracer { final ProtoOutputStream outputStream = new ProtoOutputStream(); final long transitionEntryToken = outputStream.start(TRANSITION); - outputStream.write(ID, transition.getDebugId()); + outputStream.write(ID, transition.getSyncId()); outputStream.write(TIMESTAMP, SystemClock.elapsedRealtimeNanos()); outputStream.write(TRANSITION_TYPE, transition.mType); outputStream.write(STATE, transition.getState());